Bug 914070 - Part 2 - nullify mDBConn at setClosedState and provide an isClosed helper. r=asuth
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 24 Apr 2014 11:54:12 +0200
changeset 179995 528a58952f8c2389ec7d59bf31ad15a4eb420f13
parent 179994 d3613979347679285841cab438875f86cc7ad622
child 179996 dbf72e1458ffbd7f690459da3f95a77a14d6e49c
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewersasuth
bugs914070
milestone31.0a1
Bug 914070 - Part 2 - nullify mDBConn at setClosedState and provide an isClosed helper. r=asuth
storage/src/mozStorageAsyncStatement.cpp
storage/src/mozStorageAsyncStatementExecution.cpp
storage/src/mozStorageConnection.cpp
storage/src/mozStorageConnection.h
storage/src/mozStorageService.cpp
storage/src/mozStorageStatement.cpp
toolkit/components/places/Database.cpp
--- a/storage/src/mozStorageAsyncStatement.cpp
+++ b/storage/src/mozStorageAsyncStatement.cpp
@@ -130,16 +130,17 @@ AsyncStatement::AsyncStatement()
 }
 
 nsresult
 AsyncStatement::initialize(Connection *aDBConnection,
                            sqlite3 *aNativeConnection,
                            const nsACString &aSQLStatement)
 {
   MOZ_ASSERT(aDBConnection, "No database connection given!");
+  MOZ_ASSERT(!aDBConnection->isClosed(), "Database connection should be valid");
   MOZ_ASSERT(aNativeConnection, "No native connection given!");
 
   mDBConnection = aDBConnection;
   mNativeConnection = aNativeConnection;
   mSQLString = aSQLStatement;
 
   PR_LOG(gStorageLog, PR_LOG_NOTICE, ("Inited async statement '%s' (0x%p)",
                                       mSQLString.get()));
@@ -296,17 +297,18 @@ AsyncStatement::getAsyncStatement(sqlite
   // Make sure we are never called on the connection's owning thread.
   bool onOpenedThread = false;
   (void)mDBConnection->threadOpenedOn->IsOnCurrentThread(&onOpenedThread);
   NS_ASSERTION(!onOpenedThread,
                "We should only be called on the async thread!");
 #endif
 
   if (!mAsyncStatement) {
-    int rc = mDBConnection->prepareStatement(mSQLString, &mAsyncStatement);
+    int rc = mDBConnection->prepareStatement(mNativeConnection, mSQLString,
+                                             &mAsyncStatement);
     if (rc != SQLITE_OK) {
       PR_LOG(gStorageLog, PR_LOG_ERROR,
              ("Sqlite statement prepare error: %d '%s'", rc,
               ::sqlite3_errmsg(mNativeConnection)));
       PR_LOG(gStorageLog, PR_LOG_ERROR,
              ("Statement was: '%s'", mSQLString.get()));
       *_stmt = nullptr;
       return rc;
--- a/storage/src/mozStorageAsyncStatementExecution.cpp
+++ b/storage/src/mozStorageAsyncStatementExecution.cpp
@@ -332,17 +332,17 @@ bool
 AsyncExecuteStatements::executeStatement(sqlite3_stmt *aStatement)
 {
   mMutex.AssertNotCurrentThreadOwns();
   Telemetry::AutoTimer<Telemetry::MOZ_STORAGE_ASYNC_REQUESTS_MS> finallySendExecutionDuration(mRequestStartDate);
   while (true) {
     // lock the sqlite mutex so sqlite3_errmsg cannot change
     SQLiteMutexAutoLock lockedScope(mDBMutex);
 
-    int rc = mConnection->stepStatement(aStatement);
+    int rc = mConnection->stepStatement(mNativeConnection, aStatement);
     // Stop if we have no more results.
     if (rc == SQLITE_DONE)
     {
       Telemetry::Accumulate(Telemetry::MOZ_STORAGE_ASYNC_REQUESTS_SUCCESS, true);
       return false;
     }
 
     // If we got results, we can return now.
@@ -563,16 +563,18 @@ AsyncExecuteStatements::Cancel()
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 //// nsIRunnable
 
 NS_IMETHODIMP
 AsyncExecuteStatements::Run()
 {
+  MOZ_ASSERT(!mConnection->isClosed());
+
   // Do not run if we have been canceled.
   {
     MutexAutoLock lockedScope(mMutex);
     if (mCancelRequested)
       mState = CANCELED;
   }
   if (mState == CANCELED)
     return notifyComplete();
--- a/storage/src/mozStorageConnection.cpp
+++ b/storage/src/mozStorageConnection.cpp
@@ -328,35 +328,37 @@ WaitForUnlockNotify(sqlite3* aDatabase)
 //// Local Classes
 
 namespace {
 
 class AsyncCloseConnection MOZ_FINAL: public nsRunnable
 {
 public:
   AsyncCloseConnection(Connection *aConnection,
+                       sqlite3 *aNativeConnection,
                        nsIRunnable *aCallbackEvent,
                        already_AddRefed<nsIThread> aAsyncExecutionThread)
   : mConnection(aConnection)
+  , mNativeConnection(aNativeConnection)
   , mCallbackEvent(aCallbackEvent)
   , mAsyncExecutionThread(aAsyncExecutionThread)
   {
   }
 
   NS_METHOD Run()
   {
 #ifdef DEBUG
     // This code is executed on the background thread
     bool onAsyncThread = false;
     (void)mAsyncExecutionThread->IsOnCurrentThread(&onAsyncThread);
     MOZ_ASSERT(onAsyncThread);
 #endif // DEBUG
 
     // Internal close.
-    (void)mConnection->internalClose();
+    (void)mConnection->internalClose(mNativeConnection);
 
     // Callback
     if (mCallbackEvent) {
       nsCOMPtr<nsIThread> thread;
       (void)NS_GetMainThread(getter_AddRefs(thread));
       (void)thread->Dispatch(mCallbackEvent, NS_DISPATCH_NORMAL);
     }
 
@@ -371,16 +373,17 @@ public:
     mConnection.swap(rawConnection);
     (void)NS_ProxyRelease(thread,
                           NS_ISUPPORTS_CAST(mozIStorageConnection *,
                                             rawConnection));
     (void)NS_ProxyRelease(thread, mCallbackEvent);
   }
 private:
   nsRefPtr<Connection> mConnection;
+  sqlite3 *mNativeConnection;
   nsCOMPtr<nsIRunnable> mCallbackEvent;
   nsCOMPtr<nsIThread> mAsyncExecutionThread;
 };
 
 /**
  * An event used to initialize the clone of a connection.
  *
  * Must be executed on the clone's async execution thread.
@@ -465,16 +468,17 @@ private:
 Connection::Connection(Service *aService,
                        int aFlags,
                        bool aAsyncOnly)
 : sharedAsyncExecutionMutex("Connection::sharedAsyncExecutionMutex")
 , sharedDBMutex("Connection::sharedDBMutex")
 , threadOpenedOn(do_GetCurrentThread())
 , mDBConn(nullptr)
 , mAsyncExecutionThreadShuttingDown(false)
+, mConnectionClosed(false)
 , mTransactionInProgress(false)
 , mProgressHandler(nullptr)
 , mFlags(aFlags)
 , mStorageService(aService)
 , mAsyncOnly(aAsyncOnly)
 {
   mStorageService->registerConnection(this);
 }
@@ -739,21 +743,21 @@ Connection::databaseElementExists(enum D
       query.Append("table");
       break;
   }
   query.Append("' AND name ='");
   query.Append(element);
   query.Append("'");
 
   sqlite3_stmt *stmt;
-  int srv = prepareStatement(query, &stmt);
+  int srv = prepareStatement(mDBConn, query, &stmt);
   if (srv != SQLITE_OK)
     return convertResultCode(srv);
 
-  srv = stepStatement(stmt);
+  srv = stepStatement(mDBConn, stmt);
   // we just care about the return value from step
   (void)::sqlite3_finalize(stmt);
 
   if (srv == SQLITE_ROW) {
     *_exists = true;
     return NS_OK;
   }
   if (srv == SQLITE_DONE) {
@@ -808,70 +812,87 @@ Connection::setClosedState()
   // Flag that we are shutting down the async thread, so that
   // getAsyncExecutionTarget knows not to expose/create the async thread.
   {
     MutexAutoLock lockedScope(sharedAsyncExecutionMutex);
     NS_ENSURE_FALSE(mAsyncExecutionThreadShuttingDown, NS_ERROR_UNEXPECTED);
     mAsyncExecutionThreadShuttingDown = true;
   }
 
+  // Set the property to null before closing the connection, otherwise the other
+  // functions in the module may try to use the connection after it is closed.
+  mDBConn = nullptr;
+
   return NS_OK;
 }
 
 bool
-Connection::isClosing(bool aResultOnClosed) {
+Connection::connectionReady()
+{
+  return mDBConn != nullptr;
+}
+
+bool
+Connection::isClosing()
+{
+  bool shuttingDown = false;
+  {
+    MutexAutoLock lockedScope(sharedAsyncExecutionMutex);
+    shuttingDown = mAsyncExecutionThreadShuttingDown;
+  }
+  return shuttingDown && !isClosed();
+}
+
+bool
+Connection::isClosed()
+{
   MutexAutoLock lockedScope(sharedAsyncExecutionMutex);
-  return mAsyncExecutionThreadShuttingDown &&
-    (aResultOnClosed || ConnectionReady());
+  return mConnectionClosed;
 }
 
 nsresult
-Connection::internalClose()
+Connection::internalClose(sqlite3 *aNativeConnection)
 {
+  // Sanity checks to make sure we are in the proper state before calling this.
+  MOZ_ASSERT(aNativeConnection, "Database connection is invalid!");
+  MOZ_ASSERT(!isClosed());
+
 #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(sharedAsyncExecutionMutex);
     NS_ASSERTION(mAsyncExecutionThreadShuttingDown,
                  "Did not call setClosedState!");
   }
-
-  bool onOpeningThread = false;
-  (void)threadOpenedOn->IsOnCurrentThread(&onOpeningThread);
 #endif // DEBUG
 
 #ifdef PR_LOGGING
   nsAutoCString leafName(":memory");
   if (mDatabaseFile)
       (void)mDatabaseFile->GetNativeLeafName(leafName);
   PR_LOG(gStorageLog, PR_LOG_NOTICE, ("Closing connection to '%s'",
                                       leafName.get()));
 #endif
 
-  // Set the property to null before closing the connection, otherwise the other
-  // functions in the module may try to use the connection after it is closed.
-  sqlite3 *dbConn = mDBConn;
-  mDBConn = nullptr;
-
   // At this stage, we may still have statements that need to be
   // finalized. Attempt to close the database connection. This will
   // always disconnect any virtual tables and cleanly finalize their
   // internal statements. Once this is done, closing may fail due to
   // unfinalized client statements, in which case we need to finalize
   // these statements and close again.
-
-  int srv = sqlite3_close(dbConn);
+  {
+    MutexAutoLock lockedScope(sharedAsyncExecutionMutex);
+    mConnectionClosed = true;
+  }
+  int srv = sqlite3_close(aNativeConnection);
 
   if (srv == SQLITE_BUSY) {
     // We still have non-finalized statements. Finalize them.
 
     sqlite3_stmt *stmt = nullptr;
-    while ((stmt = ::sqlite3_next_stmt(dbConn, stmt))) {
+    while ((stmt = ::sqlite3_next_stmt(aNativeConnection, stmt))) {
       PR_LOG(gStorageLog, PR_LOG_NOTICE,
              ("Auto-finalizing SQL statement '%s' (%x)",
               ::sqlite3_sql(stmt),
               stmt));
 
 #ifdef DEBUG
       char *msg = ::PR_smprintf("SQL statement '%s' (%x) should have been finalized before closing the connection",
                                 ::sqlite3_sql(stmt),
@@ -896,17 +917,17 @@ Connection::internalClose()
       // or not.
       if (srv == SQLITE_OK) {
         stmt = nullptr;
       }
     }
 
     // Now that all statements have been finalized, we
     // should be able to close.
-    srv = ::sqlite3_close(dbConn);
+    srv = ::sqlite3_close(aNativeConnection);
 
   }
 
   if (srv != SQLITE_OK) {
     MOZ_ASSERT(srv == SQLITE_OK,
                "sqlite3_close failed. There are probably outstanding statements that are listed above!");
   }
 
@@ -919,45 +940,43 @@ Connection::getFilename()
   nsCString leafname(":memory:");
   if (mDatabaseFile) {
     (void)mDatabaseFile->GetNativeLeafName(leafname);
   }
   return leafname;
 }
 
 int
-Connection::stepStatement(sqlite3_stmt *aStatement)
+Connection::stepStatement(sqlite3 *aNativeConnection, sqlite3_stmt *aStatement)
 {
   MOZ_ASSERT(aStatement);
   bool checkedMainThread = false;
   TimeStamp startTime = TimeStamp::Now();
 
-  // mDBConn may be null if the executing statement has been created and cached
-  // after a call to asyncClose() but before the connection has been nullified
-  // by internalClose().  In such a case closing the connection fails due to
-  // the existence of prepared statements, but mDBConn is set to null
-  // regardless. This usually happens when other tasks using cached statements
-  // are asynchronously scheduled for execution and any of them ends up after
-  // asyncClose. See bug 728653 for details.
-  if (!mDBConn)
+  // The connection may have been closed if the executing statement has been
+  // created and cached after a call to asyncClose() but before the actual
+  // sqlite3_close().  This usually happens when other tasks using cached
+  // statements are asynchronously scheduled for execution and any of them ends
+  // up after asyncClose. See bug 728653 for details.
+  if (isClosed())
     return SQLITE_MISUSE;
 
-  (void)::sqlite3_extended_result_codes(mDBConn, 1);
+  (void)::sqlite3_extended_result_codes(aNativeConnection, 1);
 
   int srv;
   while ((srv = ::sqlite3_step(aStatement)) == SQLITE_LOCKED_SHAREDCACHE) {
     if (!checkedMainThread) {
       checkedMainThread = true;
       if (::NS_IsMainThread()) {
         NS_WARNING("We won't allow blocking on the main thread!");
         break;
       }
     }
 
-    srv = WaitForUnlockNotify(mDBConn);
+    srv = WaitForUnlockNotify(aNativeConnection);
     if (srv != SQLITE_OK) {
       break;
     }
 
     ::sqlite3_reset(aStatement);
   }
 
   // Report very slow SQL statements to Telemetry
@@ -966,63 +985,68 @@ Connection::stepStatement(sqlite3_stmt *
     NS_IsMainThread() ? Telemetry::kSlowSQLThresholdForMainThread
                       : Telemetry::kSlowSQLThresholdForHelperThreads;
   if (duration.ToMilliseconds() >= threshold) {
     nsDependentCString statementString(::sqlite3_sql(aStatement));
     Telemetry::RecordSlowSQLStatement(statementString, getFilename(),
                                       duration.ToMilliseconds());
   }
 
-  (void)::sqlite3_extended_result_codes(mDBConn, 0);
+  (void)::sqlite3_extended_result_codes(aNativeConnection, 0);
   // Drop off the extended result bits of the result code.
   return srv & 0xFF;
 }
 
 int
-Connection::prepareStatement(const nsCString &aSQL,
+Connection::prepareStatement(sqlite3 *aNativeConnection, const nsCString &aSQL,
                              sqlite3_stmt **_stmt)
 {
+  // We should not even try to prepare statements after the connection has
+  // been closed.
+  if (isClosed())
+    return SQLITE_MISUSE;
+
   bool checkedMainThread = false;
 
-  (void)::sqlite3_extended_result_codes(mDBConn, 1);
+  (void)::sqlite3_extended_result_codes(aNativeConnection, 1);
 
   int srv;
-  while((srv = ::sqlite3_prepare_v2(mDBConn,
+  while((srv = ::sqlite3_prepare_v2(aNativeConnection,
                                     aSQL.get(),
                                     -1,
                                     _stmt,
                                     nullptr)) == SQLITE_LOCKED_SHAREDCACHE) {
     if (!checkedMainThread) {
       checkedMainThread = true;
       if (::NS_IsMainThread()) {
         NS_WARNING("We won't allow blocking on the main thread!");
         break;
       }
     }
 
-    srv = WaitForUnlockNotify(mDBConn);
+    srv = WaitForUnlockNotify(aNativeConnection);
     if (srv != SQLITE_OK) {
       break;
     }
   }
 
   if (srv != SQLITE_OK) {
     nsCString warnMsg;
     warnMsg.AppendLiteral("The SQL statement '");
     warnMsg.Append(aSQL);
     warnMsg.AppendLiteral("' could not be compiled due to an error: ");
-    warnMsg.Append(::sqlite3_errmsg(mDBConn));
+    warnMsg.Append(::sqlite3_errmsg(aNativeConnection));
 
 #ifdef DEBUG
     NS_WARNING(warnMsg.get());
 #endif
     PR_LOG(gStorageLog, PR_LOG_ERROR, ("%s", warnMsg.get()));
   }
 
-  (void)::sqlite3_extended_result_codes(mDBConn, 0);
+  (void)::sqlite3_extended_result_codes(aNativeConnection, 0);
   // Drop off the extended result bits of the result code.
   int rc = srv & 0xFF;
   // sqlite will return OK on a comment only string and set _stmt to nullptr.
   // The callers of this function are used to only checking the return value,
   // so it is safer to return an error code.
   if (rc == SQLITE_OK && *_stmt == nullptr) {
     return SQLITE_MISUSE;
   }
@@ -1083,49 +1107,56 @@ Connection::Close()
     // If this fails, the mDBConn will be left open, resulting in a leak.
     // Ideally we'd schedule some code to destroy the mDBConn once all its
     // async statements have finished executing;  see bug 704030.
     MutexAutoLock lockedScope(sharedAsyncExecutionMutex);
     bool asyncCloseWasCalled = !mAsyncExecutionThread;
     NS_ENSURE_TRUE(asyncCloseWasCalled, NS_ERROR_UNEXPECTED);
   }
 
+  // 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);
 
-  return internalClose();
+  return internalClose(nativeConn);
 }
 
 NS_IMETHODIMP
 Connection::AsyncClose(mozIStorageCompletionCallback *aCallback)
 {
   if (!NS_IsMainThread()) {
     return NS_ERROR_NOT_SAME_THREAD;
   }
   if (!mDBConn)
     return NS_ERROR_NOT_INITIALIZED;
 
   nsIEventTarget *asyncThread = getAsyncExecutionTarget();
   NS_ENSURE_TRUE(asyncThread, NS_ERROR_NOT_INITIALIZED);
 
+  // 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,
                                           mAsyncExecutionThread.forget());
   }
 
   rv = asyncThread->Dispatch(closeEvent, NS_DISPATCH_NORMAL);
   NS_ENSURE_SUCCESS(rv, rv);
 
   return NS_OK;
@@ -1247,17 +1278,17 @@ Connection::GetDefaultPageSize(int32_t *
 {
   *_defaultPageSize = Service::getDefaultPageSize();
   return NS_OK;
 }
 
 NS_IMETHODIMP
 Connection::GetConnectionReady(bool *_ready)
 {
-  *_ready = ConnectionReady();
+  *_ready = connectionReady();
   return NS_OK;
 }
 
 NS_IMETHODIMP
 Connection::GetDatabaseFile(nsIFile **_dbFile)
 {
   if (!mDBConn) return NS_ERROR_NOT_INITIALIZED;
 
--- a/storage/src/mozStorageConnection.h
+++ b/storage/src/mozStorageConnection.h
@@ -127,18 +127,22 @@ public:
    *
    * @returns an event target suitable for asynchronous statement execution.
    */
   nsIEventTarget *getAsyncExecutionTarget();
 
   /**
    * 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).  It
-   * also protects mPendingStatements.
+   * asynchronous statements (they are serialized on mAsyncExecutionThread).
+   * Currently protects:
+   *  - Connection.mAsyncExecutionThreadShuttingDown
+   *  - Connection.mAsyncExecutionThread
+   *  - Connection.mConnectionClosed
+   *  - AsyncExecuteStatements.mCancelRequested
    */
   Mutex sharedAsyncExecutionMutex;
 
   /**
    * Wraps the mutex that SQLite gives us from sqlite3_db_mutex.  This is public
    * because we already expose the sqlite3* native connection and proper
    * operation of the deadlock detector requires everyone to use the same single
    * SQLiteMutex instance for correctness.
@@ -149,59 +153,62 @@ public:
    * References the thread this database was opened on.  This MUST be thread it is
    * closed on.
    */
   const nsCOMPtr<nsIThread> threadOpenedOn;
 
   /**
    * Closes the SQLite database, and warns about any non-finalized statements.
    */
-  nsresult internalClose();
+  nsresult internalClose(sqlite3 *aDBConn);
 
   /**
    * Obtains the filename of the connection.  Useful for logging.
    */
   nsCString getFilename();
 
   /**
    * Creates an sqlite3 prepared statement object from an SQL string.
    *
+   * @param aNativeConnection
+   *        The underlying Sqlite connection to prepare the statement with.
    * @param aSQL
    *        The SQL statement string to compile.
    * @param _stmt
    *        New sqlite3_stmt object.
    * @return the result from sqlite3_prepare_v2.
    */
-  int prepareStatement(const nsCString &aSQL, sqlite3_stmt **_stmt);
+  int prepareStatement(sqlite3* aNativeConnection,
+                       const nsCString &aSQL, sqlite3_stmt **_stmt);
 
   /**
    * Performs a sqlite3_step on aStatement, while properly handling SQLITE_LOCKED
    * when not on the main thread by waiting until we are notified.
    *
+   * @param aNativeConnection
+   *        The underlying Sqlite connection to step the statement with.
    * @param aStatement
    *        A pointer to a sqlite3_stmt object.
    * @return the result from sqlite3_step.
    */
-  int stepStatement(sqlite3_stmt* aStatement);
+  int stepStatement(sqlite3* aNativeConnection, sqlite3_stmt* aStatement);
 
-  bool ConnectionReady() {
-    return mDBConn != nullptr;
-  }
+  bool connectionReady();
 
   /**
-   * True if this connection is currently shutting down.
-   *
-   * In particular, if |isClosing(true)| returns |true|, any sqlite3 statement
-   * belonging to this connection must be discarded as its memory has already
-   * been released to sqlite3.
-   *
-   * @param aResultOnceClosed
-   *        The value to return if closing has completed.
+   * True if this connection is shutting down but not yet closed.
    */
-  bool isClosing(bool aResultOnceClosed = false);
+  bool isClosing();
+
+  /**
+   * True if the underlying connection is closed.
+   * Any sqlite resources may be lost when this returns true, so nothing should
+   * try to use them.
+   */
+  bool isClosed();
 
   nsresult initializeClone(Connection *aClone, bool aReadOnly);
 
 private:
   ~Connection();
   nsresult initializeInternal(nsIFile *aDatabaseFile);
 
   /**
@@ -271,21 +278,30 @@ private:
    * If false, we guarantee both that the underlying sqlite3 database
    * connection is still open and that getAsyncExecutionTarget() can
    * return a thread. Once true, either the sqlite3 database
    * connection is being shutdown or it has been
    * shutdown. Additionally, once true, getAsyncExecutionTarget()
    * returns null.
    *
    * This variable should be accessed while holding the
-   * mAsyncExecutionMutex.
+   * sharedAsyncExecutionMutex.
    */
   bool mAsyncExecutionThreadShuttingDown;
 
   /**
+   * Set to true just prior to calling sqlite3_close on the
+   * connection.
+   *
+   * This variable should be accessed while holding the
+   * sharedAsyncExecutionMutex.
+   */
+  bool mConnectionClosed;
+
+  /**
    * Tracks if we have a transaction in progress or not.  Access protected by
    * sharedDBMutex.
    */
   bool mTransactionInProgress;
 
   /**
    * Stores the mapping of a given function by name to its instance.  Access is
    * protected by sharedDBMutex.
--- a/storage/src/mozStorageService.cpp
+++ b/storage/src/mozStorageService.cpp
@@ -341,17 +341,17 @@ Service::getConnections(/* inout */ nsTA
 void
 Service::minimizeMemory()
 {
   nsTArray<nsRefPtr<Connection> > connections;
   getConnections(connections);
 
   for (uint32_t i = 0; i < connections.Length(); i++) {
     nsRefPtr<Connection> conn = connections[i];
-    if (conn->ConnectionReady()) {
+    if (conn->connectionReady()) {
       NS_NAMED_LITERAL_CSTRING(shrinkPragma, "PRAGMA shrink_memory");
       nsCOMPtr<mozIStorageConnection> syncConn = do_QueryInterface(
         NS_ISUPPORTS_CAST(mozIStorageAsyncConnection*, conn));
       DebugOnly<nsresult> rv;
 
       if (!syncConn) {
         nsCOMPtr<mozIStoragePendingStatement> ps;
         rv = connections[i]->ExecuteSimpleSQLAsync(shrinkPragma, nullptr,
@@ -909,35 +909,32 @@ Service::Observe(nsISupports *, const ch
 
     bool anyOpen = false;
     do {
       nsTArray<nsRefPtr<Connection> > connections;
       getConnections(connections);
       anyOpen = false;
       for (uint32_t i = 0; i < connections.Length(); i++) {
         nsRefPtr<Connection> &conn = connections[i];
-
-        // While it would be nice to close all connections, we only
-        // check async ones for now.
         if (conn->isClosing()) {
           anyOpen = true;
           break;
         }
       }
       if (anyOpen) {
         nsCOMPtr<nsIThread> thread = do_GetCurrentThread();
         NS_ProcessNextEvent(thread);
       }
     } while (anyOpen);
 
     if (gShutdownChecks == SCM_CRASH) {
       nsTArray<nsRefPtr<Connection> > connections;
       getConnections(connections);
       for (uint32_t i = 0, n = connections.Length(); i < n; i++) {
-        if (connections[i]->ConnectionReady()) {
+        if (!connections[i]->isClosed()) {
           MOZ_CRASH();
         }
       }
     }
   }
 
   return NS_OK;
 }
--- a/storage/src/mozStorageStatement.cpp
+++ b/storage/src/mozStorageStatement.cpp
@@ -134,20 +134,22 @@ Statement::Statement()
 }
 
 nsresult
 Statement::initialize(Connection *aDBConnection,
                       sqlite3 *aNativeConnection,
                       const nsACString &aSQLStatement)
 {
   MOZ_ASSERT(aDBConnection, "No database connection given!");
+  MOZ_ASSERT(!aDBConnection->isClosed(), "Database connection should be valid");
   MOZ_ASSERT(!mDBStatement, "Statement already initialized!");
   MOZ_ASSERT(aNativeConnection, "No native connection given!");
 
-  int srv = aDBConnection->prepareStatement(PromiseFlatCString(aSQLStatement),
+  int srv = aDBConnection->prepareStatement(aNativeConnection,
+                                            PromiseFlatCString(aSQLStatement),
                                             &mDBStatement);
   if (srv != SQLITE_OK) {
       PR_LOG(gStorageLog, PR_LOG_ERROR,
              ("Sqlite statement prepare error: %d '%s'", srv,
               ::sqlite3_errmsg(aNativeConnection)));
       PR_LOG(gStorageLog, PR_LOG_ERROR,
              ("Statement was: '%s'", PromiseFlatCString(aSQLStatement).get()));
       return NS_ERROR_FAILURE;
@@ -279,17 +281,18 @@ int
 Statement::getAsyncStatement(sqlite3_stmt **_stmt)
 {
   // If we have no statement, we shouldn't be calling this method!
   NS_ASSERTION(mDBStatement != nullptr, "We have no statement to clone!");
 
   // If we do not yet have a cached async statement, clone our statement now.
   if (!mAsyncStatement) {
     nsDependentCString sql(::sqlite3_sql(mDBStatement));
-    int rc = mDBConnection->prepareStatement(sql, &mAsyncStatement);
+    int rc = mDBConnection->prepareStatement(mNativeConnection, sql,
+                                             &mAsyncStatement);
     if (rc != SQLITE_OK) {
       *_stmt = nullptr;
       return rc;
     }
 
     PR_LOG(gStorageLog, PR_LOG_NOTICE,
            ("Cloned statement 0x%p to 0x%p", mDBStatement, mAsyncStatement));
   }
@@ -351,17 +354,17 @@ Statement::Finalize()
 nsresult
 Statement::internalFinalize(bool aDestructing)
 {
   if (!mDBStatement)
     return NS_OK;
 
   int srv = SQLITE_OK;
 
-  if (!mDBConnection->isClosing(true)) {
+  if (!mDBConnection->isClosed()) {
     //
     // The connection is still open. While statement finalization and
     // closing may, in some cases, take place in two distinct threads,
     // we have a guarantee that the connection will remain open until
     // this method terminates:
     //
     // a. The connection will be closed synchronously. In this case,
     // there is no race condition, as everything takes place on the
@@ -627,17 +630,17 @@ Statement::ExecuteStep(bool *_moreResult
       int32_t srv;
       (void)error->GetResult(&srv);
       return convertResultCode(srv);
     }
 
     // We have bound, so now we can clear our array.
     mParamsArray = nullptr;
   }
-  int srv = mDBConnection->stepStatement(mDBStatement);
+  int srv = mDBConnection->stepStatement(mNativeConnection, mDBStatement);
 
 #ifdef PR_LOGGING
   if (srv != SQLITE_ROW && srv != SQLITE_DONE) {
       nsAutoCString errStr;
       (void)mDBConnection->GetLastErrorString(errStr);
       PR_LOG(gStorageLog, PR_LOG_DEBUG,
              ("Statement::ExecuteStep error: %s", errStr.get()));
   }
--- a/toolkit/components/places/Database.cpp
+++ b/toolkit/components/places/Database.cpp
@@ -210,56 +210,48 @@ SetJournalMode(nsCOMPtr<mozIStorageConne
     }
     // This is an unknown journal.
     MOZ_ASSERT(true);
   }
 
   return JOURNAL_DELETE;
 }
 
-class BlockingConnectionCloseCallback MOZ_FINAL : public mozIStorageCompletionCallback {
+class ConnectionCloseCallback MOZ_FINAL : public mozIStorageCompletionCallback {
   bool mDone;
 
 public:
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_MOZISTORAGECOMPLETIONCALLBACK
-  BlockingConnectionCloseCallback();
-  void Spin();
+  ConnectionCloseCallback();
 };
 
 NS_IMETHODIMP
-BlockingConnectionCloseCallback::Complete(nsresult, nsISupports*)
+ConnectionCloseCallback::Complete(nsresult, nsISupports*)
 {
   mDone = true;
   nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
   MOZ_ASSERT(os);
   if (!os)
     return NS_OK;
   DebugOnly<nsresult> rv = os->NotifyObservers(nullptr,
                                                TOPIC_PLACES_CONNECTION_CLOSED,
                                                nullptr);
   MOZ_ASSERT(NS_SUCCEEDED(rv));
   return NS_OK;
 }
 
-BlockingConnectionCloseCallback::BlockingConnectionCloseCallback()
+ConnectionCloseCallback::ConnectionCloseCallback()
   : mDone(false)
 {
   MOZ_ASSERT(NS_IsMainThread());
 }
 
-void BlockingConnectionCloseCallback::Spin() {
-  nsCOMPtr<nsIThread> thread = do_GetCurrentThread();
-  while (!mDone) {
-    NS_ProcessNextEvent(thread);
-  }
-}
-
 NS_IMPL_ISUPPORTS1(
-  BlockingConnectionCloseCallback
+  ConnectionCloseCallback
 , mozIStorageCompletionCallback
 )
 
 nsresult
 CreateRoot(nsCOMPtr<mozIStorageConnection>& aDBConn,
            const nsCString& aRootName,
            const nsXPIDLString& titleString)
 {
@@ -1934,22 +1926,21 @@ Database::Shutdown()
   mMainThreadAsyncStatements.FinalizeStatements();
 
   nsRefPtr< FinalizeStatementCacheProxy<mozIStorageStatement> > event =
     new FinalizeStatementCacheProxy<mozIStorageStatement>(
           mAsyncThreadStatements, NS_ISUPPORTS_CAST(nsIObserver*, this)
         );
   DispatchToAsyncThread(event);
 
-  nsRefPtr<BlockingConnectionCloseCallback> closeListener =
-    new BlockingConnectionCloseCallback();
+  mClosed = true;
+
+  nsRefPtr<ConnectionCloseCallback> closeListener =
+    new ConnectionCloseCallback();
   (void)mMainConn->AsyncClose(closeListener);
-  closeListener->Spin();
-
-  mClosed = true;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 //// nsIObserver
 
 NS_IMETHODIMP
 Database::Observe(nsISupports *aSubject,
                   const char *aTopic,