903818 - Handle shutdown during asynchronous load of JSON XPI Database

Status: RESOLVED INACTIVE

(Toolkit :: Add-ons Manager, defect)

Description

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

Attachment: async-load-shutdown

There is an obscure race condition if the very first time the JSON database is touched is just before shutdown, leading to the possible sequence:

1) Client requests Addon data through async API
2) XPIProviderUtils.js starts async read of DB
3) profile-before-change shuts down XPIProvider
4) Async read completes, and tries to initialize internal data structures

This was observed in a misbehaving unit test (toolkit/mozapps/extensions/test/xpcshell/test_bug619730.js), and was worked around in bug 853388 by modifying the test to always wait for that async activity to complete; see https://bugzilla.mozilla.org/show_bug.cgi?id=853388#c111

Attached is a rough patch that detects when we shut down during async read, logs a warning and returns errors for subsequent attempts to use the XPI database.

Unfortunately the AddonManager async APIs have no way to report errors back to clients, so the best we can do is return a default response (null or empty list). This leaves me concerned that some clients of the AddonManager APIs could end up with inconsistent internal state if they record a change and then fail to sync that change into the AddonManager; this is roughly what was happening (to the blocklist service) when test_bug619730.js failed.

As shutdown races go, this one feels pretty obscure to me (though I suppose it would be wise to use Telemetry to confirm that).

Comment 1

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

Comment on attachment 788600 [details] [diff] [review]
async-load-shutdown

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

::: toolkit/mozapps/extensions/XPIProviderUtils.js
@@ +706,5 @@
>            return this.addonDB;
>          }
> +        if (this._shuttingDown) {
> +          // throw over to the error handler in the next .then()
> +          throw new Error("Async XPI read succeeded");

That error message is very confusing.

@@ +1014,5 @@
> +      // safest to wait for the read to finish
> +      if (this._dbPromise) {
> +        WARN("XPI Provider shutting down during async load of database");
> +        this._shuttingDown = true;
> +        // Should always fail with "using DB after shutdown" error

Nit: That's not exactly what the error you're actually throwing.

Comment 2

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.