Bug 985655 - Ensure that Sqlite.jsm doesn't shutdown before its clients. r=mak

toolkit/modules/Sqlite.jsm

@@ -10,14 +10,18 @@ this.EXPORTED_SYMBOLS = [
 
 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",
@@ -30,6 +34,70 @@ XPCOMUtils.defineLazyModuleGetter(this, "Task",
 // 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.
@@ -72,6 +140,11 @@ function openConnection(options) {
     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);
 
@@ -161,7 +234,11 @@ function cloneStorageConnection(options) {
     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 = {};
@@ -279,6 +356,13 @@ function OpenedConnection(connection, basename, number, options) {
     // 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({
@@ -327,6 +411,7 @@ OpenedConnection.prototype = Object.freeze({
    * 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.
@@ -335,25 +420,19 @@ OpenedConnection.prototype = Object.freeze({
    */
   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
@@ -361,13 +440,13 @@ OpenedConnection.prototype = Object.freeze({
     // 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;
   },
 
   /**
@@ -430,6 +509,9 @@ OpenedConnection.prototype = Object.freeze({
       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),
     });
@@ -940,5 +1022,14 @@ OpenedConnection.prototype = Object.freeze({
 
 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;
+  }
 };

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);
+});

toolkit/modules/tests/xpcshell/xpcshell.ini

@@ -23,6 +23,7 @@ support-files =
 [test_readCertPrefs.js]
 [test_Services.js]
 [test_sqlite.js]
+[test_sqlite_shutdown.js]
 [test_task.js]
 [test_TelemetryTimestamps.js]
 [test_timer.js]