1068189 - Activating E10S via the dialog in Nightly breaks Firefox 32 (when sharing the same user profile)

Status: VERIFIED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Mike Hoye [:mhoye]]

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.

Comment 1

[Chris Peterson [:cpeterson]]

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.

Comment 2

[Jim Mathies [:jimm]]

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

Comment 3

[Jim Mathies [:jimm]]

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.

Comment 4

[Jim Mathies [:jimm]]

Attachment: pref notes summary

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

[:Gavin Sharp [email: [e-mail]]]

(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

[Dave Townsend [:mossop]]

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

Comment 7

[Jim Mathies [:jimm]]

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?

Comment 8

[Jim Mathies [:jimm]]

(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

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 10

[Jim Mathies [:jimm]]

(In reply to :Gavin Sharp [email: [e-mail]] 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.

Comment 11

[Jim Mathies [:jimm]]

Attachment: mozilla-central: disable e10s for !defined(NIGHTLY_BUILD)

lets see how this comes out - 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bf7084e40890

Comment 12

[Jim Mathies [:jimm]]

For aurora and beta we'll need bug 948238.

Comment 13

[Jim Mathies [:jimm]]

My bad, that's already been uplifted.

Comment 14

[Jim Mathies [:jimm]]

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.

Comment 15

[:Felipe Gomes (needinfo for replies!)]

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

Comment 16

[Jim Mathies [:jimm]]

(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.

Comment 17

[Jim Mathies [:jimm]]

> 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

Comment 18

[Jim Mathies [:jimm]]

Attachment: mozilla-central: disable e10s for !defined(NIGHTLY_BUILD)

Comment 19

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/764f0043b24b

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/764f0043b24b

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Jim, are you going to request approvals for uplift here?

Comment 22

[Jim Mathies [:jimm]]

(In reply to :Gavin Sharp [email: [e-mail]] 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.

Comment 23

[Jim Mathies [:jimm]]

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.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4a2494b63561
https://hg.mozilla.org/releases/mozilla-beta/rev/d41af0c7fdaf

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[Jim Mathies [:jimm]]

beta push with a fix - 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fb3b33a382c0

Comment 29

[Jim Mathies [:jimm]]

slightly better fix pushed to mc:

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

Comment 30

[Jim Mathies [:jimm]]

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

Comment 31

[Jim Mathies [:jimm]]

pushed a cleaner fix for mc:
https://tbpl.mozilla.org/?tree=Try&rev=6f04f100f02d

Comment 33

[Jim Mathies [:jimm]]

Attachment: mozilla-central patch

Comment 34

[Jim Mathies [:jimm]]

Attachment: aurora, beta patch

Comment 35

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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|.

Comment 36

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 38

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d6093ce3637

Comment 39

[Jim Mathies [:jimm]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0272a82658ff

Comment 40

[Jim Mathies [:jimm]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5217e39df54c
https://hg.mozilla.org/releases/mozilla-beta/rev/7b2887bd78a0

Comment 41

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 42

[Jim Mathies [:jimm]]

(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.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d6093ce3637

Comment 45

[Tooru Fujisawa [:arai]]

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

[Ryan VanderMeulen [:RyanVM]]

(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.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

I spun off comments 45/46 to bug 1079573.

Comment 48

[Andrei Vaida [:avaida]]

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.