Bug 496019 - mozilla::storage::Connection::Close can spin a nested event loop
☠☠ backed out by 78faeda5e970 ☠ ☠
authorShawn Wilsher <sdwilsh@shawnwilsher.com>
Mon, 09 Nov 2009 09:58:34 -0800
changeset 35397 f91a016416d11272380828cc0b20c97a7c8cf851
parent 35396 e2f77caaa367f905ec5b3820289336da2eaea0d5
child 35398 e9f64cd044f3af6a40f6fac1f4d377890b92badc
child 35407 78faeda5e9702c28f7ac40bdd2ec5bc2cce13c85
push id1
push userroot
push dateTue, 26 Apr 2011 22:38:44 +0000
treeherdermozilla-beta@bfdb6e623a36 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs496019
milestone1.9.3a1pre
Bug 496019 - mozilla::storage::Connection::Close can spin a nested event loop Creates a new close method that must be used when using asynchronous statements, and disallow Close from being called in that case. r=asuth sr=vlad a=dbaron
storage/public/Makefile.in
storage/public/mozIStorageConnection.idl
storage/src/mozStorageAsyncStatementExecution.cpp
storage/src/mozStorageConnection.cpp
storage/src/mozStorageConnection.h
storage/src/mozStoragePrivateHelpers.cpp
storage/src/mozStoragePrivateHelpers.h
storage/test/unit/test_connection_executeAsync.js
storage/test/unit/test_storage_connection.js
--- a/storage/public/Makefile.in
+++ b/storage/public/Makefile.in
@@ -59,16 +59,17 @@ XPIDLSRCS = \
 	mozIStorageValueArray.idl \
 	mozIStorageResultSet.idl \
 	mozIStorageRow.idl \
   mozIStorageError.idl \
   mozIStorageStatementCallback.idl \
   mozIStoragePendingStatement.idl \
   mozIStorageBindingParamsArray.idl \
   mozIStorageBindingParams.idl \
+  mozIStorageCompletionCallback.idl \
 	$(NULL)
 
 EXPORTS_NAMESPACES = mozilla
 
 EXPORTS = \
 	mozStorageHelper.h \
 	mozStorage.h \
 	$(NULL)
--- a/storage/public/mozIStorageConnection.idl
+++ b/storage/public/mozIStorageConnection.idl
@@ -37,43 +37,63 @@
  * the provisions above, a recipient may use your version of this file under
  * the terms of any one of the MPL, the GPL or the LGPL.
  *
  * ***** END LICENSE BLOCK ***** */
 
 #include "nsISupports.idl"
 
 interface mozIStorageAggregateFunction;
+interface mozIStorageCompletionCallback;
 interface mozIStorageFunction;
 interface mozIStorageProgressHandler;
 interface mozIStorageStatement;
 interface mozIStorageStatementCallback;
 interface mozIStoragePendingStatement;
 interface nsIFile;
 
 /**
  * mozIStorageConnection represents a database connection attached to
  * a specific file or to the in-memory data storage.  It is the
  * primary interface for interacting with a database, including
  * creating prepared statements, executing SQL, and examining database
  * errors.
  *
  * @threadsafe
  */
-[scriptable, uuid(ac3c486c-69a1-4cbe-8f25-2ad20880eab3)]
+[scriptable, uuid(5a06b207-1977-47d4-b140-271cb851bb26)]
 interface mozIStorageConnection : nsISupports {
   /*
    * Initialization and status
    */
 
   /**
-   * Closes a database connection.  C++ callers should simply set the database
-   * variable to NULL.
+   * Closes a database connection.  Callers must finalize all statements created
+   * for this connection prior to calling this method.  It is illegal to use
+   * call this method if any asynchronous statements have been executed on this
+   * connection.
+   *
+   * @throws NS_ERROR_UNEXPECTED
+   *         If any statement has been executed asynchronously on this object.
+   * @throws NS_ERROR_UNEXPECTED
+   *         If is called on a thread other than the one that opened it.
    */
-   void close();
+  void close();
+
+  /**
+   * Asynchronously closes a database connection, allowing all pending
+   * asynchronous statements to complete first.
+   *
+   * @param aCallback [optional]
+   *        A callback that will be notified when the close is completed.
+   *
+   * @throws NS_ERROR_UNEXPECTED
+   *         If is called on a thread other than the one that opened it.
+   */
+  void asyncClose([optional] in mozIStorageCompletionCallback aCallback);
 
   /**
    * Indicates if the connection is open and ready to use.  This will be false
    * if the connection failed to open, or it has been closed.
    */
   readonly attribute boolean connectionReady;
 
   /**
--- a/storage/src/mozStorageAsyncStatementExecution.cpp
+++ b/storage/src/mozStorageAsyncStatementExecution.cpp
@@ -185,17 +185,17 @@ AsyncExecuteStatements::execute(Statemen
   NS_ENSURE_TRUE(event, NS_ERROR_OUT_OF_MEMORY);
 
   // Dispatch it to the background
   nsCOMPtr<nsIEventTarget> target(aConnection->getAsyncExecutionTarget());
   NS_ENSURE_TRUE(target, NS_ERROR_NOT_AVAILABLE);
   nsresult rv = target->Dispatch(event, NS_DISPATCH_NORMAL);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  // Return it as the pending statement object
+  // Return it as the pending statement object and track it.
   NS_ADDREF(*_stmt = event);
   return NS_OK;
 }
 
 AsyncExecuteStatements::AsyncExecuteStatements(StatementDataArray &aStatements,
                                                Connection *aConnection,
                                                mozIStorageStatementCallback *aCallback)
 : mConnection(aConnection)
--- a/storage/src/mozStorageConnection.cpp
+++ b/storage/src/mozStorageConnection.cpp
@@ -49,16 +49,17 @@
 #include "nsAutoPtr.h"
 #include "nsIFile.h"
 #include "nsIPrefService.h"
 #include "nsIPrefBranch.h"
 #include "nsThreadUtils.h"
 #include "nsAutoLock.h"
 
 #include "mozIStorageAggregateFunction.h"
+#include "mozIStorageCompletionCallback.h"
 #include "mozIStorageFunction.h"
 
 #include "mozStorageAsyncStatementExecution.h"
 #include "mozStorageSQLFunctions.h"
 #include "mozStorageConnection.h"
 #include "mozStorageService.h"
 #include "mozStorageStatement.h"
 #include "mozStorageArgValueArray.h"
@@ -233,55 +234,97 @@ aggregateFunctionFinalHelper(sqlite3_con
   if (variantToSQLiteT(aCtx, result) != SQLITE_OK) {
     NS_WARNING("User aggregate final function returned invalid data type!");
     ::sqlite3_result_error(aCtx,
                            "User aggregate final function returned invalid data type",
                            -1);
   }
 }
 
+} // anonymous namespace
+
+////////////////////////////////////////////////////////////////////////////////
+//// Local Classes
+
+namespace {
+
+class AsyncCloseConnection : public nsRunnable
+{
+public:
+  AsyncCloseConnection(Connection *aConnection,
+                       nsIEventTarget *aCallingThread,
+                       nsIRunnable *aCallbackEvent)
+  : mConnection(aConnection)
+  , mCallingThread(aCallingThread)
+  , mCallbackEvent(aCallbackEvent)
+  {
+  }
+
+  NS_METHOD Run()
+  {
+    // This event is first dispatched to the background thread to ensure that
+    // all pending asynchronous events are completed, and then back to the
+    // calling thread to actually close and notify.
+    PRBool onCallingThread = PR_FALSE;
+    (void)mCallingThread->IsOnCurrentThread(&onCallingThread);
+    if (!onCallingThread) {
+      (void)mCallingThread->Dispatch(this, NS_DISPATCH_NORMAL);
+      return NS_OK;
+    }
+
+    (void)mConnection->internalClose();
+    if (mCallbackEvent)
+      (void)mCallingThread->Dispatch(mCallbackEvent, NS_DISPATCH_NORMAL);
+
+    return NS_OK;
+  }
+private:
+  nsCOMPtr<Connection> mConnection;
+  nsCOMPtr<nsIEventTarget> mCallingThread;
+  nsCOMPtr<nsIRunnable> mCallbackEvent;
+};
 
 } // anonymous namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 //// Connection
 
 Connection::Connection(Service *aService)
 : sharedAsyncExecutionMutex("Connection::sharedAsyncExecutionMutex")
 , mDBConn(nsnull)
-, mAsyncExecutionMutex(nsAutoLock::NewLock("AsyncExecutionMutex"))
-, mAsyncExecutionThreadShuttingDown(PR_FALSE)
+, mAsyncExecutionMutex("Connection::mAsyncExecutionMutex")
+, mAsyncExecutionThreadShuttingDown(false)
 , mTransactionMutex(nsAutoLock::NewLock("TransactionMutex"))
 , mTransactionInProgress(PR_FALSE)
 , mFunctionsMutex(nsAutoLock::NewLock("FunctionsMutex"))
 , mProgressHandlerMutex(nsAutoLock::NewLock("ProgressHandlerMutex"))
 , mProgressHandler(nsnull)
+, mOpenedThread(do_GetCurrentThread())
 , mStorageService(aService)
 {
   mFunctions.Init();
 }
 
 Connection::~Connection()
 {
   (void)Close();
-  nsAutoLock::DestroyLock(mAsyncExecutionMutex);
   nsAutoLock::DestroyLock(mTransactionMutex);
   nsAutoLock::DestroyLock(mFunctionsMutex);
   nsAutoLock::DestroyLock(mProgressHandlerMutex);
 }
 
 NS_IMPL_THREADSAFE_ISUPPORTS1(
   Connection,
   mozIStorageConnection
 )
 
 already_AddRefed<nsIEventTarget>
 Connection::getAsyncExecutionTarget()
 {
-  nsAutoLock mutex(mAsyncExecutionMutex);
+  MutexAutoLock lockedScope(mAsyncExecutionMutex);
 
   // If we are shutting down the asynchronous thread, don't hand out any more
   // references to the thread.
   if (mAsyncExecutionThreadShuttingDown)
     return nsnull;
 
   if (!mAsyncExecutionThread) {
     nsresult rv = ::NS_NewThread(getter_AddRefs(mAsyncExecutionThread));
@@ -295,17 +338,16 @@ Connection::getAsyncExecutionTarget()
   NS_ADDREF(eventTarget = mAsyncExecutionThread);
   return eventTarget;
 }
 
 nsresult
 Connection::initialize(nsIFile *aDatabaseFile)
 {
   NS_ASSERTION (!mDBConn, "Initialize called on already opened database!");
-  NS_ENSURE_TRUE(mAsyncExecutionMutex, NS_ERROR_OUT_OF_MEMORY);
   NS_ENSURE_TRUE(mTransactionMutex, NS_ERROR_OUT_OF_MEMORY);
   NS_ENSURE_TRUE(mFunctionsMutex, NS_ERROR_OUT_OF_MEMORY);
   NS_ENSURE_TRUE(mProgressHandlerMutex, NS_ERROR_OUT_OF_MEMORY);
 
   int srv;
   nsresult rv;
 
   mDatabaseFile = aDatabaseFile;
@@ -465,72 +507,137 @@ Connection::progressHandler()
     PRBool result;
     nsresult rv = mProgressHandler->OnProgress(this, &result);
     if (NS_FAILED(rv)) return 0; // Don't break request
     return result ? 1 : 0;
   }
   return 0;
 }
 
-////////////////////////////////////////////////////////////////////////////////
-//// mozIStorageConnection
+nsresult
+Connection::setClosedState()
+{
+  // Ensure that we are on the correct thread to close the database.
+  PRBool onOpenedThread;
+  nsresult rv = mOpenedThread->IsOnCurrentThread(&onOpenedThread);
+  NS_ENSURE_SUCCESS(rv, rv);
+  if (!onOpenedThread) {
+    NS_ERROR("Must close the database on the thread that you opened it with!");
+    return NS_ERROR_UNEXPECTED;
+  }
+
+  // Flag that we are shutting down the async thread, so that
+  // getAsyncExecutionTarget knows not to expose/create the async thread.
+  {
+    MutexAutoLock lockedScope(mAsyncExecutionMutex);
+    NS_ENSURE_FALSE(mAsyncExecutionThreadShuttingDown, NS_ERROR_UNEXPECTED);
+    mAsyncExecutionThreadShuttingDown = true;
+  }
 
-NS_IMETHODIMP
-Connection::Close()
+  return NS_OK;
+}
+
+nsresult
+Connection::internalClose()
 {
-  if (!mDBConn)
-    return NS_ERROR_NOT_INITIALIZED;
+#ifdef DEBUG
+  // Sanity checks to make sure we are in the proper state before calling this.
+  NS_ASSERTION(mDBConn, "Database connection is already null!");
+
+  { // Make sure we have marked our async thread as shutting down.
+    MutexAutoLock lockedScope(mAsyncExecutionMutex);
+    NS_ASSERTION(mAsyncExecutionThreadShuttingDown,
+                 "Did not call setClosedState!");
+  }
+
+  { // Ensure that we are being called on the thread we were opened with.
+    PRBool onOpenedThread = PR_FALSE;
+    (void)mOpenedThread->IsOnCurrentThread(&onOpenedThread);
+    NS_ASSERTION(onOpenedThread,
+                 "Not called on the thread the database was opened on!");
+  }
+#endif
 
 #ifdef PR_LOGGING
   nsCAutoString leafName(":memory");
   if (mDatabaseFile)
       (void)mDatabaseFile->GetNativeLeafName(leafName);
   PR_LOG(gStorageLog, PR_LOG_NOTICE, ("Closing connection to '%s'",
                                       leafName.get()));
 #endif
 
-  // Flag that we are shutting down the async thread, so that
-  // getAsyncExecutionTarget knows not to expose/create the async thread.
-  {
-    nsAutoLock mutex(mAsyncExecutionMutex);
-    mAsyncExecutionThreadShuttingDown = PR_TRUE;
-  }
-  // Shutdown the async thread if it exists.  (Because we just set the flag,
-  // we are the only code that is going to be touching this variable from here
-  // on out.)
-  if (mAsyncExecutionThread) {
-    mAsyncExecutionThread->Shutdown();
-    mAsyncExecutionThread = nsnull;
-  }
-
 #ifdef DEBUG
   // Notify about any non-finalized statements.
   sqlite3_stmt *stmt = NULL;
   while ((stmt = ::sqlite3_next_stmt(mDBConn, stmt))) {
     char *msg = ::PR_smprintf("SQL statement '%s' was not finalized",
                               ::sqlite3_sql(stmt));
     NS_WARNING(msg);
     ::PR_smprintf_free(msg);
   }
 #endif
 
-  {
-    nsAutoLock mutex(mProgressHandlerMutex);
-    if (mProgressHandler)
-      ::sqlite3_progress_handler(mDBConn, 0, NULL, NULL);
-  }
-
   int srv = ::sqlite3_close(mDBConn);
   NS_ASSERTION(srv == SQLITE_OK,
                "sqlite3_close failed. There are probably outstanding statements that are listed above!");
+  mDBConn = NULL;
 
-  mDBConn = NULL;
   return convertResultCode(srv);
 }
 
+////////////////////////////////////////////////////////////////////////////////
+//// mozIStorageConnection
+
+NS_IMETHODIMP
+Connection::Close()
+{
+  if (!mDBConn)
+    return NS_ERROR_NOT_INITIALIZED;
+
+  { // Make sure we have not executed any asynchronous statements.
+    MutexAutoLock lockedScope(mAsyncExecutionMutex);
+    NS_ENSURE_FALSE(mAsyncExecutionThread, NS_ERROR_UNEXPECTED);
+  }
+
+  nsresult rv = setClosedState();
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  return internalClose();
+}
+
+NS_IMETHODIMP
+Connection::AsyncClose(mozIStorageCompletionCallback *aCallback)
+{
+  if (!mDBConn)
+    return NS_ERROR_NOT_INITIALIZED;
+
+  nsCOMPtr<nsIEventTarget> asyncThread(getAsyncExecutionTarget());
+  NS_ENSURE_TRUE(asyncThread, NS_ERROR_UNEXPECTED);
+
+  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);
+    NS_ENSURE_TRUE(completeEvent, NS_ERROR_OUT_OF_MEMORY);
+  }
+
+  // Create and dispatch our close event to the background thread.
+  nsCOMPtr<nsIRunnable> closeEvent =
+    new AsyncCloseConnection(this, NS_GetCurrentThread(), completeEvent);
+  NS_ENSURE_TRUE(closeEvent, NS_ERROR_OUT_OF_MEMORY);
+
+  rv = asyncThread->Dispatch(closeEvent, NS_DISPATCH_NORMAL);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  return NS_OK;
+}
+
 NS_IMETHODIMP
 Connection::GetConnectionReady(PRBool *_ready)
 {
   *_ready = (mDBConn != nsnull);
   return NS_OK;
 }
 
 NS_IMETHODIMP
--- a/storage/src/mozStorageConnection.h
+++ b/storage/src/mozStorageConnection.h
@@ -95,20 +95,33 @@ public:
 
   /**
    * Mutex used by asynchronous statements to protect state.  The mutex is
    * declared on the connection object because there is no contention between
    * asynchronous statements (they are serialized on mAsyncExecutionThread).
    */
   Mutex sharedAsyncExecutionMutex;
 
+  /**
+   * Closes the SQLite database, and warns about any non-finalized statements.
+   */
+  nsresult internalClose();
+
 private:
   ~Connection();
 
   /**
+   * Sets the database into a closed state so no further actions can be
+   * performed.
+   *
+   * @note mDBConn is set to NULL in this method.
+   */
+  nsresult setClosedState();
+
+  /**
    * Describes a certain primitive type in the database.
    *
    * Possible Values Are:
    *  INDEX - To check for the existence of an index
    *  TABLE - To check for the existence of a table
    */
   enum DatabaseElementType {
     INDEX,
@@ -135,43 +148,49 @@ private:
   // Dispatch call to registered progress handler,
   // if there is one. Do nothing in other cases.
   int progressHandler();
 
   sqlite3 *mDBConn;
   nsCOMPtr<nsIFile> mDatabaseFile;
 
   /**
-   * Protects access to mAsyncExecutionThread.
+   * Protects access to mAsyncExecutionThread and mPendingStatements.
    */
-  PRLock *mAsyncExecutionMutex;
+  Mutex mAsyncExecutionMutex;
 
   /**
    * Lazily created thread for asynchronous statement execution.  Consumers
    * should use getAsyncExecutionTarget rather than directly accessing this
    * field.
    */
   nsCOMPtr<nsIThread> mAsyncExecutionThread;
   /**
    * Set to true by Close() prior to actually shutting down the thread.  This
    * lets getAsyncExecutionTarget() know not to hand out any more thread
    * references (or to create the thread in the first place).  This variable
    * should be accessed while holding the mAsyncExecutionMutex.
    */
-  PRBool mAsyncExecutionThreadShuttingDown;
+  bool mAsyncExecutionThreadShuttingDown;
 
   PRLock *mTransactionMutex;
   PRBool mTransactionInProgress;
 
   PRLock *mFunctionsMutex;
   nsInterfaceHashtable<nsCStringHashKey, nsISupports> mFunctions;
 
   PRLock *mProgressHandlerMutex;
   nsCOMPtr<mozIStorageProgressHandler> mProgressHandler;
 
+  /**
+   * References the thread this database was opened on.  This MUST be thread it
+   * is closed on.
+   */
+  nsCOMPtr<nsIThread> mOpenedThread;
+
   // This is here for two reasons: 1) It's used to make sure that the
   // connections do not outlive the service.  2) Our custom collating functions
   // call its localeCompareStrings() method.
   nsRefPtr<Service> mStorageService;
 };
 
 } // namespace storage
 } // namespace mozilla
--- a/storage/src/mozStoragePrivateHelpers.cpp
+++ b/storage/src/mozStoragePrivateHelpers.cpp
@@ -41,19 +41,21 @@
 #include "sqlite3.h"
 
 #include "jsapi.h"
 #include "jsdate.h"
 
 #include "nsPrintfCString.h"
 #include "nsString.h"
 #include "nsError.h"
+#include "nsThreadUtils.h"
 
 #include "mozStoragePrivateHelpers.h"
 #include "mozIStorageStatement.h"
+#include "mozIStorageCompletionCallback.h"
 
 namespace mozilla {
 namespace storage {
 
 nsresult
 convertResultCode(int aSQLiteResultCode)
 {
   switch (aSQLiteResultCode) {
@@ -158,23 +160,49 @@ bindJSValue(JSContext *aCtx,
     return true;
   }
 
   if (JSVAL_IS_OBJECT(aValue)) {
     JSObject *obj = JSVAL_TO_OBJECT(aValue);
     // some special things
     if (!::js_DateIsValid(aCtx, obj))
       return false;
-    
+
     double msecd = ::js_DateGetMsecSinceEpoch(aCtx, obj);
     msecd *= 1000.0;
     PRInt64 msec;
     LL_D2L(msec, msecd);
-    
+
     (void)aStatement->BindInt64Parameter(aIdx, msec);
     return true;
   }
 
   return false;
 }
 
+namespace {
+class CallbackEvent : public nsRunnable
+{
+public:
+  CallbackEvent(mozIStorageCompletionCallback *aCallback)
+  : mCallback(aCallback)
+  {
+  }
+
+  NS_IMETHOD Run()
+  {
+    (void)mCallback->Complete();
+    return NS_OK;
+  }
+private:
+  nsCOMPtr<mozIStorageCompletionCallback> mCallback;
+};
+} // anonymous namespace
+already_AddRefed<nsIRunnable>
+newCompletionEvent(mozIStorageCompletionCallback *aCallback)
+{
+  NS_ASSERTION(aCallback, "Passing a null callback is a no-no!");
+  nsCOMPtr<nsIRunnable> event = new CallbackEvent(aCallback);
+  return event.forget();
+}
+
 } // namespace storage
 } // namespace mozilla
--- a/storage/src/mozStoragePrivateHelpers.h
+++ b/storage/src/mozStoragePrivateHelpers.h
@@ -44,18 +44,21 @@
 /**
  * This file contains convenience methods for mozStorage.
  */
 
 #include "sqlite3.h"
 #include "nsIVariant.h"
 #include "mozStorage.h"
 #include "jsapi.h"
+#include "nsAutoPtr.h"
 
+class mozIStorageCompletionCallback;
 class mozIStorageStatement;
+class nsIRunnable;
 
 namespace mozilla {
 namespace storage {
 
 ////////////////////////////////////////////////////////////////////////////////
 //// Macros
 
 #define ENSURE_INDEX_VALUE(aIndex, aCount) \
@@ -80,23 +83,40 @@ nsresult convertResultCode(int aSQLiteRe
  * made faster with the careful use of an index.
  *
  * @param aStatement
  *        The sqlite3_stmt object to check.
  */
 void checkAndLogStatementPerformance(sqlite3_stmt *aStatement);
 
 /**
+ * Binds a jsval to a statement at the given index.
  *
+ * @param aCtx
+ *        The JSContext jsval is associated with.
+ * @param aStatement
+ *        The statement to bind to.
+ * @param aIdx
+ *        The one-based index to bind aValue to.
+ * @param aValue
+ *        The value to bind to aStatement.
+ * @return true if we bound the value to the statement, false otherwise.
  */
-bool
-bindJSValue(JSContext *aCtx,
-            mozIStorageStatement *aStatement,
-            int aIdx,
-            jsval aValue);
+bool bindJSValue(JSContext *aCtx, mozIStorageStatement *aStatement, int aIdx,
+                 jsval aValue);
+
+/**
+ * Obtains an event that will notify a completion callback about completion.
+ *
+ * @param aCallback
+ *        The callback to be notified.
+ * @return an nsIRunnable that can be dispatched to the calling thread.
+ */
+already_AddRefed<nsIRunnable>
+newCompletionEvent(mozIStorageCompletionCallback *aCallback);
 
 /**
  * Used to convert an nsIVariant to the proper SQLite type.
  */
 template <typename T>
 int variantToSQLiteT(T aObj, nsIVariant *aValue);
 #include "variantToSQLiteT_impl.h" // To keep this file easier to read.
 
--- a/storage/test/unit/test_connection_executeAsync.js
+++ b/storage/test/unit/test_connection_executeAsync.js
@@ -231,36 +231,111 @@ function test_multiple_bindings_on_state
 
       // Run the next test.
       run_next_test();
     }
   });
   stmts.forEach(function(stmt) stmt.finalize());
 }
 
+function test_asyncClose_does_not_complete_before_statements()
+{
+  let stmt = createStatement("SELECT * FROM sqlite_master");
+  let executed = false;
+  stmt.executeAsync({
+    handleResult: function(aResultSet)
+    {
+    },
+    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_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();
+  });
+}
+
+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();
+}
+
+function test_double_asyncClose_throws()
+{
+  let conn = getOpenedDatabase();
+  conn.asyncClose();
+  try {
+    conn.asyncClose();
+    do_throw("should have thrown");
+  }
+  catch (e) {
+    do_check_eq(e.result, Cr.NS_ERROR_UNEXPECTED);
+  }
+
+  // Reset gDBConn so that later tests will get a new connection object.
+  gDBConn = null;
+  run_next_test();
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 //// Test Runner
 
 let tests =
 [
   test_create_and_add,
   test_transaction_created,
   test_multiple_bindings_on_statements,
+  test_asyncClose_does_not_complete_before_statements,
+  test_asyncClose_does_not_throw_no_callback,
+  test_double_asyncClose_throws,
 ];
 let index = 0;
 
 function run_next_test()
 {
-  if (index < tests.length) {
-    do_test_pending();
-    print("Running the next test: " + tests[index].name);
-    tests[index++]();
+  function _run_next_test() {
+    if (index < tests.length) {
+      do_test_pending();
+      print("Running the next test: " + tests[index].name);
+
+      // Asynchronous tests means that exceptions don't kill the test.
+      try {
+        tests[index++]();
+      }
+      catch (e) {
+        do_throw(e);
+      }
+    }
+
+    do_test_finished();
   }
 
-  do_test_finished();
+  // For saner stacks, we execute this code RSN.
+  do_execute_soon(_run_next_test);
 }
 
 function run_test()
 {
   cleanup();
 
   do_test_pending();
   run_next_test();
--- a/storage/test/unit/test_storage_connection.js
+++ b/storage/test/unit/test_storage_connection.js
@@ -44,192 +44,271 @@ function test_connectionReady_open()
 {
   // there doesn't seem to be a way for the connection to not be ready (unless
   // we close it with mozIStorageConnection::Close(), but we don't for this).
   // It can only fail if GetPath fails on the database file, or if we run out
   // of memory trying to use an in-memory database
 
   var msc = getOpenedDatabase();
   do_check_true(msc.connectionReady);
+  run_next_test();
 }
 
 function test_connectionReady_closed()
 {
   // This also tests mozIStorageConnection::Close()
 
   var msc = getOpenedDatabase();
   msc.close();
   do_check_false(msc.connectionReady);
   gDBConn = null; // this is so later tests don't start to fail.
+  run_next_test();
 }
 
 function test_databaseFile()
 {
   var msc = getOpenedDatabase();
   do_check_true(getTestDB().equals(msc.databaseFile));
+  run_next_test();
 }
 
 function test_tableExists_not_created()
 {
   var msc = getOpenedDatabase();
   do_check_false(msc.tableExists("foo"));
+  run_next_test();
 }
 
 function test_indexExists_not_created()
 {
   var msc = getOpenedDatabase();
   do_check_false(msc.indexExists("foo"));
+  run_next_test();
 }
 
 function test_createTable_not_created()
 {
   var msc = getOpenedDatabase();
   msc.createTable("test", "id INTEGER PRIMARY KEY, name TEXT");
   do_check_true(msc.tableExists("test"));
+  run_next_test();
 }
 
 function test_indexExists_created()
 {
   var msc = getOpenedDatabase();
   msc.executeSimpleSQL("CREATE INDEX name_ind ON test (name)");
   do_check_true(msc.indexExists("name_ind"));
+  run_next_test();
 }
 
 function test_createTable_already_created()
 {
   var msc = getOpenedDatabase();
   do_check_true(msc.tableExists("test"));
   try {
     msc.createTable("test", "id INTEGER PRIMARY KEY, name TEXT");
     do_throw("We shouldn't get here!");
   } catch (e) {
     do_check_eq(Cr.NS_ERROR_FAILURE, e.result);
   }
+  run_next_test();
 }
 
 function test_lastInsertRowID()
 {
   var msc = getOpenedDatabase();
   msc.executeSimpleSQL("INSERT INTO test (name) VALUES ('foo')");
   do_check_eq(1, msc.lastInsertRowID);
+  run_next_test();
 }
 
 function test_transactionInProgress_no()
 {
   var msc = getOpenedDatabase();
   do_check_false(msc.transactionInProgress);
+  run_next_test();
 }
 
 function test_transactionInProgress_yes()
 {
   var msc = getOpenedDatabase();
   msc.beginTransaction();
   do_check_true(msc.transactionInProgress);
   msc.commitTransaction();
   do_check_false(msc.transactionInProgress);
 
   msc.beginTransaction();
   do_check_true(msc.transactionInProgress);
   msc.rollbackTransaction();
   do_check_false(msc.transactionInProgress);
+  run_next_test();
 }
 
 function test_commitTransaction_no_transaction()
 {
   var msc = getOpenedDatabase();
   do_check_false(msc.transactionInProgress);
   try {
     msc.commitTransaction();
     do_throw("We should not get here!");
   } catch (e) {
     do_check_eq(Cr.NS_ERROR_FAILURE, e.result);
   }
+  run_next_test();
 }
 
 function test_rollbackTransaction_no_transaction()
 {
   var msc = getOpenedDatabase();
   do_check_false(msc.transactionInProgress);
   try {
     msc.rollbackTransaction();
     do_throw("We should not get here!");
   } catch (e) {
     do_check_eq(Cr.NS_ERROR_FAILURE, e.result);
   }
+  run_next_test();
 }
 
 function test_get_schemaVersion_not_set()
 {
   do_check_eq(0, getOpenedDatabase().schemaVersion);
+  run_next_test();
 }
 
 function test_set_schemaVersion()
 {
   var msc = getOpenedDatabase();
   const version = 1;
   msc.schemaVersion = version;
   do_check_eq(version, msc.schemaVersion);
+  run_next_test();
 }
 
 function test_set_schemaVersion_same()
 {
   var msc = getOpenedDatabase();
   const version = 1;
   msc.schemaVersion = version; // should still work ok
   do_check_eq(version, msc.schemaVersion);
+  run_next_test();
 }
 
 function test_set_schemaVersion_negative()
 {
   var msc = getOpenedDatabase();
   const version = -1;
   msc.schemaVersion = version;
   do_check_eq(version, msc.schemaVersion);
+  run_next_test();
 }
 
 function test_createTable(){
   var temp = getTestDB().parent;
   temp.append("test_db_table");
   try {
     var con = getService().openDatabase(temp);
     con.createTable("a","");
   } catch (e) {
     if (temp.exists()) try {
       temp.remove(false);
     } catch (e2) {}
     do_check_true(e.result==Cr.NS_ERROR_NOT_INITIALIZED ||
                   e.result==Cr.NS_ERROR_FAILURE);
   }
+  run_next_test();
 }
 
 function test_defaultSynchronousAtNormal()
 {
   var msc = getOpenedDatabase();
   var stmt = createStatement("PRAGMA synchronous;");
   try {
     stmt.executeStep();
     do_check_eq(1, stmt.getInt32(0));
   }
   finally {
     stmt.reset();
     stmt.finalize();
   }
+  run_next_test();
 }
 
-function test_close_succeeds_with_finalized_async_statement()
+function test_close_does_not_spin_event_loop()
 {
+  // We want to make sure that the event loop on the calling thread does not
+  // spin when close is called.
+  let event = {
+    ran: false,
+    run: function()
+    {
+      this.ran = true;
+    },
+  };
+
+  // Post the event before we call close, so it would run if the event loop was
+  // spun during close.
+  let thread = Cc["@mozilla.org/thread-manager;1"].
+               getService(Ci.nsIThreadManager).
+               currentThread;
+  thread.dispatch(event, Ci.nsIThread.DISPATCH_NORMAL);
+
+  // Sanity check, then close the database.  Afterwards, we should not have ran!
+  do_check_false(event.ran);
+  getOpenedDatabase().close();
+  do_check_false(event.ran);
+
+  // Reset gDBConn so that later tests will get a new connection object.
+  gDBConn = null;
+  run_next_test();
+}
+
+function test_asyncClose_succeeds_with_finalized_async_statement()
+{
+  // XXX this test isn't perfect since we can't totally control when events will
+  //     run.  If this paticular function fails randomly, it means we have a
+  //     real bug.
+
   // We want to make sure we create a cached async statement to make sure that
   // when we finalize our statement, we end up finalizing the async one too so
   // close will succeed.
   let stmt = createStatement("SELECT * FROM test");
   stmt.executeAsync();
   stmt.finalize();
 
-  // Cleanup calls close, as well as removes the database file.
-  cleanup();
+  getOpenedDatabase().asyncClose(function() {
+    // Reset gDBConn so that later tests will get a new connection object.
+    gDBConn = null;
+    run_next_test();
+  });
+}
+
+function test_close_fails_with_async_statement_ran()
+{
+  let stmt = createStatement("SELECT * FROM test");
+  stmt.executeAsync();
+  stmt.finalize();
+
+  let db = getOpenedDatabase();
+  try {
+    db.close();
+    do_throw("should have thrown");
+  }
+  catch (e) {
+    do_check_eq(e.result, Cr.NS_ERROR_UNEXPECTED);
+  }
+  finally {
+    // Clean up after ourselves.
+    db.asyncClose(function() {
+      // Reset gDBConn so that later tests will get a new connection object.
+      gDBConn = null;
+      run_next_test();
+    });
+  }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 //// Test Runner
 
 var tests = [
   test_connectionReady_open,
   test_connectionReady_closed,
@@ -245,18 +324,44 @@ var tests = [
   test_commitTransaction_no_transaction,
   test_rollbackTransaction_no_transaction,
   test_get_schemaVersion_not_set,
   test_set_schemaVersion,
   test_set_schemaVersion_same,
   test_set_schemaVersion_negative,
   test_createTable,
   test_defaultSynchronousAtNormal,
-  test_close_succeeds_with_finalized_async_statement,
+  test_close_does_not_spin_event_loop, // must be ran before executeAsync tests
+  test_asyncClose_succeeds_with_finalized_async_statement,
+  test_close_fails_with_async_statement_ran,
 ];
+let index = 0;
+
+function run_next_test()
+{
+  function _run_next_test() {
+    if (index < tests.length) {
+      do_test_pending();
+      print("Running the next test: " + tests[index].name);
+
+      // Asynchronous tests means that exceptions don't kill the test.
+      try {
+        tests[index++]();
+      }
+      catch (e) {
+        do_throw(e);
+      }
+    }
+
+    do_test_finished();
+  }
+
+  // For saner stacks, we execute this code RSN.
+  do_execute_soon(_run_next_test);
+}
 
 function run_test()
 {
-  for (var i = 0; i < tests.length; i++)
-    tests[i]();
-    
   cleanup();
+
+  do_test_pending();
+  run_next_test();
 }