Closed Bug 1333423 Opened 3 years ago Closed 3 years ago

multiProcessCompatible set to false should override E10SAddonsRollout

Categories

(Firefox :: Extension Compatibility, defect, critical)

51 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox51 + verified
firefox52 --- verified
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(1 file, 2 obsolete files)

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).
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.
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
Attached patch One more trySplinter Review
Accidentally had extra stuff in the patch.
Attachment #8829964 - Attachment is obsolete: true
Attachment #8829964 - Flags: review?(felipc)
Attachment #8829968 - Flags: review?(felipc)
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+
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.
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.
[Tracking Requested - why for this release]: OK, asking for tracking then.

This patch fixes Amazon which is a major add-on (and partner).
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!
Flags: needinfo?(mozilla)
Flags: needinfo?(lhenry)
Flags: needinfo?(gchang)
Flags: needinfo?(felipc)
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?
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)
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/mozilla-central/rev/b0c8dd7ea006
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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.
Flags: needinfo?(gchang)
Track 51+ as this patch fixes a major addon.
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)
Hi
this add-ons ( x-notifier v 4.02 ) for yahoo accounts not works correctly .
please correct it .
thanks
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.