Closed
Bug 1068189
Opened 10 years ago
Closed 10 years ago
Activating E10S via the dialog in Nightly breaks Firefox 32 (when sharing the same user profile)
Categories
(Core :: DOM: Content Processes, defect)
Core
DOM: Content Processes
Tracking
()
VERIFIED
FIXED
mozilla35
People
(Reporter: mhoye, Assigned: jimm)
References
Details
Attachments
(4 files, 1 obsolete file)
1.31 KB,
text/plain
|
Details | |
901 bytes,
patch
|
Felipe
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
2.96 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
1.21 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
If people are using the same profile to try Nightly and Release, activating E10S via the presented dialog box in Nightly activates it in Release as well, and that breaks Firefox pretty badly, and in a way that's hard to understand and fix. It will survive a reinstall, for example. This hurts people who want to try Nightly but have release to fall back on when it breaks, as it's currently likely to do. This is hard to recover from unless you see that underline in the tab and know what it means and what to do about it.
Updated•10 years ago
|
tracking-e10s:
--- → ?
Comment 1•10 years ago
|
||
We should force-disable e10s initialization in Aurora, Beta, and Release channels using #ifndef NIGHTLY_BUILD. We know e10s is broken in those channels, so there is no reason not to disable it and uplift to all channels.
status-firefox32:
--- → affected
status-firefox33:
--- → affected
status-firefox34:
--- → affected
Hardware: x86 → All
Updated•10 years ago
|
Summary: Activating E10S via the dialog in Nightly breaks Firefox proper. → Activating E10S via the dialog in Nightly breaks Firefox 32 (when sharing the same user profile)
Updated•10 years ago
|
Assignee: nobody → jmathies
Assignee | ||
Comment 2•10 years ago
|
||
synopsis of prefs for reference. I'm still not sure what 'browser.tabs.remote.autostart.1' is supposed to be doing. 1) E10S_TESTING_ONLY - defined with NIGHTLY_BUILD 2) browser.tabs.remote - largely obsolete, should be removed, all functionality tied to this should be on by default 3) browser.tabs.remote.autostart - referenced in mc, aurora, beta, release. - not wrapped by E10S_TESTING_ONLY - can't force disable on release since we'd need a point release. 4) browser.tabs.remote.autostart.1 - ??? - where/when does this get set, and why? - http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/main.xul#16 - http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/main.js#91 5) browser.enabledE10SFromPrompt - tied to e10s notification, set if the user opts into e10s testing. - http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#2338 6) browser.tabs.remote.autostart.disabled-because-using-a11y - mozilla-central only at this point
Assignee | ||
Comment 3•10 years ago
|
||
updates to my summary: 1) E10S_TESTING_ONLY - defined with NIGHTLY_BUILD (mc nightly only?) 2) browser.tabs.remote - largely obsolete, should be removed, all functionality tied to this should be on by default 3) browser.tabs.remote.autostart - referenced in mc, aurora, beta, release. - not wrapped by E10S_TESTING_ONLY - can't force disable on release since we'd need a point release. 4) browser.tabs.remote.autostart.1 - overrides browser.tabs.remote.autostart for crash landings - has not been used yet - set manually through default prefs - http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/main.xul#16 - http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/main.js#91 5) browser.enabledE10SFromPrompt - tied to e10s notification, set if the user opts into e10s testing - http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#2338 6) browser.tabs.remote.autostart.disabled-because-using-a11y - browser front end detects if a11y is enabled, will prompt user with a warning - pref gets set if user requests e10s be disabled through warning - pref is only in use on mozilla-central - overrides e10s completely.
Assignee | ||
Comment 4•10 years ago
|
||
More updates. For browser.tabs.remote.autostart I don't think we can uplift patches to turn e10s off. We would need a point release for release, and we'd still have stragglers who haven't updated getting hosed if this gets turned on in a profile used by old release builds. The only other option I see is to change browser.tabs.remote.autostart on mc to something else, obsoleting the old autostart prefs so that it doesn't get messed with.
Comment 5•10 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #2) > 4) browser.tabs.remote.autostart.1 > > - ??? - where/when does this get set, and why? We're going to set it to true by default when we flip e10s on on Nightly the first time. See bug 1058358.
Comment 6•10 years ago
|
||
Can't we just make this function return false if E10S_TESTING_ONLY isn't set? http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4569
Assignee | ||
Comment 7•10 years ago
|
||
So I'm not sure this is such a big deal. I understand that some stuff can break, but generally release with autostart set to true is usable. Most importantly you can get to about:config to turn these prefs off. Yes some users might not know but these are users running test version of the browser. If they don't know about this stuff, they should. We could add a mozilla support page to help users get release fixed up. We might want to uplift a patch (aurora, beta) that turns e10s off completely which would get out to release eventually, but fx32 is kinda stuck with this. The only alternative here is to rename our remote prefs on mc, which will just add more confusion for user and testers. Gavin, any feelings on this?
Flags: needinfo?(gavin.sharp)
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #6) > Can't we just make this function return false if E10S_TESTING_ONLY isn't > set? > http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner. > cpp#4569 Sure, and that's the original point of this bug. But we can't get a change like that out to all release users.
Comment 9•10 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #8) > Sure, and that's the original point of this bug. But we can't get a change > like that out to all release users. We can, it just takes time. I think it's worth doing.
Flags: needinfo?(gavin.sharp)
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #9) > (In reply to Jim Mathies [:jimm] from comment #8) > > Sure, and that's the original point of this bug. But we can't get a change > > like that out to all release users. > > We can, it just takes time. I think it's worth doing. Sounds good to me, I'll put together patches for various repos.
Assignee | ||
Comment 11•10 years ago
|
||
lets see how this comes out - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bf7084e40890
Assignee | ||
Comment 13•10 years ago
|
||
My bad, that's already been uplifted.
No longer depends on: 948238
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8492337 [details] [diff] [review] mozilla-central: disable e10s for !defined(NIGHTLY_BUILD) Review of attachment 8492337 [details] [diff] [review]: ----------------------------------------------------------------- Lets go ahead and get this landed on mc. I'm currently working on some dependent patches in bug 1063848 for aurora and beta.
Attachment #8492337 -
Flags: review?(felipc)
Comment 15•10 years ago
|
||
Comment on attachment 8492337 [details] [diff] [review] mozilla-central: disable e10s for !defined(NIGHTLY_BUILD) I think it's not necessary to deal with browser.tabs.remote. It has always been defined as false on non-nightly, and it only toggles some configurations that shouldn't be harmful to run (and the New e10s window option). This probably also greatly simplifies the branch patches, as the dangerous property is just the autostart one. You could use #!ifndef E10S_TESTING which is defined to be the same as NIGHTLY_BUILD
Attachment #8492337 -
Flags: review?(felipc) → feedback+
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to :Felipe Gomes from comment #15) > Comment on attachment 8492337 [details] [diff] [review] > mozilla-central: disable e10s for !defined(NIGHTLY_BUILD) > > I think it's not necessary to deal with browser.tabs.remote. It has always > been defined as false on non-nightly, and it only toggles some > configurations that shouldn't be harmful to run (and the New e10s window > option). This probably also greatly simplifies the branch patches, as the > dangerous property is just the autostart one. I'll repush with just autostart patch. Generally branch patch landings in bug 1063848 look ok. Aurora's green and beta came up green on try.
Assignee | ||
Comment 17•10 years ago
|
||
> I'll repush with just autostart patch. Generally branch patch landings in > bug 1063848 look ok. Aurora's green and beta came up green on try. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8462f49898f9
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8492337 -
Attachment is obsolete: true
Attachment #8493723 -
Flags: review?(felipc)
Updated•10 years ago
|
Attachment #8493723 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 19•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/764f0043b24b
https://hg.mozilla.org/mozilla-central/rev/764f0043b24b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 21•10 years ago
|
||
Jim, are you going to request approvals for uplift here?
status-firefox32:
affected → ---
status-firefox33:
affected → ---
status-firefox34:
affected → ---
tracking-e10s:
m2+ → ---
Flags: needinfo?(jmathies)
Hardware: All → x86
Target Milestone: mozilla35 → ---
Updated•10 years ago
|
status-firefox32:
--- → affected
status-firefox33:
--- → affected
status-firefox34:
--- → affected
tracking-e10s:
--- → m2+
Hardware: x86 → All
Target Milestone: --- → mozilla35
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #21) > Jim, are you going to request approvals for uplift here? waiting in beta approval in bug 1063848 to see how far we can uplift this.
Flags: needinfo?(jmathies)
Assignee | ||
Comment 23•10 years ago
|
||
Comment on attachment 8493723 [details] [diff] [review] mozilla-central: disable e10s for !defined(NIGHTLY_BUILD) Approval Request Comment [Feature/regressing bug #]: browser.tabs.remote.autostart pref controls code on these branches that break the browser. this is an issue for nightly e10s testers who share profiles with these branches. [User impact if declined]: annoyed e10s testers. [Describe test coverage new/current, TBPL]: on mc. [Risks and why]: none, autostart features should not be enabled on these branches ever. [String/UUID change made/needed]: none.
Attachment #8493723 -
Flags: approval-mozilla-beta?
Attachment #8493723 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
status-firefox35:
--- → fixed
Comment 24•10 years ago
|
||
Comment on attachment 8493723 [details] [diff] [review] mozilla-central: disable e10s for !defined(NIGHTLY_BUILD) Taking it because it is low risk and a pain for testers.
Attachment #8493723 -
Flags: approval-mozilla-beta?
Attachment #8493723 -
Flags: approval-mozilla-beta+
Attachment #8493723 -
Flags: approval-mozilla-aurora?
Attachment #8493723 -
Flags: approval-mozilla-aurora+
Comment 25•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/4a2494b63561 https://hg.mozilla.org/releases/mozilla-beta/rev/d41af0c7fdaf
Comment 26•10 years ago
|
||
Backed out from beta for crashtest/reftest-ipc crashes and failures. https://hg.mozilla.org/releases/mozilla-beta/rev/dabbfa2c0eac https://tbpl.mozilla.org/php/getParsedLog.php?id=49134156&tree=Mozilla-Beta https://tbpl.mozilla.org/php/getParsedLog.php?id=49133836&tree=Mozilla-Beta
Assignee | ||
Comment 28•10 years ago
|
||
beta push with a fix - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fb3b33a382c0
Assignee | ||
Comment 29•10 years ago
|
||
slightly better fix pushed to mc: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2c71ba743910
Assignee | ||
Comment 30•10 years ago
|
||
mc: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2c71ba743910 ma: https://tbpl.mozilla.org/?tree=Try&rev=9ed9b88e53b8 mb: https://tbpl.mozilla.org/?tree=Try&rev=d4ca95e540f8
Assignee | ||
Comment 31•10 years ago
|
||
pushed a cleaner fix for mc: https://tbpl.mozilla.org/?tree=Try&rev=6f04f100f02d
Assignee | ||
Comment 33•10 years ago
|
||
Attachment #8498863 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 34•10 years ago
|
||
Attachment #8498864 -
Flags: review?(wmccloskey)
Comment on attachment 8498863 [details] [diff] [review] mozilla-central patch Review of attachment 8498863 [details] [diff] [review]: ----------------------------------------------------------------- It would still be nice to know why these tests depend on browser.tabs.remote.autostart, but it probably doesn't make sense to spend time on it. ::: toolkit/xre/nsAppRunner.cpp @@ +4575,1 @@ > if (!gBrowserTabsRemoteAutostartInitialized) { Can you please change this function around so it works like this: if (gBrowerTabsRemoteInitialized) { return gBrowserTabsRemote; } gBrowerTabsRemoteInitialized = true; ...do stuff... Then we can remove an indent level and I think it will be clearer that we don't do anything in the cached case. @@ +4577,3 @@ > bool optInPref = Preferences::GetBool("browser.tabs.remote.autostart", false); > bool trialPref = Preferences::GetBool("browser.tabs.remote.autostart.1", false); > + bool testPref = Preferences::GetBool("layers.offmainthreadcomposition.testing.enabled", false); Nit: extra space after |bool|.
Attachment #8498863 -
Flags: review?(wmccloskey) → review+
Comment on attachment 8498864 [details] [diff] [review] aurora, beta patch Review of attachment 8498864 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/xre/nsAppRunner.cpp @@ +4531,5 @@ > #if !defined(NIGHTLY_BUILD) > + // When running tests with 'layers.offmainthreadcomposition.testing.enabled' and autostart > + // set to true, return enabled. These tests must be allowed to run remotely. > + if (Preferences::GetBool("layers.offmainthreadcomposition.testing.enabled", false) && > + Preferences::GetBool("browser.tabs.remote.autostart", false)) { This would be a little clearer if written as an unconditional assignment: gWhatever = pref1 && pref2; Otherwise my eyes read past to the assignment below, but that's in the other #ifdef branch.
Attachment #8498864 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 37•10 years ago
|
||
Assignee | ||
Comment 38•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d6093ce3637
Assignee | ||
Comment 39•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/0272a82658ff
Assignee | ||
Comment 40•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/5217e39df54c https://hg.mozilla.org/releases/mozilla-beta/rev/7b2887bd78a0
Comment 41•10 years ago
|
||
Jim, the approval was from 6 days ago. Please, don't carry the uplift approval from a beta to an other. beta 9 will be live today and it was the last beta... The next one is RC... I hope your patch won't break anything... (ni to make sure you see this message). Adding the keywords to make sure QA checks that.
Assignee | ||
Comment 42•10 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #41) > Jim, the approval was from 6 days ago. Please, don't carry the uplift > approval from a beta to an other. beta 9 will be live today and it was the > last beta... The next one is RC... I hope your patch won't break anything... > (ni to make sure you see this message). > Adding the keywords to make sure QA checks that. It shouldn't, the patch does the same thing as the previous patch, with one added exception for tests.
Flags: needinfo?(jmathies)
Comment 45•10 years ago
|
||
prefEnabled is not declared if NIGHTLY_BUILD is not defined. https://tbpl.mozilla.org/php/getParsedLog.php?id=49742783&tree=Try > /builds/slave/try-l64-0000000000000000000000/build/toolkit/xre/nsAppRunner.cpp: In function 'bool mozilla::BrowserTabsRemoteAutostart()': > /builds/slave/try-l64-0000000000000000000000/build/toolkit/xre/nsAppRunner.cpp:4667:7: error: 'prefEnabled' was not declared in this scope
Comment 46•10 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #45) > prefEnabled is not declared if NIGHTLY_BUILD is not defined. > > https://tbpl.mozilla.org/php/getParsedLog.php?id=49742783&tree=Try > > /builds/slave/try-l64-0000000000000000000000/build/toolkit/xre/nsAppRunner.cpp: In function 'bool mozilla::BrowserTabsRemoteAutostart()': > > /builds/slave/try-l64-0000000000000000000000/build/toolkit/xre/nsAppRunner.cpp:4667:7: error: 'prefEnabled' was not declared in this scope Yep, this is going to burn when m-c merges to Aurora next week.
Flags: needinfo?(jmathies)
Comment 48•10 years ago
|
||
Verified fixed on: * Firefox 33.0 RC build1 (Build ID: 20141007073543) * Aurora 34.0a2 (2014-10-08) using Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 LTS 32-bit with a profile that had e10s enabled from Nightly 35.0a1 (2014-10-08). Testing covered the scenario depicted in Comment 0 along with a quick smoke targeting navigation in private window, new window, multiple opened tabs.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•