Bug 1237601 - Perform storage close synchronously if async thread cannot be started. r=bkelly

Also factors asyncClose tests out into their own file.

storage/mozIStorageAsyncConnection.idl

@@ -36,7 +36,12 @@ interface mozIStorageAsyncConnection : nsISupports {
    *        - value: |null|
    *
    * @throws NS_ERROR_NOT_SAME_THREAD
-   *         If is called on a thread other than the one that opened it.
+   *         If called on a thread other than the one that opened it.  The
+   *         callback will not be dispatched.
+   * @throws NS_ERROR_NOT_INITIALIZED
+   *         If called on a connection that has already been closed or was
+   *         never properly opened.  The callback will still be dispatched
+   *         to the main thread despite the returned error.
    */
   void asyncClose([optional] in mozIStorageCompletionCallback aCallback);
 

storage/mozStorageConnection.cpp

@@ -18,6 +18,7 @@
 #include "mozilla/CondVar.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/ErrorNames.h"
+#include "mozilla/unused.h"
 
 #include "mozIStorageAggregateFunction.h"
 #include "mozIStorageCompletionCallback.h"
@@ -1200,13 +1201,78 @@ Connection::AsyncClose(mozIStorageCompletionCallback *aCallback)
     return NS_ERROR_NOT_SAME_THREAD;
   }
 
-  // It's possible to get here with a null mDBConn but a non-null async
+  // The two relevant factors at this point are whether we have a database
-  // execution target if OpenAsyncDatabase failed somehow, so don't exit early
+  // connection and whether we have an async execution thread.  Here's what the
-  // in that case.
+  // states mean and how we handle them:
+  //
+  // - (mDBConn && asyncThread): The expected case where we are either an
+  //   async connection or a sync connection that has been used asynchronously.
+  //   Either way the caller must call us and not Close().  Nothing surprising
+  //   about this.  We'll dispatch AsyncCloseConnection to the already-existing
+  //   async thread.
+  //
+  // - (mDBConn && !asyncThread): A somewhat unusual case where the caller
+  //   opened the connection synchronously and was planning to use it
+  //   asynchronously, but never got around to using it asynchronously before
+  //   needing to shutdown.  This has been observed to happen for the cookie
+  //   service in a case where Firefox shuts itself down almost immediately
+  //   after startup (for unknown reasons).  In the Firefox shutdown case,
+  //   we may also fail to create a new async execution thread if one does not
+  //   already exist.  (nsThreadManager will refuse to create new threads when
+  //   it has already been told to shutdown.)  As such, we need to handle a
+  //   failure to create the async execution thread by falling back to
+  //   synchronous Close() and also dispatching the completion callback because
+  //   at least Places likes to spin a nested event loop that depends on the
+  //   callback being invoked.
+  //
+  //   Note that we have considered not trying to spin up the async execution
+  //   thread in this case if it does not already exist, but the overhead of
+  //   thread startup (if successful) is significantly less expensive than the
+  //   worst-case potential I/O hit of synchronously closing a database when we
+  //   could close it asynchronously.
+  //
+  // - (!mDBConn && asyncThread): This happens in some but not all cases where
+  //   OpenAsyncDatabase encountered a problem opening the database.  If it
+  //   happened in all cases AsyncInitDatabase would just shut down the thread
+  //   directly and we would avoid this case.  But it doesn't, so for simplicity
+  //   and consistency AsyncCloseConnection knows how to handle this and we
+  //   act like this was the (mDBConn && asyncThread) case in this method.
+  //
+  // - (!mDBConn && !asyncThread): The database was never successfully opened or
+  //   Close() or AsyncClose() has already been called (at least) once.  This is
+  //   undeniably a misuse case by the caller.  We could optimize for this
+  //   case by adding an additional check of mAsyncExecutionThread without using
+  //   getAsyncExecutionTarget() to avoid wastefully creating a thread just to
+  //   shut it down.  But this complicates the method for broken caller code
+  //   whereas we're still correct and safe without the special-case.
   nsIEventTarget *asyncThread = getAsyncExecutionTarget();
 
-  if (!mDBConn && !asyncThread)
+  // Create our callback event if we were given a callback.  This will
-    return NS_ERROR_NOT_INITIALIZED;
+  // eventually be dispatched in all cases, even if we fall back to Close() and
+  // the database wasn't open and we return an error.  The rationale is that
+  // no existing consumer checks our return value and several of them like to
+  // spin nested event loops until the callback fires.  Given that, it seems
+  // preferable for us to dispatch the callback in all cases.  (Except the
+  // wrong thread misuse case we bailed on up above.  But that's okay because
+  // that is statically wrong whereas these edge cases are dynamic.)
+  nsCOMPtr<nsIRunnable> completeEvent;
+  if (aCallback) {
+    completeEvent = newCompletionEvent(aCallback);
+  }
+
+  if (!asyncThread) {
+    // We were unable to create an async thread, so we need to fall back to
+    // using normal Close().  Since there is no async thread, Close() will
+    // not complain about that.  (Close() may, however, complain if the
+    // connection is closed, but that's okay.)
+    if (completeEvent) {
+      // Closing the database is more important than returning an error code
+      // about a failure to dispatch, especially because all existing native
+      // callers ignore our return value.
+      Unused << NS_DispatchToMainThread(completeEvent.forget());
+    }
+    return Close();
+  }
 
   // setClosedState nullifies our connection pointer, so we take a raw pointer
   // off it, to pass it through the close procedure.
@@ -1214,12 +1280,6 @@ Connection::AsyncClose(mozIStorageCompletionCallback *aCallback)
   nsresult rv = setClosedState();
   NS_ENSURE_SUCCESS(rv, rv);
 
-  // Create our callback event if we were given a callback.
-  nsCOMPtr<nsIRunnable> completeEvent;
-  if (aCallback) {
-    completeEvent = newCompletionEvent(aCallback);
-  }
-
   // Create and dispatch our close event to the background thread.
   nsCOMPtr<nsIRunnable> closeEvent;
   {
@@ -1528,7 +1588,7 @@ Connection::ExecuteAsync(mozIStorageBaseStatement **aStatements,
 {
   nsTArray<StatementData> stmts(aNumStatements);
   for (uint32_t i = 0; i < aNumStatements; i++) {
-    nsCOMPtr<StorageBaseStatementInternal> stmt = 
+    nsCOMPtr<StorageBaseStatementInternal> stmt =
       do_QueryInterface(aStatements[i]);
 
     // Obtain our StatementData.

storage/test/unit/head_storage.js

@@ -41,6 +41,17 @@ function getFakeDB()
   return do_get_file("fakeDB.sqlite");
 }
 
+/**
+ * Delete the test database file.
+ */
+function deleteTestDB()
+{
+  print("*** Storage Tests: Trying to remove file!");
+  var dbFile = getTestDB();
+  if (dbFile.exists())
+    try { dbFile.remove(false); } catch (e) { /* stupid windows box */ }
+}
+
 function cleanup()
 {
   // close the connection
@@ -52,10 +63,7 @@ function cleanup()
   gDBConn = null;
 
   // removing test db
-  print("*** Storage Tests: Trying to remove file!");
+  deleteTestDB();
-  var dbFile = getTestDB();
-  if (dbFile.exists())
-    try { dbFile.remove(false); } catch (e) { /* stupid windows box */ }
 }
 
 /**
@@ -80,10 +88,7 @@ function asyncCleanup()
   gDBConn = null;
 
   // removing test db
-  print("*** Storage Tests: Trying to remove file!");
+  deleteTestDB();
-  var dbFile = getTestDB();
-  if (dbFile.exists())
-    try { dbFile.remove(false); } catch (e) { /* stupid windows box */ }
 }
 
 function getService()
@@ -327,6 +332,24 @@ function executeAsync(statement, onResult) {
   return deferred.promise;
 }
 
+function executeMultipleStatementsAsync(db, statements, onResult) {
+  let deferred = Promise.defer();
+  db.executeAsync(statements, statements.length, {
+    handleError: function (error) {
+      deferred.reject(error);
+    },
+    handleResult: function (result) {
+      if (onResult) {
+        onResult(result);
+      }
+    },
+    handleCompletion: function (result) {
+      deferred.resolve(result);
+    }
+  });
+  return deferred.promise;
+}
+
 function executeSimpleSQLAsync(db, query, onResult) {
   let deferred = Promise.defer();
   db.executeSimpleSQLAsync(query, {

storage/test/unit/test_connection_asyncClose.js

@@ -0,0 +1,125 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+/*
+ * Thorough branch coverage for asyncClose.
+ *
+ * Coverage of asyncClose by connection state at time of AsyncClose invocation:
+ * - (asyncThread && mDBConn) => AsyncCloseConnection used, actually closes
+ *   - test_asyncClose_does_not_complete_before_statements
+ *   - test_double_asyncClose_throws
+ *   - test_asyncClose_does_not_throw_without_callback
+ * - (asyncThread && !mDBConn) => AsyncCloseConnection used, although no close
+ *   is required.  Note that this is only possible in the event that
+ *   openAsyncDatabase was used and we failed to open the database.
+ *   Additionally, the async connection will never be exposed to the caller and
+ *   AsyncInitDatabase will be the one to (automatically) call AsyncClose.
+ *   - test_asyncClose_failed_open
+ * - (!asyncThread && mDBConn) => Close() invoked, actually closes
+ *   - test_asyncClose_on_sync_db
+ * - (!asyncThread && !mDBConn) => Close() invoked, no close needed, errors.
+ *   This happens if the database has already been closed.
+ *   - test_double_asyncClose_throws
+ */
+
+
+/**
+ * Sanity check that our close indeed happens after asynchronously executed
+ * statements scheduled during the same turn of the event loop.  Note that we
+ * just care that the statement says it completed without error, we're not
+ * worried that the close will happen and then the statement will magically
+ * complete.
+ */
+add_task(function* test_asyncClose_does_not_complete_before_statements() {
+  let db = getService().openDatabase(getTestDB());
+  let stmt = db.createStatement("SELECT * FROM sqlite_master");
+  // Issue the executeAsync but don't yield for it...
+  let asyncStatementPromise = executeAsync(stmt);
+  stmt.finalize();
+
+  // Issue the close.  (And now the order of yielding doesn't matter.)
+  // Branch coverage: (asyncThread && mDBConn)
+  yield asyncClose(db);
+  equal((yield asyncStatementPromise),
+        Ci.mozIStorageStatementCallback.REASON_FINISHED);
+});
+
+/**
+ * Open an async database (ensures the async thread is created) and then invoke
+ * AsyncClose() twice without yielding control flow.  The first will initiate
+ * the actual async close after calling setClosedState which synchronously
+ * impacts what the second call will observe.  The second call will then see the
+ * async thread is not available and fall back to invoking Close() which will
+ * notice the mDBConn is already gone.
+ */
+add_task(function test_double_asyncClose_throws() {
+  let db = yield openAsyncDatabase(getTestDB());
+
+  // (Don't yield control flow yet, save the promise for after we make the
+  // second call.)
+  // Branch coverage: (asyncThread && mDBConn)
+  let realClosePromise = yield asyncClose(db);
+  try {
+    // Branch coverage: (!asyncThread && !mDBConn)
+    db.asyncClose();
+    ok(false, "should have thrown");
+  } catch (e) {
+    equal(e.result, Cr.NS_ERROR_NOT_INITIALIZED);
+  }
+
+  yield realClosePromise;
+});
+
+/**
+ * Create a sync db connection and never take it asynchronous and then call
+ * asyncClose on it.  This will bring the async thread to life to perform the
+ * shutdown to avoid blocking the main thread, although we won't be able to
+ * tell the difference between this happening and the method secretly shunting
+ * to close().
+ */
+add_task(function* test_asyncClose_on_sync_db() {
+  let db = getService().openDatabase(getTestDB());
+
+  // Branch coverage: (!asyncThread && mDBConn)
+  yield asyncClose(db);
+  ok(true, 'closed sync connection asynchronously');
+});
+
+/**
+ * Fail to asynchronously open a DB in order to get an async thread existing
+ * without having an open database and asyncClose invoked.  As per the file
+ * doc-block, note that asyncClose will automatically be invoked by the
+ * AsyncInitDatabase when it fails to open the database.  We will never be
+ * provided with a reference to the connection and so cannot call AsyncClose on
+ * it ourselves.
+ */
+add_task(function* test_asyncClose_failed_open() {
+  // This will fail and the promise will be rejected.
+  let openPromise = openAsyncDatabase(getFakeDB());
+  yield openPromise.then(
+    () => {
+      ok(false, 'we should have failed to open the db; this test is broken!');
+    },
+    () => {
+      ok(true, 'correctly failed to open db; bg asyncClose should happen');
+    }
+  );
+  // (NB: we are unable to observe the thread shutdown, but since we never open
+  // a database, this test is not going to interfere with other tests so much.)
+});
+
+// THE TEST BELOW WANTS TO BE THE LAST TEST WE RUN.  DO NOT MAKE IT SAD.
+/**
+ * Verify that asyncClose without a callback does not explode.  Without a
+ * callback the shutdown is not actually observable, so we run this test last
+ * in order to avoid weird overlaps.
+ */
+add_task(function test_asyncClose_does_not_throw_without_callback() {
+  let db = yield openAsyncDatabase(getTestDB());
+  // Branch coverage: (asyncThread && mDBConn)
+  db.asyncClose();
+  ok(true, 'if we shutdown cleanly and do not crash, then we succeeded');
+});
+// OBEY SHOUTING UPPER-CASE COMMENTS.
+// ADD TESTS ABOVE THE FORMER TEST, NOT BELOW IT.

storage/test/unit/test_connection_executeAsync.js

@@ -5,6 +5,10 @@
 /*
  * This file tests the functionality of mozIStorageConnection::executeAsync for
  * both mozIStorageStatement and mozIStorageAsyncStatement.
+ *
+ * A single database connection is used for the entirety of the test, which is
+ * a legacy thing, but we otherwise use the modern promise-based driver and
+ * async helpers.
  */
 
 const INTEGER = 1;
@@ -12,8 +16,12 @@ const TEXT = "this is test text";
 const REAL = 3.23;
 const BLOB = [1, 2];
 
-add_test(function test_create_and_add() {
+add_task(function* test_first_create_and_add() {
-  getOpenedDatabase().executeSimpleSQL(
+  // synchronously open the database and let gDBConn hold onto it because we
+  // use this database
+  let db = getOpenedDatabase();
+  // synchronously set up our table *that will be used for the rest of the file*
+  db.executeSimpleSQL(
     "CREATE TABLE test (" +
       "id INTEGER, " +
       "string TEXT, " +
@@ -24,7 +32,7 @@ add_test(function test_create_and_add() {
   );
 
   let stmts = [];
-  stmts[0] = getOpenedDatabase().createStatement(
+  stmts[0] = db.createStatement(
     "INSERT INTO test (id, string, number, nuller, blober) VALUES (?, ?, ?, ?, ?)"
   );
   stmts[0].bindByIndex(0, INTEGER);
@@ -40,62 +48,54 @@ add_test(function test_create_and_add() {
   stmts[1].bindByIndex(2, null);
   stmts[1].bindBlobByIndex(3, BLOB, BLOB.length);
 
-  getOpenedDatabase().executeAsync(stmts, stmts.length, {
+  // asynchronously execute the statements
-    handleResult: function (aResultSet) {
+  let execResult = yield executeMultipleStatementsAsync(
-      dump("handleResult(" + aResultSet + ")\n");
+    db,
-      do_throw("unexpected results obtained!");
+    stmts,
-    },
+    function(aResultSet) {
-    handleError: function (aError)
+      ok(false, 'we only did inserts so we should not have gotten results!');
-    {
+    });
-      dump("handleError(" + aError.result + ")\n");
+  equal(Ci.mozIStorageStatementCallback.REASON_FINISHED, execResult,
-      do_throw("unexpected error!");
+        'execution should have finished successfully.');
-    },
-    handleCompletion: function (aReason) {
-      dump("handleCompletion(" + aReason + ")\n");
-      do_check_eq(Ci.mozIStorageStatementCallback.REASON_FINISHED, aReason);
 
-      // Check that the result is in the table
+  // Check that the result is in the table
-      let stmt = getOpenedDatabase().createStatement(
+  let stmt = db.createStatement(
-        "SELECT string, number, nuller, blober FROM test WHERE id = ?"
+    "SELECT string, number, nuller, blober FROM test WHERE id = ?"
-      );
+  );
-      stmt.bindByIndex(0, INTEGER);
+  stmt.bindByIndex(0, INTEGER);
-      try {
+  try {
-        do_check_true(stmt.executeStep());
+    do_check_true(stmt.executeStep());
-        do_check_eq(TEXT, stmt.getString(0));
+    do_check_eq(TEXT, stmt.getString(0));
-        do_check_eq(REAL, stmt.getDouble(1));
+    do_check_eq(REAL, stmt.getDouble(1));
-        do_check_true(stmt.getIsNull(2));
+    do_check_true(stmt.getIsNull(2));
-        let count = { value: 0 };
+    let count = { value: 0 };
-        let blob = { value: null };
+    let blob = { value: null };
-        stmt.getBlob(3, count, blob);
+    stmt.getBlob(3, count, blob);
-        do_check_eq(BLOB.length, count.value);
+    do_check_eq(BLOB.length, count.value);
-        for (let i = 0; i < BLOB.length; i++)
+    for (let i = 0; i < BLOB.length; i++)
-          do_check_eq(BLOB[i], blob.value[i]);
+      do_check_eq(BLOB[i], blob.value[i]);
-      }
+  }
-      finally {
+  finally {
-        stmt.finalize();
+    stmt.finalize();
-      }
+  }
 
-      // Make sure we have two rows in the table
+  // Make sure we have two rows in the table
-      stmt = getOpenedDatabase().createStatement(
+  stmt = db.createStatement(
-        "SELECT COUNT(1) FROM test"
+    "SELECT COUNT(1) FROM test"
-      );
+  );
-      try {
+  try {
-        do_check_true(stmt.executeStep());
+    do_check_true(stmt.executeStep());
-        do_check_eq(2, stmt.getInt32(0));
+    do_check_eq(2, stmt.getInt32(0));
-      }
+  }
-      finally {
+  finally {
-        stmt.finalize();
+    stmt.finalize();
-      }
+  }
 
-      // Run the next test.
-      run_next_test();
-    }
-  });
   stmts[0].finalize();
   stmts[1].finalize();
 });
 
-add_test(function test_multiple_bindings_on_statements() {
+add_task(function* test_last_multiple_bindings_on_statements() {
   // This tests to make sure that we pass all the statements multiply bound
   // parameters when we call executeAsync.
   const AMOUNT_TO_ADD = 5;
@@ -140,96 +140,32 @@ add_test(function test_multiple_bindings_on_statements() {
   }
 
   // Execute asynchronously.
-  getOpenedDatabase().executeAsync(stmts, stmts.length, {
+  let execResult = yield executeMultipleStatementsAsync(
-    handleResult: function (aResultSet) {
+    db,
-      do_throw("Unexpected call to handleResult!");
+    stmts,
-    },
+    function(aResultSet) {
-    handleError: function (aError) {
+      ok(false, 'we only did inserts so we should not have gotten results!');
-      print("Error code " + aError.result + " with message '" +
+    });
-            aError.message + "' returned.");
+  equal(Ci.mozIStorageStatementCallback.REASON_FINISHED, execResult,
-      do_throw("Unexpected error!");
+        'execution should have finished successfully.');
-    },
-    handleCompletion: function (aReason) {
-      print("handleCompletion(" + aReason +
-            ") for test_multiple_bindings_on_statements");
-      do_check_eq(Ci.mozIStorageStatementCallback.REASON_FINISHED, aReason);
 
-      // Check to make sure we added all of our rows.
+  // Check to make sure we added all of our rows.
-      try {
-        do_check_true(countStmt.executeStep());
-        do_check_eq(currentRows + (ITERATIONS * AMOUNT_TO_ADD),
-                    countStmt.row.count);
-      }
-      finally {
-        countStmt.finalize();
-      }
-
-      // Run the next test.
-      run_next_test();
-    }
-  });
-  stmts.forEach(stmt => stmt.finalize());
-});
-
-add_test(function test_asyncClose_does_not_complete_before_statements() {
-  let stmt = createStatement("SELECT * FROM sqlite_master");
-  let executed = false;
-  stmt.executeAsync({
-    handleResult(aResultSet) {},
-    handleError(aError) {
-      print("Error code " + aError.result + " with message '" +
-            aError.message + "' returned.");
-      do_throw("Unexpected error!");
-    },
-    handleCompletion(aReason) {
-      print("handleCompletion(" + aReason +
-            ") for test_asyncClose_does_not_complete_before_statements");
-      do_check_eq(Ci.mozIStorageStatementCallback.REASON_FINISHED, aReason);
-      executed = true;
-    }
-  });
-  stmt.finalize();
-
-  getOpenedDatabase().asyncClose(function () {
-    // Ensure that the statement executed to completion.
-    do_check_true(executed);
-
-    // Reset gDBConn so that later tests will get a new connection object.
-    gDBConn = null;
-    run_next_test();
-  });
-});
-
-add_test(function test_asyncClose_does_not_throw_no_callback() {
-  getOpenedDatabase().asyncClose();
-
-  // Reset gDBConn so that later tests will get a new connection object.
-  gDBConn = null;
-  run_next_test();
-});
-
-add_test(function test_double_asyncClose_throws() {
-  let conn = getOpenedDatabase();
-  conn.asyncClose();
   try {
-    conn.asyncClose();
+    do_check_true(countStmt.executeStep());
-    do_throw("should have thrown");
+    do_check_eq(currentRows + (ITERATIONS * AMOUNT_TO_ADD),
-    // There is a small race condition here, which can cause either of
+                countStmt.row.count);
-    // Cr.NS_ERROR_NOT_INITIALIZED or Cr.NS_ERROR_UNEXPECTED to be thrown.
+  }
-  } catch (e) {
+  finally {
-    if ("result" in e && e.result == Cr.NS_ERROR_NOT_INITIALIZED) {
+    countStmt.finalize();
-      do_print("NS_ERROR_NOT_INITIALIZED");
-    } else if ("result" in e && e.result == Cr.NS_ERROR_UNEXPECTED) {
-      do_print("NS_ERROR_UNEXPECTED");
-    }
   }
 
-  // Reset gDBConn so that later tests will get a new connection object.
+  stmts.forEach(stmt => stmt.finalize());
+
+  // we are the last test using this connection and since it has gone async
+  // we *must* call asyncClose on it.
+  yield asyncClose(db);
   gDBConn = null;
-  run_next_test();
 });
 
-function run_test() {
+// If you add a test down here you will need to move the asyncClose or clean
-  cleanup();
+// things up a little more.
-  run_next_test();
-}

storage/test/unit/xpcshell.ini

@@ -17,6 +17,7 @@ support-files =
 [test_chunk_growth.js]
 # Bug 676981: test fails consistently on Android
 fail-if = os == "android"
+[test_connection_asyncClose.js]
 [test_connection_executeAsync.js]
 [test_connection_executeSimpleSQLAsync.js]
 [test_js_helpers.js]