Bug 914070 - Part 2 - nullify mDBConn at setClosedState and provide an isClosed helper. r=asuth

storage/src/mozStorageAsyncStatement.cpp

@@ -135,6 +135,7 @@ AsyncStatement::initialize(Connection *aDBConnection,
                            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;
@@ -301,7 +302,8 @@ AsyncStatement::getAsyncStatement(sqlite3_stmt **_stmt)
 #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,

storage/src/mozStorageAsyncStatementExecution.cpp

@@ -337,7 +337,7 @@ AsyncExecuteStatements::executeStatement(sqlite3_stmt *aStatement)
     // 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)
     {
@@ -568,6 +568,8 @@ AsyncExecuteStatements::Cancel()
 NS_IMETHODIMP
 AsyncExecuteStatements::Run()
 {
+  MOZ_ASSERT(!mConnection->isClosed());
+
   // Do not run if we have been canceled.
   {
     MutexAutoLock lockedScope(mMutex);

storage/src/mozStorageConnection.cpp

@@ -333,9 +333,11 @@ 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)
   {
@@ -351,7 +353,7 @@ public:
 #endif // DEBUG
 
     // Internal close.
-    (void)mConnection->internalClose();
+    (void)mConnection->internalClose(mNativeConnection);
 
     // Callback
     if (mCallbackEvent) {
@@ -376,6 +378,7 @@ public:
   }
 private:
   nsRefPtr<Connection> mConnection;
+  sqlite3 *mNativeConnection;
   nsCOMPtr<nsIRunnable> mCallbackEvent;
   nsCOMPtr<nsIThread> mAsyncExecutionThread;
 };
@@ -470,6 +473,7 @@ Connection::Connection(Service *aService,
 , threadOpenedOn(do_GetCurrentThread())
 , mDBConn(nullptr)
 , mAsyncExecutionThreadShuttingDown(false)
+, mConnectionClosed(false)
 , mTransactionInProgress(false)
 , mProgressHandler(nullptr)
 , mFlags(aFlags)
@@ -744,11 +748,11 @@ Connection::databaseElementExists(enum DatabaseElementType aElementType,
   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);
 
@@ -813,31 +817,50 @@ Connection::setClosedState()
     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 &&
+  return mConnectionClosed;
-    (aResultOnClosed || ConnectionReady());
 }
 
 nsresult
-Connection::internalClose()
+Connection::internalClose(sqlite3 *aNativeConnection)
 {
-#ifdef DEBUG
   // Sanity checks to make sure we are in the proper state before calling this.
-  NS_ASSERTION(mDBConn, "Database connection is already null!");
+  MOZ_ASSERT(aNativeConnection, "Database connection is invalid!");
+  MOZ_ASSERT(!isClosed());
 
+#ifdef DEBUG
   { // 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
@@ -848,25 +871,23 @@ Connection::internalClose()
                                       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),
@@ -901,7 +922,7 @@ Connection::internalClose()
 
     // Now that all statements have been finalized, we
     // should be able to close.
-    srv = ::sqlite3_close(dbConn);
+    srv = ::sqlite3_close(aNativeConnection);
 
   }
 
@@ -924,23 +945,21 @@ Connection::getFilename()
 }
 
 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
+  // The connection may have been closed if the executing statement has been
-  // after a call to asyncClose() but before the connection has been nullified
+  // created and cached after a call to asyncClose() but before the actual
-  // by internalClose().  In such a case closing the connection fails due to
+  // sqlite3_close().  This usually happens when other tasks using cached
-  // the existence of prepared statements, but mDBConn is set to null
+  // statements are asynchronously scheduled for execution and any of them ends
-  // regardless. This usually happens when other tasks using cached statements
+  // up after asyncClose. See bug 728653 for details.
-  // are asynchronously scheduled for execution and any of them ends up after
+  if (isClosed())
-  // asyncClose. See bug 728653 for details.
-  if (!mDBConn)
     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) {
@@ -952,7 +971,7 @@ Connection::stepStatement(sqlite3_stmt *aStatement)
       }
     }
 
-    srv = WaitForUnlockNotify(mDBConn);
+    srv = WaitForUnlockNotify(aNativeConnection);
     if (srv != SQLITE_OK) {
       break;
     }
@@ -971,21 +990,26 @@ Connection::stepStatement(sqlite3_stmt *aStatement)
                                       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,
@@ -998,7 +1022,7 @@ Connection::prepareStatement(const nsCString &aSQL,
       }
     }
 
-    srv = WaitForUnlockNotify(mDBConn);
+    srv = WaitForUnlockNotify(aNativeConnection);
     if (srv != SQLITE_OK) {
       break;
     }
@@ -1009,7 +1033,7 @@ Connection::prepareStatement(const nsCString &aSQL,
     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());
@@ -1017,7 +1041,7 @@ Connection::prepareStatement(const nsCString &aSQL,
     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.
@@ -1088,10 +1112,13 @@ Connection::Close()
     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
@@ -1106,6 +1133,9 @@ Connection::AsyncClose(mozIStorageCompletionCallback *aCallback)
   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);
 
@@ -1121,6 +1151,7 @@ Connection::AsyncClose(mozIStorageCompletionCallback *aCallback)
     // We need to lock because we're modifying mAsyncExecutionThread
     MutexAutoLock lockedScope(sharedAsyncExecutionMutex);
     closeEvent = new AsyncCloseConnection(this,
+                                          nativeConn,
                                           completeEvent,
                                           mAsyncExecutionThread.forget());
   }
@@ -1252,7 +1283,7 @@ Connection::GetDefaultPageSize(int32_t *_defaultPageSize)
 NS_IMETHODIMP
 Connection::GetConnectionReady(bool *_ready)
 {
-  *_ready = ConnectionReady();
+  *_ready = connectionReady();
   return NS_OK;
 }
 

storage/src/mozStorageConnection.h

@@ -132,8 +132,12 @@ 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).  It
+   * asynchronous statements (they are serialized on mAsyncExecutionThread).
-   * also protects mPendingStatements.
+   * Currently protects:
+   *  - Connection.mAsyncExecutionThreadShuttingDown
+   *  - Connection.mAsyncExecutionThread
+   *  - Connection.mConnectionClosed
+   *  - AsyncExecuteStatements.mCancelRequested
    */
   Mutex sharedAsyncExecutionMutex;
 
@@ -154,7 +158,7 @@ public:
   /**
    * 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.
@@ -164,39 +168,42 @@ public:
   /**
    * 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() {
+  bool connectionReady();
-    return mDBConn != nullptr;
-  }
 
   /**
-   * True if this connection is currently shutting down.
+   * True if this connection is shutting down but not yet closed.
-   *
-   * 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.
    */
-  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);
 
@@ -276,10 +283,19 @@ private:
    * 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.

storage/src/mozStorageService.cpp

@@ -346,7 +346,7 @@ Service::minimizeMemory()
 
   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));
@@ -914,9 +914,6 @@ Service::Observe(nsISupports *, const char *aTopic, const char16_t *)
       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;
@@ -932,7 +929,7 @@ Service::Observe(nsISupports *, const char *aTopic, const char16_t *)
       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();
         }
       }

storage/src/mozStorageStatement.cpp

@@ -139,10 +139,12 @@ Statement::initialize(Connection *aDBConnection,
                       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,
@@ -284,7 +286,8 @@ Statement::getAsyncStatement(sqlite3_stmt **_stmt)
   // 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;
@@ -356,7 +359,7 @@ Statement::internalFinalize(bool aDestructing)
 
   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,
@@ -632,7 +635,7 @@ Statement::ExecuteStep(bool *_moreResults)
     // 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) {

toolkit/components/places/Database.cpp

@@ -215,18 +215,17 @@ SetJournalMode(nsCOMPtr<mozIStorageConnection>& aDBConn,
   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();
+  ConnectionCloseCallback();
-  void Spin();
 };
 
 NS_IMETHODIMP
-BlockingConnectionCloseCallback::Complete(nsresult, nsISupports*)
+ConnectionCloseCallback::Complete(nsresult, nsISupports*)
 {
   mDone = true;
   nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
@@ -240,21 +239,14 @@ BlockingConnectionCloseCallback::Complete(nsresult, nsISupports*)
   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
 )
 
@@ -1939,12 +1931,11 @@ Database::Shutdown()
         );
   DispatchToAsyncThread(event);
 
-  nsRefPtr<BlockingConnectionCloseCallback> closeListener =
-    new BlockingConnectionCloseCallback();
-  (void)mMainConn->AsyncClose(closeListener);
-  closeListener->Spin();
-
   mClosed = true;
+
+  nsRefPtr<ConnectionCloseCallback> closeListener =
+    new ConnectionCloseCallback();
+  (void)mMainConn->AsyncClose(closeListener);
 }
 
 ////////////////////////////////////////////////////////////////////////////////