multiProcessCompatible set to false should override E10SAddonsRollout

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Extension Compatibility
--
critical
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: mkaply, Assigned: mkaply)

Tracking

51 Branch
Firefox 54
Points:
---

Firefox Tracking Flags

(firefox51+ verified, firefox52 verified, firefox53 fixed, firefox54 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 months ago
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).
(Assignee)

Comment 1

4 months ago
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.
Attachment #8829887 - Flags: review?(mconley)
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
Attachment #8829887 - Flags: review?(mconley) → review-
(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.
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.
(Assignee)

Comment 5

4 months ago
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.
Assignee: nobody → mozilla
Attachment #8829887 - Attachment is obsolete: true
Attachment #8829964 - Flags: review?(felipc)
Duplicate of this bug: 1333510
(Assignee)

Comment 7

4 months ago
Created attachment 8829968 [details] [diff] [review]
One more try

Accidentally had extra stuff in the patch.
Attachment #8829964 - Attachment is obsolete: true
Attachment #8829964 - Flags: review?(felipc)
Attachment #8829968 - Flags: review?(felipc)

Comment 8

4 months ago
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.
Attachment #8829968 - Flags: review?(felipc) → review+

Comment 9

4 months ago
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)
(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
(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

4 months ago
ignore comment 9 - this fix would fix amazon since it's marked MPC=False.  no other changes needed
We might be rebuilding 51 today as I had to turn off updates for bug 1333516.
(Assignee)

Comment 14

4 months ago
[Tracking Requested - why for this release]: OK, asking for tracking then.

This patch fixes Amazon which is a major add-on (and partner).
tracking-firefox51: --- → ?

Comment 15

4 months ago
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!
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
Flags: needinfo?(mozilla)
Flags: needinfo?(lhenry)
Flags: needinfo?(gchang)
Flags: needinfo?(felipc)
(Assignee)

Comment 16

4 months ago
str
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
Attachment #8829968 - Flags: approval-mozilla-release?
Attachment #8829968 - Flags: approval-mozilla-beta?
Attachment #8829968 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 17

4 months ago
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.
Flags: needinfo?(mozilla)
(Assignee)

Comment 18

4 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0c8dd7ea0067b9b63359c9e61fc1dd675807457
Bug 1333423 - multiProcessCompatible=false should always be honored. r=felipe
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.
Flags: needinfo?(lhenry)
Attachment #8829968 - Flags: approval-mozilla-release?
Attachment #8829968 - Flags: approval-mozilla-release+
Attachment #8829968 - Flags: approval-mozilla-beta?
Attachment #8829968 - Flags: approval-mozilla-beta+
Attachment #8829968 - Flags: approval-mozilla-aurora?
Attachment #8829968 - Flags: approval-mozilla-aurora+
Flags: needinfo?(felipc)
https://hg.mozilla.org/releases/mozilla-release/rev/b9774d8f19b2a939fe107a870208f62a18b9f572
status-firefox51: affected → fixed

Comment 21

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b0c8dd7ea006
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54

Comment 22

4 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/9c78f6d58ab9
status-firefox52: affected → fixed
Flags: qe-verify+

Comment 23

4 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/99104f0554c3
status-firefox53: affected → fixed
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.
status-firefox51: fixed → verified

Updated

4 months ago
Flags: needinfo?(gchang)
Track 51+ as this patch fixes a major addon.
tracking-firefox51: ? → +
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?
Flags: needinfo?(mozilla)
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.
Flags: needinfo?(mozilla)

Comment 28

4 months ago
Hi
this add-ons ( x-notifier v 4.02 ) for yahoo accounts not works correctly .
please correct it .
thanks
Based on comment 26 and comment 27, set Verified flag for 52 build.
status-firefox52: fixed → verified
Removing the qe-verify flag since this bug has been confirmed fixed on Fx52, which has now been released.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.