Bug 911109 - Statement::internalFinalize now doesn't cause finalization if the connection is already closed. r=asuth

storage/src/mozStorageConnection.cpp

@@ -792,9 +792,10 @@ Connection::setClosedState()
 }
 
 bool
-Connection::isAsyncClosing() {
+Connection::isClosing(bool aResultOnClosed) {
   MutexAutoLock lockedScope(sharedAsyncExecutionMutex);
-  return mAsyncExecutionThreadShuttingDown && ConnectionReady();
+  return mAsyncExecutionThreadShuttingDown &&
+    (aResultOnClosed || ConnectionReady());
 }
 
 nsresult
@@ -842,7 +843,7 @@ Connection::internalClose()
               stmt));
 
 #ifdef DEBUG
-      char *msg = ::PR_smprintf("SQL statement '%s' (%x) should have been finalized",
+      char *msg = ::PR_smprintf("SQL statement '%s' (%x) should have been finalized before closing the connection",
                                 ::sqlite3_sql(stmt),
                                 stmt);
       NS_WARNING(msg);

storage/src/mozStorageConnection.h

@@ -167,11 +167,16 @@ public:
   }
 
   /**
-   * True if this is an async connection, it is shutting down and it is not
-   * closed yet.
+   * 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.
    */
-  bool isAsyncClosing();
-
+  bool isClosing(bool aResultOnceClosed = false);
 
   nsresult initializeClone(Connection *aClone, bool aReadOnly);
 
@@ -239,11 +244,19 @@ private:
    * 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.
+   * Set to true by Close() or AsyncClose() prior to shutdown.
+   *
+   * 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.
    */
   bool mAsyncExecutionThreadShuttingDown;
 

storage/src/mozStorageService.cpp

@@ -902,7 +902,7 @@ Service::Observe(nsISupports *, const char *aTopic, const PRUnichar *)
 
         // While it would be nice to close all connections, we only
         // check async ones for now.
-        if (conn->isAsyncClosing()) {
+        if (conn->isClosing()) {
           anyOpen = true;
           break;
         }

storage/src/mozStorageStatement.cpp

@@ -357,12 +357,63 @@ Statement::internalFinalize(bool aDestructing)
   if (!mDBStatement)
     return NS_OK;
 
-#ifdef PR_LOGGING
-  PR_LOG(gStorageLog, PR_LOG_NOTICE, ("Finalizing statement '%s'",
-                                      ::sqlite3_sql(mDBStatement)));
-#endif
+  int srv = SQLITE_OK;
+
+  if (!mDBConnection->isClosing(true)) {
+    //
+    // 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
+    // same thread.
+    //
+    // b. The connection is closed asynchronously and this code is
+    // executed on the opener thread. In this case, asyncClose() has
+    // not been called yet and will not be called before we return
+    // from this function.
+    //
+    // c. The connection is closed asynchronously and this code is
+    // executed on the async execution thread. In this case,
+    // AsyncCloseConnection::Run() has not been called yet and will
+    // not be called before we return from this function.
+    //
+    // In either case, the connection is still valid, hence closing
+    // here is safe.
+    //
+#ifdef PR_LOGGING
+    PR_LOG(gStorageLog, PR_LOG_NOTICE, ("Finalizing statement '%s' during garbage-collection",
+                                        ::sqlite3_sql(mDBStatement)));
+#endif
+    srv = ::sqlite3_finalize(mDBStatement);
+  }
+#ifdef DEBUG
+  else {
+    //
+    // The database connection is either closed or closing. The sqlite
+    // statement has either been finalized already by the connection
+    // or is about to be finalized by the connection.
+    //
+    // Finalizing it here would be useless and segfaultish.
+    //
+
+    char *msg = ::PR_smprintf("SQL statement (%x) should have been finalized"
+      "before garbage-collection. For more details on this statement, set"
+      "NSPR_LOG_MESSAGES=mozStorage:5 .",
+      mDBStatement);
+    //
+    // Note that we can't display the statement itself, as the data structure
+    // is not valid anymore. However, the address shown here should help
+    // developers correlate with the more complete debug message triggered
+    // by AsyncClose().
+    //
+    NS_WARNING(msg);
+    ::PR_smprintf_free(msg);
+  }
+#endif // DEBUG
 
-  int srv = ::sqlite3_finalize(mDBStatement);
   mDBStatement = nullptr;
 
   if (mAsyncStatement) {

storage/test/unit/test_storage_connection.js

@@ -294,6 +294,32 @@ add_task(function test_asyncClose_succeeds_with_finalized_async_statement()
   gDBConn = null;
 });
 
+add_task(function test_close_then_release_statement() {
+  // Testing the behavior in presence of a bad client that finalizes
+  // statements after the database has been closed (typically by
+  // letting the gc finalize the statement).
+  let db = getOpenedDatabase();
+  let stmt = createStatement("SELECT * FROM test -- test_close_then_release_statement");
+  db.close();
+  stmt.finalize(); // Finalize too late - this should not crash
+
+  // Reset gDBConn so that later tests will get a new connection object.
+  gDBConn = null;
+});
+
+add_task(function test_asyncClose_then_release_statement() {
+  // Testing the behavior in presence of a bad client that finalizes
+  // statements after the database has been async closed (typically by
+  // letting the gc finalize the statement).
+  let db = getOpenedDatabase();
+  let stmt = createStatement("SELECT * FROM test -- test_asyncClose_then_release_statement");
+  yield asyncClose(db);
+  stmt.finalize(); // Finalize too late - this should not crash
+
+  // Reset gDBConn so that later tests will get a new connection object.
+  gDBConn = null;
+});
+
 add_task(function test_close_fails_with_async_statement_ran()
 {
   let deferred = Promise.defer();