1247497 - [e10s] Mechanism to limit when e10s is turned on based on add-ons presence or absense

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[:shell escalante]

As e10s rolls out - there are phases where we want to determine if e10s will be activated based on presence or absense of add-ons:

Phase 1: e10s not enabled with any user who has any add-ons
Phase 2: allow-list: e10s enabled on if there are compatible add-ons - checked against a list.
Phase 3: TBD - but likely a block list and UX (bug 1181835) to handle add-ons that misbehave with e10s

more details: https://docs.google.com/document/d/1VtngmmU-X46VR7CcmTXKwU6UPdR6AA4La1xh1AcC8T0/edit#

Comment 1

[Benjamin Smedberg]

This can probably be accomplished through additions to the system addon that will be making the staged rollout decision: see bug 1249845.

I am certain that we need the ability to update the lists much faster than the trains, which is why I suggested a system addon.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1247497 - Mechanism to control when e10s is turned on based on add-ons presence or absense.

Review commit: https://reviewboard.mozilla.org/r/65732/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65732/

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1247497 - Inform the system add-on that there is at least one non-exempt add-on installed.

This is used by the system add-on when determining the cohort, to differentiate e10s-enabled users with no addons (part of test cohort) and with add-ons (part of addons-test cohort)

Review commit: https://reviewboard.mozilla.org/r/65734/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65734/

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1247497. Test for e10s-addons mechanism.

Review commit: https://reviewboard.mozilla.org/r/65736/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65736/

Comment 5

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8773097 [details]
Bug 1247497 - Mechanism to control when e10s is turned on based on add-ons presence or absense.

https://reviewboard.mozilla.org/r/65732/#review62822

I have a few questions / concerns with this patch so I'm cancelling review for now. One thing that would be extremly usefull is to add some tests...

::: toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm:16
(Diff revision 1)
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const PREF_E10S_ADDON_BLOCKLIST = "extensions.e10s.rollout.blocklist";
> +const PREF_E10S_ADDON_POLICY    = "extensions.e10s.rollout.policy";
> +
> +const ADDONS = {

Benjamin pointed it out earlier that a built in list is not ideal... I assume it was talked over, what was the outcome of that conversation?

::: toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm:70
(Diff revision 1)
> +// The results of the experiments are tracked against which policy
> +// was tested, in order to understand which ones are good to be
> +// rolled out to release.

OK, so this is for an experiment to decide what add-ons are we going to enable?

Where is this experiement going to run? (which channel I mean?)

::: toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm:84
(Diff revision 1)
> +};
> +
> +Object.defineProperty(this, "isAddonPartOfE10SRollout", {
> +  configurable: false,
> +  enumerable: false,
> +  writable: false,

I'm not sure what are you trying to guard against here. If the goal is to prevent scripts with system principal (xul add-ons / SDK add-ons with require('chrome')) to mess with this function it is not going to work :(

Such script could also modify EXPORTED_SYMBOLS, but more importantly anything else too in the scope that could alter the behavior of any line of this function.

Bottom line is, if one has system principal, we have zero protection against it, so this part is purely visual cosmetics.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4347
(Diff revision 1)
> +      case PREF_E10S_ADDON_BLOCKLIST:
> +      case PREF_E10S_ADDON_POLICY:
> +        XPIDatabase.updateAddonsBlockingE10s();
> +        break;

I don't think this is right. So once this pref is changed, we will be notified, and because of this call we will update the PREF_E10S_BLOCKED_BY_ADDONS flag, but will not restart so the pref will indicate that e10s is blocked by an add-on, but e10s will be still active. This would screw up the metrics no?

The other issue is that if something triggers XPIDatabase.saveChanges, and the BLOCKLIST pref has been changed but there was no restart yet, the same problem can happen.

We must be careful with this BLOCKLIST pref update. It should be only allowed to be changed at startup, and the requested changed should be temporarily stored in another pref for example.

We could still monitor if someone has changed the pref manually and just mark the user as invalid for the metrics...

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8773099 [details]
Bug 1247497. Test for e10s-addons mechanism.

https://reviewboard.mozilla.org/r/65736/#review62960

::: browser/extensions/e10srollout/bootstrap.js:19
(Diff revision 1)
>  const TEST_THRESHOLD = {
>    "beta"    : 0.5,  // 50%
>  };
>  
> +const ADDON_ROLLOUT_POLICY = {
> +  // per-channel policies go here

So we've got no policies here. I assume that's intentional and that we plan on adding some in a follow-up?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/65736/#review62960

> So we've got no policies here. I assume that's intentional and that we plan on adding some in a follow-up?

Yep - the first one is getting added in bug 1282120.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> Comment on attachment 8773097 [details]
> Bug 1247497 - Mechanism to control when e10s is turned on based on add-ons
> presence or absense.
> 
> https://reviewboard.mozilla.org/r/65732/#review62822
> 
> I have a few questions / concerns with this patch so I'm cancelling review
> for now. One thing that would be extremly usefull is to add some tests...

Yeah, I'll add some tests for this.

> 
> ::: toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm:16
> (Diff revision 1)
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +
> > +const PREF_E10S_ADDON_BLOCKLIST = "extensions.e10s.rollout.blocklist";
> > +const PREF_E10S_ADDON_POLICY    = "extensions.e10s.rollout.policy";
> > +
> > +const ADDONS = {
> 
> Benjamin pointed it out earlier that a built in list is not ideal... I
> assume it was talked over, what was the outcome of that conversation?

Yeah, I talked with Shell and Kev about this. This is a balance reached between the flexibility of expanding the list in the middle of a release (through the system add-on), and not making it trivial for other add-ons to add themselves to the list.

Another reason is that it makes it a lot easier to analyze the data we get back from the experiments. With a totally open-ended whitelist we'd need to be comparing the telemetry pings very carefully to make sure we're comparing apples-to-apples. With this it's easier to say "during Aug 3 - Aug 12 we were analyzing the stability of e10s with Set #2 of add-ons" and so on.

The add-ons will be iteratively chosen during the beta cycle and when it goes to release we'll know in advance what are the add-ons that we plan on activating during the release cycle, so we can ship all sets built-in.

> 
> ::: toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm:70
> (Diff revision 1)
> > +// The results of the experiments are tracked against which policy
> > +// was tested, in order to understand which ones are good to be
> > +// rolled out to release.
> 
> OK, so this is for an experiment to decide what add-ons are we going to
> enable?
> 
> Where is this experiement going to run? (which channel I mean?)

This is meant for both an experiment on Beta 49, and also to use it on Release 49. This was a bit confusing, sorry, as I posted the actual activation of it in another bug: bug 1282120. I'll remove the mention of "experiment" from the comment to reduce the confusion.

> 
> ::: toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm:84
> (Diff revision 1)
> > +};
> > +
> > +Object.defineProperty(this, "isAddonPartOfE10SRollout", {
> > +  configurable: false,
> > +  enumerable: false,
> > +  writable: false,
> 
> I'm not sure what are you trying to guard against here. If the goal is to
> prevent scripts with system principal (xul add-ons / SDK add-ons with
> require('chrome')) to mess with this function it is not going to work :(
> 
> Such script could also modify EXPORTED_SYMBOLS, but more importantly
> anything else too in the scope that could alter the behavior of any line of
> this function.
> 
> Bottom line is, if one has system principal, we have zero protection against
> it, so this part is purely visual cosmetics.

I believe this actually works. This is the same approach done by AddonConstants.jsm (see http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/AddonConstants.jsm ) and hopefully that is working well :P

Another thing that could be a problem is that Cu.import(..) returns all the symbols (not just the exported ones) in the BackstagePass object. But I tested it and the const variables are not included there. I did some testing with privileged code and couldn't find a way to mess with it. I'll test some more but presumably this should work equally well to the protection on AddonConstants.
 

> 
> ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4347
> (Diff revision 1)
> > +      case PREF_E10S_ADDON_BLOCKLIST:
> > +      case PREF_E10S_ADDON_POLICY:
> > +        XPIDatabase.updateAddonsBlockingE10s();
> > +        break;
> 
> I don't think this is right. So once this pref is changed, we will be
> notified, and because of this call we will update the
> PREF_E10S_BLOCKED_BY_ADDONS flag, but will not restart so the pref will
> indicate that e10s is blocked by an add-on, but e10s will be still active.
> This would screw up the metrics no?


No. I was confused at first too but you might be misremembering how this code works. These checks don't run on startup at all, they only run when there's a modification to the DB (i.e., add-on install/update or firefox update). The PREF_E10S_BLOCKED_BY_ADDONS flag is the final answer from those checks, and it's the only thing consulted on startup to determine if e10s should be blocked or not.

Metrics won't be messed up because what's measured is Services.appinfo.browserTabsRemoteAutostart, and not the value of these controlling prefs. And that will only change value after a restart.

The same applies for both the POLICY or BLOCKLIST pref. The only real issue here is that e10s needs a restart to be required, so that for example an add-on added to the blocklist will only effect e10s after a restart. But there's no way around that unless we force the user to restart.



Hope that clarifies things. I'm open to suggestions of course, but please reconsider the approach taken here with this new info

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to :Felipe Gomes (needinfo me!) from comment #8)
> I believe this actually works. This is the same approach done by
> AddonConstants.jsm (see
> http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/AddonConstants.jsm ) and hopefully that is working well :P

Sorry, but that also gives zero security against add-ons who already have system
principal.

I've CC'ed you to a bug where I had similar discussion the last time.

> No. I was confused at first too but you might be misremembering how this
> code works. These checks don't run on startup at all, they only run when
> there's a modification to the DB (i.e., add-on install/update or firefox
> update). The PREF_E10S_BLOCKED_BY_ADDONS flag is the final answer from those
> checks, and it's the only thing consulted on startup to determine if e10s
> should be blocked or not.

That's close to how I remember it but let me double check it tomorrow I
think IF something change then we run it at startup, but I have to refresh my
memories. You might be totally right. But that explains to me why you want
to do this. I have to do some more code reading tomorrow.
Anyway, test coverage would be great here, this code surprised me a few times
before...

> Hope that clarifies things. I'm open to suggestions of course, but please
> reconsider the approach taken here with this new info

Sure I'll give it another try tomorrow. Thanks for all the extra info!

Comment 10

[Gabor Krizsanits (INACTIVE)]

(In reply to :Felipe Gomes (needinfo me!) from comment #8)
> No. I was confused at first too but you might be misremembering how this
> code works. These checks don't run on startup at all, they only run when
> there's a modification to the DB (i.e., add-on install/update or firefox
> update). The PREF_E10S_BLOCKED_BY_ADDONS flag is the final answer from those
> checks, and it's the only thing consulted on startup to determine if e10s
> should be blocked or not.

So it is mostly as I remembered. When something changes in the database and
there is a pending operation that will be triggered at next startup (enable
add-ons that need restart for example) then at start up we do checkForChanges
and from there processFileChanges [1] which then calls saveChanges [2].

And that is the time when those add-ons will be actually active and then
will we recalculate the pref (when the operation that should change the
pref is only pending until next startup the pref is not changed yet)

[1] http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#3726
[2] http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProviderUtils.js#2212

> 
> Metrics won't be messed up because what's measured is
> Services.appinfo.browserTabsRemoteAutostart, and not the value of these
> controlling prefs. And that will only change value after a restart.

I don't think that is true either, we annotate crash reporter according to
this pref: http://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4574

Comment 11

[:Felipe Gomes (needinfo for replies!)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #10)

Yeah, whenever there's a change in the database (from an install for example), that code there is enough to make it pick up the changes. But the main use case here is when there was no change to the add-ons db, but there's a change in the policy or blocklist. Whenever that happens we need to trigger a recalculation, and the pref observer works well for it. If I remove it I need to replace it with something else.

I'll do some testing to see how this will happen when the system add-on is updating itself, but I feel that the checkForChanges will happen before it has the chance to run the new code and update the pref.

My fear here is that the pref change for extensions.e10s.rollout.policy or extensions.e10s.rollout.blocklist will go unnoticed until the user make any other change to the add-on list.

> > 
> > Metrics won't be messed up because what's measured is
> > Services.appinfo.browserTabsRemoteAutostart, and not the value of these
> > controlling prefs. And that will only change value after a restart.
> 
> I don't think that is true either, we annotate crash reporter according to
> this pref:
> http://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4574

That annotation is still correct, because it's only annotated once on startup. So it's annotated with the value that it was on startup, not the new value change. In general it's fine to pre-compute and change these values during runtime because they will only get picked after a restart.

(fwiw, that annotation is not needed anymore and should have been removed with bug 1275791, which I forgot. I'll file a follow-up to remove it.)

Comment 12

[Gabor Krizsanits (INACTIVE)]

(In reply to :Felipe Gomes (needinfo me!) from comment #11)
> That annotation is still correct, because it's only annotated once on
> startup.

Than I think it should be OK. This part was my main worry.

Alright let's add some tests then, and then we can go with the pref observer.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8773097 [details]
Bug 1247497 - Mechanism to control when e10s is turned on based on add-ons presence or absense.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65732/diff/1-2/

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8773098 [details]
Bug 1247497 - Inform the system add-on that there is at least one non-exempt add-on installed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65734/diff/1-2/

Comment 15

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8773099 [details]
Bug 1247497. Test for e10s-addons mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65736/diff/1-2/

Comment 16

[:Felipe Gomes (needinfo for replies!)]

So I added tests that should make more clear the expected behavior for this. I added them to the same test file as from bug 1232274 and bug 1234675

Reviewboard got very confused and removed the part of patch that was reviewed by mconley and replaced it with the new test patch..

There are no differences to the other parts other than a comment adjustment. I suggest using these two links to see what's new:

comment change: https://reviewboard.mozilla.org/r/65732/diff/1-2/

test added: https://reviewboard.mozilla.org/r/65736/diff/2/

Comment 17

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8773098 [details]
Bug 1247497 - Inform the system add-on that there is at least one non-exempt add-on installed.

https://reviewboard.mozilla.org/r/65734/#review64438

Comment 18

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8773097 [details]
Bug 1247497 - Mechanism to control when e10s is turned on based on add-ons presence or absense.

https://reviewboard.mozilla.org/r/65732/#review64436

Comment 19

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8773099 [details]
Bug 1247497. Test for e10s-addons mechanism.

https://reviewboard.mozilla.org/r/65736/#review64440

Thanks for the test, looks great.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0e9c0e8f36
Mechanism to control when e10s is turned on based on add-ons presence or absense. r=krizsa
https://hg.mozilla.org/integration/mozilla-inbound/rev/512a3cc1a9fc
Inform the system add-on that there is at least one non-exempt add-on installed. r=krizsa
https://hg.mozilla.org/integration/mozilla-inbound/rev/62f0ff98f9c1
Test for e10s-addons mechanism. r=krizsa
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a835245849
Let the system add-on control the add-ons rollout policy, and properly tag the cohort based on it. r=mconley

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bc0e9c0e8f36
https://hg.mozilla.org/mozilla-central/rev/512a3cc1a9fc
https://hg.mozilla.org/mozilla-central/rev/62f0ff98f9c1
https://hg.mozilla.org/mozilla-central/rev/a2a835245849

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8773097 [details]
Bug 1247497 - Mechanism to control when e10s is turned on based on add-ons presence or absense.

Approval Request Comment
[Feature/regressing bug #]: allows add-ons to be listed as not blockers for e10s. needed for the planned e10s-addons experiment on beta 49
[User impact if declined]: no e10s-addons experiment
[Describe test coverage new/current, TreeHerder]: landed on central, with tests
[Risks and why]: contained to the enabling/disabling of e10s
[String/UUID change made/needed]: none

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8773098 [details]
Bug 1247497 - Inform the system add-on that there is at least one non-exempt add-on installed.

Approval Request Comment:
small follow-up from the previous patch, needed to better control the cohort names for the e10s experiment

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8773099 [details]
Bug 1247497. Test for e10s-addons mechanism.

Approval Request Comment
tests

Comment 25

[:Felipe Gomes (needinfo for replies!)]

Sylvestre, this is one of the two bugs that are needed for the e10s-addons test on beta 49. These are the actual patches that implement the mechanism needed, and then the patch in bug 1282120 just activates it for beta.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8773097 [details]
Bug 1247497 - Mechanism to control when e10s is turned on based on add-ons presence or absense.

Addons & e10s, taking it.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06deb7bd6e66
Remove Flashblock from first experiment list. r=me

Comment 28

[:Felipe Gomes (needinfo for replies!)]

I removed Flashblock from the experiment list on central and uplifted all patches (with Flashblock already removed) to Aurora. We might include Personas Plus to the list if QA says it works ok.

https://hg.mozilla.org/releases/mozilla-aurora/rev/d2076125a317
https://hg.mozilla.org/releases/mozilla-aurora/rev/ed1ccacac3d8
https://hg.mozilla.org/releases/mozilla-aurora/rev/3ef9694e9341
https://hg.mozilla.org/releases/mozilla-aurora/rev/40e4aa52d1d0

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/06deb7bd6e66

Comment 30

[Gerry Chang [:gchang]]

Comment on attachment 8773098 [details]
Bug 1247497 - Inform the system add-on that there is at least one non-exempt add-on installed.

Per comment #26, take it in aurora.