Last Comment Bug 1333423 - multiProcessCompatible set to false should override E10SAddonsRollout
: multiProcessCompatible set to false should override E10SAddonsRollout
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Extension Compatibility (show other bugs)
: 51 Branch
: Unspecified Unspecified
-- critical (vote)
: Firefox 54
Assigned To: Mike Kaply [:mkaply]
:
:
Mentors:
: 1333510 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-01-24 07:34 PST by Mike Kaply [:mkaply]
Modified: 2017-02-06 02:33 PST (History)
12 users (show)
cornel.ionce: qe‑verify+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
verified
fixed
fixed


Attachments
Don't check for mpcOptout inside alladdons (887 bytes, patch)
2017-01-24 07:52 PST, Mike Kaply [:mkaply]
felipc: review-
Details | Diff | Splinter Review
Don't check for mpcOptout inside alladdons (4.91 KB, patch)
2017-01-24 11:05 PST, Mike Kaply [:mkaply]
no flags Details | Diff | Splinter Review
One more try (820 bytes, patch)
2017-01-24 11:16 PST, Mike Kaply [:mkaply]
felipc: review+
lhenry: approval‑mozilla‑aurora+
lhenry: approval‑mozilla‑beta+
lhenry: approval‑mozilla‑release+
Details | Diff | Splinter Review

Description User image Mike Kaply [:mkaply] 2017-01-24 07:34:12 PST
If an add-on explicitly sets multiProcessCompatible to false in install.rdf, but they are on the list in E10SAddonsRollout.jsm we should honor the value in install.rdf.

An add-on should be able to choose to not be on the list (for now).
Comment 1 User image Mike Kaply [:mkaply] 2017-01-24 07:52:51 PST
Created attachment 8829887 [details] [diff] [review]
Don't check for mpcOptout inside alladdons

Switching the order so that alladdons means "all addons" and we then explicitly check for mpc=false.

If we believe that alladdons should NOT include mpc=false, we would reverse these two statements.
Comment 2 User image :Felipe Gomes (needinfo me!) 2017-01-24 07:54:32 PST
Comment on attachment 8829887 [details] [diff] [review]
Don't check for mpcOptout inside alladdons

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

(In reply to Mike Kaply [:mkaply] from comment #1)
> Created attachment 8829887 [details] [diff] [review]
> Don't check for mpcOptout inside alladdons
> 
> If we believe that alladdons should NOT include mpc=false, we would reverse
> these two statements.

Yeah we should do this, otherwise the exact same complaint will happen again when we move to alladdons
Comment 3 User image Andreas Wagner [:TheOne] 2017-01-24 08:02:36 PST
(In reply to :Felipe Gomes (needinfo me!) from comment #2)
> (In reply to Mike Kaply [:mkaply] from comment #1)
> > If we believe that alladdons should NOT include mpc=false, we would reverse
> > these two statements.
> 
> Yeah we should do this, otherwise the exact same complaint will happen again
> when we move to alladdons

I guess that depends on what "alladdons" means. Does it really mean "all", or "all that don't have mpc=false".
In other words, are we going to always (until 57) disable e10s for mpc=false add-ons, or will there be a release (before 57) where we enable e10s regardless of the mpc flag in add-ons.
Comment 4 User image :Felipe Gomes (needinfo me!) 2017-01-24 08:15:52 PST
I know, but it's the same argument as now that we are regretting. What does "allow list" mean? "allow", or "allow except if they have mpc=false".

In the end it's giving the addon author the final chance of disabling e10s to making sure the browser is not broken.

Down the road when we decide e10s must be used be everyone, it will probably make more sense to entirely disable the remaining mpc=false addon rather than forcing them on with known bugs.
Comment 5 User image Mike Kaply [:mkaply] 2017-01-24 11:05:48 PST
Created attachment 8829964 [details] [diff] [review]
Don't check for mpcOptout inside alladdons

I'm inclined to agree with felipe, the more I think about it.

An addon developer should always be able to say "This doesn't work with e10s, period."

If we want a way to enable ALL addons no matter what, it should be a different mechanism.
Comment 6 User image :Felipe Gomes (needinfo me!) 2017-01-24 11:12:14 PST
*** Bug 1333510 has been marked as a duplicate of this bug. ***
Comment 7 User image Mike Kaply [:mkaply] 2017-01-24 11:16:47 PST
Created attachment 8829968 [details] [diff] [review]
One more try

Accidentally had extra stuff in the patch.
Comment 8 User image Kev Needham [:kev] 2017-01-24 11:18:43 PST
My understanding is that the scope of the system addon check is limited to versions of Firefox where e10s can be preffed off. My assumption is that with a future release of Firefox e10s will be an integral feature of Firefox, and disabling e10s via preference will no longer be possible. At that point extensions that are known not to be e10s compatible would be disabled as they would be incompatible with that release of Firefox (and future releases).

The scope of this fix is for the opt-in/opt-out process of e10s while it available, and agree that any installed and enabled add-on that is marked as MPC=false should prevent the user from having e10s turned on. Processes outside the system add-on will be used for when we throttle up to e10s as a default Firefox feature.

(In reply to Andreas Wagner [:TheOne] from comment #3)
> 
> > Yeah we should do this, otherwise the exact same complaint will happen again
> > when we move to alladdons
> 
> I guess that depends on what "alladdons" means. Does it really mean "all",
> or "all that don't have mpc=false".
> In other words, are we going to always (until 57) disable e10s for mpc=false
> add-ons, or will there be a release (before 57) where we enable e10s
> regardless of the mpc flag in add-ons.
Comment 9 User image :shell escalante 2017-01-24 11:56:43 PST
sorry for this late addition - but if this is going out in .dot release... 

does it make sense to at the same time remove amazon assistant from "allow list" (and possibly block list).. so will just use MPC=False setting and not rely on system add-on after dot release is pushed?

Amazon Assistant for Firefox	abb-acer@amazon.com	all 10.x is ideal.  
two version showed up in specific 10.161.13.1002 and 10.1612.1.304)
Comment 10 User image Andreas Wagner [:TheOne] 2017-01-24 12:00:19 PST
(In reply to :shell escalante from comment #9)
> sorry for this late addition - but if this is going out in .dot release... 
> 
> does it make sense to at the same time remove amazon assistant from "allow
> list" (and possibly block list).. so will just use MPC=False setting and not
> rely on system add-on after dot release is pushed?

If so, please remove both:

abb@amazon.com
abb-acer@amazon.com
Comment 11 User image :Felipe Gomes (needinfo me!) 2017-01-24 12:29:02 PST
(In reply to :shell escalante from comment #9)
> sorry for this late addition - but if this is going out in .dot release... 
> 
> does it make sense to at the same time remove amazon assistant from "allow
> list" (and possibly block list).. so will just use MPC=False setting and not
> rely on system add-on after dot release is pushed?
> 
> Amazon Assistant for Firefox	abb-acer@amazon.com	all 10.x is ideal.  
> two version showed up in specific 10.161.13.1002 and 10.1612.1.304)

yeah, but let's do that as a separate bug, and mark it as tracking 51. There might be a dot release for it to ride.
Comment 12 User image :shell escalante 2017-01-24 12:30:52 PST
ignore comment 9 - this fix would fix amazon since it's marked MPC=False.  no other changes needed
Comment 13 User image Liz Henry (:lizzard) (needinfo? me) 2017-01-24 12:31:09 PST
We might be rebuilding 51 today as I had to turn off updates for bug 1333516.
Comment 14 User image Mike Kaply [:mkaply] 2017-01-24 12:34:15 PST
[Tracking Requested - why for this release]: OK, asking for tracking then.

This patch fixes Amazon which is a major add-on (and partner).
Comment 15 User image Ritu Kothari (:ritu) 2017-01-24 13:49:51 PST
Hi Mike, Felipe, could you please nominate this patch for uplift to Aurora52, Beta52 and Release51? Can you please comment in the bug whether you have manually verified that this fixes bug 1332643? Thanks!

Liz, Gerry, fyi!
Comment 16 User image Mike Kaply [:mkaply] 2017-01-24 13:53:06 PST
Comment on attachment 8829968 [details] [diff] [review]
One more try

Approval Request Comment
[Feature/Bug causing the regression]: 1314429
[User impact if declined]: Add-ons that are marked not compatible with e10s might still run in e10s (if they are on our list).
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Verified locally
[Needs manual test from QE? If yes, steps to reproduce]: Install Amazon Assistant from AMO. Verify that browser restarts.
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: Very low risk. 
[Why is the change risky/not risky?]: Move an existing code path to be more explicit.
[String changes made/needed]: None
Comment 17 User image Mike Kaply [:mkaply] 2017-01-24 13:54:30 PST
I have manually verified that this fixes bug 1332643. I made sure my browser was e10s, installed from AMO and the browser was restarted into non e10s.
Comment 18 User image Mike Kaply [:mkaply] 2017-01-24 13:57:20 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0c8dd7ea0067b9b63359c9e61fc1dd675807457
Bug 1333423 - multiProcessCompatible=false should always be honored. r=felipe
Comment 19 User image Liz Henry (:lizzard) (needinfo? me) 2017-01-24 14:02:34 PST
Comment on attachment 8829968 [details] [diff] [review]
One more try

Let's take this for the new 51 build, which I hope to start tonight. 
Kwierso note this hasn't landed on m-c yet but we may as well uplift it now anyway.
Comment 21 User image Phil Ringnalda (:philor) 2017-01-24 18:56:39 PST
https://hg.mozilla.org/mozilla-central/rev/b0c8dd7ea006
Comment 22 User image Ryan VanderMeulen [:RyanVM] 2017-01-24 19:35:54 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/9c78f6d58ab9
Comment 23 User image Ryan VanderMeulen [:RyanVM] 2017-01-25 06:25:33 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/99104f0554c3
Comment 24 User image Iulia Cristescu, QA [:IuliaC] 2017-01-25 07:16:27 PST
The issue is verified fixed on 51.0.1-build2 (20170124203848) using Windows 10 x64, Windows 7 x64, Mac OS X 10.11 and Ubuntu 16.04 x64.
Comment 25 User image Gerry Chang [:gchang] 2017-02-01 23:54:56 PST
Track 51+ as this patch fixes a major addon.
Comment 26 User image Iulia Cristescu, QA [:IuliaC] 2017-02-03 07:43:08 PST
Investigated this bug on 52.0b3 build1 (20170202101509), 53.0a2 (2017-02-03) and 54.0a1 (2017-02-03) using Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 16.04 x64. These are the results:
- on the beta 3 build, there are no issues (Amazon Assistant is successfully installed, the restart is required and after that the e10s gets disabled);
- on latest aurora and latest nightly builds Amazon Assistant is installed, but no restart is required;                     e10s remains enabled even after restart.
Any idea about this?
Comment 27 User image :Felipe Gomes (needinfo me!) 2017-02-03 08:50:49 PST
Thanks for looking into that. That's because Nightly and Aurora builds do not follow these e10s add-on rules, and allow any add-on to run with e10s. Those rules only apply to Beta and Release.
Comment 28 User image ssam2050@yahoo.com 2017-02-05 23:19:04 PST
Hi
this add-ons ( x-notifier v 4.02 ) for yahoo accounts not works correctly .
please correct it .
thanks
Comment 29 User image Iulia Cristescu, QA [:IuliaC] 2017-02-06 02:33:22 PST
Based on comment 26 and comment 27, set Verified flag for 52 build.

Note You need to log in before you can comment on or make changes to this bug.