Bug 1338280 - Add `AsyncShutdown` phase tags; use tags to shut down wrapped Places connections. draft
authorKit Cambridge <kit@yakshaving.ninja>
Wed, 22 Feb 2017 14:27:18 -0800
changeset 488407 03e6a3f5f3782719de19bb103e1d722073050e41
parent 488406 d30e2adc8d6ff44a8028fcdbbfea5965862e194c
child 488408 b8c700b5254dabdbaf915c54ea71e9d44bfd595e
push id46512
push userbmo:kit@mozilla.com
push dateThu, 23 Feb 2017 02:18:17 +0000
bugs1338280
milestone54.0a1
Bug 1338280 - Add `AsyncShutdown` phase tags; use tags to shut down wrapped Places connections. A tag is a unique ID passed as the `data` parameter to a shutdown notification. This makes it possible to restart Places multiple times, without invalidating the blocker after the first shutdown. MozReview-Commit-ID: Cos072ZDUAO
toolkit/components/asyncshutdown/AsyncShutdown.jsm
toolkit/components/places/PlacesUtils.jsm
--- a/toolkit/components/asyncshutdown/AsyncShutdown.jsm
+++ b/toolkit/components/asyncshutdown/AsyncShutdown.jsm
@@ -368,28 +368,39 @@ this.AsyncShutdown = {
     }
     if (accepted) {
       return getPhase;
     }
     return undefined;
   }
 };
 
+function makePhaseKey(topic, tag) {
+  if (tag === null) {
+    return topic;
+  }
+  if (typeof tag != "string") {
+    throw new TypeError(`Tag ${tag} must be a string`);
+  }
+  return topic + "|" + tag;
+}
+
 /**
  * Register a new phase.
  *
  * @param {string} topic The notification topic for this Phase.
  * @see {https://developer.mozilla.org/en-US/docs/Observer_Notifications}
  */
-function getPhase(topic) {
-  let phase = gPhases.get(topic);
+function getPhase(topic, tag = null) {
+  let key = makePhaseKey(topic, tag);
+  let phase = gPhases.get(key);
   if (phase) {
     return phase;
   }
-  let spinner = new Spinner(topic);
+  let spinner = new Spinner(topic, tag);
   phase = Object.freeze({
     /**
      * Register a blocker for the completion of a phase.
      *
      * @param {string} name The human-readable name of the blocker. Used
      * for debugging/error reporting. Please make sure that the name
      * respects the following model: "Some Service: some action in progress" -
      * for instance "OS.File: flushing all pending I/O";
@@ -471,29 +482,32 @@ function getPhase(topic) {
         // Ignore errors
       }
       if (accepted) {
         return () => spinner.observe();
       }
       return undefined;
     }
   });
-  gPhases.set(topic, phase);
+  gPhases.set(key, phase);
   return phase;
 }
 
 /**
  * Utility class used to spin the event loop until all blockers for a
  * Phase are satisfied.
  *
  * @param {string} topic The xpcom notification for that phase.
+ * @param {string} [tag] Tag for this phase, passed as the `data` parameter to
+ * the observer.
  */
-function Spinner(topic) {
+function Spinner(topic, tag = null) {
   this._barrier = new Barrier(topic);
   this._topic = topic;
+  this._tag = tag;
   Services.obs.addObserver(this, topic, false);
 }
 
 Spinner.prototype = {
   /**
    * Register a new condition for this phase.
    *
    * See the documentation of `addBlocker` in property `client`
@@ -517,18 +531,20 @@ Spinner.prototype = {
     return this._barrier.client.removeBlocker(condition);
   },
 
   get name() {
     return this._barrier.client.name;
   },
 
   // nsIObserver.observe
-  observe() {
-    let topic = this._topic;
+  observe(subject, topic, data) {
+    if (data !== this._tag) {
+      return;
+    }
     debug(`Starting phase ${ topic }`);
     Services.obs.removeObserver(this, topic);
 
     let satisfied = false; // |true| once we have satisfied all conditions
     let promise;
     try {
       promise = this._barrier.wait({
         warnAfterMS: DELAY_WARNING_MS,
@@ -1014,17 +1030,18 @@ Barrier.prototype = Object.freeze({
 // Ideally, phases should be registered from the component that decides
 // when they start/stop. For compatibility with existing startup/shutdown
 // mechanisms, we register a few phases here.
 
 // Parent process
 if (!isContent) {
   this.AsyncShutdown.profileChangeTeardown = getPhase("profile-change-teardown");
   this.AsyncShutdown.profileBeforeChange = getPhase("profile-before-change");
-  this.AsyncShutdown.placesClosingInternalConnection = getPhase("places-will-close-connection");
+  this.AsyncShutdown.getPlacesClosingInternalConnectionPhase =
+    dbTag => getPhase("places-will-close-connection", dbTag);
   this.AsyncShutdown.sendTelemetry = getPhase("profile-before-change-telemetry");
 }
 
 // Notifications that fire in the parent and content process, but should
 // only have phases in the parent process.
 if (!isContent) {
   this.AsyncShutdown.quitApplicationGranted = getPhase("quit-application-granted");
 }
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -330,16 +330,21 @@ this.PlacesUtils = {
   TOPIC_EXPIRATION_FINISHED: "places-expiration-finished",
   TOPIC_FEEDBACK_UPDATED: "places-autocomplete-feedback-updated",
   TOPIC_FAVICONS_EXPIRED: "places-favicons-expired",
   TOPIC_VACUUM_STARTING: "places-vacuum-starting",
   TOPIC_BOOKMARKS_RESTORE_BEGIN: "bookmarks-restore-begin",
   TOPIC_BOOKMARKS_RESTORE_SUCCESS: "bookmarks-restore-success",
   TOPIC_BOOKMARKS_RESTORE_FAILED: "bookmarks-restore-failed",
 
+  PREF_DATABASE_TESTING_ENABLED: "places.database.testing.enabled",
+
+  _asyncDBConnPromised: null,
+  _asyncDBWrapperPromised: null,
+
   asContainer: aNode => asContainer(aNode),
   asQuery: aNode => asQuery(aNode),
 
   endl: NEWLINE,
 
   /**
    * Makes a URI from a spec.
    * @param   aSpec
@@ -1398,31 +1403,61 @@ this.PlacesUtils = {
     if (!wasOpen) {
       root.containerOpen = false;
       if (!didSuppressNotifications)
         result.suppressNotifications = false;
     }
     return urls;
   },
 
+  // Indicates whether testing conveniences are enabled. If true, the wrapped
+  // database connection will be reset on shutdown, allowing tests to completely
+  // shut down and restart Places.
+  _isDBTestingEnabled() {
+    let testingEnabled = false;
+    try {
+      testingEnabled =
+        Services.prefs.getBoolPref(this.PREF_DATABASE_TESTING_ENABLED);
+    } catch (ex) {}
+    return testingEnabled;
+  },
+
   /**
    * Gets a shared Sqlite.jsm readonly connection to the Places database,
    * usable only for SELECT queries.
    *
    * This is intended to be used mostly internally, components outside of
    * Places should, when possible, use API calls and file bugs to get proper
    * APIs, where they are missing.
    * Keep in mind the Places DB schema is by no means frozen or even stable.
    * Your custom queries can - and will - break overtime.
    *
    * Example:
    * let db = yield PlacesUtils.promiseDBConnection();
    * let rows = yield db.executeCached(sql, params);
    */
-  promiseDBConnection: () => gAsyncDBConnPromised,
+  promiseDBConnection() {
+    if (!this._asyncDBConnPromised) {
+      // Check if testing is enabled before registering the shutdown blocker,
+      // for consistency with the C++ database code.
+      let testingEnabled = this._isDBTestingEnabled();
+      this._asyncDBConnPromised = Sqlite.cloneStorageConnection({
+        connection: PlacesUtils.history.DBConnection,
+        readOnly:   true
+      }).then(conn => {
+        setupDbForShutdown(conn, PlacesUtils.history.DBTag, "PlacesUtils read-only connection", () => {
+          if (testingEnabled) {
+            this._asyncDBConnPromised = null;
+          }
+        });
+        return conn;
+      }).catch(Cu.reportError);
+    }
+    return this._asyncDBConnPromised;
+  },
 
   /**
    * Performs a read/write operation on the Places database through a Sqlite.jsm
    * wrapped connection to the Places database.
    *
    * This is intended to be used only by Places itself, always use APIs if you
    * need to modify the Places database. Use promiseDBConnection if you need to
    * SELECT from the database and there's no covering API.
@@ -1441,22 +1476,34 @@ this.PlacesUtils = {
    * }));
    *
    * @param {string} name The name of the operation. Used for debugging, logging
    *   and crash reporting.
    * @param {function(db)} task A function that takes as argument a Sqlite.jsm
    *   connection and returns a Promise. Shutdown is guaranteed to not interrupt
    *   execution of `task`.
    */
-  withConnectionWrapper: (name, task) => {
+  withConnectionWrapper(name, task) {
     if (!name) {
       throw new TypeError("Expecting a user-readable name");
     }
-    return Task.spawn(function*() {
-      let db = yield gAsyncDBWrapperPromised;
+    if (!this._asyncDBWrapperPromised) {
+      let testingEnabled = this._isDBTestingEnabled();
+      this._asyncDBWrapperPromised = Sqlite.wrapStorageConnection({
+          connection: PlacesUtils.history.DBConnection,
+      }).then(conn => {
+        setupDbForShutdown(conn, PlacesUtils.history.DBTag, "PlacesUtils wrapped connection", () => {
+          if (testingEnabled) {
+            this._asyncDBWrapperPromised = null;
+          }
+        });
+        return conn;
+      }).catch(Cu.reportError);
+    }
+    return this._asyncDBWrapperPromised.then(function(db) {
       return db.executeBeforeShutdown(name, task);
     });
   },
 
   /**
    * Given a uri returns list of itemIds associated to it.
    *
    * @param aURI
@@ -2054,76 +2101,61 @@ XPCOMUtils.defineLazyGetter(this, "bundl
  *
  * 1. Places initiates shutdown.
  * 2. Before places can move to the step where it closes the low-level connection,
  *   we need to make sure that we have closed `conn`.
  * 3. Before we can close `conn`, we need to make sure that all external clients
  *   have stopped using `conn`.
  * 4. Before we can close Sqlite, we need to close `conn`.
  */
-function setupDbForShutdown(conn, name) {
+function setupDbForShutdown(conn, dbTag, name, afterShutdown) {
   try {
     let state = "0. Not started.";
     let promiseClosed = new Promise((resolve, reject) => {
       // The service initiates shutdown.
       // Before it can safely close its connection, we need to make sure
       // that we have closed the high-level connection.
       try {
-        AsyncShutdown.placesClosingInternalConnection.addBlocker(`${name} closing as part of Places shutdown`,
+        let phase = AsyncShutdown.getPlacesClosingInternalConnectionPhase(dbTag);
+        phase.addBlocker(`${name} closing as part of ${dbTag} shutdown`,
           Task.async(function*() {
             state = "1. Service has initiated shutdown";
 
             // At this stage, all external clients have finished using the
             // database. We just need to close the high-level connection.
             yield conn.close();
             state = "2. Closed Sqlite.jsm connection.";
 
+            afterShutdown();
             resolve();
           }),
           () => state
         );
       } catch (ex) {
         // It's too late to block shutdown, just close the connection.
         conn.close();
+        afterShutdown();
         reject(ex);
       }
     });
 
     // Make sure that Sqlite.jsm doesn't close until we are done
     // with the high-level connection.
     Sqlite.shutdown.addBlocker(`${name} must be closed before Sqlite.jsm`,
       () => promiseClosed.catch(Cu.reportError),
       () => state
     );
   } catch (ex) {
     // It's too late to block shutdown, just close the connection.
     conn.close();
+    afterShutdown();
     throw ex;
   }
 }
 
-XPCOMUtils.defineLazyGetter(this, "gAsyncDBConnPromised",
-  () => Sqlite.cloneStorageConnection({
-    connection: PlacesUtils.history.DBConnection,
-    readOnly:   true
-  }).then(conn => {
-      setupDbForShutdown(conn, "PlacesUtils read-only connection");
-      return conn;
-  }).catch(Cu.reportError)
-);
-
-XPCOMUtils.defineLazyGetter(this, "gAsyncDBWrapperPromised",
-  () => Sqlite.wrapStorageConnection({
-      connection: PlacesUtils.history.DBConnection,
-  }).then(conn => {
-    setupDbForShutdown(conn, "PlacesUtils wrapped connection");
-    return conn;
-  }).catch(Cu.reportError)
-);
-
 /**
  * Keywords management API.
  * Sooner or later these keywords will merge with search keywords, this is an
  * interim API that should then be replaced by a unified one.
  * Keywords are associated with URLs and can have POST data.
  * A single URL can have multiple keywords, provided they differ by POST data.
  */
 var Keywords = {