1075625 - AddonManager can race between provider startup / shutdown and methods that call back to providers

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[:Irving Reid (No longer working on Firefox)]

+++ This bug was initially created as a clone of Bug #1074135 +++

From the parent bug, comments 8 and 9:

gfritzsche spotted this in the TBPL log:

09:47 (gfritzsche) hm: ERROR	Exception calling provider getAddonsByTypes: TypeError: this.timerLocations is null
09:50 (gfritzsche) ok, that looks like test-failure here: http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/head.js#1041

The mock provider's getAddonsByTypes callback is being called after the mock provider is shut down. This could be caused by some other provider calling AddonManager.getAddonsByTypes in its own shutdown, or by an async call to getAddonsByTypes running in the background when AddonManager.shutdownManager is called. Investigating...


Just thought of another possible failure path - if AddonManager is in the process of starting up providers and one of those providers calls one of the AddonManager iterate-through-all-providers methods, a provider could be called before it is initialized.

Experiments.jsm / _evaluateExperiments calls installedExerimentAddons() which calls AddonManager.getAddonsByTypes(), so that could be the trigger.

Note that calling getAddonsByTypes forces the AddonManager to load the XPIProviderUtils and the XPI database, so in the long term it would be nice to find a better solution for installedExperimentAddons.

Need to think a bit more about what the right solution is. My current ideas are:
1) Don't allow clients to call AddonManager APIs that call providers, until all the providers are initialized. This might require adding another notification to tell clients when that initialization is done (there might be an existing notification that's good enough).

2) Start up and shut down providers sequentially, and add/remove them on the active providers list as they are ready / stopped. That would make things like the current behaviour of the Experiments provider difficult, since it relies on the XPIProvider being available and having loaded the experiments; this would break if those providers got initialized in the wrong order.

(Blair McBride [:Unfocused] from comment #12)
> (In reply to :Irving Reid from comment #11)
> > I'd really like to burn AsyncObjectCaller with fire
> 
> Please.
> 
> (In reply to :Irving Reid from comment #9)
> > 1) Don't allow clients to call AddonManager APIs that call providers, until
> > all the providers are initialized. This might require adding another
> > notification to tell clients when that initialization is done (there might
> > be an existing notification that's good enough).
> 
> +1. This is the simplest and most deterministic.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Seems this is causing problems, especially in relation to bug 1114567.

(In reply to :Irving Reid (No longer working on Firefox) from comment #0)
> (Blair McBride [:Unfocused] from comment #12)
> > (In reply to :Irving Reid from comment #9)
> > > 1) Don't allow clients to call AddonManager APIs that call providers, until
> > > all the providers are initialized. This might require adding another
> > > notification to tell clients when that initialization is done (there might
> > > be an existing notification that's good enough).
> > 
> > +1. This is the simplest and most deterministic.

So, thinking about this, it's not quite as simple as that - as providers can be dynamically registered at runtime, and that can be async. So we need to make sure that's handled nicely too.

Comment 2

[:Irving Reid (No longer working on Firefox)]

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #1)
> So, thinking about this, it's not quite as simple as that - as providers can
> be dynamically registered at runtime, and that can be async. So we need to
> make sure that's handled nicely too.

For runtime registrations, I had been thinking that the new provider just doesn't get added to the list of providers to call back to until its startup is done; that handles the case of other clients calling the APIs while a new provider is starting.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to :Irving Reid (No longer working on Firefox) from comment #2)
> For runtime registrations, I had been thinking that the new provider just
> doesn't get added to the list of providers to call back to until its startup
> is done; that handles the case of other clients calling the APIs while a new
> provider is starting.

Yea, that's pretty much what I did. The upcoming patch adds providers first to `pendingProviders`, and once each has started up it's moved over to `providers`.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Had some fun with notifyAddonChanged, interrupted startups, and the multiple startups we do in tests. But this is finally working nicely.

Can't say I like the solution for notifyAddonChanged (intentionally making it unsafe), but I don't see any way around that without major refactoring. And that didn't seem worth it, given that API has a very limited use and it'll be removed anyway.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 8550183 [details] [diff] [review]
Patch v1

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

WHITESPACE!

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +819,5 @@
>          let url = catman.getCategoryEntry(CATEGORY_PROVIDER_MODULE, entry);
>  
>          try {
> +          let scope = {};
> +          Components.utils.import(url, scope);

Why the need to define scope?

@@ +838,5 @@
>  
>        // Once we start calling providers we must allow all normal methods to work.
>        gStarted = true;
>  
> +      for (let provider of this.pendingProviders) {

Is it safe to mutate the set while you're iterating over it?

@@ +1010,5 @@
>      if (!aMethod || typeof aMethod != "string")
>        throw Components.Exception("aMethod must be a non-empty string",
>                                   Cr.NS_ERROR_INVALID_ARG);
>  
> +    let providers = [...this.providers];

This works on sets? Nice!

@@ +1612,5 @@
>      if (!aType || typeof aType != "string")
>        throw Components.Exception("aType must be a non-empty string",
>                                   Cr.NS_ERROR_INVALID_ARG);
>  
> +    // Temporary hack until bug 520124 lands.

How many hacks do we have in place waiting on bug 520124 :(

@@ +1614,5 @@
>                                   Cr.NS_ERROR_INVALID_ARG);
>  
> +    // Temporary hack until bug 520124 lands.
> +    // We can get here during synchronous startup, at which point it's
> +    // considered unsafe (and therefore disallowed by AddonManaher.jsm) to

*AddonManager.jsm

@@ +1625,5 @@
> +    // both providers; so we have this hack to allow bypassing the normal
> +    // safetey guard.
> +    // The notifyAddonChanged/addonChanged API will be unneeded and therefore
> +    // removed by bug 520124, so this is a temporary quick'n'dirty hack.
> +    let providers = [...this.providers, ...this.pendingProviders];

So XPIProvider is already marked safe and I don't see any obvious reason why the lwtheme provider can't be marked safe too. So is this hack necessary?

@@ +2480,5 @@
>  
>    unregisterProvider: function AMP_unregisterProvider(aProvider) {
>      AddonManagerInternal.unregisterProvider(aProvider);
>    },
> +  

Nit: whitespace

::: toolkit/mozapps/extensions/test/xpcshell/test_isReady.js
@@ +23,5 @@
> +  startupManager();
> +  equal(AddonManager.isReady, true, "isReady should be true after startup");
> +  equal(gotStartupEvent, true, "Should have seen onStartup event after startup");
> +  equal(gotShutdownEvent, false, "Should not have seen onShutdown event before shutdown");
> +  

Nit: whitespace

::: toolkit/mozapps/extensions/test/xpcshell/test_provider_markSafe.js
@@ +9,5 @@
> +
> +    startup() {
> +      if (this.markSafe)
> +        AddonManagerPrivate.markProviderSafe(this);
> +      

Nit: whitespace

::: toolkit/mozapps/extensions/test/xpcshell/test_provider_unsafe_access_shutdown.js
@@ +39,5 @@
> +  let secondProvider = mockAddonProvider("Mock2");
> +  AddonManagerPrivate.registerProvider(secondProvider);
> +
> +  startupManager();
> +  

Nit: whitespace

::: toolkit/mozapps/extensions/test/xpcshell/test_provider_unsafe_access_startup.js
@@ +40,5 @@
> +    firstProvider.startupCallback = function() {
> +      AddonManager.getAddonByID("does-not-exist", resolve);
> +    };
> +    AddonManagerPrivate.registerProvider(firstProvider);
> +    

Nit: whitespace

@@ +43,5 @@
> +    AddonManagerPrivate.registerProvider(firstProvider);
> +    
> +    secondProvider = mockAddonProvider("Mock2");
> +    AddonManagerPrivate.registerProvider(secondProvider);
> +    

Nit: whitespace

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dave Townsend [:mossop] from comment #5)
> WHITESPACE!

*sigh* My editor has been mishaving...

> Why the need to define scope?

Leftover from some debug logging I had - reverted.

> Is it safe to mutate the set while you're iterating over it?

Yes - it's safe to both delete and add items while iterating. Deleted items won't be visited, added items will be, and items won't be skipped.

> > +    let providers = [...this.providers];
> 
> This works on sets? Nice!

Yep, anything that is iterable.

> > +    // Temporary hack until bug 520124 lands.
> 
> How many hacks do we have in place waiting on bug 520124 :(

Too many :(

> @@ +1625,5 @@
> > +    // both providers; so we have this hack to allow bypassing the normal
> > +    // safetey guard.
> > +    // The notifyAddonChanged/addonChanged API will be unneeded and therefore
> > +    // removed by bug 520124, so this is a temporary quick'n'dirty hack.
> > +    let providers = [...this.providers, ...this.pendingProviders];
> 
> So XPIProvider is already marked safe and I don't see any obvious reason why
> the lwtheme provider can't be marked safe too. So is this hack necessary?

It is needed, yes. Providers are marked safe from inside their startup() method, after the required initialization tasks. So we can have:
1) XPIProvider startup
2) XPIProvider makes itself safe
3) XPIProvider uses notifyAddonChanged
4) LWTM startup
5) LWTM marks itself safe

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/669b9d53990d

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/669b9d53990d