Last Comment Bug 903818 - Handle shutdown during asynchronous load of JSON XPI Database
: Handle shutdown during asynchronous load of JSON XPI Database
Status: NEW
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
: Andy McKay [:andym]
Depends on: 853388
  Show dependency treegraph
Reported: 2013-08-10 19:19 PDT by :Irving Reid (No longer working on Firefox)
Modified: 2013-08-14 11:44 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

async-load-shutdown (3.88 KB, patch)
2013-08-10 19:19 PDT, :Irving Reid (No longer working on Firefox)
blair: feedback+
dteller: feedback+
Details | Diff | Splinter Review

Description User image :Irving Reid (No longer working on Firefox) 2013-08-10 19:19:15 PDT
Created attachment 788600 [details] [diff] [review]

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

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 User image David Teller [:Yoric] (please use "needinfo") 2013-08-13 07:32:13 PDT
Comment on attachment 788600 [details] [diff] [review]

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.

Note You need to log in before you can comment on or make changes to this bug.