[e10s] Disable autostart and suppress prompt for e10s if privacy.trackingprotection.enabled is true

RESOLVED FIXED in mozilla36

Status

()

Core
DOM: Security
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mmc, Assigned: mmc)

Tracking

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+)

Details

Attachments

(2 attachments)

This is a stopgap solution so that if we advertise e10s and tracking protection being available in Nightly, or make e10s the default, people who turn on e10s will not be unpleasantly surprised by turning on tracking protection.
Bill or Chris, what are the right prefs to flip here?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(cpeterson)
The function BrowserTabsRemoteAutostart() controls whether e10s should be enabled:

https://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4515

See also bug 1047076, bug 1063848, and bug 1063669.
tracking-e10s: --- → +
Flags: needinfo?(wmccloskey)
Flags: needinfo?(cpeterson)
You might also be interested in nsBrowserGlue, where we decide whether to put up the e10s prompt:
http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#2325
Ugh. What's the minimum amount of work we can do here? Do we need a warning like the bug 1047076 is someone turns on tracking protection mid-session then restart? The current behavior is simply that nothing gets blocked in e10s windows and the shield never shows up.
I think the minimum amount of work is to not show any UI and to just return false (i.e. "do not initialize e10s") from BrowserTabsRemoteAutostart() if Preferences::GetBool("privacy.trackingprotection.enabled", false) is true. You should also log a console warning with LogE10sBlockedReason() if you return false from BrowserTabsRemoteAutostart().
(In reply to Chris Peterson (:cpeterson) from comment #5)
> I think the minimum amount of work is to not show any UI and to just return
> false (i.e. "do not initialize e10s") from BrowserTabsRemoteAutostart() if
> Preferences::GetBool("privacy.trackingprotection.enabled", false) is true.
> You should also log a console warning with LogE10sBlockedReason() if you
> return false from BrowserTabsRemoteAutostart().

That will result in a poor experience for existing e10s users toggling on tracking protection (e10s can't be turned off dynamically). Since we're going to have e10s on by default, and will be encouraging users to also test tracking protection, that is not sufficient.

In addition to the changes mentioned above, the "polaris activation" code from bug 1081343 should detect that e10s is enabled and prompt the user to restart (using code similar to the existing prompt-to-restart in preferences, https://hg.mozilla.org/mozilla-central/annotate/8230834302c9/browser/components/preferences/in-content/main.js#l125).
Created attachment 8512126 [details] [diff] [review]
Disable e10s autostart and prompt when tracking protection is enabled (
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Comment on attachment 8512126 [details] [diff] [review]
Disable e10s autostart and prompt when tracking protection is enabled (

Review of attachment 8512126 [details] [diff] [review]:
-----------------------------------------------------------------

Tested by patching browser/app/profile/firefox.js to set app.update.channel to "nightly", privacy.trackingprotection.enabled to true, then commenting out checks for hardware acceleration (https://bugzil.la/1089003) and accessibility (http://bugzilla.mozilla.org/show_bug.cgi?id=1089744). Toggling tracking protection on and off and restarting the browser shows the prompt as expected. When tracking protection is on and e10s is enabled, LogE10sBlockedReason shows the correct message in the console.
Attachment #8512126 - Flags: review?(wmccloskey)
Attachment #8512126 - Flags: review?(felipc)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #6)
> (In reply to Chris Peterson (:cpeterson) from comment #5)
> > I think the minimum amount of work is to not show any UI and to just return
> > false (i.e. "do not initialize e10s") from BrowserTabsRemoteAutostart() if
> > Preferences::GetBool("privacy.trackingprotection.enabled", false) is true.
> > You should also log a console warning with LogE10sBlockedReason() if you
> > return false from BrowserTabsRemoteAutostart().
> 
> That will result in a poor experience for existing e10s users toggling on
> tracking protection (e10s can't be turned off dynamically). Since we're
> going to have e10s on by default, and will be encouraging users to also test
> tracking protection, that is not sufficient.
> 
> In addition to the changes mentioned above, the "polaris activation" code
> from bug 1081343 should detect that e10s is enabled and prompt the user to
> restart (using code similar to the existing prompt-to-restart in
> preferences,
> https://hg.mozilla.org/mozilla-central/annotate/8230834302c9/browser/
> components/preferences/in-content/main.js#l125).

Filed bug 1089774 for this, since it sounds like we need to do that in addition to suppressing the prompt and disabling autostart.
Summary: [e10s] make enabling privacy.trackingprotection.enabled turn off e10s → [e10s] Disable autostart and suppress prompt for e10s if privacy.trackingprotection.enabled is true
Comment on attachment 8512126 [details] [diff] [review]
Disable e10s autostart and prompt when tracking protection is enabled (

Review of attachment 8512126 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for this patch. It would be ideal to address comment 6 (spun off to bug 1089774) at the same time and land it together. But I'll let you figure it out.

There's also the question of the related experience in the other direction: if tracking protection is enabled, and the user decides to enable the e10s checkbox. What would then happen?  Maybe the e10s checkbox should be in a disabled state in that case..
Attachment #8512126 - Flags: review?(felipc) → review+
Attachment #8512126 - Flags: review?(wmccloskey) → review+
(In reply to :Felipe Gomes from comment #10)
> r+ for this patch. It would be ideal to address comment 6 (spun off to bug
> 1089774) at the same time and land it together. But I'll let you figure it
> out.

Without this patch, the behavior is that the user enables tracking protection and it just doesn't work. With this patch, it still doesn't work, but will start working on restart. I think that's an improvement, so I'd like to go ahead and land.
 
> There's also the question of the related experience in the other direction:
> if tracking protection is enabled, and the user decides to enable the e10s
> checkbox. What would then happen?  Maybe the e10s checkbox should be in a
> disabled state in that case..

Enabling e10s will prompt a restart in which case autostart will be false because tracking protection is on, but the checkbox will be checked. That's the same behavior as in the accessibility case. I will file a bug.
https://hg.mozilla.org/mozilla-central/rev/a6240694a609
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8515078 [details] [diff] [review]
backout_a6240694a609.patch

backout patch

It is not appropriate to silently disable e10s because tracking protection is turned on. The standard we've had is if your browsing experience is fundamentally broken by e10s. This is true of accessibility (visually impaired person is unable to use their screen reader) and IME (unable to enter text for the users native language), but not true of tracking protection.
Attachment #8515078 - Flags: review?(jmathies)

Updated

4 years ago
Attachment #8515078 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/integration/fx-team/rev/0f8732b3b6e7
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/0f8732b3b6e7
Target Milestone: mozilla36 → ---
Depends on: 1092163
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #15)
> It is not appropriate to silently disable e10s because tracking protection
> is turned on. The standard we've had is if your browsing experience is
> fundamentally broken by e10s. This is true of accessibility (visually
> impaired person is unable to use their screen reader) and IME (unable to
> enter text for the users native language), but not true of tracking
> protection.

We're going to be telling people to test this feature, and they can't do that if it's broken. We can't fix that in time, so we need to do this in the interim.
Re-landed this:
https://hg.mozilla.org/integration/fx-team/rev/cd3e7db459a8
OS: Linux → All
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/cd3e7db459a8
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.