Split the ::BrowserTabsRemoteAutostart() function into two parts, to allow for the blocking policies to be checked independently from the prefs checks

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

46 Branch
Firefox 48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed, firefox47 fixed, firefox48 fixed)

Details

Attachments

(2 attachments)

In order to get statistically significant data for the e10s cohorts, we need to first check if there are any e10s blocking conditions for a user (e.g., add-ons, accessibility), and then split the remaining users 50%/50% and setting the "browser.tabs.remote.autostart" pref to true to the test group.

However, the function that does the blocking checks is the same that checks prefs, and it doesn't allow the former to happen indenpendently of the latter.

This leads us to need to use workarounds and duplicate the logic. But I want to fix this the proper way while there is still time. The fix is simple, just split the function into two.
Version: 45 Branch → 46 Branch
The behavior of ::BrowserTabsRemoteAutostart() shouldn't change, meaning that every result remains the same. The new getter can be accessed by JS through Services.appinfo.multiprocessBlockPolicy

Review commit: https://reviewboard.mozilla.org/r/40499/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40499/
Attachment #8731329 - Flags: review?(jmathies)
The diff looks kinda messy but it's basically code moves
Attachment #8731330 - Flags: review?(jmathies) → review+
Comment on attachment 8731330 [details]
MozReview Request: Bug 1257242 - Improve RTL checks to actually check for RTL instead of using a hardcoded list. r=jimm

https://reviewboard.mozilla.org/r/40501/#review37027
Attachment #8731329 - Flags: review?(jmathies) → review+
Comment on attachment 8731329 [details]
MozReview Request: Bug 1257242 - Split the ::BrowserTabsRemoteAutostart() function into two parts, to allow for the blocking policies to be checked independently from the prefs checks. r=jimm

https://reviewboard.mozilla.org/r/40499/#review37025

::: toolkit/xre/nsAppRunner.h:126
(Diff revision 1)
>  void SetupErrorHandling(const char* progname);
>  
> +/**
> + * A numeric value indicating whether multiprocess might be blocked.
> + * Possible values can be found at nsAppRunner.cpp. A value of 0
> + * represents not blocking.  

nit - white space
Blocks: 1257265
https://hg.mozilla.org/mozilla-central/rev/0278f6e0475c
https://hg.mozilla.org/mozilla-central/rev/1de5e65c1fff
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment on attachment 8731329 [details]
MozReview Request: Bug 1257242 - Split the ::BrowserTabsRemoteAutostart() function into two parts, to allow for the blocking policies to be checked independently from the prefs checks. r=jimm

Approval Request Comment
[Feature/regressing bug #]: Code needed to properly tag "disqualified" cohorts in the system add-on, as requested in bug 1251259 comment 13. This is really important to get clear data out of the test/control groups.
[User impact if declined]: needed to test e10s on beta46 as planned
[Describe test coverage new/current, TreeHerder]: landed on central, QA'ed by Softvision
[Risks and why]: the risk is contained in it not working properly and thus not yielding the results for this experiment phase. But that's what we want to test.
[String/UUID change made/needed]: none
Attachment #8731329 - Flags: approval-mozilla-beta?
Attachment #8731329 - Flags: approval-mozilla-aurora?
Comment on attachment 8731330 [details]
MozReview Request: Bug 1257242 - Improve RTL checks to actually check for RTL instead of using a hardcoded list. r=jimm

Approval Request Comment
[Feature/regressing bug #]: a straightforward improvement that actually simplifies things in this code and improves correctness. Since I was touch this code in the other patch I decided to do it here.
[User impact if declined]: Not strictly necessary but very simple and unlikely to be harmful. It would be better to have this uplifted so that all branches are using the same code.
[Describe test coverage new/current, TreeHerder]: landed on central, QA'ed by softvision
[Risks and why]: restricted to the "block e10s for RTL locales" feature
[String/UUID change made/needed]: none
Attachment #8731330 - Flags: approval-mozilla-beta?
Attachment #8731330 - Flags: approval-mozilla-aurora?
Comment on attachment 8731330 [details]
MozReview Request: Bug 1257242 - Improve RTL checks to actually check for RTL instead of using a hardcoded list. r=jimm

This should land in time for beta 4 build today.
Attachment #8731330 - Flags: approval-mozilla-beta?
Attachment #8731330 - Flags: approval-mozilla-beta+
Attachment #8731330 - Flags: approval-mozilla-aurora?
Attachment #8731330 - Flags: approval-mozilla-aurora+
Hi Felipe, has SV already done the verification? I don't see any QA footprint on the bug and therefore wondering. Thanks!
Flags: needinfo?(felipc)
Yeah, it was done on a spreadsheet (https://docs.google.com/spreadsheets/d/1odM2iPc3yYQLT-LeNI3NBGV_8xS5GBxFpfAhHC0QH9o/edit#gid=280492042), as it involved a couple of different bugs together (tracked at bug 1257265)

SV sent an e-mail to release-drivers saying that the QA was green and all testcases passed.
Flags: needinfo?(felipc)
(In reply to :Felipe Gomes (needinfo me!) from comment #13)
> Yeah, it was done on a spreadsheet
> (https://docs.google.com/spreadsheets/d/1odM2iPc3yYQLT-
> LeNI3NBGV_8xS5GBxFpfAhHC0QH9o/edit#gid=280492042), as it involved a couple
> of different bugs together (tracked at bug 1257265)
> 
> SV sent an e-mail to release-drivers saying that the QA was green and all
> testcases passed.

Awesome! Thank you for sharing that link.
Attachment #8731329 - Flags: approval-mozilla-beta?
Attachment #8731329 - Flags: approval-mozilla-beta+
Attachment #8731329 - Flags: approval-mozilla-aurora?
Attachment #8731329 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.