Beta 51 target all add-ons except those marked MPC=False or specifically excluded

RESOLVED FIXED in Firefox 51

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: shell, Assigned: Felipe)

Tracking

Trunk
Firefox 52
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51+ fixed, firefox52 fixed, firefox53 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

3 years ago
update the system add-on for the "go big" test.  Very interested in % of population that qualifies. 

Make e10s available to all add-ons except those marked MPC=False or specifically excluded (same list as in 50 - checking now for additional specific exclusions).

This will test how well the shims cover the compatibility gap - and how large it still is.  From Soft Vision work we have a good idea that we'll be safe on featured add-ons with over 75,000 users.  Theory is add-ons with fewer users tend to be less complex.

Expect to get more insight into unlisted add-ons 

Will file a related metrics bug - to see if there are other items we need to be ready for.

Asking in email if we need to throttle the experiment - since it could be as many as 40% (so instead of 160,000 in experiment and control - it could be 1M)
Comment hidden (mozreview-request)

Comment 2

3 years ago
mozreview-review
Comment on attachment 8809524 [details]
Bug 1314429 - Allow every add-on to run on e10s, except those explictly marked with multiprocessCompatible=false.

https://reviewboard.mozilla.org/r/92086/#review92356

Thanks!

Do you know if the Add-ons team is watching the spinner probe for regressions along with things like crashy-ness and other performance metrics?
Attachment #8809524 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) (PTO - Nov 11 - Nov 14) from comment #2)
> Do you know if the Add-ons team is watching the spinner probe for
> regressions along with things like crashy-ness and other performance metrics?

I don't know. I'll leave that question for shell or dzeber
Flags: needinfo?(sescalante)

Comment 4

3 years ago
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/faf82a6f0dd4
Allow every add-on to run on e10s, except those explictly marked with multiprocessCompatible=false. r=mconley
Flags: needinfo?(dzeber)

Comment 5

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/faf82a6f0dd4
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
(In reply to Mike Conley (:mconley) (PTO - Nov 11 - Nov 14) from comment #2)
> Do you know if the Add-ons team is watching the spinner probe for
> regressions along with things like crashy-ness and other performance metrics?

As far as I know, we're not, but Shell may have more info.
Flags: needinfo?(dzeber)
Reporter

Comment 7

3 years ago
[Tracking Requested - why for this release]:
we are expanding add-ons/e10s experiment in Beta for all MPC=False.  This is important for e10s
Flags: needinfo?(sescalante)
Reporter

Comment 8

3 years ago
sorry - saved before i was done.

we are going to use this experiment to limit audience some when we go live in 51.  If there are no issues - we will limit expansion to add-ons that were newly qualified with Beta users (in addition to the existing MPC=true and webextension users).

beta add-on use data gives us the ability for a middle ground step before going to all not marked MPC=false.  this is the safest way to expand with the least change required to the system add-on.
So, this patch didn't work, and there's a problem: turns out that when we read the data from install.rdf, the information about whether false was explicitly set or not is lost, and we only store a true or false about multiprocessCompatibility. See [1]

[1] http://searchfox.org/mozilla-central/rev/904bf9addd03b03d4cad11b82f19f43d875b7f27/toolkit/mozapps/extensions/internal/XPIProvider.jsm#1198


rhelmer or andym, do you know of any way that I can trigger a re-read of this? Or does one by any chance already happen automatically at, say, Firefox upgrade time?
Flags: needinfo?(rhelmer)
Flags: needinfo?(amckay)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Discussed in IRC - Felipe and I discussed storing whether the multiprocessCompatible property is explicitly set in a new property in the addons DB, and bumping the schema version.

The bump to the schema version should cause all manifests to be re-read when Firefox upgrades, and the addons DB to be rebuilt and populate this new property.
Flags: needinfo?(rhelmer)
Flags: needinfo?(amckay)
Posted patch WIP, doesn't seem to work (obsolete) — Splinter Review
Trying to get the addons manifest to be re-read to include the necessary information, but bumping the DB Schema doesn't appear to trigger it
Reporter

Comment 12

3 years ago
so comment 11 we'll need to keep looking for solution for release - but for Beta 51...

Jorge - can you pull a list of "listed" and "unlisted" add-ons marked mpc=false?  we'd need ID and name (name is for human readability only).  

The reason: see comment 9
  
The action for Beta: we can do a static blocklist for MPC=False for the Beta 51 experiment (testing allowing e10s to go to all but those marked MPC=False).  For beta i'd rather be overly blocking, than tackle the issue of versions - since versioning on the blocklist could get out of date.

In the Beta experiment anyone that had ever marked themselves as MPC=False would stay that way. will start  email with rhelmer, jorge, felipe for Release ideas, if other solution can't be worked out (comment 10 and comment 11).
Flags: needinfo?(jorge)
Reporter

Updated

3 years ago
Flags: needinfo?(sescalante)
Depends on: 1272446
Hi Gerry, just FYI this is a system add-on update planned for 51. This came up in the GoFaster meeting today.
Flags: needinfo?(gchang)
Track 51+ as we will make e10s available to all add-ons except those marked MPC=False or specifically excluded in 51.
Flags: needinfo?(gchang)
I'll send you the list. FTR, there are 43 add-ons with MPC=false, about half of them listed.
Flags: needinfo?(jorge)
This works now! It depends on bug 1272446
Attachment #8813301 - Attachment is obsolete: true
Attachment #8814174 - Flags: review?(rhelmer)
Attachment #8814174 - Flags: review?(kmaglione+bmo)
Comment on attachment 8814174 [details] [diff] [review]
Add new mpcOptedOut field and bump schema version to re-read it

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

::: toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm
@@ +222,5 @@
>      }
>  
>      let policy = RolloutPolicy[policyId];
>  
> +    dump("\n\nFLEIPE----\n");

Remember to remove this
Attachment #8814174 - Flags: review?(rhelmer) → review+
Attachment #8814174 - Flags: review?(kmaglione+bmo)

Comment 18

3 years ago
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7bf172373ff
Bump XPIProvider's schema version to re-read info about multiprocessCompatible. Store information about whether that is false in a new field in the DB to avoid conflicts with any existing code. r=rhelmer

Comment 19

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f7bf172373ff
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Comment on attachment 8814174 [details] [diff] [review]
Add new mpcOptedOut field and bump schema version to re-read it

Approval Request Comment
[Feature/Bug causing the regression]: new e10s-addons experiment for beta 51 where every addon (except those marked as mpc=false) will get activated
[User impact if declined]: won't run the experiment on beta so we can't roll out  e10s for more users on release
[Is this code covered by automated tests?]: yes, by the next patch
[Has the fix been verified in Nightly?]: no, this code is only active on beta
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: the other patch here and bug 1272446 are required to land together
[Is the change risky?]: somewhat
[Why is the change risky/not risky?]: the code change itself is not risky, but this will enable e10s for many addon users. There will still be a 50/50 control/test group which will be used for comparisons.
[String changes made/needed]: none
Attachment #8814174 - Flags: approval-mozilla-beta?
Attachment #8814174 - Flags: approval-mozilla-aurora?
Comment on attachment 8809524 [details]
Bug 1314429 - Allow every add-on to run on e10s, except those explictly marked with multiprocessCompatible=false.

Approval Request Comment
Same as above, the two patches go together. That one contains the major code changes, and this one just flips the toggle to activate it
Attachment #8809524 - Flags: approval-mozilla-beta?
Attachment #8809524 - Flags: approval-mozilla-aurora?
Comment on attachment 8809524 [details]
Bug 1314429 - Allow every add-on to run on e10s, except those explictly marked with multiprocessCompatible=false.

For e10s add-on rollout on Beta51. Beta51+ and Aurora52+. Should be in 51 beta 6.
Attachment #8809524 - Flags: approval-mozilla-beta?
Attachment #8809524 - Flags: approval-mozilla-beta+
Attachment #8809524 - Flags: approval-mozilla-aurora?
Attachment #8809524 - Flags: approval-mozilla-aurora+
Attachment #8814174 - Flags: approval-mozilla-beta?
Attachment #8814174 - Flags: approval-mozilla-beta+
Attachment #8814174 - Flags: approval-mozilla-aurora?
Attachment #8814174 - Flags: approval-mozilla-aurora+
This needs rebasing for Beta. Maybe just a roll-up patch at this point?
Flags: needinfo?(felipc)
Reporter

Updated

3 years ago
Flags: needinfo?(sescalante)
You need to log in before you can comment on or make changes to this bug.