At the moment, bootstrap methods are all assumed to be synchronous. This tends to cause a lot of issues. But, in particular, when a shutdown method has to do asynchronous work during an upgrade, it means that the new version of the add-on may be loaded, and its startup method called, before shutdown of the old version is done. We do some work to hack around this for WebExtensions, but we should really just handle it in the add-on manager instead.

Comment on attachment 8975306 [details] Bug 1461145: Support async startup/shutdown bootstrap methods. https://reviewboard.mozilla.org/r/243634/#review251514 We have a ton of tests that are now doing extra unnecessary checks for asynchronous webextension startup/shutdown, do you want to just clean those up on a case-by-case basis? I worry that given how often code gets cut&pasted in tests that its going to be hard to stamp out... ::: toolkit/components/extensions/test/xpcshell/test_ext_webRequest_startup.js:129 (Diff revision 2) > }); > > // Test that a block listener that uses filterResponseData() works > // properly (i.e., that the delayed call to registerTraceableChannel > // works properly). > -add_task(async function() { > +add_task(async function test_3_there_is_a_special_place_in_hell_for_people_who_dont_name_their_test_tasks() { okay mea culpa, but please change this before landing ::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:113 (Diff revision 2) > - hook: null, > - status: null, > - profileBeforeChange: { > - addBlocker(name, blocker, options) { > + get status() { > + let blocker = this.profileBeforeChange.blockers[0]; > + return blocker && blocker.options && blocker.options.fetchState; > + }, this looks specific to a single test, can we remove it here? ::: toolkit/mozapps/extensions/internal/XPIDatabase.jsm:2225 (Diff revision 2) > * True if we're disabling this add-on because we're selecting > * another. > - * @returns {boolean?} > + * @returns {Promise<boolean?>} > * A tri-state indicating the action taken for the add-on: > * - undefined: The add-on did not change state > * - true: The add-on because disabled became ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1528 (Diff revision 2) > - this.callBootstrapMethod("shutdown", reason, aExtraParams); > + this.shutdownPromise = this._shutdown(reason, aExtraParams); > + await this.shutdownPromise; > + this.shutdownPromise = null; The fact that startupPromise is set inside `callBootstrapMethod("startup", ...)` but shutdownPromise is just set in `shutdown()` is annoying, can we standardize on setting both promises in either `callBootstrapMethod()` or in the wrappers? ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1997 (Diff revision 2) > - } > + } > > - // If the add-on was pending disable then shut it down and remove it > + // If the add-on was pending disable then shut it down and remove it > - // from the persisted data. > + // from the persisted data. > - let reason = BOOTSTRAP_REASONS.APP_SHUTDOWN; > + let reason = BOOTSTRAP_REASONS.APP_SHUTDOWN; > - if (addon._pendingDisable) { > + if (addon._disable) { is this just in need of a rebase? ::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:141 (Diff revision 2) > - await AddonManager.installTemporaryAddon(addonDir); > - await promiseWebExtensionStartup(); > + await Promise.all([ > + promiseWebExtensionStartup(), > + AddonManager.installTemporaryAddon(addonDir), > + ]); just waiting for installTemporaryAddon() should be sufficient now

(In reply to Andrew Swan [:aswan] from comment #3) > We have a ton of tests that are now doing extra unnecessary checks for > asynchronous webextension startup/shutdown, do you want to just clean those > up on a case-by-case basis? I worry that given how often code gets > cut&pasted in tests that its going to be hard to stamp out... I was thinking about doing a follow-up for that. A lot of those tests are a mess, and will require some detangling to figure out what we can remove. > ::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:113 > (Diff revision 2) > > - hook: null, > > - status: null, > > - profileBeforeChange: { > > - addBlocker(name, blocker, options) { > > + get status() { > > + let blocker = this.profileBeforeChange.blockers[0]; > > + return blocker && blocker.options && blocker.options.fetchState; > > + }, > > this looks specific to a single test, can we remove it here? Probably. > ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1528 > (Diff revision 2) > > - this.callBootstrapMethod("shutdown", reason, aExtraParams); > > + this.shutdownPromise = this._shutdown(reason, aExtraParams); > > + await this.shutdownPromise; > > + this.shutdownPromise = null; > > The fact that startupPromise is set inside `callBootstrapMethod("startup", > ...)` but shutdownPromise is just set in `shutdown()` is annoying, can we > standardize on setting both promises in either `callBootstrapMethod()` or in > the wrappers? Maybe. > ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1997 > (Diff revision 2) > > - } > > + } > > > > - // If the add-on was pending disable then shut it down and remove it > > + // If the add-on was pending disable then shut it down and remove it > > - // from the persisted data. > > + // from the persisted data. > > - let reason = BOOTSTRAP_REASONS.APP_SHUTDOWN; > > + let reason = BOOTSTRAP_REASONS.APP_SHUTDOWN; > > - if (addon._pendingDisable) { > > + if (addon._disable) { > > is this just in need of a rebase? Something like that. The code in my local branch is correct, but there's no way mozreview is going to give you an inter-diff that makes sense when I push an update.

(In reply to Andrew Swan [:aswan] from comment #3) > We have a ton of tests that are now doing extra unnecessary checks for > asynchronous webextension startup/shutdown, do you want to just clean those > up on a case-by-case basis? I worry that given how often code gets > cut&pasted in tests that its going to be hard to stamp out... I updated the simple cases. The cases involving non-temporary installs and provider startup still need some work, though.

Comment on attachment 8975306 [details] Bug 1461145: Support async startup/shutdown bootstrap methods. https://reviewboard.mozilla.org/r/243634/#review252300 ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1968 (Diff revision 4) > - shutdown(reason, aExtraParams) { > - this.callBootstrapMethod("shutdown", reason, aExtraParams); > + async shutdown(reason, aExtraParams) { > + this.shutdownPromise = this._shutdown(reason, aExtraParams); > + await this.shutdownPromise; > + this.shutdownPromise = null; > + } > + > + async _shutdown(reason, aExtraParams) { > + await this.startupPromise; > + return this.callBootstrapMethod("shutdown", reason, aExtraParams); > } This is an improvement but is there a reason we can't make `startup()` and `shutdown()` totally symmetric? ie, inline `_shutdown` into `shutdown` and use the same technique in both (ie, either await {startup,shutdown}Promise or return it)

Comment on attachment 8975306 [details] Bug 1461145: Support async startup/shutdown bootstrap methods. https://reviewboard.mozilla.org/r/243634/#review252300 > This is an improvement but is there a reason we can't make `startup()` and `shutdown()` totally symmetric? ie, inline `_shutdown` into `shutdown` and use the same technique in both (ie, either await {startup,shutdown}Promise or return it) For startup, we check the startupPromise elsewhere, e.g., in the add-on manager UI, so we technically want it to stick around for the lifetime of the add-on. For shutdown, we really want it to go away as soon as the shutdown method finishes executing. So there's a difference in behavior that we want, not just in implementation.

https://hg.mozilla.org/integration/mozilla-inbound/rev/09cdbde4184ef10e5278539ef8f59c432dda8128 Bug 1461145: Support async startup/shutdown bootstrap methods. r=aswan

Pushed by maglione.k@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/09cdbde4184e Support async startup/shutdown bootstrap methods. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/973e0f9c1232cb9c1d425b6d4c94b91367aa35a2 Bug 1461145: Follow-up: Fix racy tests. r=trivial,test-only

https://hg.mozilla.org/integration/mozilla-inbound/rev/60a031dd99f49058e7b6a5d7d0a45ab1c7885134 Bug 1461145: Follow-up: Fix more racy tests. r=bustage,test-only

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4d1f1c2e30ff501d267926b3d48def81b7179a Bug 1461145: Follow-up: Temporarily disable test task for too many failures. r=bustage,test-only DONTBUILD

https://hg.mozilla.org/integration/mozilla-inbound/rev/4843891ea2288c743d056661d3da76dcb0492b8c Bug 1461145: Follow-up follow-up: Fix setTimeout flakiness in tests that load SpecialPowersObserverAPI. r=bustage

https://hg.mozilla.org/mozilla-central/rev/09cdbde4184e https://hg.mozilla.org/mozilla-central/rev/973e0f9c1232 https://hg.mozilla.org/mozilla-central/rev/60a031dd99f4 https://hg.mozilla.org/mozilla-central/rev/2b4d1f1c2e30 https://hg.mozilla.org/mozilla-central/rev/4843891ea228

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d5a08599487e879c06bf7293c9cba23e4105c16 Bug 1461145: Follow-up: Fix more racy tests. r=bustage CLOSED TREE

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b406fa5a8a35fe00792d698bedd58eef07ad367 Bug 1461145: Follow-up: Fix still more racy tests. r=bustage,test-only

https://hg.mozilla.org/mozilla-central/rev/6d5a08599487 https://hg.mozilla.org/mozilla-central/rev/6b406fa5a8a3

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.