1264437 - Manage browser.tabs.remote.autostart.2 pref even for disqualified users

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

There's a number of things related to the e10s activation that are tied to the pref being set to true and letting nsAppRunner::BrowserTabsRemoteAutostart to fully run, like for example the E10S_STATUS histogram, and the textual description that shows up in about:support for users who got e10s blocked.

However, with bug 1255013, the blocking is calculated in advance, and the pref is never toggled for those cases. The end result for e10s is the same, however the function short-circuits due to the pref being false, and then the histogram won't contain info about disqualified users.

With this, the "disqualified" cohort should be just a marking to help in the data analysis afterwards, but every user should still get the same test/control treatment.

I don't see a risk with the calculation going wrong because with the refactoring (bug 1225496), the system add-on and nsAppRunner uses the exact same function for it (i.e., nsAppRunner::MultiprocessBlockyPolicy).

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1264437 - Manage browser.tabs.remote.autostart.2 pref even for disqualified users. r=rvitillo

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

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Mossop has reviewed most of this code, but he's not taking review requests right now, so I'm sending it to Roberto. Roberto, this doesn't change anything on the e10sCohort tagging. It's just so that E10S_STATUS can follow it more closely.

Since you're here, I wanted to ask you if you'd like to see a separate "disqualified-test", and "disqualified-control" groups, or if you prefer to keep them together (as it is right now)?  It would be a trivial change to make on this patch to have the two separate groups.

Comment 3

[Roberto Agostino Vitillo (:rvitillo)]

(In reply to :Felipe Gomes (needinfo me!) from comment #2)
> Since you're here, I wanted to ask you if you'd like to see a separate
> "disqualified-test", and "disqualified-control" groups, or if you prefer to
> keep them together (as it is right now)?  It would be a trivial change to
> make on this patch to have the two separate groups.

I would prefer to have separate "disqualified-test", and "disqualified-control" groups.

Comment 4

[Roberto Agostino Vitillo (:rvitillo)]

Comment on attachment 8741126 [details]
MozReview Request: Bug 1264437 - Manage browser.tabs.remote.autostart.2 pref even for disqualified users. r=rvitillo

https://reviewboard.mozilla.org/r/46207/#review42945

::: browser/extensions/e10srollout/bootstrap.js:67
(Diff revision 1)
>    } else if (userOptedIn) {
>      setCohort("optedIn");
> -  } else if (disqualified) {
> -    setCohort("disqualified");
>    } else if (testGroup) {
> -    setCohort("test");
> +    setCohort(disqualified ? "disqualified" : "test");

I would prefer to have separate "disqualified-test", and "disqualified-control" groups.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8741126 [details]
MozReview Request: Bug 1264437 - Manage browser.tabs.remote.autostart.2 pref even for disqualified users. r=rvitillo

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

Comment 6

[Roberto Agostino Vitillo (:rvitillo)]

Comment on attachment 8741126 [details]
MozReview Request: Bug 1264437 - Manage browser.tabs.remote.autostart.2 pref even for disqualified users. r=rvitillo

https://reviewboard.mozilla.org/r/46207/#review43025

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b33bd4e99069

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8741126 [details]
MozReview Request: Bug 1264437 - Manage browser.tabs.remote.autostart.2 pref even for disqualified users. r=rvitillo

Approval Request Comment
[Feature/regressing bug #]: e10s rollout system add-on
[User impact if declined]: this patch improves the details of a telemetry probe and the telemetry environment data that is used in the e10s analysis
[Describe test coverage new/current, TreeHerder]: landed on central
[Risks and why]: limited to the e10s system add-on
[String/UUID change made/needed]: none

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Felipe, have we verified that the telemetry data from Nightly looks good (is being categorized under "disqualified-test" and "disqualified-control" as expected)? Thanks for the due diligence.

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Hi Ritu, this landed after the experiments on current beta have ended, and is intended to improve the data for the next one. This code is not applicable to Nightly/Aurora, so it won't bring in new data until running on Beta.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to :Felipe Gomes (needinfo me!) from comment #11)
> Hi Ritu, this landed after the experiments on current beta have ended, and
> is intended to improve the data for the next one. This code is not
> applicable to Nightly/Aurora, so it won't bring in new data until running on
> Beta.

Thanks Felipe! I believe if we wanted to we could run this on Nightly (even though we haven't done it in the past) for the sake of verification. I am a bit worried that we have this system add-on and any changes to it can only be verified on Beta channel which increases the risk to Beta experiment. Is there a reason why we have chosen not to set things up in a manner that changes to this system add-on on get verified on Nightly/Aurora channel before being uplifted to Beta channel? Just curious.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

The main reason is that the main purpose of this add-on is to split the users into test and control groups, however we have been running e10s on Nightly/Aurora as 100% of the users for quite a while, and given the small amount of users there I don't think we should remove some of them from e10s.

After we're done with the e10s m9s I'll see if I can come up with some test suite to test this system add-on. It's a bit tricky because it involves restarting the browser, etc., but should be doable. In the meantime, I don't expect any more changes to the system add-on, and every change has been manually tested, so I believe this one should be safe to take.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to :Felipe Gomes (needinfo me!) from comment #13)
> The main reason is that the main purpose of this add-on is to split the
> users into test and control groups, however we have been running e10s on
> Nightly/Aurora as 100% of the users for quite a while, and given the small
> amount of users there I don't think we should remove some of them from e10s.
> 
> After we're done with the e10s m9s I'll see if I can come up with some test
> suite to test this system add-on. It's a bit tricky because it involves
> restarting the browser, etc., but should be doable. In the meantime, I don't
> expect any more changes to the system add-on, and every change has been
> manually tested, so I believe this one should be safe to take.

Thanks Felipe! I appreciate your due diligence here.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8741126 [details]
MozReview Request: Bug 1264437 - Manage browser.tabs.remote.autostart.2 pref even for disqualified users. r=rvitillo

Improves e10s telemetry data quality, Aurora47+

Comment 16

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/90fef35d3694