911621 - addon gets stuck in weird state

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dietrich Ayala (:dietrich)]

Attachment: extensions.json

enabled Mass Password Reset (was already installed disabled), and restarted.

after restarting, addon was not enabled but [disable] button was there, and the "restart to enable" message is still visible. restarting multiple times and still in this state.

screenshot: http://cl.ly/image/1T37263D0J2a

screenshot of values unfocused asked for: http://cl.ly/image/1I1E2s313X3Y

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: extensions.sqlite

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

My assumption is that this is a regression from bug 853388, since neither Mossop or I have heard of this happening before.

Additional details:

* addon.isActive is false
* extensions.pendingOperations is true, even after multiple restarts
* Dietrich said no error showed up in the logs


So, I guess there's some silent error happening somewhere in startup *and* shutdown, since we check that pref in both places. Thought, if something was throwing on startup, I'd expect there to be a log of it. But it does make XPIDatabase.updateActiveAddons() and XPIDatabase.writeAddonsList() look suspicious.

Comment 3

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

Attachment: WIP - use prototype RunStateDependency module to ensure all writes are done before shutdown

This patch links together the shutdown for AddonRepository, AddonManager and all the providers managed by AddonManager (especially XPIProvider) using Promises. The resulting promise is passed to the prototype RunStateDependency module from bug 913899 to spin an event loop and ensure the writes are all done before the end of the profile-before-change notification.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 801993 [details] [diff] [review]
WIP - use prototype RunStateDependency module to ensure all writes are done before shutdown

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

Looks ok to me. Probably not compatible with the latest version of my patch at bug 913899, but that patch is still a WIP.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 801993 [details] [diff] [review]
WIP - use prototype RunStateDependency module to ensure all writes are done before shutdown

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

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +551,5 @@
>  
> +    // Register our shutdown handler with the RunStateDependency manager
> +    RunStateDependency({
> +      runstate: "profile-before-change",
> +      name: "Add-on manager",

Nit: "Add-on Manager"

@@ +688,5 @@
>    },
>  
>    /**
> +   * Calls a method on all registered providers, if the provider implements
> +   * the method. The called method is expected to return a promise, and 

Nit: Trailing whitespace

@@ +710,5 @@
> +    for (let provider of providers) {
> +      try {
> +        if (aMethod in provider) {
> +          // Resolve a new promise with the result of the method, to handle both
> +          // methods that return values (or nothing) and methods that return promises.

Me like.

@@ +713,5 @@
> +          // Resolve a new promise with the result of the method, to handle both
> +          // methods that return values (or nothing) and methods that return promises.
> +          // Log and swallow the errors from methods that do return promises.
> +          let nextPromise = Promise.resolve(provider[aMethod].apply(provider, aArgs))
> +            .then(null, e => ERROR("Exception calling provider " + aMethod, e));

Can this be made a bit easier to read?

(Also: my kingdom for bug 762363)

@@ +721,5 @@
> +      catch (e) {
> +        ERROR("Exception calling provider " + aMethod, e);
> +      }
> +    }
> +    return Promise.all(allProviders);

If I understand Promise.all() correctly, it's returned promise will be rejected immediately if any of the allProviders promises is rejected. So if any provider misbehaves, RunStateDependancy can end up stop spinning the event loop before all the providers have completed shutting down.

Comment 6

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

Attachment: Use AsyncShutdown module to wait for Addon Manager I/O to complete

gps, this is where we ended up after the discussion I had with you last week about needing to spin the event loop during Addon Manager shutdown. Several other projects within the Perf team need similar behaviour, so we built a shared module in bug 913899.

This patch depends on the (not quite past review) patch in bug 913899; if the API in that module changes, this patch will need to be updated.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 804194 [details] [diff] [review]
Use AsyncShutdown module to wait for Addon Manager I/O to complete

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

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +711,5 @@
> +          // methods that return values (or nothing) and methods that return promises.
> +          let providerResult = provider[aMethod].apply(provider, aArgs);
> +          let nextPromise = Promise.resolve(providerResult);
> +          // Log and swallow the errors from methods that do return promises.
> +          nextPromise = nextPromise.then(

Nit/curiosity: Isn't this assignment unneeded?

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a16ae206017

Comment 11

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

(In reply to Blair McBride [:Unfocused] from comment #9)
> Comment on attachment 804194 [details] [diff] [review]
> 
> ::: toolkit/mozapps/extensions/AddonManager.jsm
> @@ +711,5 @@
> > +          let nextPromise = Promise.resolve(providerResult);
> > +          // Log and swallow the errors from methods that do return promises.
> > +          nextPromise = nextPromise.then(
> 
> Nit/curiosity: Isn't this assignment unneeded?

Nope - this is the difference between

thing1.then(thing2).then(thing3), where thing3 gets the output of thing2 as its input

and

thing1.then(thing2)
thing1.then(thing3) (where both thing2 and 3 get the output of thing1)

In this case we want to carry forward the new promise (returned by .then) that won't reject with the underlying error, because the .then handled it.

This leads to a nifty (but perhaps too subtle) coding pattern:

return thing1.then(handlers);

lets you process the result of thing1 and then your handlers give the result to the caller (for example by hiding errors), whereas

thing1.then(handlers);
return thing1;

lets you do post-processing and/or cleanup after thing1, but the caller receives exactly the results of thing1. I use this pattern for error propagation in the XPI shutdown methods.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 804194 [details] [diff] [review]
Use AsyncShutdown module to wait for Addon Manager I/O to complete

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

Don't think you need me on this any more.

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6a16ae206017