Remove the e10srollout addon

RESOLVED FIXED in Firefox 58

Status

()

enhancement
RESOLVED FIXED
2 years ago
Last month

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

(Depends on 1 bug)

54 Branch
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(8 attachments, 1 obsolete attachment)

59 bytes, text/x-review-board-request
Felipe
: review+
Details
59 bytes, text/x-review-board-request
Felipe
: review+
Details
59 bytes, text/x-review-board-request
Felipe
: review+
Details
59 bytes, text/x-review-board-request
timdream
: review+
Details
59 bytes, text/x-review-board-request
Felipe
: review+
Details
59 bytes, text/x-review-board-request
Felipe
: review+
Details
59 bytes, text/x-review-board-request
Felipe
: review+
Details
59 bytes, text/x-review-board-request
jmaher
: review+
Details
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee: nobody → mrbkap
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"
Attachment #8915772 - Flags: review?(timdream) → review+

Comment 6

2 years ago
mozreview-review
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?
Attachment #8915769 - Flags: review?(felipc)

Comment 7

2 years ago
mozreview-review
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)
Attachment #8915770 - Flags: review?(felipc) → review+
Ah, let's also remove the E10S_BLOCKED_FROM_RUNNING telemetry probe, and only leave the E10S_STATUS one.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

2 years ago
mozreview-review
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
Attachment #8917194 - Flags: review?(felipc) → review+
(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

2 years ago
mozreview-review
Comment on attachment 8915769 [details]
Bug 1406212 - Default to e10s on with 4 content processes.

https://reviewboard.mozilla.org/r/186988/#review194152
Attachment #8915769 - Flags: review?(felipc) → review+

Comment 17

2 years ago
mozreview-review
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?
Attachment #8915771 - Flags: review?(felipc) → review+
Comment hidden (mozreview-request)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8917932 [details]
Bug 1406212 - Remove multiprocessBlockPolicy.

https://reviewboard.mozilla.org/r/188842/#review194164
Attachment #8917932 - Flags: review?(felipc) → review+

Comment 20

2 years ago
Pushed by mrbkap@mozilla.com:
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
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Oops, ignore "felipe's patch" there, it's already pushed (I needed to rebase over it).

Comment 34

2 years ago
mozreview-review
Comment on attachment 8919088 [details]
Bug 1406212 - Remove references to extensions.e10sBlocksEnabling.

https://reviewboard.mozilla.org/r/190004/#review195164
Attachment #8919088 - Flags: review?(felipc) → review+

Comment 35

2 years ago
mozreview-review
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?
Attachment #8919089 - Flags: review?(jmaher) → review+
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8919087 - Attachment is obsolete: true

Comment 45

2 years ago
Pushed by mrbkap@mozilla.com:
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
Depends on: 1414091
You need to log in before you can comment on or make changes to this bug.