1226487 - e10s needs to be allowed on Beta 43

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Vladan Djeric (:vladan)]

Currently e10s is not allowed on Beta, making the browser.tabs.remote.autostart prefs no-ops:

https://hg.mozilla.org/releases/mozilla-beta/annotate/f34e97d70cfe/configure.in#l3405

Can we just change this line to check for release only?

Comment 1

[Jim Mathies [:jimm]]

Passing this off to felipe, he understand what impact this would have.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

We actually don't want to change that line because that will enable all the e10s UI. We just need to change the conditions in nsAppRunner.cpp.

But there are no compile checks to differentiate between Beta and Release. We may just allow release users to toggle the pref, or check based on the channel. I'll take the bug.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: allow-e10s-beta

So this basically removes the E10S_TESTING_ONLY parts of nsAppRunner.cpp.  The nsIObserver that was ifdef'ed is used in the Preferences dialog to display the possible reason e10s was disabled. It won't be used on Beta right now, but eventually I want to show that information in about:support, so that's why I'm leaving it there

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Attachment: e10s-a11ychecks

In order to allow e10s to be used on beta, we need to also enable the code that checks for a11y and disables it.

This code is all duplicated from the E10S_TESTING_UI block above in the same file. Instead of removing the parts that can be removed, I duplicated the parts that I want to keep for simplicity. Everything else not used will be removed in a separate bug for clarity: bug 1227230

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Note: in order to run the e10s beta experiment (bug 1222894), we need to uplift these two patches to Beta

Comment 6

[Vladan Djeric (:vladan)]

Comment on attachment 8690916 [details] [diff] [review]
allow-e10s-beta

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

This looks good to me, but seems like an e10s peer should review this.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8690916 [details] [diff] [review]
allow-e10s-beta

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

Yes, this looks right. Thanks felipe!

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8690928 [details] [diff] [review]
e10s-a11ychecks

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

Thanks for pointing out bug 1227230. It'll be nice to close that one out. :)

::: browser/components/nsBrowserGlue.js
@@ +3219,5 @@
>  };
> +
> +#else // E10S_TESTING_ONLY
> +
> +var E10SAccessibilityCheck = {

Can you add a quick docstring above this object talking about its responsibilities?

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a59d57130e96
https://hg.mozilla.org/integration/fx-team/rev/0ed0b39c3541

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a59d57130e96
https://hg.mozilla.org/mozilla-central/rev/0ed0b39c3541

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8690916 [details] [diff] [review]
allow-e10s-beta

Approval Request Comment
[Feature/regressing bug #]: Needed for e10s experiment on Beta
[User impact if declined]: Can't run e10s experiment (bug 1222894)
[Describe test coverage new/current, TreeHerder]: landed on central
[Risks and why]: this patch allows the e10s pref to take effect on beta
[String/UUID change made/needed]: -

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8690928 [details] [diff] [review]
e10s-a11ychecks

Approval Request Comment
[Feature/regressing bug #]: Needed for e10s experiment on Beta
[User impact if declined]: Can't run e10s experiment (bug 1222894)
[Describe test coverage new/current, TreeHerder]: landed on central
[Risks and why]: this patch enables on beta the same a11y checks that e10s has been running on nightly/aurora. It is required to disable e10s for a11y users.
[String/UUID change made/needed]: -

Comment 13

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

Felipe, have we made a decision on whether to conduct an A/B experiment on e10s on Beta44? I know it was planned for Beta43 but did it really happen. I am just trying to confirm the plan because bug 1222894 mentions Beta43.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Ritu: yeah, the plan is to conduct a short A/B with a small population on Beta 43 (7.5%, which is bug 1222894), and then a larger one on Beta 44 (bug 1225496)

Comment 15

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

Comment on attachment 8690928 [details] [diff] [review]
e10s-a11ychecks

Please uplift to aurora and beta, needed for planned a/b test.

Comment 16

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

Felipe, are we doing a11y checks on nightly? I'm asking because this came up in the channel meeting this morning, as many of the top crashes on nightly are a11y-related.

Comment 17

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

Comment on attachment 8690916 [details] [diff] [review]
allow-e10s-beta

Please uplift both patches on this bug for e10s a/b testing to aurora and beta.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cb3cd0920e76
https://hg.mozilla.org/releases/mozilla-aurora/rev/4a24c951617a

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/20905912e6e0
https://hg.mozilla.org/releases/mozilla-beta/rev/6adfcf56bd53

Comment 20

[:Felipe Gomes (needinfo for replies!)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #16)
> Felipe, are we doing a11y checks on nightly? I'm asking because this came up
> in the channel meeting this morning, as many of the top crashes on nightly
> are a11y-related.

Yeah, there are checks, but on Windows there's a blacklist of a11y clients known to be problematic, and if it doesn't match that blacklist (like remote desktop), e10s is still enabled together with a11y

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Attachment: Don't use the blocklist for the a11y check on beta

Comment 22

[Jim Mathies [:jimm]]

Comment on attachment 8692005 [details] [diff] [review]
Don't use the blocklist for the a11y check on beta

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

::: browser/components/nsBrowserGlue.js
@@ +3396,5 @@
>  
>    observe: function(subject, topic, data) {
>      if (topic == "a11y-init-or-shutdown"
>          && data == "1" &&
> +        Services.appinfo.accessibilityEnabled) {

nit - you don't need this one, you're in the a11y notification observer.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/18346cf05ada

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Attachment: Don't use the blocklist for the a11y check on beta, r=jmathies

Approval Request Comment
[Feature/regressing bug #]: follow-up here with the question raised in comment 16.
[User impact if declined]: The a11y check for e10s would skip some known software that activates a11y but are not really a11y related. For the beta experiment we should really not skip them to avoid related crashes
[Describe test coverage new/current, TreeHerder]: landed on central
[Risks and why]: Given the changes here we'll need to QA the beta experiment well to ensure everything is working as expected (i.e., e10s can be enabled on beta and can be disabled by a11y tools)
[String/UUID change made/needed]: none

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/18346cf05ada

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8692626 [details] [diff] [review]
Don't use the blocklist for the a11y check on beta, r=jmathies

This change has no impact when e10s is disabled (which is how 43 and 44 will ship) but is required for a Telemetry experiment.

Vladan will land this himself after QA has finished verifying that this fix is sufficient for the experiment.

Aurora+ Beta+

Comment 27

[Vladan Djeric (:vladan)]

needinfo for myself to land this after Kamil's testing

Comment 28

[Vladan Djeric (:vladan)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c38e578502c0
https://hg.mozilla.org/releases/mozilla-beta/rev/04f89d19556d

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/c38e578502c0