Closed Bug 1226487 Opened 6 years ago Closed 6 years ago

e10s needs to be allowed on Beta 43

Categories

(Firefox :: General, defect)

42 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox43 --- fixed
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: vladan, Assigned: Felipe)

References

Details

Attachments

(3 files, 1 obsolete file)

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?
Flags: needinfo?(jmathies)
Passing this off to felipe, he understand what impact this would have.
Flags: needinfo?(jmathies) → needinfo?(felipc)
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.
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Flags: needinfo?(felipc)
Attached patch allow-e10s-betaSplinter Review
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
Attachment #8690916 - Flags: review?(vladan.bugzilla)
Attached patch e10s-a11ychecksSplinter Review
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
Attachment #8690928 - Flags: review?(mconley)
Note: in order to run the e10s beta experiment (bug 1222894), we need to uplift these two patches to Beta
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.
Attachment #8690916 - Flags: review?(vladan.bugzilla)
Comment on attachment 8690916 [details] [diff] [review]
allow-e10s-beta

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

Yes, this looks right. Thanks felipe!
Attachment #8690916 - Flags: review+
Blocks: 1198459
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?
Attachment #8690928 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/a59d57130e96
https://hg.mozilla.org/mozilla-central/rev/0ed0b39c3541
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
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]: -
Attachment #8690916 - Flags: approval-mozilla-beta?
Attachment #8690916 - Flags: approval-mozilla-aurora?
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]: -
Attachment #8690928 - Flags: approval-mozilla-beta?
Attachment #8690928 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(felipc)
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)
Flags: needinfo?(felipc)
Comment on attachment 8690928 [details] [diff] [review]
e10s-a11ychecks

Please uplift to aurora and beta, needed for planned a/b test.
Attachment #8690928 - Flags: approval-mozilla-beta?
Attachment #8690928 - Flags: approval-mozilla-beta+
Attachment #8690928 - Flags: approval-mozilla-aurora?
Attachment #8690928 - Flags: approval-mozilla-aurora+
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.
Flags: needinfo?(felipc)
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.
Attachment #8690916 - Flags: approval-mozilla-beta?
Attachment #8690916 - Flags: approval-mozilla-beta+
Attachment #8690916 - Flags: approval-mozilla-aurora?
Attachment #8690916 - Flags: approval-mozilla-aurora+
(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
Flags: needinfo?(felipc)
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.
Attachment #8692005 - Flags: review?(jmathies) → review+
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
Attachment #8692005 - Attachment is obsolete: true
Attachment #8692626 - Flags: review+
Attachment #8692626 - Flags: approval-mozilla-beta?
Attachment #8692626 - Flags: approval-mozilla-aurora?
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+
Attachment #8692626 - Flags: approval-mozilla-beta?
Attachment #8692626 - Flags: approval-mozilla-beta+
Attachment #8692626 - Flags: approval-mozilla-aurora?
Attachment #8692626 - Flags: approval-mozilla-aurora+
needinfo for myself to land this after Kamil's testing
Flags: needinfo?(vladan.bugzilla)
You need to log in before you can comment on or make changes to this bug.