Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Disable e10s-multi for SDK addon users

RESOLVED FIXED in Firefox 54

Status

()

Core
DOM: Content Processes
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: krizsa, Assigned: krizsa)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [e10s-multi:+])

Attachments

(4 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

4 months ago
Whiteboard: [e10s-multi:?]

Updated

4 months ago
Whiteboard: [e10s-multi:?] → [e10s-multi:+]
(Assignee)

Comment 1

4 months ago
We decided to turn off e10s-multi for users with Add-on SDK based Add-ons. My plan is to flips some flags for that somewhere here: http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProviderUtils.js#1361

But I could not find a way to identify SDK based add-ons. The type should be "extension" I assume, maybe I can even check if it's restartles, but that's still not really what I want. Do you guys know how can I identify if an entry in the addonDB refers to an SDK based add-on?
Flags: needinfo?(felipc)
Flags: needinfo?(dtownsend)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #1)
> We decided to turn off e10s-multi for users with Add-on SDK based Add-ons.
> My plan is to flips some flags for that somewhere here:
> http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/XPIProviderUtils.js#1361
> 
> But I could not find a way to identify SDK based add-ons. The type should be
> "extension" I assume, maybe I can even check if it's restartles, but that's
> still not really what I want. Do you guys know how can I identify if an
> entry in the addonDB refers to an SDK based add-on?

There isn't any way. You'd have to check if there is a package.json file in the root of the add-on I guess but even that might not be perfect.
Flags: needinfo?(dtownsend)
One ugly hack would be to check that the id of the addon begins with "jid"
Flags: needinfo?(felipc)
(In reply to :Felipe Gomes (needinfo me!) from comment #3)
> One ugly hack would be to check that the id of the addon begins with "jid"

That's true. I don't think it catches everything but maybe it's enough?
(Assignee)

Comment 5

4 months ago
(In reply to Dave Townsend [:mossop] from comment #2)
> There isn't any way. You'd have to check if there is a package.json file in
> the root of the add-on I guess but even that might not be perfect.

I was afraid that this is the case :(

(In reply to Dave Townsend [:mossop] from comment #4)
> (In reply to :Felipe Gomes (needinfo me!) from comment #3)
> > One ugly hack would be to check that the id of the addon begins with "jid"
> 
> That's true. I don't think it catches everything but maybe it's enough?

Thanks guys, that might help... so that would filter out some random add-ons if there are any who's ID start with "jid" and what SDK add-ons would it miss? I don't know much about this "jid".
> (In reply to Dave Townsend [:mossop] from comment #4)
> > (In reply to :Felipe Gomes (needinfo me!) from comment #3)
> > > One ugly hack would be to check that the id of the addon begins with "jid"
> > 
> > That's true. I don't think it catches everything but maybe it's enough?
> 
> Thanks guys, that might help... so that would filter out some random add-ons
> if there are any who's ID start with "jid" and what SDK add-ons would it
> miss? I don't know much about this "jid".

Unless the developer provided a specific ID for their SDK add-on we autogenerate one for them which begins "jid". I don't have a good sense of what percentage we'd miss but AMO might be able to figure that out.
(Assignee)

Comment 7

4 months ago
(In reply to Dave Townsend [:mossop] from comment #6)
> > (In reply to Dave Townsend [:mossop] from comment #4)
> > > (In reply to :Felipe Gomes (needinfo me!) from comment #3)
> > > > One ugly hack would be to check that the id of the addon begins with "jid"
> > > 
> > > That's true. I don't think it catches everything but maybe it's enough?
> > 
> > Thanks guys, that might help... so that would filter out some random add-ons
> > if there are any who's ID start with "jid" and what SDK add-ons would it
> > miss? I don't know much about this "jid".
> 
> Unless the developer provided a specific ID for their SDK add-on we
> autogenerate one for them which begins "jid". I don't have a good sense of
> what percentage we'd miss but AMO might be able to figure that out.

Shell, turning off e10s-multi for users with SDK add-ons have some technical difficulties. We currently have no way of telling for sure that an installed add-on is SDK based or not. There is a proposal that would work for _most_ of the cases, but we should evaluate first how well would that work.

Could you get in touch with someone from the AMO team to help us figure out how many of the SDK add-ons start their id with "jid" roughly? (or actually what percentage didn't). Many thanks!
Flags: needinfo?(sescalante)

Updated

4 months ago
Iteration: --- → 55.2 - Apr 3
(Assignee)

Comment 8

4 months ago
I think we're going with the more brute version and block multi for all bootstrapped add-ons. If I understand it correctly SDK add-ons are a subgroup of that and I exclude all the others (web extensions, themes, system add-ons).
Flags: needinfo?(sescalante)
(Assignee)

Comment 9

4 months ago
Created attachment 8849556 [details] [diff] [review]
e10sMultiBlockedByAddons platform side. v1

This patch handles the pref from the platform side. If the pref says we should block multi AND the user does not have user pref for the process count we return 1. I would like to have only one place where we check the max process count, so I removed the JS function that does the same.
Assignee: nobody → gkrizsanits
Attachment #8849556 - Flags: review?(mrbkap)
(Assignee)

Comment 10

4 months ago
Created attachment 8849560 [details] [diff] [review]
e10sMultiBlockedByAddons toolkit side. v1

Not sure who to set r? flag on... Please feel free to pass the ball to Mossop if you don't feel comfortable reviewing this, I'm just afraid he might be very busy.

I'm trying to catch all the SDK add-ons (and other bootstrapped add-ons as a collateral damage), and exclude all the system/experiment/hotfix/etc ones.
Attachment #8849560 - Flags: review?(felipc)
Comment on attachment 8849560 [details] [diff] [review]
e10sMultiBlockedByAddons toolkit side. v1

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

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +4542,5 @@
> +    if (locName == KEY_APP_SYSTEM_DEFAULTS ||
> +        locName == KEY_APP_SYSTEM_ADDONS)
> +      return false;
> +
> +    return aAddon.bootstrap;

This will also block webextensions, do you want that?
Comment on attachment 8849556 [details] [diff] [review]
e10sMultiBlockedByAddons platform side. v1

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

::: dom/base/ProcessSelector.js
@@ +55,5 @@
>  
>      let min = Number.MAX_VALUE;
>      let candidate = Ci.nsIContentProcessProvider.NEW_PROCESS;
>  
> +    for (let i = 0; i < aMaxCount; i++) {

Oops, this is a loop over aProcesses, which has valid indices in [0, aCount). We should probably use aCount intead of aMaxCount in the loop condition.
Attachment #8849556 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 13

4 months ago
(In reply to Dave Townsend [:mossop] from comment #11)
> Comment on attachment 8849560 [details] [diff] [review]
> e10sMultiBlockedByAddons toolkit side. v1
> 
> Review of attachment 8849560 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
> @@ +4542,5 @@
> > +    if (locName == KEY_APP_SYSTEM_DEFAULTS ||
> > +        locName == KEY_APP_SYSTEM_ADDONS)
> > +      return false;
> > +
> > +    return aAddon.bootstrap;
> 
> This will also block webextensions, do you want that?

Oh I thought this part:
    if (aAddon.type != "extension")	
      return false;

covers it. As webextensions have their own type no?

http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4498

I should add a test for that too regardless.
(Assignee)

Comment 14

4 months ago
(In reply to Blake Kaplan (:mrbkap) from comment #12)
> Comment on attachment 8849556 [details] [diff] [review]
> e10sMultiBlockedByAddons platform side. v1
> 
> Review of attachment 8849556 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/ProcessSelector.js
> @@ +55,5 @@
> >  
> >      let min = Number.MAX_VALUE;
> >      let candidate = Ci.nsIContentProcessProvider.NEW_PROCESS;
> >  
> > +    for (let i = 0; i < aMaxCount; i++) {
> 
> Oops, this is a loop over aProcesses, which has valid indices in [0,
> aCount). We should probably use aCount intead of aMaxCount in the loop
> condition.

The reason we don't do that is the keepProcessesAlive hack for our tests. If a test sets dom.ipc.contentProcess to 1 let's say, but we keep alive 4 content processes we want this code to always select the first process instead of one from the 4. Once we can get rid of that hack, we can use aCount here. Does that make sense?
(Assignee)

Comment 15

4 months ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)
> > Oops, this is a loop over aProcesses, which has valid indices in [0,
> > aCount). We should probably use aCount intead of aMaxCount in the loop
> > condition.
> 
> The reason we don't do that is the keepProcessesAlive hack for our tests. If
> a test sets dom.ipc.contentProcess to 1 let's say, but we keep alive 4
> content processes we want this code to always select the first process
> instead of one from the 4. Once we can get rid of that hack, we can use
> aCount here. Does that make sense?

Oh, and if you're worried about hitting invalid indexes with the loop we
have a check for that a few lines above:

    if (aCount < maxContentParents) {
      return Ci.nsIContentProcessProvider.NEW_PROCESS;
    }
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)
> instead of one from the 4. Once we can get rid of that hack, we can use
> aCount here. Does that make sense?

Ah, I see. Can you add a comment in the code to that effect?
Comment on attachment 8849560 [details] [diff] [review]
e10sMultiBlockedByAddons toolkit side. v1

This patch looks fine, but I suggest some follow-ups here or in another bug to make this better to analyse later:

- with this running, the pref "dom.ipc.processCount" will be lying sometimes.. And if you use it for any data analysis you'll need to take into account this new pref now too.  And if other code checks for "dom.ipc.processCount" it might need to be aware.

What we did for e10s-single is that instead of using the "browser.tabs.remote.autostart" pref as the source of truth, we created the getter Services.appinfo.browserTabsRemoteAutostart to return the value with all things considered. Might be something that you want to do here or in the near future before it rolls out.

- It would be good to tag this properly in the e10srollout add-on (browser/extensions/e10srollout/bootstrap.js) so that we can see users of e10s-multi in the dashboard. It's another way to cover the correct data analysis question

- I suggest not doing this for Nightly and maybe Aurora. We only activated the extensions-blocking on _Beta_ during all of the e10s-single testing.. Users on Nightly were fine with the hiccups and it provided a lot more useful feedback to have them running e10s.
Attachment #8849560 - Flags: review?(felipc) → review+
(Assignee)

Comment 18

4 months ago
Thanks for the hints, they are really really helpful.

(In reply to :Felipe Gomes (needinfo me!) from comment #17)
> Comment on attachment 8849560 [details] [diff] [review]
> e10sMultiBlockedByAddons toolkit side. v1
> 
> This patch looks fine, but I suggest some follow-ups here or in another bug
> to make this better to analyse later:
> 
> - with this running, the pref "dom.ipc.processCount" will be lying
> sometimes.. And if you use it for any data analysis you'll need to take into
> account this new pref now too.  And if other code checks for
> "dom.ipc.processCount" it might need to be aware.
> 
> What we did for e10s-single is that instead of using the
> "browser.tabs.remote.autostart" pref as the source of truth, we created the
> getter Services.appinfo.browserTabsRemoteAutostart to return the value with
> all things considered. Might be something that you want to do here or in the
> near future before it rolls out.
> 

Yeah, I've realized this and that's why I made one of the changes in the first patch. That function is now ContentParent::GetMaxProcessCount. Probably it would be a good idea to expose that to JS I think I will do a followup for that. What makes this trickier than e10s is that there are cases where the number of (web content) processes alive is higher than the limit. So we need to be extra careful about the assumptions we make during the beta experiment. 

> - It would be good to tag this properly in the e10srollout add-on
> (browser/extensions/e10srollout/bootstrap.js) so that we can see users of
> e10s-multi in the dashboard. It's another way to cover the correct data
> analysis question
>

I think that's something for Blake to keep in mind as he is going to do that work, but I've already mentioned this to him.
 
> - I suggest not doing this for Nightly and maybe Aurora. We only activated
> the extensions-blocking on _Beta_ during all of the e10s-single testing..
> Users on Nightly were fine with the hiccups and it provided a lot more
> useful feedback to have them running e10s.

Yeah, I think you're right. How did you do it for e10s? Landing the patch only on the Beta channel, or try to figure out programmatically what channel it is using?
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #18)
> Yeah, I think you're right. How did you do it for e10s? Landing the patch
> only on the Beta channel, or try to figure out programmatically what channel
> it is using?

There was another pref that toggled this behavior on or off (extensions.e10sBlocksEnabling), and it was #ifdef'ed RELEASE_OR_BETA
(Assignee)

Comment 20

4 months ago
(In reply to :Felipe Gomes (needinfo me!) from comment #19)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #18)
> > Yeah, I think you're right. How did you do it for e10s? Landing the patch
> > only on the Beta channel, or try to figure out programmatically what channel
> > it is using?
> 
> There was another pref that toggled this behavior on or off
> (extensions.e10sBlocksEnabling), and it was #ifdef'ed RELEASE_OR_BETA

Right, I think that should work for us or I can just use something like https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/AppConstants.jsm#320

Comment 21

4 months ago
Pushed by gkrizsanits@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c884edd5cf81
Block e10s-multi if e10sMultiBlockedByAddons is set. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/cad1604f3dbf
Setting e10sMultiBlockedByAddons for bootrapped add-on users. r=felipe
https://hg.mozilla.org/mozilla-central/rev/c884edd5cf81
https://hg.mozilla.org/mozilla-central/rev/cad1604f3dbf
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 23

3 months ago
Comment on attachment 8849556 [details] [diff] [review]
e10sMultiBlockedByAddons platform side. v1

Approval Request Comment
[Feature/Bug causing the regression]: not a regression.

[User impact if declined]: we need this for e10s-multi beta experiment

[Is this code covered by automated tests?]: yes in the second patch

[Has the fix been verified in Nightly?]: it was on nightly for a while and now it's on aurora

[Needs manual test from QE? If yes, steps to reproduce]: the experiment add-on will be tested manually which will cover this patch as well

[List of other uplifts needed for the feature/fix]: the other patch in this bug

[Is the change risky?]: not at all

[Why is the change risky/not risky?]: it won't have any effect until the beta experiment triggers it

[String changes made/needed]: none
Attachment #8849556 - Flags: approval-mozilla-beta?
(Assignee)

Comment 24

3 months ago
Comment on attachment 8849560 [details] [diff] [review]
e10sMultiBlockedByAddons toolkit side. v1

Approval Request Comment
[Is this code covered by automated tests?]: yes

everything else: same as above for the first patch
Attachment #8849560 - Flags: approval-mozilla-beta?
(Assignee)

Comment 25

3 months ago
Created attachment 8859340 [details] [diff] [review]
rebased for beta: platform side
(Assignee)

Comment 26

3 months ago
Created attachment 8859341 [details] [diff] [review]
rebased for beta: toolkit side (patch 2)

Updated

3 months ago
Blocks: 1349363

Updated

3 months ago
status-firefox54: --- → affected
Comment on attachment 8849556 [details] [diff] [review]
e10sMultiBlockedByAddons platform side. v1

Support e10s-multi beta experiment. Beta54+. Should be in 54 beta 1.
Attachment #8849556 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

3 months ago
Attachment #8849560 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Created attachment 8859772 [details] [diff] [review]
rebased for beta: toolkit side (patch 2)

The try push at [1] showed that this patch would have turned eslint orange on beta because of nsIPrefBranch API drift. This patch fixes has a fix for that orange.

I don't think I can manually carry across the beta approval, but I think this patch implicitly inherits it.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=6874b25be46a97e7ce49017be4028b21426e3a97
Attachment #8859772 - Flags: review+

Updated

3 months ago
Attachment #8859340 - Attachment is obsolete: true
The two attached Beta patches look the same?
Flags: needinfo?(mrbkap)

Updated

3 months ago
Attachment #8859340 - Attachment description: rebased for beta: toolkit side → rebased for beta: platform side
Attachment #8859340 - Attachment is obsolete: false
Flags: needinfo?(mrbkap)

Updated

3 months ago
Attachment #8859341 - Attachment description: rebased for beta: platform side (patch 2) → rebased for beta: toolkit side (patch 2)
Attachment #8859341 - Attachment is obsolete: true

Updated

3 months ago
Attachment #8859772 - Attachment description: rebased for beta: toolkit side → rebased for beta: toolkit side (patch 2)
I obsoleted the wrong patch. things should be fixed now.
Oh, this needs to land on Aurora too!

Updated

3 months ago
Attachment #8859772 - Flags: approval-mozilla-aurora?
Comment on attachment 8859772 [details] [diff] [review]
rebased for beta: toolkit side (patch 2)

Aurora doesn't exist anymore :)
Attachment #8859772 - Flags: approval-mozilla-aurora?

Comment 33

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/549670514df6
https://hg.mozilla.org/releases/mozilla-beta/rev/6a768cc3c85a
status-firefox54: affected → fixed
Flags: in-testsuite+
(Assignee)

Comment 34

3 months ago
Thanks Blake and Ryan!
Depends on: 1360354
You need to log in before you can comment on or make changes to this bug.