Bug 1237601 - Perform storage close synchronously if async thread cannot be started. r=bkelly
authorAndrew Sutherland <asutherland@asutherland.org>
Thu, 07 Jan 2016 11:18:00 -0500
changeset 314792 42ee91882a503bfb9e216f19111dc935bd400caf
parent 314791 d6a85731d2aa518779451ebf8583815151565c56
child 314793 29a5d0c6ea47ffb17df9bebc7da22df25e7eb451
push id5703
push userraliiev@mozilla.com
push dateMon, 07 Mar 2016 14:18:41 +0000
treeherdermozilla-beta@31e373ad5b5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbkelly
bugs1237601
milestone46.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 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
storage/mozStorageConnection.cpp
storage/test/unit/head_storage.js
storage/test/unit/test_connection_asyncClose.js
storage/test/unit/test_connection_executeAsync.js
storage/test/unit/xpcshell.ini
--- a/storage/mozIStorageAsyncConnection.idl
+++ b/storage/mozIStorageAsyncConnection.idl
@@ -31,17 +31,22 @@ interface mozIStorageAsyncConnection : n
    *
    * @param aCallback [optional]
    *        A callback that will be notified when the close is completed,
    *        with the following arguments:
    *        - status: the status of the call
    *        - 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);
 
   /**
    * Clone a database and make the clone read only if needed.
    *
    * @param aReadOnly
    *        If true, the returned database should be put into read-only mode.
--- a/storage/mozStorageConnection.cpp
+++ b/storage/mozStorageConnection.cpp
@@ -13,16 +13,17 @@
 #include "nsThreadUtils.h"
 #include "nsIFile.h"
 #include "nsIFileURL.h"
 #include "mozilla/Telemetry.h"
 #include "mozilla/Mutex.h"
 #include "mozilla/CondVar.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/ErrorNames.h"
+#include "mozilla/unused.h"
 
 #include "mozIStorageAggregateFunction.h"
 #include "mozIStorageCompletionCallback.h"
 #include "mozIStorageFunction.h"
 
 #include "mozStorageAsyncStatementExecution.h"
 #include "mozStorageSQLFunctions.h"
 #include "mozStorageConnection.h"
@@ -1195,36 +1196,95 @@ Connection::Close()
 
 NS_IMETHODIMP
 Connection::AsyncClose(mozIStorageCompletionCallback *aCallback)
 {
   if (!NS_IsMainThread()) {
     return NS_ERROR_NOT_SAME_THREAD;
   }
 
-  // It's possible to get here with a null mDBConn but a non-null async
-  // execution target if OpenAsyncDatabase failed somehow, so don't exit early
-  // in that case.
+  // The two relevant factors at this point are whether we have a database
+  // connection and whether we have an async execution thread.  Here's what the
+  // 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)
-    return NS_ERROR_NOT_INITIALIZED;
+  // Create our callback event if we were given a callback.  This will
+  // 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.
   sqlite3 *nativeConn = mDBConn;
   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;
   {
     // We need to lock because we're modifying mAsyncExecutionThread
     MutexAutoLock lockedScope(sharedAsyncExecutionMutex);
     closeEvent = new AsyncCloseConnection(this,
                                           nativeConn,
                                           completeEvent,
@@ -1523,17 +1583,17 @@ Connection::ExecuteSimpleSQL(const nsACS
 NS_IMETHODIMP
 Connection::ExecuteAsync(mozIStorageBaseStatement **aStatements,
                          uint32_t aNumStatements,
                          mozIStorageStatementCallback *aCallback,
                          mozIStoragePendingStatement **_handle)
 {
   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.
     StatementData data;
     nsresult rv = stmt->getAsynchronousStatementData(data);
     NS_ENSURE_SUCCESS(rv, rv);
 
     NS_ASSERTION(stmt->getOwner() == this,
--- a/storage/test/unit/head_storage.js
+++ b/storage/test/unit/head_storage.js
@@ -36,31 +36,39 @@ function getCorruptDB()
 /**
  * Obtains a fake (non-SQLite format) database to test against.
  */
 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
   print("*** Storage Tests: Trying to close!");
   getOpenedDatabase().close();
 
   // we need to null out the database variable to get a new connection the next
   // time getOpenedDatabase is called
   gDBConn = null;
 
   // removing test db
-  print("*** Storage Tests: Trying to remove file!");
-  var dbFile = getTestDB();
-  if (dbFile.exists())
-    try { dbFile.remove(false); } catch (e) { /* stupid windows box */ }
+  deleteTestDB();
 }
 
 /**
  * Use asyncClose to cleanup a connection.  Synchronous by means of internally
  * spinning an event loop.
  */
 function asyncCleanup()
 {
@@ -75,20 +83,17 @@ function asyncCleanup()
   while (!closed)
     curThread.processNextEvent(true);
 
   // we need to null out the database variable to get a new connection the next
   // time getOpenedDatabase is called
   gDBConn = null;
 
   // removing test db
-  print("*** Storage Tests: Trying to remove file!");
-  var dbFile = getTestDB();
-  if (dbFile.exists())
-    try { dbFile.remove(false); } catch (e) { /* stupid windows box */ }
+  deleteTestDB();
 }
 
 function getService()
 {
   return Cc["@mozilla.org/storage/service;1"].getService(Ci.mozIStorageService);
 }
 
 /**
@@ -322,16 +327,34 @@ function executeAsync(statement, onResul
     },
     handleCompletion: function (result) {
       deferred.resolve(result);
     }
   });
   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, {
     handleError(error) {
       deferred.reject(error);
     },
     handleResult(result) {
       if (onResult) {
new file mode 100644
--- /dev/null
+++ b/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.
--- a/storage/test/unit/test_connection_executeAsync.js
+++ b/storage/test/unit/test_connection_executeAsync.js
@@ -1,106 +1,106 @@
 /* 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/. */
 
 /*
  * 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;
 const TEXT = "this is test text";
 const REAL = 3.23;
 const BLOB = [1, 2];
 
-add_test(function test_create_and_add() {
-  getOpenedDatabase().executeSimpleSQL(
+add_task(function* test_first_create_and_add() {
+  // 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, " +
       "number REAL, " +
       "nuller NULL, " +
       "blober BLOB" +
     ")"
   );
 
   let stmts = [];
-  stmts[0] = getOpenedDatabase().createStatement(
+  stmts[0] = db.createStatement(
     "INSERT INTO test (id, string, number, nuller, blober) VALUES (?, ?, ?, ?, ?)"
   );
   stmts[0].bindByIndex(0, INTEGER);
   stmts[0].bindByIndex(1, TEXT);
   stmts[0].bindByIndex(2, REAL);
   stmts[0].bindByIndex(3, null);
   stmts[0].bindBlobByIndex(4, BLOB, BLOB.length);
   stmts[1] = getOpenedDatabase().createAsyncStatement(
     "INSERT INTO test (string, number, nuller, blober) VALUES (?, ?, ?, ?)"
   );
   stmts[1].bindByIndex(0, TEXT);
   stmts[1].bindByIndex(1, REAL);
   stmts[1].bindByIndex(2, null);
   stmts[1].bindBlobByIndex(3, BLOB, BLOB.length);
 
-  getOpenedDatabase().executeAsync(stmts, stmts.length, {
-    handleResult: function (aResultSet) {
-      dump("handleResult(" + aResultSet + ")\n");
-      do_throw("unexpected results obtained!");
-    },
-    handleError: function (aError)
-    {
-      dump("handleError(" + aError.result + ")\n");
-      do_throw("unexpected error!");
-    },
-    handleCompletion: function (aReason) {
-      dump("handleCompletion(" + aReason + ")\n");
-      do_check_eq(Ci.mozIStorageStatementCallback.REASON_FINISHED, aReason);
+  // asynchronously execute the statements
+  let execResult = yield executeMultipleStatementsAsync(
+    db,
+    stmts,
+    function(aResultSet) {
+      ok(false, 'we only did inserts so we should not have gotten results!');
+    });
+  equal(Ci.mozIStorageStatementCallback.REASON_FINISHED, execResult,
+        'execution should have finished successfully.');
 
-      // Check that the result is in the table
-      let stmt = getOpenedDatabase().createStatement(
-        "SELECT string, number, nuller, blober FROM test WHERE id = ?"
-      );
-      stmt.bindByIndex(0, INTEGER);
-      try {
-        do_check_true(stmt.executeStep());
-        do_check_eq(TEXT, stmt.getString(0));
-        do_check_eq(REAL, stmt.getDouble(1));
-        do_check_true(stmt.getIsNull(2));
-        let count = { value: 0 };
-        let blob = { value: null };
-        stmt.getBlob(3, count, blob);
-        do_check_eq(BLOB.length, count.value);
-        for (let i = 0; i < BLOB.length; i++)
-          do_check_eq(BLOB[i], blob.value[i]);
-      }
-      finally {
-        stmt.finalize();
-      }
+  // Check that the result is in the table
+  let stmt = db.createStatement(
+    "SELECT string, number, nuller, blober FROM test WHERE id = ?"
+  );
+  stmt.bindByIndex(0, INTEGER);
+  try {
+    do_check_true(stmt.executeStep());
+    do_check_eq(TEXT, stmt.getString(0));
+    do_check_eq(REAL, stmt.getDouble(1));
+    do_check_true(stmt.getIsNull(2));
+    let count = { value: 0 };
+    let blob = { value: null };
+    stmt.getBlob(3, count, blob);
+    do_check_eq(BLOB.length, count.value);
+    for (let i = 0; i < BLOB.length; i++)
+      do_check_eq(BLOB[i], blob.value[i]);
+  }
+  finally {
+    stmt.finalize();
+  }
 
-      // Make sure we have two rows in the table
-      stmt = getOpenedDatabase().createStatement(
-        "SELECT COUNT(1) FROM test"
-      );
-      try {
-        do_check_true(stmt.executeStep());
-        do_check_eq(2, stmt.getInt32(0));
-      }
-      finally {
-        stmt.finalize();
-      }
+  // Make sure we have two rows in the table
+  stmt = db.createStatement(
+    "SELECT COUNT(1) FROM test"
+  );
+  try {
+    do_check_true(stmt.executeStep());
+    do_check_eq(2, stmt.getInt32(0));
+  }
+  finally {
+    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;
   const ITERATIONS = 5;
 
   let stmts = [];
   let db = getOpenedDatabase();
   let sqlString = "INSERT INTO test (id, string, number, nuller, blober) " +
@@ -135,101 +135,37 @@ add_test(function test_multiple_bindings
     do_check_true(countStmt.executeStep());
     currentRows = countStmt.row.count;
   }
   finally {
     countStmt.reset();
   }
 
   // Execute asynchronously.
-  getOpenedDatabase().executeAsync(stmts, stmts.length, {
-    handleResult: function (aResultSet) {
-      do_throw("Unexpected call to handleResult!");
-    },
-    handleError: function (aError) {
-      print("Error code " + aError.result + " with message '" +
-            aError.message + "' returned.");
-      do_throw("Unexpected error!");
-    },
-    handleCompletion: function (aReason) {
-      print("handleCompletion(" + aReason +
-            ") for test_multiple_bindings_on_statements");
-      do_check_eq(Ci.mozIStorageStatementCallback.REASON_FINISHED, aReason);
+  let execResult = yield executeMultipleStatementsAsync(
+    db,
+    stmts,
+    function(aResultSet) {
+      ok(false, 'we only did inserts so we should not have gotten results!');
+    });
+  equal(Ci.mozIStorageStatementCallback.REASON_FINISHED, execResult,
+        'execution should have finished successfully.');
 
-      // 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();
-      }
+  // 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());
+
+  // 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;
 });
 
-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_throw("should have thrown");
-    // There is a small race condition here, which can cause either of
-    // Cr.NS_ERROR_NOT_INITIALIZED or Cr.NS_ERROR_UNEXPECTED to be thrown.
-  } catch (e) {
-    if ("result" in e && e.result == Cr.NS_ERROR_NOT_INITIALIZED) {
-      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.
-  gDBConn = null;
-  run_next_test();
-});
-
-function run_test() {
-  cleanup();
-  run_next_test();
-}
+// If you add a test down here you will need to move the asyncClose or clean
+// things up a little more.
--- a/storage/test/unit/xpcshell.ini
+++ b/storage/test/unit/xpcshell.ini
@@ -12,16 +12,17 @@ support-files =
 [test_bug-365166.js]
 [test_bug-393952.js]
 [test_bug-429521.js]
 [test_bug-444233.js]
 [test_cache_size.js]
 [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]
 [test_levenshtein.js]
 [test_like.js]
 [test_like_escape.js]
 [test_locale_collation.js]
 [test_page_size_is_32k.js]