1222894 - [meta] Run an e10s A/B experiment on Beta 43

Status: RESOLVED WONTFIX

(Firefox :: General, defect)

Description

[Vladan Djeric (:vladan)]

We're not doing an opt-in on Beta 43 anymore (bug 1218475) and we have Nick's approval to do an A/B experiment on Beta.

This will be the same experiment code as bug 1193089, but we should wait for a few issues to be fixed and uplifted (see blockers)

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking this so I know what's up for 43.

Comment 2

[Vladan Djeric (:vladan)]

The current plan is to run this experiment in beta 4 of Beta 43.
Beta 4 will be built on Monday, Nov 16th and it will be released on Wednesday, Nov 18th.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Noting that there are a few patches about to uplift and land on beta 4, bug 1213780, bug 1211411, bug 1215540, and bug 1219751.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Also, usually we release beta on Tuesday. As far as I know, beta 4 should release on Tuesday Nov. 17th.  The timing depends on QE signoff and whether we run into problems.

Comment 5

[Vladan Djeric (:vladan)]

Attachment: e10s experiment on beta

This is the updated TelemetryExperiment for Beta 43

Notes:
- Experiment start date is Nov 18 noon EST, end date is Dec 18 noon EST, max duration is still 10 days
- Sampling is set to 15% of Beta population
- There is a concurrent search experiment running on 10% of Beta population
- I'll also post an interdiff against the previous experiment patch

Questions:
- How do I restrict this experiment to beta4 and higher? manifest.json's version field doesn't like the b4 suffix. Is there a buildID field?

Comment 6

[Vladan Djeric (:vladan)]

Attachment: interdiff patch

Comment 7

[Roberto Agostino Vitillo (:rvitillo)]

(In reply to Vladan Djeric (:vladan) -- please needinfo! PTO Nov 16/19/20 from comment #5)

> Questions:
> - How do I restrict this experiment to beta4 and higher? manifest.json's
> version field doesn't like the b4 suffix. Is there a buildID field?

Have a look at https://dxr.mozilla.org/mozilla-central/source/browser/experiments/docs/manifest.rst#163

Comment 8

[Roberto Agostino Vitillo (:rvitillo)]

Comment on attachment 8689913 [details] [diff] [review]
e10s experiment on beta

Review of attachment 8689913 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 9

[Robert Kaiser]

(In reply to Vladan Djeric (:vladan) -- please needinfo! PTO Nov 16/19/20 from comment #5)
> Created attachment 8689913 [details] [diff] [review]
> e10s experiment on beta
> 
> This is the updated TelemetryExperiment for Beta 43
> 
> Notes:
> - Experiment start date is Nov 18 noon EST, end date is Dec 18 noon EST, max
> duration is still 10 days


Makes me sad that it will be active while we need to qualify beta as being ready for release. I probably will have to guess if we are fine to release as I do not have the tooling right now to qualify without counting those e10s users in the experiment and there is 0 hours time left for me to figure something out this year.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8689913 [details] [diff] [review]
e10s experiment on beta

Review of attachment 8689913 [details] [diff] [review]:
-----------------------------------------------------------------

The backupAndSetPref/restorePref functions (used for POPUP_PREF/DISPLAY_PREF) will not be used in beta, so I'd prefer to have these parts removed from this experiment.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

This experiment hasn't started yet, and there is still one more thing to uplift for Thursday's beta 7 desktop build. So we can aim for after that and see what data we can gather.  

For measuring the non-e10s crash rate for 43, I'm hoping we can figure that out from crash-stats searches but I realize KaiRo's usual tools may not be prepared yet to handle two streams in beta. KaiRo is there anything we can do in support?

Comment 12

[Robert Kaiser]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11)
> For measuring the non-e10s crash rate for 43, I'm hoping we can figure that
> out from crash-stats searches but I realize KaiRo's usual tools may not be
> prepared yet to handle two streams in beta. KaiRo is there anything we can
> do in support?

With the tools I have in my hands right now, what we can provide is searches for crashes where e10s is vs. search for crashes where it's off. That's all I have. Crash rates we cannot measure with the tooling I have at this time. We can do that via Telemetry, but I will only get my first introduction into how to get any data from there at Mozlando, so I don't expect me to be of any help with that before Q1. (In fact, I know that running any experiment like this will mess up the only crash rate data I have for reporting right now and I will need to make educated guesses on beta health of the release until I have both better tooling based on Telemetry and historic comparative data of those metrics over some time.)

So, someone well-versed with Telemetry will need to provide comparative crash rate data between the experiment buckets. I can help with search queries of submitted crashes where needed.

Comment 13

[Jim Mathies [:jimm]]

Roberto, are we filtering a11y users out of this set at all? I don't think we track a11y across sessions so it's probably not possible.

Comment 14

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #13)
> Roberto, are we filtering a11y users out of this set at all? I don't think
> we track a11y across sessions so it's probably not possible.

actually, this might be a better question for felipe..

Comment 15

[:Felipe Gomes (needinfo for replies!)]

The experiment code itself does not check for a11y, but we are enabling the e10s a11y checks on beta in bug 1226487 which blocks this one.

But now that you mention, it might be a good idea to check for that in the experiment and not even attempt to include users as part of the test population if they had a11y enabled.

Comment 16

[Jim Mathies [:jimm]]

(In reply to :Felipe Gomes from comment #15)
> The experiment code itself does not check for a11y, but we are enabling the
> e10s a11y checks on beta in bug 1226487 which blocks this one.
> 
> But now that you mention, it might be a good idea to check for that in the
> experiment and not even attempt to include users as part of the test
> population if they had a11y enabled.

I don't think we can check if it was enabled, but maybe we can check if it's currently enabled and if so, skip off those users? I don't see much in the current accessibility prefs to indicate a past run.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

With the change in bug 1226487, it should set browser.tabs.remote.disabled-for-a11y whenever a11y runs, regardless if e10s was running or not. That way we can know the value of that pref in advance (a simpler version of the time-gated alternative that you're implementing)

Comment 18

[Vladan Djeric (:vladan)]

Attachment: beta experiment v2

* Experiment is now set to run from Friday Nov 27th to Monday Dec 7th (roughly 10 days). This means the experiment will stop about a week before beta 43 becomes Release 43 to allow RelMan to assess Beta stability before release.
* The minimum buildID is beta7 or later. We need to use unofficially builds to test (see next comment)
* I removed the POPUP_PREF/DISPLAY_PREF prefs that don't exist on Beta
* I removed the checks for changes in default value of browser.tabs.remote.autostart.2 since this pref does not have a default value on Beta
* I added 2 new branches "first-session-[control|experiment]"

Comment 19

[Vladan Djeric (:vladan)]

Does the experiment code need to check that accessibility pref?

Comment 20

[Vladan Djeric (:vladan)]

Let's do some preliminary tests on this experiment tomorrow

A few points:
* There will be some additional cases to check around disabling the experiment when accessibility is in effect.
* We need to test with an unreleased Beta build because a follow-up patch in bug 1226487 hasn't landed on the beta tree yet
* There is an unofficial build ready here https://treeherder.mozilla.org/#/jobs?repo=try&revision=444d206f97bb&selectedJob=14176668, but you'll need to modify the channel to "beta" in defaults\pref\channel-prefs.js of the program (not profile) directory after installing

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8692781 [details] [diff] [review]
beta experiment v2

Review of attachment 8692781 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #19)
> Does the experiment code need to check that accessibility pref?

Yeah, not strictly necessary but it would be nice (and easy). Just check that pref and don't try to enable e10s for these users (you could set a new branch "disabled-because-a11y").

Also, that pref might change on later sessions.  You don't need to do anything about this if you don't want (you'll fall in the browserTabsRemoteAutostart != expected branch, and the branch will be set to force-disabled).

Comment 22

[:Felipe Gomes (needinfo for replies!)]

The extra tests to perform would be:

1:
 - Run a beta7 build before the experiment starts, and use any tool that triggers accessibility (remote desktop for example). Make sure that the disabled-for-a11y pref is set
 - Restart firefox and don't use accessibility again
 - Install experiment
 - Make sure the experiment doesn't try to enable e10s, because a11y was used in this profile before


2:
 - Run a beta7 build, and install experiment before any a11y is used (force the branch to test to make sure e10s is activated)
 - Restart Firefox (so e10s is actually running)
 - Use an accessibility tool
 - Make sure that the popup saying that e10s must be disabled shows up
 - Accept the popup (which will restart Firefox)
 - Make sure that e10s got disabled, and that the experiment got set to force-disabled

Comment 23

[Vladan Djeric (:vladan)]

Attachment: beta experiment v3 patch

Added a check for the browser.tabs.remote.disabled-for-a11y pref, and a new experiment "a11y-disabled" branch

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Attachment: Disable e10s (by setting the disabled-for-a11y pref) manually

So this adds to the experiment code the code that didn't get uplifted to beta 7.

If accessibility is enabled on startup, or if it eventually gets enabled while running, it would set the pref browser.tabs.remote.disabled-for-a11y

On next startup the browser won't use e10s anymore, and the experiment will figure that out and set the branch correctly

(Untested)

Comment 25

[Vladan Djeric (:vladan)]

Comment on attachment 8693102 [details] [diff] [review]
Disable e10s (by setting the disabled-for-a11y pref) manually

Review of attachment 8693102 [details] [diff] [review]:
-----------------------------------------------------------------

::: experiments/e10s-enabled-beta/code/bootstrap.js
@@ +68,5 @@
>          }
>  
> +        if (A11YWatcher.disabledForA11y) {
> +          return;
> +        }

shouldn't we have this check after the switch-statement? i.e. the experiment check this flag on startup when it's also on the first-session-experiment or experiment branch

Comment 26

[:Felipe Gomes (needinfo for replies!)]

That is not really necessary but it avoids a race in A11YWatcher setting the branch (which is async), and we getting a null branch and calling Math.random() to possibly enable e10s for this user.

Comment 27

[:Felipe Gomes (needinfo for replies!)]

Attachment: Merged patch with new xpi

Comment 28

[:Felipe Gomes (needinfo for replies!)]

Attachment: Merged patch, now with new xpi added

Comment 29

[vdjeric]

Attachment: Merged patch, v2

The autostart.2 pref wasn't being reset when A11YWatcher switch the branch to a11y-disabled

Comment 30

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following builds:
- https://archive.mozilla.org/pub/firefox/releases/43.0b7/
- http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-1cd5affd9b77b97f86ecc3125dca7b718d23d8d5/try-win32/

* Note: a11y testing was done on both of the builds listed below. All other testing was done using the currently released 43.0b7 build *

Test Cases Used: https://public.etherpad-mozilla.org/p/e10sTelemetryExperimentBETA
- if someone could take a quick look to make sure nothing obvious has been missed, that would much appreciated :)

Possible Issues:
================

Issue #1:

It looks like the experiment is still being installed even though datareporting.healthreport.service.enabled or datareporting.healthreport.uploadEnabled are set as false. Should the experiment be installed if these two prefs are set as false? The filter function is working correctly with toolkit.telemetry.enabled.

datareporting.healthreport.service.enabled;false <-- experiment is still being installed
datareporting.healthreport.uploadEnabled;false <--- experiment is still being installed
toolkit.telemetry.enabled;false <-- PASSED (jsfilter-false)

Issue #2:

When using the try-build, the "would you like to disable e10s" doorhanger only appears when the user is in the "experiment" branch and not the "control" branch (which makes sense). If you use an accessible tool while you're on the "control" branch, the behaviour is exactly the same as if you were on the "experiment" branch but without the doorhanger. Just making sure that the "control" branch shouldn't be receiving a doorhanger as well. I'm guessing no because e10s isn't enabled so it's pointless but I would rather make sure then guess :)

Other worthwhile mentions:

- the "would you like to disable e10s" doorhanger only appears under the try-build and not the released 43.0b7 build

Comment 31

[Kamil Jozwiak [:kjozwiak]]

Forgot to mention this in the earlier comment :/

When you select "Don't Disable" under the "Disable e10s" doorhanger via the try-build, after restarting the browser you'll get the following:

- browser.tabs.remote.autostart.2;true will be removed
- browser.tabs.remote.disabled-for-a11y;false
- Services.appinfo.browserTabsRemoteAutostart -> false (browser console)

Cu.import("resource:///modules/experiments/Experiments.jsm");
Experiments.instance()._getActiveExperiment().branch;
"a11y-disabled"

Is this expected? I assumed "Don't Disable" wouldn't disable e10s?

Comment 32

[Jonathan Howard]

It is correct in that neither control nor experiment should be running a11y and e10s isn't officially available outside the experiment. ...autostart.2=true is used as indication that e10s is on by default.
bug 1198459 is likely to make changes for 44.

My expectation is the vast majority a11y users will go straight onto the a11y-disabled branch.

per comment 11/12: manual crash-stats search lets you pick the experiment and branch. The results are slightly poisoned due to the branch being set before a restart.

Comment 33

[Vladan Djeric (:vladan)]

Attachment: experiment.xpi

Jake: can you sign this experiment XPI and attach the signed version to this bug?

Comment 34

[:Felipe Gomes (needinfo for replies!)]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #30)
> Issue #1:
> 
> It looks like the experiment is still being installed even though
> datareporting.healthreport.service.enabled or
> datareporting.healthreport.uploadEnabled are set as false. Should the
> experiment be installed if these two prefs are set as false? The filter
> function is working correctly with toolkit.telemetry.enabled.
> 
> datareporting.healthreport.service.enabled;false <-- experiment is still
> being installed
> datareporting.healthreport.uploadEnabled;false <--- experiment is still
> being installed
> toolkit.telemetry.enabled;false <-- PASSED (jsfilter-false)

Yeah, this is fine. There's no way for us to work around that. We'll depend on a general fix in bug 1217282.

This is not really bad because 1) people would likely have both disabled, 2) if those prefs are false, no data from the user is sent anyway. They will just get a brief period of e10s..


> 
> Issue #2:
> 
> When using the try-build, the "would you like to disable e10s" doorhanger
> only appears when the user is in the "experiment" branch and not the
> "control" branch (which makes sense). If you use an accessible tool while
> you're on the "control" branch, the behaviour is exactly the same as if you
> were on the "experiment" branch but without the doorhanger. Just making sure
> that the "control" branch shouldn't be receiving a doorhanger as well. I'm
> guessing no because e10s isn't enabled so it's pointless but I would rather
> make sure then guess :)

correct

> 
> Other worthwhile mentions:
> 
> - the "would you like to disable e10s" doorhanger only appears under the
> try-build and not the released 43.0b7 build

correct


(In reply to Kamil Jozwiak [:kjozwiak] from comment #31)
> Forgot to mention this in the earlier comment :/
> 
> When you select "Don't Disable" under the "Disable e10s" doorhanger via the
> try-build, after restarting the browser you'll get the following:
> 
> - browser.tabs.remote.autostart.2;true will be removed
> - browser.tabs.remote.disabled-for-a11y;false
> - Services.appinfo.browserTabsRemoteAutostart -> false (browser console)
> 
> Cu.import("resource:///modules/experiments/Experiments.jsm");
> Experiments.instance()._getActiveExperiment().branch;
> "a11y-disabled"
> 
> Is this expected? I assumed "Don't Disable" wouldn't disable e10s?

This is a bit unfortunate, but we can live with that. The experiment itself forces-disable e10s if accessibility was used, which will override the doorhanger decision.

Comment 35

[Jason Thomas [:jason]]

Attachment: experiment.xpi

Signed experiment.xpi.

Comment 36

[Vladan Djeric (:vladan)]

The e10s A/B experiment is no longer shipping on Beta 43. We ran into delays with Beta not being ready for e10s (bug 1226487) and there is a consensus that the current Beta 8 is too late to ship the experiment, since uplift is less than 2 weeks away.

Instead, we'll be running the Beta experiment in 2 weeks on Beta 44 beta1. The tracking bug is bug 1229104.

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

Untracking.