addon gets stuck in weird state

RESOLVED FIXED in mozilla26

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dietrich, Assigned: Irving)

Tracking

({regression})

unspecified
mozilla26
x86
Mac OS X
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 798354 [details]
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
(Reporter)

Comment 1

5 years ago
Created attachment 798355 [details]
extensions.sqlite
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.
Assignee: nobody → irving
Blocks: 853388
Blocks: 903093
Created attachment 801993 [details] [diff] [review]
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.
Attachment #801993 - Flags: feedback?(felipc)
Attachment #801993 - Flags: feedback?(dteller)
Attachment #801993 - Flags: feedback?(bmcbride)
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.
Attachment #801993 - Flags: feedback?(dteller) → feedback+
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.
Attachment #801993 - Flags: feedback?(bmcbride) → feedback+
Created attachment 804194 [details] [diff] [review]
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.
Attachment #801993 - Attachment is obsolete: true
Attachment #801993 - Flags: feedback?(felipc)
Attachment #804194 - Flags: review?(bmcbride)
Attachment #804194 - Flags: feedback?(gps)
Duplicate of this bug: 911820
Status: NEW → ASSIGNED
Keywords: regression
Whiteboard: [mozmill]
Duplicate of this bug: 903093
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?
Attachment #804194 - Flags: review?(bmcbride) → review+
(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 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.
Attachment #804194 - Flags: feedback?(gps)
https://hg.mozilla.org/mozilla-central/rev/6a16ae206017
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.