Opened 4 months ago

Closed 5 weeks ago

#21999 closed defect (fixed)

The "request English language web pages"-prompt is not working in 7.0a3

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff52-esr, tbb-e10s, tbb-7.0-must, tbb-7.0-issues, tbb-regression, TorBrowserTeam201707R
Cc: mcs, brade, arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Using a non-en-US locale does not show the "request English language web pages"-prompt anymore with the switch to 7.0a3. I have not looked closer but I guess the progress listener is broken due to e10s.

Child Tickets

Change History (18)

comment:1 Changed 4 months ago by gk

Keywords: tbb-7.0-must added

comment:2 Changed 4 months ago by gk

Keywords: tbb-7.0-must-alpha added; tbb-7.0-must removed

comment:3 Changed 4 months ago by gk

Keywords: TorBrowserTeam201705 added

Moving more tickets on our May 2015 radar.

comment:4 Changed 3 months ago by gk

Keywords: tbb-7.0-must added; tbb-7.0-must-alpha removed

We are beyond the alpha testing. Moving tickets for tbb-7.0-must.

comment:5 Changed 3 months ago by cypherpunks

Why is this prompt at all? Shouldn't it ask the opposite "Do you want to request web pages of your locale (reduces antifingerprinting protection)?" if should?
(Never seen that switching GUI language affects some program's behavior.)

comment:6 Changed 2 months ago by gk

Keywords: TorBrowserTeam201706 added; TorBrowserTeam201705 removed

Moving our tickets to June.

comment:7 Changed 2 months ago by arthuredelstein

Cc: arthuredelstein added

comment:8 Changed 2 months ago by gk

Keywords: tbb-7.0-issues tbb-regression added

Regression from 6.X.

comment:9 Changed 2 months ago by arthuredelstein

Keywords: TorBrowserTeam201706R added; TorBrowserTeam201706 removed
Status: newneeds_review

comment:10 in reply to:  9 ; Changed 2 months ago by mcs

Replying to arthuredelstein:

Here's a patch for review:
https://github.com/arthuredelstein/torbutton/commit/21999

The code looks okay but Kathy and I have a couple of questions:

  • Why is the delay before showing the prompt only imposed for about:tor?
  • The old code only showed the prompt when the user accessed an HTTP page. The new code will show it at startup on about:tor, which seems like a less friendly "first run" experience (mostly because the prompt will cover up a lot of the about:tor content). Are we okay with this change in behavior?

comment:11 in reply to:  10 Changed 2 months ago by gk

Status: needs_reviewneeds_revision

Replying to mcs:

Replying to arthuredelstein:

Here's a patch for review:
https://github.com/arthuredelstein/torbutton/commit/21999

The code looks okay but Kathy and I have a couple of questions:

  • Why is the delay before showing the prompt only imposed for about:tor?
  • The old code only showed the prompt when the user accessed an HTTP page. The new code will show it at startup on about:tor, which seems like a less friendly "first run" experience (mostly because the prompt will cover up a lot of the about:tor content). Are we okay with this change in behavior?

No. I think we should try to get it going the way it was implemented in #18019. Or at least the patch should result in a similar experience.

Last edited 2 months ago by gk (previous) (diff)

comment:12 Changed 2 months ago by arthuredelstein

Status: needs_revisionneeds_review

Makes sense to me. Here's a different patch that I think preserves the behavior from before. One problem is that with e10s, the nsIWebProgressListener no longer delays the request. But "http-on-modify-request" observers correctly delays the request until their observe() function returns, so I am using one of those instead.

The only drawback I see with this approach is that sometimes Firefox makes a background http request (such as checking for updates) and I'm not sure whether or not we want to show the prompt when that happens, and what the criteria should be for ignoring such requests.

https://github.com/arthuredelstein/torbutton/commit/21999+2

comment:13 in reply to:  12 ; Changed 2 months ago by gk

Keywords: TorBrowserTeam201706 added; TorBrowserTeam201706R removed
Status: needs_reviewneeds_revision

Replying to arthuredelstein:

Makes sense to me. Here's a different patch that I think preserves the behavior from before. One problem is that with e10s, the nsIWebProgressListener no longer delays the request. But "http-on-modify-request" observers correctly delays the request until their observe() function returns, so I am using one of those instead.

Sounds like a good idea. While testing this for a while I found two possible outcomes both of them being suboptimal:

1) The dialog does not get shown at all. This happens when the Torbutton update check is the first request captured by the observer. Subsequent surfing to different websites does not bring the dialog up either. Not sure why that is happening.

2) I get the dialog shown with the about:tor page (triggered by the updater check). This is pretty confusing as it boils down to a modal dialog during the start-up process, a thing we should avoid if possible.

I think what we could do is to use the setTimeout() call which we need for the homepage case anyway and apply it generally. There should not be any automatic requests on start-up besides the update checks for extensions and the browser itself and the Torbutton version check. As those happen often on start-up and there is no need to show localized web content in a response basically exempting those requests seems fine to me.

The only drawback I see with this approach is that sometimes Firefox makes a background http request (such as checking for updates) and I'm not sure whether or not we want to show the prompt when that happens, and what the criteria should be for ignoring such requests.

The prompt is shown once. And it is not really important for background update requests/checks as the response content displayed in the browser (if it is displayed at all) is not dependent on the Accept-Language header. So it seems fine to me we can ignore this kind of requests.

https://github.com/arthuredelstein/torbutton/commit/21999+2

Apart from the suggestion above there are some nits:

    // add a web progress listener that will show a "request English language
    // web pages?" prompt the first time an http or https page is opened.

But we have no web progress listener anymore.

+function torbutton_http_connection_observed(aRequest, aData) {
+    // If we are loading an HTTP page, show the
+  // "request English language web pages?" prompt.

Indentation.

comment:14 Changed 2 months ago by cypherpunks

As you don't provide UI to change this setting later, you might move it to installer.

comment:15 Changed 2 months ago by gk

#22659 is related to that feature.

comment:16 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201707 added; TorBrowserTeam201706 removed

Moving Tickets to July 2017.

comment:17 in reply to:  13 Changed 6 weeks ago by arthuredelstein

Keywords: TorBrowserTeam201707R added; TorBrowserTeam201707 removed
Status: needs_revisionneeds_review

Replying to gk:

Thanks for the review and testing. Here is a revised branch, with the same patch revised to trigger the prompt only when a request is launched from a content "load context". I tested this new version with update requests and various about: pages and the prompt was not triggered.

I also noticed that some homepage URLs were not being properly detected by function torbutton_is_homepage_url(aURI). So I added a second patch to this new branch to try to take care of that problem.

https://github.com/arthuredelstein/torbutton/commits/21999+3

comment:18 Changed 5 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks, looks good to me. Merged to master (commit 90c35ef999de8b9ee09876f5772a2a728d2bafc1 and 8af90b3222529f9b863229a32dd62b2f9f941d19).

Note: See TracTickets for help on using tickets.