1333423 - multiProcessCompatible set to false should override E10SAddonsRollout

Status: RESOLVED FIXED

(Firefox :: Extension Compatibility, defect)

Description

[Mike Kaply [:mkaply]]

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

[Mike Kaply [:mkaply]]

Attachment: 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

[:Felipe Gomes (needinfo for replies!)]

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

[Andreas Wagner [:TheOne] [use NI]]

(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

[:Felipe Gomes (needinfo for replies!)]

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

[Mike Kaply [:mkaply]]

Attachment: 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 7

[Mike Kaply [:mkaply]]

Attachment: One more try

Accidentally had extra stuff in the patch.

Comment 8

[Kev Needham [:kev]]

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

[:shell escalante]

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	[e-mail]	all 10.x is ideal.  
two version showed up in specific 10.161.13.1002 and 10.1612.1.304)

Comment 10

[Andreas Wagner [:TheOne] [use NI]]

(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:

[e-mail]
[e-mail]

Comment 11

[:Felipe Gomes (needinfo for replies!)]

(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	[e-mail]	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

[:shell escalante]

ignore comment 9 - this fix would fix amazon since it's marked MPC=False.  no other changes needed

Comment 13

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

We might be rebuilding 51 today as I had to turn off updates for bug 1333516.

Comment 14

[Mike Kaply [:mkaply]]

[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

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

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

[Mike Kaply [:mkaply]]

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

[Mike Kaply [:mkaply]]

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

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0c8dd7ea0067b9b63359c9e61fc1dd675807457
Bug 1333423 - multiProcessCompatible=false should always be honored. r=felipe

Comment 19

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

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 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-release/rev/b9774d8f19b2a939fe107a870208f62a18b9f572

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b0c8dd7ea006

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9c78f6d58ab9

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/99104f0554c3

Comment 24

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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

[Gerry Chang [:gchang]]

Track 51+ as this patch fixes a major addon.

Comment 26

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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

[:Felipe Gomes (needinfo for replies!)]

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

[[e-mail]]

Hi
this add-ons ( x-notifier v 4.02 ) for yahoo accounts not works correctly .
please correct it .
thanks

Comment 29

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Based on comment 26 and comment 27, set Verified flag for 52 build.

Comment 30

[Andrei Vaida [:avaida]]

Removing the qe-verify flag since this bug has been confirmed fixed on Fx52, which has now been released.