Bug 1358907 Part 2 Wait for database load before shutting down draft
authorAndrew Swan <aswan@mozilla.com>
Fri, 30 Jun 2017 10:09:59 -0700
changeset 605543 7913e2acfdf14f490a1e0b8dd55823b8350dfeef
parent 605542 0c82971f8390d5519291fc4560535d26b8868c22
child 605544 470b01c48959221799a7aa0ec4419cfa97b5dfea
push id67445
push useraswan@mozilla.com
push dateFri, 07 Jul 2017 22:57:23 +0000
bugs1358907
milestone56.0a1
Bug 1358907 Part 2 Wait for database load before shutting down Make sure the XPI database is fully loaded before running the XPIDatabase.shutdown() logic. This race has been present for a long time but it suddenly became much more common when loading of the XPI database was deferred until after startup. MozReview-Commit-ID: 1llKuH3It19
toolkit/mozapps/extensions/internal/XPIProviderUtils.js
toolkit/mozapps/extensions/test/xpcshell/test_corrupt.js
--- a/toolkit/mozapps/extensions/internal/XPIProviderUtils.js
+++ b/toolkit/mozapps/extensions/internal/XPIProviderUtils.js
@@ -654,17 +654,17 @@ this.XPIDatabase = {
   },
 
   /**
    * Shuts down the database connection and releases all cached objects.
    * Return: Promise{integer} resolves / rejects with the result of the DB
    *                          flush after the database is flushed and
    *                          all cleanup is done
    */
-  shutdown() {
+  async shutdown() {
     logger.debug("shutdown");
     if (this.initialized) {
       // If our last database I/O had an error, try one last time to save.
       if (this.lastError)
         this.saveChanges();
 
       this.initialized = false;
 
@@ -672,38 +672,43 @@ this.XPIDatabase = {
         AddonManagerPrivate.recordSimpleMeasure(
             "XPIDB_saves_total", this._deferredSave.totalSaves);
         AddonManagerPrivate.recordSimpleMeasure(
             "XPIDB_saves_overlapped", this._deferredSave.overlappedSaves);
         AddonManagerPrivate.recordSimpleMeasure(
             "XPIDB_saves_late", this._deferredSave.dirty ? 1 : 0);
       }
 
-      // Return a promise that any pending writes of the DB are complete and we
-      // are finished cleaning up
-      let flushPromise = this.flush();
-      flushPromise.catch(error => {
-          logger.error("Flush of XPI database failed", error);
-          AddonManagerPrivate.recordSimpleMeasure("XPIDB_shutdownFlush_failed", 1);
-          // If our last attempt to read or write the DB failed, force a new
-          // extensions.ini to be written to disk on the next startup
-          Services.prefs.setBoolPref(PREF_PENDING_OPERATIONS, true);
-        })
-        .then(count => {
-          // Clear out the cached addons data loaded from JSON
-          delete this.addonDB;
-          delete this._dbPromise;
-          // same for the deferred save
-          delete this._deferredSave;
-          // re-enable the schema version setter
-          delete this._schemaVersionSet;
-        });
-      return flushPromise;
+      // If we're shutting down while still loading, finish loading
+      // before everything else!
+      if (this._dbPromise) {
+        await this._dbPromise;
+      }
+
+      // Await and pending DB writes and finish cleaning up.
+      try {
+        await this.flush();
+      } catch (error) {
+        logger.error("Flush of XPI database failed", error);
+        AddonManagerPrivate.recordSimpleMeasure("XPIDB_shutdownFlush_failed", 1);
+        // If our last attempt to read or write the DB failed, force a new
+        // extensions.ini to be written to disk on the next startup
+        Services.prefs.setBoolPref(PREF_PENDING_OPERATIONS, true);
+
+        throw error;
+      }
+
+      // Clear out the cached addons data loaded from JSON
+      delete this.addonDB;
+      delete this._dbPromise;
+      // same for the deferred save
+      delete this._deferredSave;
+      // re-enable the schema version setter
+      delete this._schemaVersionSet;
     }
-    return Promise.resolve(0);
   },
 
   /**
    * Asynchronously list all addons that match the filter function
    * @param  aFilter
    *         Function that takes an addon instance and returns
    *         true if that addon should be included in the selected array
    * @param  aCallback
--- a/toolkit/mozapps/extensions/test/xpcshell/test_corrupt.js
+++ b/toolkit/mozapps/extensions/test/xpcshell/test_corrupt.js
@@ -250,16 +250,25 @@ function run_test_1() {
     // Shutdown and replace the database with a corrupt file (a directory
     // serves this purpose). On startup the add-ons manager won't rebuild
     // because there is a file there still.
     shutdownManager();
     gExtensionsJSON.remove(true);
     gExtensionsJSON.create(AM_Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
     startupManager(false);
 
+    // Load the database.
+    awaitPromise(new Promise(resolve => {
+      Services.obs.addObserver(function listener() {
+        Services.obs.removeObserver(listener, "xpi-database-loaded");
+        resolve();
+      }, "xpi-database-loaded");
+      Services.obs.notifyObservers(null, "sessionstore-windows-restored");
+    }));
+
     // Accessing the add-ons should open and recover the database
     AddonManager.getAddonsByIDs(["addon1@tests.mozilla.org",
                                  "addon2@tests.mozilla.org",
                                  "addon3@tests.mozilla.org",
                                  "addon4@tests.mozilla.org",
                                  "addon5@tests.mozilla.org",
                                  "addon6@tests.mozilla.org",
                                  "addon7@tests.mozilla.org",