Closed
Bug 1372824
Opened 7 years ago
Closed 7 years ago
e10s not enabled the first time on new profiles
Categories
(Toolkit :: Startup and Profile System, defect)
Tracking
()
RESOLVED
FIXED
mozilla56
People
(Reporter: glandium, Assigned: mrbkap)
References
Details
(Keywords: multiprocess)
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
Felipe
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
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.
Reporter | ||
Comment 1•7 years ago
|
||
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.
Reporter | ||
Updated•7 years ago
|
Summary: e10s not enabled for me on 54 → e10s not enabled the first time on new installs
Version: 43 Branch → 54 Branch
Reporter | ||
Updated•7 years ago
|
Summary: e10s not enabled the first time on new installs → e10s not enabled the first time on new profiles
Updated•7 years ago
|
Updated•7 years ago
|
Flags: needinfo?(felipc)
Comment 2•7 years ago
|
||
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..
Flags: needinfo?(felipc)
Assignee | ||
Comment 3•7 years ago
|
||
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
Assignee: nobody → mrbkap
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
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!
Attachment #8885025 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 6•7 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8885025 -
Flags: review+ → review?(felipc)
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8885025 [details] Bug 1372824 - Work around race conditions on startup enabling e10s. https://reviewboard.mozilla.org/r/155854/#review162628
Attachment #8885025 -
Flags: review?(felipc) → review+
Pushed by mrbkap@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c9b3f02b072b Work around race conditions on startup enabling e10s. r=Felipe
Assignee | ||
Comment 10•7 years ago
|
||
[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.
tracking-firefox55:
--- → ?
![]() |
||
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c9b3f02b072b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
status-firefox54:
--- → wontfix
status-firefox55:
--- → affected
status-firefox-esr52:
--- → unaffected
tracking-firefox56:
--- → ?
Assignee | ||
Comment 12•7 years ago
|
||
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
Attachment #8885025 -
Flags: approval-mozilla-beta?
Comment 13•7 years ago
|
||
Comment on attachment 8885025 [details] Bug 1372824 - Work around race conditions on startup enabling e10s. fix e10s pref setting corner cases, beta55+
Attachment #8885025 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•7 years ago
|
Comment 14•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a05565c8035c
Updated•7 years ago
|
Flags: qe-verify+
Comment 15•7 years ago
|
||
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.
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•