Closed Bug 1089774 Opened 10 years ago Closed 10 years ago

[e10s] Restart with e10s disabled if an existing e10s user enables tracking protection

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 36
Tracking Flags:
Tracking Status
e10s m4+ ---

People

(Reporter: mmc, Assigned: mmc)

References

Details

Attachments

(2 files, 6 obsolete files)

Restart when an e10s user enables tracking protection
4.24 KB, patch
mmc
: review+
Details | Diff | Splinter Review
Followup to fix test now that DNT is not being reset
1.69 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
Component: Security: PSM → General
Product: Core → Firefox
Assignee: nobody → mrbkap
tracking-e10s: --- → m4+
Assignee: mrbkap → mmc
Comment on attachment 8514493 [details] [diff] [review]
Restart when an e10s user enables tracking protection

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

This patch makes it so that if browser.tabs.remote.autostart is true, then enabling browser.polaris.enabled throws up a prompt to restart. If the user accepts, then e10s is disabled and restarts. If the user declines, browser.polaris.enabled is set to false.
Attachment #8514493 - Flags: review?(felipc)
Hm the prompt to restart doesn't mention to the user that e10s will be disabled..
It's the generic feature requires restart prompt. What would you like it to say?
This never needs to be localized, right? Do you want me to just use a warning similar to https://bugzilla.mozilla.org/show_bug.cgi?id=1047076?

let promptMessage = "Multiprocess Nightly (e10s) does not yet support tracking protection. Multiprocessing will be disabled if you restart Firefox. Would you like to restart?";
Yep, that sounds good. Maybe "[..] Would you like to continue?" which implies continuing with the choice of enabling tracking protection + disabling e10s
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #6)
> This never needs to be localized, right?

Right, we hope that by the time these features get promoted to Aurora, they won't be mutually incompatible anymore and we will be able to remove all of this special code to enable/disable them
Attachment #8514493 - Attachment is obsolete: true
Attachment #8514493 - Flags: review?(felipc)
Comment on attachment 8514606 [details] [diff] [review]
Restart when an e10s user enables tracking protection

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

Please take another look. I also missed disabling the polaris pref test on e10s.
Attachment #8514606 - Flags: review?(felipc)
Comment on attachment 8514606 [details] [diff] [review]
Restart when an e10s user enables tracking protection

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>+            if (shouldProceed) {
>+              Services.prefs.clearUserPref("browser.tabs.remote.autostart");

We shouldn't do this - we want these people to continue testing e10s once we resolve bug 1055186 and rip out the code from bug 1088904. The code in bug 1088904 means this is unnecessary to disable e10s anyways.

>+            } else {
>+              Services.prefs.clearUserPref(POLARIS_ENABLED);

Don't we also need to undo the prefs we just set above. It would actually be better to avoid setting them until after we've confirmed we're restarting.
Attachment #8514606 - Attachment is obsolete: true
Attachment #8514606 - Flags: review?(felipc)
Attachment #8516147 - Attachment is obsolete: true
Comment on attachment 8516160 [details] [diff] [review]
Restart when an e10s user enables tracking protection

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

Please take another look. This does not fix https://bugzilla.mozilla.org/show_bug.cgi?id=1092808.
Attachment #8516160 - Flags: review?(gavin.sharp)
Comment on attachment 8516160 [details] [diff] [review]
Restart when an e10s user enables tracking protection

I would prefer splitting the prompting code out to a helper function, just to make the code a bit easier to read.

if (enabled) {
  // E10S and polaris temporarily not compatible with each other
  let shouldRestart = e10sEnabled && this._promptToRestart();
  if (!e10sEnabled || shouldRestart) {
    // enable polaris
    if (shouldRestart) {
      Services.startup.quit(...);
    }
  } else {
    // clear polaris pref
  }
} else {
  // disable polaris
}

The "disable polaris" branch should be consistent with what we have now (it should set all prefs to false, so that people who toggled it on can toggle it off consistently).

r=me with those changes, but I'll look at the final patch.
Attachment #8516160 - Flags: review?(gavin.sharp) → feedback+
Attachment #8516160 - Attachment is obsolete: true
Comment on attachment 8516281 [details] [diff] [review]
Restart when an e10s user enables tracking protection

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

Please take another look. I disabled all related polaris prefs as you asked when polaris is disabled, but I don't understand what our messaging will be around DNT and polaris considering that the UI for DNT is available without polaris, and we will probably end up reverting existing DNT users who try polaris then turn it off.
Attachment #8516281 - Flags: review?(gavin.sharp)
Attachment #8516281 - Attachment is obsolete: true
Attachment #8516281 - Flags: review?(gavin.sharp)
Attachment #8516284 - Flags: review?(gavin.sharp)
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #18)
> Please take another look. I disabled all related polaris prefs as you asked
> when polaris is disabled, but I don't understand what our messaging will be
> around DNT and polaris considering that the UI for DNT is available without
> polaris, and we will probably end up reverting existing DNT users who try
> polaris then turn it off.

You're right, this is potentially confusing. We could in theory set a pref when enabling polaris that tracks the pre-Polaris DNT state, and then just restore that when disabling Polaris. But just not re-setting DNT is probably the simpler choice.
Comment on attachment 8516284 [details] [diff] [review]
Restart when an e10s user enables tracking protection

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>+            } else {
>+              Services.prefs.clearUserPref(POLARIS_ENABLED);

A comment here explaining ("E10S is enabled, but user didn't want to restart, so we can't enable Polaris") would help clarity a little, I think.

>+            // Disable all related preferences if polaris is disabled.
>+            Services.prefs.clearUserPref("privacy.donottrackheader.enabled");

So let's remove this line only, and adjust the comment to explain (e.g. "Don't reset DNT because that has visible prefs UI and may have been previously set independently.").

>+  _promptToRestart: function () {

We should name this something a bit more specific, let's go with "_promptForE10SRestart".

r=me with those changes, thanks for fixing this.
Attachment #8516284 - Flags: review?(gavin.sharp) → review+
Attachment #8516284 - Attachment is obsolete: true
Attachment #8516386 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/mozilla-central/rev/ceece478af3d
https://hg.mozilla.org/mozilla-central/rev/cee4cbf3b140
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
I've used the latest Nightly  Build Id:20150205030205 and a new profile that had e10s enabled, after that I've enabled Tracking protection but no need for restart was prompted also e10s stayed enabled. Also bug 1055186 is still reproducible. Was mt expected behavior wrong?
Flags: needinfo?(mmc)
This bug is no longer valid since e10s is no longer incompatible with tracking protection. It got reverted a while back.

What do you mean bug 1055186 is still reproducible?
Flags: needinfo?(mmc)
Thanks for the clarification I retested bug 1055186 it's been fixed it's not reproducible, I just misunderstood the expected behavior.
OK, great. In that case there is no need to QA this bug since it's already been reverted.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: