Bug 939072: Auto-close Sqlite connections on finalization; r=Yoric
authorMichael Brennan <brennan.brisad@gmail.com>
Wed, 11 Jun 2014 23:40:17 +0200
changeset 210568 7ccab6cb126fd867e7bc58e2334cfbda79481884
parent 210567 b018c57be4cba8b1ec865aa3288c426fff2ac399
child 210569 84f04ef3add0ea3c9b585edc19348dfe0854bfa5
push id3857
push userraliiev@mozilla.com
push dateTue, 02 Sep 2014 16:39:23 +0000
treeherdermozilla-beta@5638b907b505 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersYoric
bugs939072
milestone33.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 939072: Auto-close Sqlite connections on finalization; r=Yoric
toolkit/modules/Sqlite.jsm
toolkit/modules/tests/xpcshell/test_sqlite.js
--- a/toolkit/modules/Sqlite.jsm
+++ b/toolkit/modules/Sqlite.jsm
@@ -23,27 +23,43 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 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");
+XPCOMUtils.defineLazyServiceGetter(this, "FinalizationWitnessService",
+                                   "@mozilla.org/toolkit/finalizationwitness;1",
+                                   "nsIFinalizationWitnessService");
 
 
 // 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;
 
+// Displays a script error message
+function logScriptError(message) {
+  let consoleMessage = Cc["@mozilla.org/scripterror;1"].
+                       createInstance(Ci.nsIScriptError);
+  let stack = new Error();
+  consoleMessage.init(message, stack.fileName, null, stack.lineNumber, 0,
+                      Ci.nsIScriptError.errorFlag, "component javascript");
+  Services.console.logMessage(consoleMessage);
+
+  // Always dump errors, in case the Console Service isn't listening anymore
+  dump("*** " + message + "\n");
+}
+
 /**
  * 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
@@ -57,16 +73,39 @@ XPCOMUtils.defineLazyGetter(this, "Barri
      * 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"),
   };
 
   /**
+   * Observer for the event which is broadcasted when the finalization
+   * witness `_witness` of `OpenedConnection` is garbage collected.
+   *
+   * The observer is passed the connection identifier of the database
+   * connection that is being finalized.
+   */
+  let finalizationObserver = function (subject, topic, connectionIdentifier) {
+    let connectionData = ConnectionData.byId.get(connectionIdentifier);
+
+    if (connectionData === undefined) {
+      logScriptError("Error: Attempt to finalize unknown Sqlite connection: " +
+                     connectionIdentifier + "\n");
+      return;
+    }
+
+    ConnectionData.byId.delete(connectionIdentifier);
+    logScriptError("Warning: Sqlite connection '" + connectionIdentifier +
+                   "' was not properly closed. Auto-close triggered by garbage collection.\n");
+    connectionData.close();
+  };
+  Services.obs.addObserver(finalizationObserver, "sqlite-finalization-witness", false);
+
+  /**
    * 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();
@@ -74,16 +113,19 @@ XPCOMUtils.defineLazyGetter(this, "Barri
       // 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();
+
+      // Everything closed, no finalization events to catch
+      Services.obs.removeObserver(finalizationObserver, "sqlite-finalization-witness");
     }),
 
     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 };
@@ -150,16 +192,28 @@ function ConnectionData(connection, base
   this._deferredClose = Promise.defer();
 
   Barriers.connections.client.addBlocker(
     this._connectionIdentifier + ": waiting for shutdown",
     this._deferredClose.promise
   );
 }
 
+/**
+ * Map of connection identifiers to ConnectionData objects
+ *
+ * The connection identifier is a human-readable name of the
+ * database. Used by finalization witnesses to be able to close opened
+ * connections on garbage collection.
+ *
+ * Key: _connectionIdentifier of ConnectionData
+ * Value: ConnectionData object
+ */
+ConnectionData.byId = new Map();
+
 ConnectionData.prototype = Object.freeze({
   close: function () {
     if (!this._dbConn) {
       return this._deferredClose.promise;
     }
 
     this._log.debug("Request to close connection.");
     this._clearIdleShrinkTimer();
@@ -715,17 +769,17 @@ function cloneStorageConnection(options)
   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.");
   }
 
   if (isClosed) {
-    throw new Error("Sqlite.jsm has been shutdown. Cannot close connection to: " + source.database.path);
+    throw new Error("Sqlite.jsm has been shutdown. Cannot clone 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);
@@ -810,16 +864,29 @@ function cloneStorageConnection(options)
  */
 function OpenedConnection(connection, basename, number, options) {
   // Store all connection data in a field distinct from the
   // witness. This enables us to store an additional reference to this
   // field without preventing garbage collection of
   // OpenedConnection. On garbage collection, we will still be able to
   // close the database using this extra reference.
   this._connectionData = new ConnectionData(connection, basename, number, options);
+
+  // Store the extra reference in a map with connection identifier as
+  // key.
+  ConnectionData.byId.set(this._connectionData._connectionIdentifier,
+                          this._connectionData);
+
+  // Make a finalization witness. If this object is garbage collected
+  // before its `forget` method has been called, an event with topic
+  // "sqlite-finalization-witness" is broadcasted along with the
+  // connection identifier string of the database.
+  this._witness = FinalizationWitnessService.make(
+    "sqlite-finalization-witness",
+    this._connectionData._connectionIdentifier);
 }
 
 OpenedConnection.prototype = Object.freeze({
   TRANSACTION_DEFERRED: "DEFERRED",
   TRANSACTION_IMMEDIATE: "IMMEDIATE",
   TRANSACTION_EXCLUSIVE: "EXCLUSIVE",
 
   TRANSACTION_TYPES: ["DEFERRED", "IMMEDIATE", "EXCLUSIVE"],
@@ -866,16 +933,23 @@ OpenedConnection.prototype = Object.free
    * Successive calls to close() return the same promise.
    *
    * IMPROVEMENT: Resolve the promise to a closed connection which can be
    * reopened.
    *
    * @return Promise<>
    */
   close: function () {
+    // Unless cleanup has already been done by a previous call to
+    // `close`, delete the database entry from map and tell the
+    // finalization witness to forget.
+    if (ConnectionData.byId.has(this._connectionData._connectionIdentifier)) {
+      ConnectionData.byId.delete(this._connectionData._connectionIdentifier);
+      this._witness.forget();
+    }
     return this._connectionData.close();
   },
 
   /**
    * Clones this connection to a new Sqlite one.
    *
    * The following parameters can control the cloned connection:
    *
--- a/toolkit/modules/tests/xpcshell/test_sqlite.js
+++ b/toolkit/modules/tests/xpcshell/test_sqlite.js
@@ -901,8 +901,106 @@ add_task(function* test_readOnly_clone()
   try {
     yield clone.execute("CREATE TABLE test (id INTEGER PRIMARY KEY)");
     do_throw(new Error("Should not be able to write to a read-only clone."));
   } catch (ex) {}
   // Closing order should not matter.
   yield c.close();
   yield clone.close();
 });
+
+/**
+ * Test finalization
+ */
+add_task(function* test_closed_by_witness() {
+  let c = yield getDummyDatabase("closed_by_witness");
+
+  Services.obs.notifyObservers(null, "sqlite-finalization-witness",
+                               c._connectionData._connectionIdentifier);
+  // Since we triggered finalization ourselves, tell the witness to
+  // forget the connection so it does not trigger a finalization again
+  c._witness.forget();
+  yield c._connectionData._deferredClose.promise;
+  do_check_false(c._connectionData._open);
+});
+
+add_task(function* test_warning_message_on_finalization() {
+  let c = yield getDummyDatabase("warning_message_on_finalization");
+  let connectionIdentifier = c._connectionData._connectionIdentifier;
+  let deferred = Promise.defer();
+
+  let listener = {
+    observe: function(msg) {
+      let messageText = msg.message;
+      // Make sure the message starts with a warning containing the
+      // connection identifier
+      if (messageText.indexOf("Warning: Sqlite connection '" + connectionIdentifier + "'") !== -1) {
+        deferred.resolve();
+      }
+    }
+  };
+  Services.console.registerListener(listener);
+
+  Services.obs.notifyObservers(null, "sqlite-finalization-witness", connectionIdentifier);
+  // Since we triggered finalization ourselves, tell the witness to
+  // forget the connection so it does not trigger a finalization again
+  c._witness.forget();
+
+  yield deferred.promise;
+  Services.console.unregisterListener(listener);
+});
+
+add_task(function* test_error_message_on_unknown_finalization() {
+  let deferred = Promise.defer();
+
+  let listener = {
+    observe: function(msg) {
+      let messageText = msg.message;
+      if (messageText.indexOf("Error: Attempt to finalize unknown " +
+                              "Sqlite connection: foo") !== -1) {
+        deferred.resolve();
+      }
+    }
+  };
+  Services.console.registerListener(listener);
+  Services.obs.notifyObservers(null, "sqlite-finalization-witness", "foo");
+
+  yield deferred.promise;
+  Services.console.unregisterListener(listener);
+});
+
+add_task(function* test_forget_witness_on_close() {
+  let c = yield getDummyDatabase("forget_witness_on_close");
+
+  let forgetCalled = false;
+  let oldWitness = c._witness;
+  c._witness = {
+    forget: function () {
+      forgetCalled = true;
+      oldWitness.forget();
+    },
+  };
+
+  yield c.close();
+  // After close, witness should have forgotten the connection
+  do_check_true(forgetCalled);
+});
+
+add_task(function* test_close_database_on_gc() {
+  let deferred = Promise.defer();
+
+  for (let i = 0; i < 100; ++i) {
+    let c = yield getDummyDatabase("gc_" + i);
+    c._connectionData._deferredClose.promise.then(deferred.resolve);
+  }
+
+  // Call getDummyDatabase once more to clear any remaining
+  // references. This is needed at the moment, otherwise
+  // garbage-collection takes place after the shutdown barrier and the
+  // test will timeout. Once that is fixed, we can remove this line
+  // and be fine as long as at least one connection is
+  // garbage-collected.
+  let last = yield getDummyDatabase("gc_last");
+  yield last.close();
+
+  Components.utils.forceGC();
+  yield deferred.promise;
+});