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 id6769
push userryanvm@gmail.com
push dateTue, 13 May 2014 13:28:53 +0000
treeherderfx-team@7f23305ed404 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs985655
milestone32.0a1
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]