At the moment, we naively assume that the bootstrap shutdown() and uninstall() functions do everything synchronously. ie, we call them, and continue on doing work as soon as they return. However, add-ons may need to do some async work. Currently, that may or may not work, depending on what they do - the sandbox will be kept alive by a reference, but they may depend on mappings in their chrome.manifest. However, if we start nuking sandboxes (bug 771409), then any async work is pretty much guaranteed to fail. And if it's an upgrade, then the old version can still be running while the new version starts up. However, there is the issue of something going wrong, and the add-on not notifying the Add-ons Manager that they've completed the async work (or possibly just take far too long). So we'll need some timeout (5 seconds?), after which we just pretend we got notified that the async work has completed, and continue on. I can think of a few possible approaches to altering the bootstrap.js API to support this: * Detect and use async versions of the methods - shutdownAsync() and uninstallAsync() * Add an additional parameter to the bootstrap functions that is a callback function. bootstrap.js will need to signal somehow that they'll use this, either by a separate API, or have the bootstrap function return a special value. * Have the bootstrap functions return a Promise. If anything but a Promise is returned, then we assume it's all sync. This requires defining a Promise API in bootstrap.js. * Add an additional parameter to the bootstrap function that is an object similar to an opt-in Promise. The add-on code would call .wait() on that object before returning, then call .done() when it's finished. Of those, the 3rd method seems nicest to me. It's easily opt-in, flexible, and doesn't pollute the bootstrap.js scope.

(In reply to Blair McBride (:Unfocused) from comment #0) > * Have the bootstrap functions return a Promise. If anything but a Promise > is returned, then we assume it's all sync. This requires defining a Promise > API in bootstrap.js. This seems to make sense to me, a couple of questions: When you say "assume it's all sync" do you mean that we nuke the sandbox immediately or wait on a timeout still? At what point do you think we update the UI etc. to show the add-on is disabled, after the promise is resolved or immediately? Bug 708984 will be adding a shared promise implementation, I suggest we make use of it.

Bah, by "3rd" I actually meant "4th"/"last" (added one after I wrote that part). Still, 3rd is my other top choice. (In reply to Dave Townsend (:Mossop) from comment #1) > Bug 708984 will be adding a shared promise implementation, I suggest we make > use of it. Yep. My only issue is polluting the bootstrap.js scope (thus my slight preference to option 4). I haven't checked yet if any addons on AMO are already using a Promise implementation in bootstrap.js. > When you say "assume it's all sync" do you mean that we nuke the sandbox > immediately or wait on a timeout still? Nuke it immediately. I'd expect most addons won't need the async functionality, so defaulting to doing everything immediately will keep us responsive. > At what point do you think we update the UI etc. to show the add-on is > disabled, after the promise is resolved or immediately? Both. I'd like to keep the UI as responsive as possible - which means updating it immediately. But we'd need an intermediate state. eg, if you're disabling, the Enable button wouldn't be available until the promise is resolved - to avoid enabling before the addon is truely disabled. I think that's doable without any API changes (onDisabling, wait, onDisabled - with Addon.permissions updated accordingly), but it'd be nice to have some affordance to indicate the transition state which probably would need an API change.

(In reply to Blair McBride (:Unfocused) from comment #2) > Bah, by "3rd" I actually meant "4th"/"last" (added one after I wrote that > part). Still, 3rd is my other top choice. > > (In reply to Dave Townsend (:Mossop) from comment #1) > > Bug 708984 will be adding a shared promise implementation, I suggest we make > > use of it. > > Yep. My only issue is polluting the bootstrap.js scope (thus my slight > preference to option 4). I haven't checked yet if any addons on AMO are > already using a Promise implementation in bootstrap.js. I can live with the 4th option too. Probably easier to implement in fact and could use a promise internally ;) I'd probably stick it on the data object that we pass around, easier for bootstrap scripts to detect it there by name rather than counting the arguments passed.

(In reply to Dave Townsend (:Mossop) from comment #3) > I'd probably stick it on the data object that we pass around, easier for > bootstrap scripts to detect it there by name rather than counting the > arguments passed. Oh, yes, I like that better.

Alternate crazy idea. How about just call startup/shutdown normally. If they return a promise then assume they are async and wait till resolved?

(In reply to Dave Townsend [:mossop] from comment #6) > Alternate crazy idea. How about just call startup/shutdown normally. If they > return a promise then assume they are async and wait till resolved? Oh you said that already. I'll shut up now

I assume that the plan is to continue starting Firefox while async operations are happening in add-ons but to block Firefox shutdown until async operations are complete?

(In reply to Dave Townsend [:mossop] from comment #8) > I assume that the plan is to continue starting Firefox while async > operations are happening in add-ons but to block Firefox shutdown until > async operations are complete? Sounds good to me.

What seemed like a fun simplish project is actually crazily complicated by the need to run uninstall bootstrap scripts during synchronous startup. We could spin an event loop I guess but either way I can't dedicate as much time as this needs right now