Bug 985655 - Ensure that Sqlite.jsm doesn't shutdown before its clients. r=mak
authorDavid Rajchenbach-Teller <dteller@mozilla.com>
Tue, 13 May 2014 01:00:00 -0400
changeset 182885 a2405cd005f696cb98e7e991fcb6fb3e2f484c8a
parent 182884 01bc471d7a7f35e19b147c387dfabf1d0c4eb909
child 182886 decf35a875bf8ccff2539fb780076b6eec8e21dc
push id26772
push userryanvm@gmail.com
push dateTue, 13 May 2014 20:03:29 +0000
treeherdermozilla-central@08070d4b85f4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs985655
milestone32.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 985655 - Ensure that Sqlite.jsm doesn't shutdown before its clients. r=mak
toolkit/modules/Sqlite.jsm
toolkit/modules/tests/xpcshell/test_sqlite_shutdown.js
toolkit/modules/tests/xpcshell/xpcshell.ini
--- a/toolkit/modules/Sqlite.jsm
+++ b/toolkit/modules/Sqlite.jsm
@@ -5,36 +5,104 @@
 "use strict";
 
 this.EXPORTED_SYMBOLS = [
   "Sqlite",
 ];
 
 const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
-Cu.import("resource://gre/modules/Promise.jsm");
-Cu.import("resource://gre/modules/osfile.jsm");
-Cu.import("resource://gre/modules/Services.jsm");
-Cu.import("resource://gre/modules/XPCOMUtils.jsm");
-Cu.import("resource://gre/modules/Log.jsm");
+Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
 
 XPCOMUtils.defineLazyModuleGetter(this, "AsyncShutdown",
                                   "resource://gre/modules/AsyncShutdown.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "Promise",
+                                  "resource://gre/modules/Promise.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "Services",
+                                  "resource://gre/modules/Services.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "OS",
+                                  "resource://gre/modules/osfile.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "Log",
+                                  "resource://gre/modules/Log.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "CommonUtils",
                                   "resource://services-common/utils.js");
 XPCOMUtils.defineLazyModuleGetter(this, "FileUtils",
                                   "resource://gre/modules/FileUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Task",
                                   "resource://gre/modules/Task.jsm");
 
 
 // Counts the number of created connections per database basename(). This is
 // used for logging to distinguish connection instances.
 let connectionCounters = new Map();
 
+/**
+ * Once `true`, reject any attempt to open or close a database.
+ */
+let isClosed = false;
+
+/**
+ * Barriers used to ensure that Sqlite.jsm is shutdown after all
+ * its clients.
+ */
+XPCOMUtils.defineLazyGetter(this, "Barriers", () => {
+  let Barriers = {
+    /**
+     * Public barrier that clients may use to add blockers to the
+     * shutdown of Sqlite.jsm. Triggered by profile-before-change.
+     * Once all blockers of this barrier are lifted, we close the
+     * ability to open new connections.
+     */
+    shutdown: new AsyncShutdown.Barrier("Sqlite.jsm: wait until all clients have completed their task"),
+
+    /**
+     * Private barrier blocked by connections that are still open.
+     * Triggered after Barriers.shutdown is lifted and `isClosed` is
+     * set to `true`.
+     */
+    connections: new AsyncShutdown.Barrier("Sqlite.jsm: wait until all connections are closed"),
+  };
+
+  /**
+   * Ensure that Sqlite.jsm:
+   * - informs its clients before shutting down;
+   * - lets clients open connections during shutdown, if necessary;
+   * - waits for all connections to be closed before shutdown.
+   */
+  AsyncShutdown.profileBeforeChange.addBlocker("Sqlite.jsm shutdown blocker",
+    Task.async(function* () {
+      yield Barriers.shutdown.wait();
+      // At this stage, all clients have had a chance to open (and close)
+      // their databases. Some previous close operations may still be pending,
+      // so we need to wait until they are complete before proceeding.
+
+      // Prevent any new opening.
+      isClosed = true;
+
+      // Now, wait until all databases are closed
+      yield Barriers.connections.wait();
+    }),
+
+    function status() {
+      if (isClosed) {
+        // We are waiting for the connections to close. The interesting
+        // status is therefore the list of connections still pending.
+        return { description: "Waiting for connections to close",
+                 status: Barriers.connections.status };
+      }
+
+      // We are still in the first stage: waiting for the barrier
+      // to be lifted. The interesting status is therefore that of
+      // the barrier.
+      return { description: "Waiting for the barrier to be lifted",
+               status: Barriers.shutdown.status };
+  });
+
+  return Barriers;
+});
 
 /**
  * Opens a connection to a SQLite database.
  *
  * The following parameters can control the connection:
  *
  *   path -- (string) The filesystem path of the database file to open. If the
  *       file does not exist, a new database will be created.
@@ -67,16 +135,21 @@ let connectionCounters = new Map();
  */
 function openConnection(options) {
   let log = Log.repository.getLogger("Sqlite.ConnectionOpener");
 
   if (!options.path) {
     throw new Error("path not specified in connection options.");
   }
 
+  if (isClosed) {
+    throw new Error("Sqlite.jsm has been shutdown. Cannot open connection to: " + options.path);
+  }
+
+
   // Retains absolute paths and normalizes relative as relative to profile.
   let path = OS.Path.join(OS.Constants.Path.profileDir, options.path);
 
   let sharedMemoryCache = "sharedMemoryCache" in options ?
                             options.sharedMemoryCache : true;
 
   let openedOptions = {};
 
@@ -156,17 +229,21 @@ function openConnection(options) {
 function cloneStorageConnection(options) {
   let log = Log.repository.getLogger("Sqlite.ConnectionCloner");
 
   let source = options && options.connection;
   if (!source) {
     throw new TypeError("connection not specified in clone options.");
   }
   if (!source instanceof Ci.mozIStorageAsyncConnection) {
-    throw new TypeError("Connection must be a valid Storage connection.")
+    throw new TypeError("Connection must be a valid Storage connection.");
+  }
+
+  if (isClosed) {
+    throw new Error("Sqlite.jsm has been shutdown. Cannot close connection to: " + source.database.path);
   }
 
   let openedOptions = {};
 
   if ("shrinkMemoryOnConnectionIdleMS" in options) {
     if (!Number.isInteger(options.shrinkMemoryOnConnectionIdleMS)) {
       throw new TypeError("shrinkMemoryOnConnectionIdleMS must be an integer. " +
                           "Got: " + options.shrinkMemoryOnConnectionIdleMS);
@@ -274,16 +351,23 @@ function OpenedConnection(connection, ba
 
   this._idleShrinkMS = options.shrinkMemoryOnConnectionIdleMS;
   if (this._idleShrinkMS) {
     this._idleShrinkTimer = Cc["@mozilla.org/timer;1"]
                               .createInstance(Ci.nsITimer);
     // We wait for the first statement execute to start the timer because
     // shrinking now would not do anything.
   }
+
+  this._deferredClose = Promise.defer();
+
+  Barriers.connections.client.addBlocker(
+    this._connectionIdentifier + ": waiting for shutdown",
+    this._deferredClose.promise
+  );
 }
 
 OpenedConnection.prototype = Object.freeze({
   TRANSACTION_DEFERRED: "DEFERRED",
   TRANSACTION_IMMEDIATE: "IMMEDIATE",
   TRANSACTION_EXCLUSIVE: "EXCLUSIVE",
 
   TRANSACTION_TYPES: ["DEFERRED", "IMMEDIATE", "EXCLUSIVE"],
@@ -322,57 +406,52 @@ OpenedConnection.prototype = Object.free
    * This must be performed when you are finished with the database.
    *
    * Closing the database connection has the side effect of forcefully
    * cancelling all active statements. Therefore, callers should ensure that
    * all active statements have completed before closing the connection, if
    * possible.
    *
    * The returned promise will be resolved once the connection is closed.
+   * Successive calls to close() return the same promise.
    *
    * IMPROVEMENT: Resolve the promise to a closed connection which can be
    * reopened.
    *
    * @return Promise<>
    */
   close: function () {
     if (!this._connection) {
-      return Promise.resolve();
+      return this._deferredClose.promise;
     }
 
     this._log.debug("Request to close connection.");
     this._clearIdleShrinkTimer();
-    let deferred = Promise.defer();
-
-    AsyncShutdown.profileBeforeChange.addBlocker(
-      "Sqlite.jsm: " + this._connectionIdentifier,
-      deferred.promise
-    );
 
     // We need to take extra care with transactions during shutdown.
     //
     // If we don't have a transaction in progress, we can proceed with shutdown
     // immediately.
     if (!this._inProgressTransaction) {
-      this._finalize(deferred);
-      return deferred.promise;
+      this._finalize(this._deferredClose);
+      return this._deferredClose.promise;
     }
 
     // Else if we do have a transaction in progress, we forcefully roll it
     // back. This is an async task, so we wait on it to finish before
     // performing finalization.
     this._log.warn("Transaction in progress at time of close. Rolling back.");
 
-    let onRollback = this._finalize.bind(this, deferred);
+    let onRollback = this._finalize.bind(this, this._deferredClose);
 
     this.execute("ROLLBACK TRANSACTION").then(onRollback, onRollback);
     this._inProgressTransaction.reject(new Error("Connection being closed."));
     this._inProgressTransaction = null;
 
-    return deferred.promise;
+    return this._deferredClose.promise;
   },
 
   /**
    * Clones this connection to a new Sqlite one.
    *
    * The following parameters can control the cloned connection:
    *
    * @param readOnly
@@ -425,16 +504,19 @@ OpenedConnection.prototype = Object.free
     // function and asyncClose() finishing. See also bug 726990.
     this._open = false;
 
     this._log.debug("Calling asyncClose().");
     this._connection.asyncClose({
       complete: function () {
         this._log.info("Closed");
         this._connection = null;
+        // Now that the connection is closed, no need to keep
+        // a blocker for Barriers.connections.
+        Barriers.connections.client.removeBlocker(deferred.promise);
         deferred.resolve();
       }.bind(this),
     });
   },
 
   /**
    * Execute a SQL statement and cache the underlying statement object.
    *
@@ -935,10 +1017,19 @@ OpenedConnection.prototype = Object.free
     this._idleShrinkTimer.initWithCallback(this.shrinkMemory.bind(this),
                                            this._idleShrinkMS,
                                            this._idleShrinkTimer.TYPE_ONE_SHOT);
   },
 });
 
 this.Sqlite = {
   openConnection: openConnection,
-  cloneStorageConnection: cloneStorageConnection
+  cloneStorageConnection: cloneStorageConnection,
+  /**
+   * Shutdown barrier client. May be used by clients to perform last-minute
+   * cleanup prior to the shutdown of this module.
+   *
+   * See the documentation of AsyncShutdown.Barrier.prototype.client.
+   */
+  get shutdown() {
+    return Barriers.shutdown.client;
+  }
 };
new file mode 100644
--- /dev/null
+++ b/toolkit/modules/tests/xpcshell/test_sqlite_shutdown.js
@@ -0,0 +1,121 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
+
+do_get_profile();
+
+Cu.import("resource://gre/modules/osfile.jsm");
+  // OS.File doesn't like to be first imported during shutdown
+Cu.import("resource://gre/modules/Sqlite.jsm");
+Cu.import("resource://gre/modules/Task.jsm");
+Cu.import("resource://gre/modules/Services.jsm");
+Cu.import("resource://gre/modules/AsyncShutdown.jsm");
+
+function getConnection(dbName, extraOptions={}) {
+  let path = dbName + ".sqlite";
+  let options = {path: path};
+  for (let [k, v] in Iterator(extraOptions)) {
+    options[k] = v;
+  }
+
+  return Sqlite.openConnection(options);
+}
+
+function getDummyDatabase(name, extraOptions={}) {
+  const TABLES = {
+    dirs: "id INTEGER PRIMARY KEY AUTOINCREMENT, path TEXT",
+    files: "id INTEGER PRIMARY KEY AUTOINCREMENT, dir_id INTEGER, path TEXT",
+  };
+
+  let c = yield getConnection(name, extraOptions);
+  c._initialStatementCount = 0;
+
+  for (let [k, v] in Iterator(TABLES)) {
+    yield c.execute("CREATE TABLE " + k + "(" + v + ")");
+    c._initialStatementCount++;
+  }
+
+  throw new Task.Result(c);
+}
+
+function sleep(ms) {
+  let deferred = Promise.defer();
+
+  let timer = Cc["@mozilla.org/timer;1"]
+                .createInstance(Ci.nsITimer);
+
+  timer.initWithCallback({
+    notify: function () {
+      deferred.resolve();
+    },
+  }, ms, timer.TYPE_ONE_SHOT);
+
+  return deferred.promise;
+}
+
+function run_test() {
+  run_next_test();
+}
+
+
+//
+// -----------  Don't add a test after this one, as it shuts down Sqlite.jsm
+//
+add_task(function* test_shutdown_clients() {
+  do_print("Ensuring that Sqlite.jsm doesn't shutdown before its clients");
+
+  let assertions = [];
+
+  let sleepStarted = false;
+  let sleepComplete = false;
+  Sqlite.shutdown.addBlocker("test_sqlite.js shutdown blocker (sleep)",
+    Task.async(function*() {
+      sleepStarted = true;
+      yield sleep(100);
+      sleepComplete = true;
+    }));
+  assertions.push({name: "sleepStarted", value: () => sleepStarted});
+  assertions.push({name: "sleepComplete", value: () => sleepComplete});
+
+  Sqlite.shutdown.addBlocker("test_sqlite.js shutdown blocker (immediate)",
+    true);
+
+  let dbOpened = false;
+  let dbClosed = false;
+
+  Sqlite.shutdown.addBlocker("test_sqlite.js shutdown blocker (open a connection during shutdown)",
+    Task.async(function*() {
+      let db = yield getDummyDatabase("opened during shutdown");
+      dbOpened = true;
+      db.close().then(
+        () => dbClosed = true
+      ); // Don't wait for this task to complete, Sqlite.jsm must wait automatically
+  }));
+
+  assertions.push({name: "dbOpened", value: () => dbOpened});
+  assertions.push({name: "dbClosed", value: () => dbClosed});
+
+  do_print("Now shutdown Sqlite.jsm synchronously");
+  Services.prefs.setBoolPref("toolkit.asyncshutdown.testing", true);
+  AsyncShutdown.profileBeforeChange._trigger();
+  Services.prefs.clearUserPref("toolkit.asyncshutdown.testing");
+
+
+  for (let {name, value} of assertions) {
+    do_print("Checking: " + name);
+    do_check_true(value());
+  }
+
+  do_print("Ensure that we cannot open databases anymore");
+  let exn;
+  try {
+    yield getDummyDatabase("opened after shutdown");
+  } catch (ex) {
+    exn = ex;
+  }
+  do_check_true(!!exn);
+  do_check_true(exn.message.indexOf("Sqlite.jsm has been shutdown") != -1);
+});
--- a/toolkit/modules/tests/xpcshell/xpcshell.ini
+++ b/toolkit/modules/tests/xpcshell/xpcshell.ini
@@ -18,12 +18,13 @@ support-files =
 [test_NewTabUtils.js]
 [test_PermissionsUtils.js]
 [test_Preferences.js]
 [test_Promise.js]
 [test_propertyListsUtils.js]
 [test_readCertPrefs.js]
 [test_Services.js]
 [test_sqlite.js]
+[test_sqlite_shutdown.js]
 [test_task.js]
 [test_TelemetryTimestamps.js]
 [test_timer.js]
 [test_ZipUtils.js]