1372824 - e10s not enabled the first time on new profiles

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Mike Hommey [:glandium]]

Not sure what the right component is here, so I'm leaving it untriaged.

Originally, I thought this was a distro build thing, but it turns out it also happen if I use a mozilla.org build.

STR:
- Download and unpack firefox-54.0.tar.bz2
- Create an empty directory, e.g. /tmp/f
- Run firefox with the empty directory as new profile: firefox -profile /tmp/f -new-instance (this also works when using the profile manager to create a new profile ; presumably, this also works after removing ~/.mozilla entirely too)

Result:
- No `firefox -contentproc` process is running even when browsing web sites
- about:support says "Multiprocess Windows  0/1 (Disabled)"
- about:config gives the following values:
    browser.tabs.remote.autostart  false (default)
    browser.tabs.remote.autostart.2  true (user set)
    e10s.rollout.cohort   multiBucket4 (user set)
    e10s.rollout.cohortSample   0.339305 (user set)
    e10s.rollout.cohortSample.multi  0.598137 (user set)
    extensions.e10s.rollout.hasAddon false (user set)
    extensions.e10s.rollout.policy  50allmpc (user set)
    extensions.e10sBlockedByAddons  false (user set)
    extensions.e10sBlocksEnabling  true (default)
    extensions.e10sMultiBlockedByAddons  false (user set)
    extensions.e10sMultiBlocksEnabling  true (default)
    dom.ipc.processCount 1 (default)
    dom.ipc.processCount.extension 1 (default)
    dom.ipc.processCount.web  4 (user set)

I don't get e10s until I flip browser.tabs.remote.autostart manually.

Comment 1

[Mike Hommey [:glandium]]

Oh, actually it *does* get enabled eventually, after restarting once. Presumably, that's because mozilla::BrowserTabsRemoteAutostart is called before the e10srollout addon has done anything the first time.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

This has always been a possibility. The problem is that e10s requires a restart to work, so Services.appinfo.browserTabsRemoteAutostart is a one-time getter that caches its value. So it's a race on whether the e10srollout addon has a chance of running and setting up the prefs before any other code tries to read whether e10s is enabled or not. So, in that case, it takes one start for things to be set up.

It used to be for a long time that the addon wins the race in a clean profile, but I wouldn't be surprised if any code that landed recently has changed the balance.

(A profile with add-ons has always had small chances of winning the races because there are various addons out there that read Services.appinfo.browserTabsRemoteAutostart before the e10srollout one runs)


If everything stays on track for 57 (e10s-a11y ships and we drop support for legacy addons), we will be able to directly toggle the prefs to true on release and not need the rollout addon anymore, so this problem will go away.


I'm inclined to mark this as WONTFIX, unless someone wants to debug which code is reading nsAppRunner::BrowserTabsRemoteAutostart() before the addons manager startup, and how to delay said code..

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

This is separate from bug 1374653. It is caused because we create a DOM document very early in startup in order to read the install.rdf for one of the built-in addons, which creates a DOM wrapper for the document, touching [1], which calls [2] and finally [3] which causes us to cache the default "e10s off" value before we run the e10srollout code.

I think there are a few ways to attack this problem, though none of them are particularly appealing. I don't see an easy way of avoiding creation of the document or its associated WebIDL wrapper. This code also cannot be moved back, as it is run as part of the addon module startup -- we need to read the install.rdf before running the e10srollout code.

That leaves me with the caching behavior in mozilla::BrowserTabsRemoteAutostart. If we avoid caching the default value or update it after the e10srollout addon runs for the first time, that would fix this bug. I've audited the code and the behavior is correct here. After this first call to BrowserTabsRemoteAutostart, then next code to run is the e10srollout addon followed by gfxVars::Initialize. Note that if gfxVars::Initialize ran earlier in startup then we'd have a problem because it caches the return value. Fortunately it doesn't. I think that given comment 2 (second to last paragraph), I'm willing to work around this by listening for changes to the cohort.

[1] http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/dom/webidl/Document.webidl#343
[2] http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/dom/events/TouchEvent.cpp#184,220
[3] http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/gfx/thebes/gfxPlatform.cpp#2657,2664

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1372824 - Work around race conditions on startup enabling e10s.

On startup, we create a document which (through a series of calls) ends up
querying the state of e10s. On Release and Beta, this happens before we run
the e10s rollout addon and where the default is off. Because we cache the
value after the first call, this disables e10s until the user restarts with
the e10srollout prefs set properly. We work around this by noticing when
e10srollout sets the cohort and recomputing the "enabled" value again.

Review commit: https://reviewboard.mozilla.org/r/155854/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155854/

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8885025 [details]
Bug 1372824 - Work around race conditions on startup enabling e10s.

https://reviewboard.mozilla.org/r/155854/#review161270

Alright, I'm sold!

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

I'm hesitating a little about landing this because it turns out that bug 1374653 is indeed separate from this, but related. It's likely that the fix for that bug will be similar or related to the fix from this bug.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8885025 [details]
Bug 1372824 - Work around race conditions on startup enabling e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155854/diff/1-2/

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8885025 [details]
Bug 1372824 - Work around race conditions on startup enabling e10s.

https://reviewboard.mozilla.org/r/155854/#review162628

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9b3f02b072b
Work around race conditions on startup enabling e10s. r=Felipe

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

[Tracking Requested - why for this release]: We're going to definitely want to land this on Beta. I'll let it bake on Nightly for a few days, then request approval.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c9b3f02b072b

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8885025 [details]
Bug 1372824 - Work around race conditions on startup enabling e10s.

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: Users on Windows and Linux won't receive e10s on their first run. Furthermore, if they install addons, the first launch after the addon is installed would not disable or enable e10s as appropriate.
[Is this code covered by automated tests?]: No :(
[Has the fix been verified in Nightly?]: I have tested locally, but I do not know of any independent verification.
[Needs manual test from QE? If yes, steps to reproduce]:

1. Start Firefox with new profile.
2. Go to about:support to check whether e10s is enabled.

--

1. Start Firefox with a clean profile.
2. Install an *mpc=no* add-on.
2.1. Accept the prompt to restart the browser to complete installation.
3. On restart: check about:support to see if e10s is off (it should be).
4. Uninstall the add-on.
4.1. Accept the prompt to restart the browser to complete uninstallation.
5. On restart: check about:support to verify that e10s is back on.

[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It only affects start-up.
[String changes made/needed]: n/a

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8885025 [details]
Bug 1372824 - Work around race conditions on startup enabling e10s.

fix e10s pref setting corner cases, beta55+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a05565c8035c

Comment 15

[Bogdan Maris, Desktop Test Engineering]

I tried to verify this bug both on Windows 10 x64 and Ubuntu 16.04 x64. I managed to reproduce it on Firefox-54.0, but I can't run the test on the latest beta because users using Firefox 55 are not getting e10s 100% of the time.

We'll verify this again when beta gets e10s 100% of the time.