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

--HG--
extra : rebase_source : 8470e30ef4ca1e9fb516284cafb77b070e46fde3

storage/public/Makefile.in

@@ -64,6 +64,7 @@ XPIDLSRCS = \
   mozIStoragePendingStatement.idl \
   mozIStorageBindingParamsArray.idl \
   mozIStorageBindingParams.idl \
+  mozIStorageCompletionCallback.idl \
 	$(NULL)
 
 EXPORTS_NAMESPACES = mozilla

storage/public/mozIStorageConnection.idl

@@ -42,6 +42,7 @@
 #include "nsISupports.idl"
 
 interface mozIStorageAggregateFunction;
+interface mozIStorageCompletionCallback;
 interface mozIStorageFunction;
 interface mozIStorageProgressHandler;
 interface mozIStorageStatement;
@@ -58,17 +59,36 @@ interface nsIFile;
  *
  * @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
+   * Closes a database connection.  Callers must finalize all statements created
-   * variable to NULL.
+   * 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

storage/src/mozStorageAsyncStatementExecution.cpp

@@ -190,7 +190,7 @@ AsyncExecuteStatements::execute(StatementDataArray &aStatements,
   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;
 }

storage/src/mozStorageConnection.cpp

@@ -54,6 +54,7 @@
 #include "nsAutoLock.h"
 
 #include "mozIStorageAggregateFunction.h"
+#include "mozIStorageCompletionCallback.h"
 #include "mozIStorageFunction.h"
 
 #include "mozStorageAsyncStatementExecution.h"
@@ -238,6 +239,48 @@ aggregateFunctionFinalHelper(sqlite3_context *aCtx)
   }
 }
 
+} // 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
 
@@ -247,13 +290,14 @@ aggregateFunctionFinalHelper(sqlite3_context *aCtx)
 Connection::Connection(Service *aService)
 : sharedAsyncExecutionMutex("Connection::sharedAsyncExecutionMutex")
 , mDBConn(nsnull)
-, mAsyncExecutionMutex(nsAutoLock::NewLock("AsyncExecutionMutex"))
+, mAsyncExecutionMutex("Connection::mAsyncExecutionMutex")
-, mAsyncExecutionThreadShuttingDown(PR_FALSE)
+, 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();
@@ -262,7 +306,6 @@ Connection::Connection(Service *aService)
 Connection::~Connection()
 {
   (void)Close();
-  nsAutoLock::DestroyLock(mAsyncExecutionMutex);
   nsAutoLock::DestroyLock(mTransactionMutex);
   nsAutoLock::DestroyLock(mFunctionsMutex);
   nsAutoLock::DestroyLock(mProgressHandlerMutex);
@@ -276,7 +319,7 @@ NS_IMPL_THREADSAFE_ISUPPORTS1(
 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.
@@ -300,7 +343,6 @@ 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);
@@ -470,14 +512,49 @@ Connection::progressHandler()
   return 0;
 }
 
-////////////////////////////////////////////////////////////////////////////////
+nsresult
-//// mozIStorageConnection
+Connection::setClosedState()
-
-NS_IMETHODIMP
-Connection::Close()
 {
-  if (!mDBConn)
+  // Ensure that we are on the correct thread to close the database.
-    return NS_ERROR_NOT_INITIALIZED;
+  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;
+  }
+
+  return NS_OK;
+}
+
+nsresult
+Connection::internalClose()
+{
+#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");
@@ -487,20 +564,6 @@ Connection::Close()
                                       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;
@@ -512,20 +575,64 @@ Connection::Close()
   }
 #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;
+
   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)
 {

storage/src/mozStorageConnection.h

@@ -100,9 +100,22 @@ public:
    */
   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.
    *
@@ -140,9 +153,9 @@ private:
   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
@@ -156,7 +169,7 @@ private:
    * 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;
@@ -167,6 +180,12 @@ private:
   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.

storage/src/mozStoragePrivateHelpers.cpp

@@ -46,9 +46,11 @@
 #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 {
@@ -163,12 +165,12 @@ bindJSValue(JSContext *aCtx,
     // 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;
   }
@@ -176,5 +178,31 @@ bindJSValue(JSContext *aCtx,
   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

storage/src/mozStoragePrivateHelpers.h

@@ -49,8 +49,11 @@
 #include "nsIVariant.h"
 #include "mozStorage.h"
 #include "jsapi.h"
+#include "nsAutoPtr.h"
 
+class mozIStorageCompletionCallback;
 class mozIStorageStatement;
+class nsIRunnable;
 
 namespace mozilla {
 namespace storage {
@@ -85,13 +88,30 @@ nsresult convertResultCode(int aSQLiteResultCode);
 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
+bool bindJSValue(JSContext *aCtx, mozIStorageStatement *aStatement, int aIdx,
-bindJSValue(JSContext *aCtx,
+                 jsval aValue);
-            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.

storage/test/unit/test_connection_executeAsync.js

@@ -236,6 +236,66 @@ function test_multiple_bindings_on_statements()
   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
 
@@ -244,18 +304,33 @@ 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) {
+  function _run_next_test() {
-    do_test_pending();
+    if (index < tests.length) {
-    print("Running the next test: " + tests[index].name);
+      do_test_pending();
-    tests[index++]();
+      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()

storage/test/unit/test_storage_connection.js

@@ -49,6 +49,7 @@ function test_connectionReady_open()
 
   var msc = getOpenedDatabase();
   do_check_true(msc.connectionReady);
+  run_next_test();
 }
 
 function test_connectionReady_closed()
@@ -59,24 +60,28 @@ function test_connectionReady_closed()
   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()
@@ -84,6 +89,7 @@ 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()
@@ -91,6 +97,7 @@ 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()
@@ -103,6 +110,7 @@ function test_createTable_already_created()
   } catch (e) {
     do_check_eq(Cr.NS_ERROR_FAILURE, e.result);
   }
+  run_next_test();
 }
 
 function test_lastInsertRowID()
@@ -110,12 +118,14 @@ 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()
@@ -130,6 +140,7 @@ function test_transactionInProgress_yes()
   do_check_true(msc.transactionInProgress);
   msc.rollbackTransaction();
   do_check_false(msc.transactionInProgress);
+  run_next_test();
 }
 
 function test_commitTransaction_no_transaction()
@@ -142,6 +153,7 @@ function test_commitTransaction_no_transaction()
   } catch (e) {
     do_check_eq(Cr.NS_ERROR_FAILURE, e.result);
   }
+  run_next_test();
 }
 
 function test_rollbackTransaction_no_transaction()
@@ -154,11 +166,13 @@ function test_rollbackTransaction_no_transaction()
   } 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()
@@ -167,6 +181,7 @@ function test_set_schemaVersion()
   const version = 1;
   msc.schemaVersion = version;
   do_check_eq(version, msc.schemaVersion);
+  run_next_test();
 }
 
 function test_set_schemaVersion_same()
@@ -175,6 +190,7 @@ function test_set_schemaVersion_same()
   const version = 1;
   msc.schemaVersion = version; // should still work ok
   do_check_eq(version, msc.schemaVersion);
+  run_next_test();
 }
 
 function test_set_schemaVersion_negative()
@@ -183,6 +199,7 @@ function test_set_schemaVersion_negative()
   const version = -1;
   msc.schemaVersion = version;
   do_check_eq(version, msc.schemaVersion);
+  run_next_test();
 }
 
 function test_createTable(){
@@ -198,6 +215,7 @@ function test_createTable(){
     do_check_true(e.result==Cr.NS_ERROR_NOT_INITIALIZED ||
                   e.result==Cr.NS_ERROR_FAILURE);
   }
+  run_next_test();
 }
 
 function test_defaultSynchronousAtNormal()
@@ -212,10 +230,44 @@ function test_defaultSynchronousAtNormal()
     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.
@@ -223,8 +275,35 @@ function test_close_succeeds_with_finalized_async_statement()
   stmt.executeAsync();
   stmt.finalize();
 
-  // Cleanup calls close, as well as removes the database file.
+  getOpenedDatabase().asyncClose(function() {
-  cleanup();
+    // 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();
+    });
+  }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -250,13 +329,39 @@ var tests = [
   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();
 }