1346288 - Disable e10s-multi for SDK addon users

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Comment 1

[Gabor Krizsanits (INACTIVE)]

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?

Comment 2

[Dave Townsend [:mossop]]

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

Comment 3

[:Felipe Gomes (needinfo for replies!)]

One ugly hack would be to check that the id of the addon begins with "jid"

Comment 4

[Dave Townsend [:mossop]]

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

Comment 5

[Gabor Krizsanits (INACTIVE)]

(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".

Comment 6

[Dave Townsend [:mossop]]

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

Comment 7

[Gabor Krizsanits (INACTIVE)]

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

Comment 8

[Gabor Krizsanits (INACTIVE)]

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).

Comment 9

[Gabor Krizsanits (INACTIVE)]

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

Comment 10

[Gabor Krizsanits (INACTIVE)]

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

Comment 11

[Dave Townsend [:mossop]]

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 12

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 13

[Gabor Krizsanits (INACTIVE)]

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

Comment 14

[Gabor Krizsanits (INACTIVE)]

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

Comment 15

[Gabor Krizsanits (INACTIVE)]

(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;
    }

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

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

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 18

[Gabor Krizsanits (INACTIVE)]

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?

Comment 19

[:Felipe Gomes (needinfo for replies!)]

(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

Comment 20

[Gabor Krizsanits (INACTIVE)]

(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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c884edd5cf81
https://hg.mozilla.org/mozilla-central/rev/cad1604f3dbf

Comment 23

[Gabor Krizsanits (INACTIVE)]

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

Comment 24

[Gabor Krizsanits (INACTIVE)]

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

Comment 25

[Gabor Krizsanits (INACTIVE)]

Attachment: rebased for beta: platform side

Comment 26

[Gabor Krizsanits (INACTIVE)]

Attachment: rebased for beta: toolkit side (patch 2)

Comment 27

[Gerry Chang [:gchang]]

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

Support e10s-multi beta experiment. Beta54+. Should be in 54 beta 1.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

The two attached Beta patches look the same?

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

I obsoleted the wrong patch. things should be fixed now.

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Oh, this needs to land on Aurora too!

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8859772 [details] [diff] [review]
rebased for beta: toolkit side (patch 2)

Aurora doesn't exist anymore :)

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/549670514df6
https://hg.mozilla.org/releases/mozilla-beta/rev/6a768cc3c85a

Comment 34

[Gabor Krizsanits (INACTIVE)]

Thanks Blake and Ryan!