Closed
Bug 1063848
Opened 10 years ago
Closed 10 years ago
Disable e10s in safe mode ("Restart with Add-ons Disabled")
Categories
(Core :: General, defect)
Core
General
Tracking
()
Tracking | Status | |
---|---|---|
e10s | m2+ | --- |
firefox33 | --- | unaffected |
firefox34 | --- | unaffected |
firefox35 | --- | verified |
People
(Reporter: cpeterson, Assigned: Felipe)
References
Details
(Whiteboard: [e10s-m2])
Attachments
(5 files, 1 obsolete file)
3.41 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
6.38 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
3.82 KB,
patch
|
ally
:
review+
|
Details | Diff | Splinter Review |
8.66 KB,
patch
|
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
8.70 KB,
patch
|
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Updated•10 years ago
|
Updated•10 years ago
|
Flags: firefox-backlog+
Updated•10 years ago
|
Flags: qe-verify?
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Comment 1•10 years ago
|
||
Hi Felipe, can you provide a point value and mark the bug as qe-verify or qe-verify-.
Iteration: --- → 35.1
Flags: needinfo?(felipc)
Assignee | ||
Comment 3•10 years ago
|
||
Bjacob will also add more conditions to this function related to A11y tools and IME being enabled.
Assignee | ||
Comment 4•10 years ago
|
||
Use this function from cpp callers instead of directly accessing the pref
Assignee | ||
Updated•10 years ago
|
Points: --- → 3
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(felipc)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8485940 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
Let's see how it goes on try: https://tbpl.mozilla.org/?tree=Try&rev=d2d94f62dfbc
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8486024 -
Flags: review?(benjamin)
Assignee | ||
Updated•10 years ago
|
Attachment #8486025 -
Flags: review?(ally)
Comment 10•10 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•10 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 12•10 years ago
|
||
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+
Assignee | ||
Comment 13•10 years ago
|
||
(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)
Assignee | ||
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/075a07c452d2
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/075a07c452d2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•10 years ago
|
QA Contact: jbecerra
Comment 16•10 years ago
|
||
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•10 years ago
|
||
> [String/UUID change made/needed]:
Hmm, this did change an interface uuid - nsIXULRuntime,
Comment 18•10 years ago
|
||
aurora push: https://tbpl.mozilla.org/?tree=Try&showall=0&rev=44f154f1649f
Comment 19•10 years ago
|
||
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•10 years ago
|
||
Comment 21•10 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.
Comment 23•10 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?
Comment 24•10 years ago
|
||
Jorge, could you approve or reject the UUID change? Thanks
Flags: needinfo?(jorge)
Comment 26•10 years ago
|
||
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+
Comment 28•10 years ago
|
||
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
Assignee | ||
Comment 29•10 years ago
|
||
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)
Comment 30•10 years ago
|
||
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
Comment 31•10 years ago
|
||
Setting 34 and 33 as unaffected based on comment 29, so this won't keep showing up in our queries.
You need to log in
before you can comment on or make changes to this bug.
Description
•