1406212 - Remove the e10srollout addon

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Blake Kaplan (:mrbkap) (inactive)]

The e10srollout addon allowed us to closely control who got e10s and multi. It also allowed us to force certain people to not get e10s in order to do A/B-style control tests. As of now, we are at 100% on all branches, except for 10% of users on Beta who are forced to not have e10s. Because we're shipping e10s-a11y (yay!), we no longer need to keep those users as a control group and can go to 100%/100% for e10s/multi.

This has some implications for telemetry and dashboard measurements:
  * We will no longer be updating the cohort name.
  * I will be removing the extensions.e10sMultiBlockedByAddons (which is what the addon was using to figure out if the user should be in the "webextensions" group).

We can track whether or not multi is enabled by the "e10sMultiProcesses" telemetry key (1 means disabled, any other value would mean enabled). We also have telemetry on each extension and can record how many of them are webextensions.

A notable upside of getting rid of the e10srollout extension is that enabling or disabling multi programmatically will become much simpler. Because we will be defaulting dom.ipc.processCount to 4 everywhere, setting it to 1 will disable multi everywhere. Otherwise, setting dom.ipc.multiOptOut to 1 will continue to disable e10s-multi.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1406212 - Default to e10s on with 4 content processes.

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

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1406212 - Remove the e10srollout extension.

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

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1406212 - Remove the code that handles extensions for e10s{,-multi}.

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

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1406212 - Remove old uses of the now-defunct .web pref.

I'm pretty sure these changes are correct, but please double-check my work
(especially on the test, which becomes much simpler now that there's only one
pref controlling the behavior).

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

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8915772 [details]
Bug 1406212 - Remove old uses of the now-defunct .web pref.

https://reviewboard.mozilla.org/r/186994/#review192112

This is great! Thank you for simplify the logic.

::: browser/components/preferences/in-content/main.js:1238
(Diff revision 1)
>        document.getElementById("browser.preferences.defaultPerformanceSettings.enabled");
>      let performanceSettings = document.getElementById("performanceSettings");
>      let processCountPref = document.getElementById("dom.ipc.processCount");
>      if (defaultPerformancePref.value) {
>        let accelerationPref = document.getElementById("layers.acceleration.disabled");
>        // Unset the value so process count will be decided by e10s rollout.

s/e10s rollout/the platform/

::: browser/components/preferences/in-content/tests/browser_performance_e10srollout.js:37
(Diff revision 1)
>       "pref value should be false after clicking on checkbox");
>    ok(!useRecommendedPerformanceSettings.checked, "checkbox should not be checked after clicking on checkbox");
>  
>    let contentProcessCount = doc.querySelector("#contentProcessCount");
>    is(contentProcessCount.disabled, false, "process count control should be enabled");
> -  is(Services.prefs.getIntPref("dom.ipc.processCount"), DEFAULT_PROCESS_COUNT + 1, "e10s rollout value should be default value");
> +  is(Services.prefs.getIntPref("dom.ipc.processCount"), DEFAULT_PROCESS_COUNT, "e10s rollout value should be default value");

"default pref value should be the current value"

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8915769 [details]
Bug 1406212 - Default to e10s on with 4 content processes.

https://reviewboard.mozilla.org/r/186988/#review192890

We should also remove the Nightly-only checkbox in about:preferences to avoid it getting confused with the pref changes.. I've got a partial patch already in my queue to remove that, so I'll finish it and post to a separate bug to land this together.

::: browser/app/profile/firefox.js:1551
(Diff revision 1)
>  pref("browser.tabs.remote.autostart.1", false);
>  pref("browser.tabs.remote.autostart.2", true);

it could be on this patch or a separate one (if you prefer), but let's also remove the .1 and .2 prefs. There are some references to them on the codebase, mostly related to the test harnesses setting up e10s to be off by default, and toggled on by a command.

::: toolkit/xre/nsAppRunner.cpp:5168
(Diff revision 1)
>    bool prefEnabled = optInPref || trialPref;
>    int status;
>    if (optInPref) {

this part here accumulates the E10S_STATUS telemetry.. let's adjust now to say that browser.tabs.remote.autostart sets as kEnabledByDefault, and comment out at the beginning of this file the kE10sEnabledByUser

::: toolkit/xre/nsAppRunner.cpp
(Diff revision 1)
> -  const char* useDefaultPerformanceSettings =
> -    "browser.preferences.defaultPerformanceSettings.enabled";
> -  bool useDefaultPerformanceSettingsValue =
> -    Preferences::GetBool(useDefaultPerformanceSettings, true);

i'm a bit confused about the dropping support for this pref.. It looks like this would be the only place where it remains used, so.. should that UI also be removed in about:preferences?

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8915770 [details]
Bug 1406212 - Remove the e10srollout extension.

https://reviewboard.mozilla.org/r/186990/#review192898

\o/

(there's also a reference to this system addon to be removed here: http://searchfox.org/mozilla-central/source/testing/talos/talos/xtalos/xperf_whitelist.json#10)

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Ah, let's also remove the E10S_BLOCKED_FROM_RUNNING telemetry probe, and only leave the E10S_STATUS one.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1406212 - Remove browser.tabs.remote.autostart.N.

These are no longer needed.

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

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8915769 [details]
Bug 1406212 - Default to e10s on with 4 content processes.

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

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8915770 [details]
Bug 1406212 - Remove the e10srollout extension.

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

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8915771 [details]
Bug 1406212 - Remove the code that handles extensions for e10s{,-multi}.

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

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8915772 [details]
Bug 1406212 - Remove old uses of the now-defunct .web pref.

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

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917194 [details]
Bug 1406212 - Remove browser.tabs.remote.autostart.N.

https://reviewboard.mozilla.org/r/188208/#review194128

::: browser/components/preferences/in-content/main.js
(Diff revision 1)
> -      let e10sTempPref = document.getElementById("e10sTempPref");
>        let e10sForceEnable = document.getElementById("e10sForceEnable");
>  
> -      let preffedOn = e10sPref.value || e10sTempPref.value || e10sForceEnable.value;
> +      let preffedOn = e10sPref.value || e10sForceEnable.value;

ah, all of this will be gone with bug 1407351

Comment 15

[:Felipe Gomes (needinfo for replies!)]

(In reply to :Felipe Gomes (needinfo me!) from comment #14)
> 
> ah, all of this will be gone with bug 1407351

although I guess that with the changes you've done here, you can land it before and not wait for bug 1407351. Either way works for me.

Comment 16

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8915769 [details]
Bug 1406212 - Default to e10s on with 4 content processes.

https://reviewboard.mozilla.org/r/186988/#review194152

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8915771 [details]
Bug 1406212 - Remove the code that handles extensions for e10s{,-multi}.

https://reviewboard.mozilla.org/r/186992/#review194156

There's also nsAppRunner::MultiprocessBlockPolicy to be removed.. or did I miss it somewhere else?

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1406212 - Remove multiprocessBlockPolicy.

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

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917932 [details]
Bug 1406212 - Remove multiprocessBlockPolicy.

https://reviewboard.mozilla.org/r/188842/#review194164

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b5e78113f06
Default to e10s on with 4 content processes. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/1a050da96e9e
Remove the e10srollout extension. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/5bf2f08f01f9
Remove the code that handles extensions for e10s{,-multi}. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/d9ea9cff849f
Remove old uses of the now-defunct .web pref. r=timdream
https://hg.mozilla.org/integration/autoland/rev/1acc4c270bf9
Remove browser.tabs.remote.autostart.N. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/1c9fe35de901
Remove multiprocessBlockPolicy. r=Felipe

Comment 21

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/c42cf7677816 for mass reftest failure, at least on Win7 debug (we're not very good at actually getting things to run, but that has actually finished), along the lines of https://treeherder.mozilla.org/logviewer.html#?job_id=136678383&repo=autoland but hitting something in pretty much every chunk.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45f85cd249a79dc1919c31a67bfe73c18fe8702b

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7a1aa9aff8ff04f3d8f0526fe7b9ca4421bc66e

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: felipe's patch

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

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1406212 - Remove references to extensions.e10sBlocksEnabling.

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

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1406212 - Fix the reftest harness's logic for enabling or disabling e10s.

Right now, e10s defaults to "on" in Nightly via the default pref
browser.tabs.remote.autostart.2=true. The reftest harness ignores that pref
and attempts to enable it by setting browser.tabs.remote.autostart to true (as
well as disabling the checks for non-mpc-compatible extensions). There's a
bug, however, and we overwrite the value for .autostart by reading
reftest-prefernces.js (which unconditionally sets .autostart to false).

Therefore, the existing code works because it ignores the .autostart.2 pref
and whether we enable e10s is entirely controlled by setting
extensions.e10sBlocksEnabling (the reftest extension is not mpc-compatible).
With this change, we unconditionally set .remote to the value that we want
(and we trust that the only non-mpc-compatible extensions are specially vetted
and part of our build system) so that things work as advertised on the box.

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

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8915769 [details]
Bug 1406212 - Default to e10s on with 4 content processes.

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

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8915770 [details]
Bug 1406212 - Remove the e10srollout extension.

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

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8915771 [details]
Bug 1406212 - Remove the code that handles extensions for e10s{,-multi}.

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

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8915772 [details]
Bug 1406212 - Remove old uses of the now-defunct .web pref.

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

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8917194 [details]
Bug 1406212 - Remove browser.tabs.remote.autostart.N.

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

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8917932 [details]
Bug 1406212 - Remove multiprocessBlockPolicy.

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

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Oops, ignore "felipe's patch" there, it's already pushed (I needed to rebase over it).

Comment 34

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8919088 [details]
Bug 1406212 - Remove references to extensions.e10sBlocksEnabling.

https://reviewboard.mozilla.org/r/190004/#review195164

Comment 35

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8919089 [details]
Bug 1406212 - Fix the reftest harness's logic for enabling or disabling e10s.

https://reviewboard.mozilla.org/r/190006/#review195166

do you need to update talos/mochitest/wpt?

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #35)
> do you need to update talos/mochitest/wpt?

It looks like the rest of the code adds in extra prefs after reading the defaults file [1], so it works but for a different reason.

[1] http://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/testing/mochitest/runtests.py#1795-1797,1818-1821

Comment 37

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8915769 [details]
Bug 1406212 - Default to e10s on with 4 content processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186988/diff/3-4/

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8915770 [details]
Bug 1406212 - Remove the e10srollout extension.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186990/diff/3-4/

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8915771 [details]
Bug 1406212 - Remove the code that handles extensions for e10s{,-multi}.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186992/diff/3-4/

Comment 40

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8915772 [details]
Bug 1406212 - Remove old uses of the now-defunct .web pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186994/diff/3-4/

Comment 41

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8917194 [details]
Bug 1406212 - Remove browser.tabs.remote.autostart.N.

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

Comment 42

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8917932 [details]
Bug 1406212 - Remove multiprocessBlockPolicy.

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

Comment 43

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8919088 [details]
Bug 1406212 - Remove references to extensions.e10sBlocksEnabling.

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

Comment 44

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8919089 [details]
Bug 1406212 - Fix the reftest harness's logic for enabling or disabling e10s.

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

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1238ee9eced8
Default to e10s on with 4 content processes. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/ad317c2cffb4
Remove the e10srollout extension. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/4e602a14b110
Remove the code that handles extensions for e10s{,-multi}. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/e5fac7c4ee65
Remove old uses of the now-defunct .web pref. r=timdream
https://hg.mozilla.org/integration/autoland/rev/30f7a39f1cfb
Remove browser.tabs.remote.autostart.N. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/976cc4e68b45
Remove multiprocessBlockPolicy. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/0d1925ea1d26
Remove references to extensions.e10sBlocksEnabling. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/38758d31bd62
Fix the reftest harness's logic for enabling or disabling e10s. r=jmaher

Comment 46

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/1238ee9eced8
https://hg.mozilla.org/mozilla-central/rev/ad317c2cffb4
https://hg.mozilla.org/mozilla-central/rev/4e602a14b110
https://hg.mozilla.org/mozilla-central/rev/e5fac7c4ee65
https://hg.mozilla.org/mozilla-central/rev/30f7a39f1cfb
https://hg.mozilla.org/mozilla-central/rev/976cc4e68b45
https://hg.mozilla.org/mozilla-central/rev/0d1925ea1d26
https://hg.mozilla.org/mozilla-central/rev/38758d31bd62