If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Disable e10s in safe mode ("Restart with Add-ons Disabled")

VERIFIED FIXED in Firefox 35

Status

()

Core
General
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: cpeterson, Assigned: Felipe)

Tracking

(Blocks: 1 bug)

unspecified
mozilla35
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(e10sm2+, firefox33 unaffected, firefox34 unaffected, firefox35 verified)

Details

(Whiteboard: [e10s-m2])

Attachments

(5 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Reporter)

Updated

3 years ago
tracking-e10s: + → ?
Flags: firefox-backlog+

Updated

3 years ago
Flags: qe-verify?
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Hi Felipe, can you provide a point value and mark the bug as qe-verify or qe-verify-.
Iteration: --- → 35.1
Flags: needinfo?(felipc)
(Reporter)

Comment 2

3 years ago
This bug should be fixed for the e10s M2 milestone.
tracking-e10s: ? → m2+
Created attachment 8485938 [details] [diff] [review]
Implement common function to check for e10s start, tied to the pref + safe mode

Bjacob will also add more conditions to this function related to A11y tools and IME being enabled.
Created attachment 8485940 [details] [diff] [review]
Use BrowserTabsRemoteAutostart() from cpp callers

Use this function from cpp callers instead of directly accessing the pref
Points: --- → 3
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(felipc)
Created attachment 8486024 [details] [diff] [review]
Use BrowserTabsRemoteAutostart() from cpp callers
Attachment #8485940 - Attachment is obsolete: true
Created attachment 8486025 [details] [diff] [review]
Use Services.appinfo.browserTabsRemoteAutostart from js callers
Let's see how it goes on try: https://tbpl.mozilla.org/?tree=Try&rev=d2d94f62dfbc
Blocks: 1047076
Comment on attachment 8485938 [details] [diff] [review]
Implement common function to check for e10s start, tied to the pref + safe mode

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

Requesting review from Benjamin as he reviewed Bug 948238 and this is basically the same thing, for this other pref.  Benjamin, besides checking the pref (once), this getter is also used to disable e10s-autostart in safe-mode, and Benoit will likely be adding more conditions to it in bug 1047076.  Also it becomes a convenient place to add ENV var checks, but I'm not adding any at the moment.

There's a lot of scattered code through the tree that checks this pref directly and will be changed to check this getter in order to properly follow the pref+safemode conditions. This will be done in another patch.
Attachment #8485938 - Flags: review?(benjamin)
Also, worth noting: we are almost at a point where we can get rid of the browser.tabs.remote pref (as it's always true now for nightly, false otherwise). I was very tempted to do it here, but it will require some extra care in some circumstances, and I don't want to expand the scope of this bug.
Attachment #8486024 - Flags: review?(benjamin)
Attachment #8486025 - Flags: review?(ally)

Comment 10

3 years ago
Comment on attachment 8486024 [details] [diff] [review]
Use BrowserTabsRemoteAutostart() from cpp callers

Please file a followup to remove the code from both of these once we've stabilized some. Beyond this temporary nightly period, safe-mode shouldn't affect e10s.
Attachment #8486024 - Flags: review?(benjamin) → review+

Comment 11

3 years ago
Comment on attachment 8485938 [details] [diff] [review]
Implement common function to check for e10s start, tied to the pref + safe mode

And add a big API comment about how this is temporary and if you're using this method something is probably wrong.
Attachment #8485938 - Flags: review?(benjamin) → review+
Comment on attachment 8486025 [details] [diff] [review]
Use Services.appinfo.browserTabsRemoteAutostart from js callers

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

Assuming the first patch is kosher, your patch is 'splendid'.

What concerns me the most is gSafeMode in the first patch. Could you explain how we are sure that stays set correctly? (bsmedberg approved the patch, so I'm sure it's fine, and I'd like to understand why)

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +963,5 @@
>        }
>      };
>    }
>  
> +  let openRemote = !Services.appinfo.browserTabsRemoteAutostart;

Could we add the string E10S_TESTING_ONLY only to this comment, so that we can find this when bug 1004533 rolls around?
Attachment #8486025 - Flags: review?(ally) → review+
(In reply to :ally Allison Naaktgeboren from comment #12)
> Comment on attachment 8486025 [details] [diff] [review]
> Use Services.appinfo.browserTabsRemoteAutostart from js callers
> 
> Review of attachment 8486025 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Assuming the first patch is kosher, your patch is 'splendid'.
> 
> What concerns me the most is gSafeMode in the first patch. Could you explain
> how we are sure that stays set correctly? (bsmedberg approved the patch, so
> I'm sure it's fine, and I'd like to understand why)

Yeah, the only place in the code that sets this gSafeMode variable is here in this file, on XRE_mainInit. It sets based on an ENV var, command-line arg, or if the option/shift key is being pressed during startup. Otherwise, there's no exposed setter to it (only a getter), so it will always have the correct value.
> 
> ::: browser/components/customizableui/CustomizableWidgets.jsm
> @@ +963,5 @@
> >        }
> >      };
> >    }
> >  
> > +  let openRemote = !Services.appinfo.browserTabsRemoteAutostart;
> 
> Could we add the string E10S_TESTING_ONLY only to this comment, so that we
> can find this when bug 1004533 rolls around?

Oh, it doesn't appear in the patch's 8 lines of context, but this block of code is already wrapped in E10S_TESTING_ONLY.  (/me eagerly waits for ReviewBoard)
Blocks: 1065561
https://hg.mozilla.org/integration/mozilla-inbound/rev/075a07c452d2
Blocks: 1063842
https://hg.mozilla.org/mozilla-central/rev/075a07c452d2
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35

Updated

3 years ago
QA Contact: jbecerra

Updated

3 years ago
Blocks: 1068189

Comment 16

3 years ago
Created attachment 8493128 [details] [diff] [review]
rollup merged to aurora

Approval Request Comment
[Feature/regressing bug #]:
Disable e10s mode on everything but nightly so testers don't break their release profiles after working with nightly and e10s. This patch is required for uplifting bug 1068189 to aurora.
[User impact if declined]:
Can't uplift 1068189.
[Describe test coverage new/current, TBPL]:
This work has been on mc for a a couple weeks.
[Risks and why]: 
pretty minimal, e10s is not used on aurora/beta.
[String/UUID change made/needed]:
none
Attachment #8493128 - Flags: approval-mozilla-aurora?

Comment 17

3 years ago
> [String/UUID change made/needed]:

Hmm, this did change an interface uuid - nsIXULRuntime,

Comment 18

3 years ago
aurora push:
https://tbpl.mozilla.org/?tree=Try&showall=0&rev=44f154f1649f
Comment on attachment 8493128 [details] [diff] [review]
rollup merged to aurora

>> [String/UUID change made/needed]:
> Hmm, this did change an interface uuid - nsIXULRuntime,

Thanks for the correction. UUID changes are acceptable on Aurora.

Aurora+
Attachment #8493128 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 20

3 years ago
Created attachment 8493148 [details] [diff] [review]
rollup merged to beta

Comment 21

3 years ago
(In reply to Lawrence Mandel [:lmandel] from comment #19)
> Comment on attachment 8493128 [details] [diff] [review]
> rollup merged to aurora
> 
> >> [String/UUID change made/needed]:
> > Hmm, this did change an interface uuid - nsIXULRuntime,
> 
> Thanks for the correction. UUID changes are acceptable on Aurora.
> 
> Aurora+

How about beta? Ryan tells me I have to get approved from jorgev, but generally this is still possible.
https://hg.mozilla.org/releases/mozilla-aurora/rev/4ae055d7bc84
status-firefox33: --- → affected
status-firefox34: --- → fixed
status-firefox35: --- → fixed

Comment 23

3 years ago
Comment on attachment 8493148 [details] [diff] [review]
rollup merged to beta

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6497b388d439

Approval Request Comment
[Feature/regressing bug #]:
Disable e10s mode on everything but nightly so testers don't break their release profiles after working with nightly and e10s. This patch is required for uplifting bug 1068189 to beta.
[User impact if declined]:
Can't uplift 1068189.
[Describe test coverage new/current, TBPL]:
This work has been on mc for a couple weeks.
[Risks and why]: 
pretty minimal, e10s is not used on aurora/beta.
[String/UUID change made/needed]:
yes - nsIXULRuntime updated.
Attachment #8493148 - Flags: approval-mozilla-beta?
Jorge, could you approve or reject the UUID change? Thanks
Flags: needinfo?(jorge)
Looks good to me. a+
Flags: needinfo?(jorge)
Comment on attachment 8493148 [details] [diff] [review]
rollup merged to beta

Thanks! Taking it to make sure we have in 33.
Attachment #8493148 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/2b061899d368
status-firefox33: affected → fixed
I took this bug for verification. 

- I've verified that the e10s mode is disabled by default and after restarting in safe mode (browser.tabs.remote and browser.tabs.remote.autostart prefs are false) 
- On Nightly: I've activated e10s mode via dialog (browser.tabs.remote and browser.tabs.remote.autostart prefs are now true), then I restarted in safe mode("Restart with Add-ons Disabled") -> the "browser.tabs.remote" and "browser.tabs.remote.autostart" prefs remain true and the e10s mode isn't deactivated -> it is ok? 

I tested those on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.8.5 using latest Nightly 35.0a1 (buildID: 20140929030205), latest Aurora 34.0a2 (buildID: 20140929004005) and Firefox 33 Beta 8 (buildID: 20140929180120). 

Could you please tell me which is the exact implementation here? 
In summary is mentioned that the e10s should be disable after restart in safe mode and in comment #23 is mentioned that "Disable e10s mode on everything but nightly so testers don't break their release profiles after working with nightly and e10s. This patch is required for uplifting bug 1068189 to beta" without any reference to restart in safe mode.
Flags: needinfo?(felipc)
QA Contact: jbecerra → camelia.badau
Camelia, the checkbox and prefs won't be changed while in Safe Mode.  Here's what you should test:

- Go to Preferences -> General -> check "Enable e10s (multi-process)"
- Firefox will restart, browser.tabs.remote.autostart will be true, and the titles in the tabs for webpages will be underlined (meaning that e10s is active)
- Restart Firefox in Safe Mode, and verify that:
  - browser.tabs.remote.autostart is still true
  - but the tab titles are no longer underlined

You only need to check this for Nightly. Aurora and Beta are not supported for e10s.
Flags: needinfo?(felipc)
Thank you, Felipe! 

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.8.5 using latest Nightly 35.0a1 (buildID: 20141001030205).
Status: RESOLVED → VERIFIED
status-firefox35: fixed → verified
Setting 34 and 33 as unaffected based on comment 29, so this won't keep showing up in our queries.
status-firefox33: fixed → unaffected
status-firefox34: fixed → unaffected
You need to log in before you can comment on or make changes to this bug.