Bug 1355561 - Add a new API to spinningly close the database when strictly needed, and ensure Close() does what it's named after. r=asuth

Introduce a new SpinningSynchronousClose API that allows to synchronously close
a connection that executed asynchronous statements, by spinning the events loop.
This is expected to be used rarely in particular cases like database corruption.
It is currently [noscript] since the only consumer is cpp, in the future we can
evaluate removing that, if we find more uses for it.

MozReview-Commit-ID: 7SPqutoF9jJ

--HG--
extra : rebase_source : d053e308192f335dfdca089668e56115b068ff8c

dom/cache/Connection.cpp

@@ -62,6 +62,13 @@ Connection::AsyncClose(mozIStorageCompletionCallback*)
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
+NS_IMETHODIMP
+Connection::SpinningSynchronousClose()
+{
+  // not supported
+  return NS_ERROR_NOT_IMPLEMENTED;
+}
+
 NS_IMETHODIMP
 Connection::AsyncClone(bool, mozIStorageCompletionCallback*)
 {

storage/mozIStorageAsyncConnection.idl

@@ -42,9 +42,23 @@ interface mozIStorageAsyncConnection : nsISupports {
    *         If called on a connection that has already been closed or was
    *         never properly opened.  The callback will still be dispatched
    *         to the main thread despite the returned error.
+   * @note If this call should fail, the callback won't be invoked.
    */
   void asyncClose([optional] in mozIStorageCompletionCallback aCallback);
 
+  /**
+   * Forcibly closes a database connection synchronously.
+   * This should only be used when it's required to close and replace the
+   * database synchronously to return control to the consumer, for example in
+   * case of a detected corruption on database opening.
+   * Since this spins the events loop, it should be used only in very particular
+   * and rare situations, or it may cause unexpected consequences (crashes).
+   *
+   * @throws NS_ERROR_NOT_SAME_THREAD
+   *         If called on a thread other than the one that opened it.
+   */
+  [noscript] void spinningSynchronousClose();
+
   /**
    * Clone a database and make the clone read only if needed.
    * SQL Functions and attached on-disk databases are applied to the new clone.

storage/mozStorageConnection.cpp

@@ -369,13 +369,9 @@ WaitForUnlockNotify(sqlite3* aDatabase)
   return srv;
 }
 
-} // namespace
-
 ////////////////////////////////////////////////////////////////////////////////
 //// Local Classes
 
-namespace {
-
 class AsyncCloseConnection final: public Runnable
 {
 public:
@@ -488,6 +484,32 @@ private:
   nsCOMPtr<mozIStorageCompletionCallback> mCallback;
 };
 
+/**
+ * A listener for async connection closing.
+ */
+class CloseListener final :  public mozIStorageCompletionCallback
+{
+public:
+  NS_DECL_ISUPPORTS
+  CloseListener()
+    : mClosed(false)
+  {
+  }
+
+  NS_IMETHOD Complete(nsresult, nsISupports*) override
+  {
+    mClosed = true;
+    return NS_OK;
+  }
+
+  bool mClosed;
+
+private:
+  ~CloseListener() = default;
+};
+
+NS_IMPL_ISUPPORTS(CloseListener, mozIStorageCompletionCallback)
+
 } // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -517,8 +539,7 @@ Connection::Connection(Service *aService,
 
 Connection::~Connection()
 {
-  (void)Close();
-
+  Unused << Close();
   MOZ_ASSERT(!mAsyncExecutionThread,
              "The async thread has not been shutdown properly!");
 }
@@ -1250,11 +1271,22 @@ Connection::Close()
 #endif // DEBUG
 
   // Make sure we have not executed any asynchronous statements.
-  // 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.
-  bool asyncCloseWasCalled = !mAsyncExecutionThread;
-  NS_ENSURE_TRUE(asyncCloseWasCalled, NS_ERROR_UNEXPECTED);
+  // If this fails, the mDBConn may be left open, resulting in a leak.
+  // We'll try to finalize the pending statements and close the connection.
+  if (isAsyncExecutionThreadAvailable()) {
+#ifdef DEBUG
+    if (NS_IsMainThread()) {
+      nsCOMPtr<nsIXPConnect> xpc = do_GetService(nsIXPConnect::GetCID());
+      Unused << xpc->DebugDumpJSStack(false, false, false);
+    }
+#endif
+    MOZ_ASSERT(false,
+               "Close() was invoked on a connection that executed asynchronous statements. "
+               "Should have used asyncClose().");
+    // Try to close the database regardless, to free up resources.
+    Unused << SpinningSynchronousClose();
+    return NS_ERROR_UNEXPECTED;
+  }
 
   // setClosedState nullifies our connection pointer, so we take a raw pointer
   // off it, to pass it through the close procedure.
@@ -1265,6 +1297,32 @@ Connection::Close()
   return internalClose(nativeConn);
 }
 
+NS_IMETHODIMP
+Connection::SpinningSynchronousClose()
+{
+  if (threadOpenedOn != NS_GetCurrentThread()) {
+    return NS_ERROR_NOT_SAME_THREAD;
+  }
+
+  // As currently implemented, we can't spin to wait for an existing AsyncClose.
+  // Our only existing caller will never have called close; assert if misused
+  // so that no new callers assume this works after an AsyncClose.
+  MOZ_DIAGNOSTIC_ASSERT(connectionReady());
+  if (!connectionReady()) {
+    return NS_ERROR_UNEXPECTED;
+  }
+
+  RefPtr<CloseListener> listener = new CloseListener();
+  nsresult rv = AsyncClose(listener);
+  NS_ENSURE_SUCCESS(rv, rv);
+  MOZ_ALWAYS_TRUE(SpinEventLoopUntil([&]() {
+    return listener->mClosed;
+  }));
+  MOZ_ASSERT(isClosed(), "The connection should be closed at this point");
+
+  return rv;
+}
+
 NS_IMETHODIMP
 Connection::AsyncClose(mozIStorageCompletionCallback *aCallback)
 {
@@ -1344,7 +1402,10 @@ Connection::AsyncClose(mozIStorageCompletionCallback *aCallback)
       // callers ignore our return value.
       Unused << NS_DispatchToMainThread(completeEvent.forget());
     }
-    return Close();
+    MOZ_ALWAYS_SUCCEEDS(Close());
+    // Return a success inconditionally here, since Close() is unlikely to fail
+    // and we want to reassure the consumer that its callback will be invoked.
+    return NS_OK;
   }
 
   // setClosedState nullifies our connection pointer, so we take a raw pointer

storage/mozStorageHelper.h

@@ -17,6 +17,7 @@
 #include "mozIStorageStatement.h"
 #include "mozIStoragePendingStatement.h"
 #include "nsError.h"
+#include "nsIXPConnect.h"
 
 /**
  * This class wraps a transaction inside a given C++ scope, guaranteeing that
@@ -227,6 +228,12 @@ protected:
       }                                                                            \
     }                                                                              \
   }                                                                                \
+  if (NS_IsMainThread()) {                                                         \
+    nsCOMPtr<nsIXPConnect> xpc = do_GetService(nsIXPConnect::GetCID());            \
+    if (xpc) {                                                                     \
+      mozilla::Unused << xpc->DebugDumpJSStack(false, false, false);               \
+    }                                                                              \
+  }                                                                                \
   MOZ_ASSERT(false, "You are trying to use a deprecated mozStorage method. "       \
                     "Check error message in console to identify the method name.");\
   PR_END_MACRO

storage/test/gtest/moz.build

@@ -12,6 +12,7 @@ UNIFIED_SOURCES += [
     'test_file_perms.cpp',
     'test_mutex.cpp',
     'test_service_init_background_thread.cpp',
+    'test_spinningSynchronousClose.cpp',
     'test_statement_scoper.cpp',
     'test_StatementCache.cpp',
     'test_transaction_helper.cpp',

storage/test/gtest/test_spinningSynchronousClose.cpp

@@ -0,0 +1,78 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
+ * vim: sw=2 ts=2 et lcs=trail\:.,tab\:>~ :
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "storage_test_harness.h"
+#include "prinrval.h"
+
+/**
+ * Helper to verify that the event loop was spun.  As long as this is dispatched
+ * prior to a call to Close()/SpinningSynchronousClose() we are guaranteed this
+ * will be run if the event loop is spun to perform a close.  This is because
+ * SpinningSynchronousClose must spin the event loop to realize the close
+ * completed and our runnable will already be enqueued and therefore run before
+ * the AsyncCloseConnection's callback.  Note that this invariant may be
+ * violated if our runnables end up in different queues thanks to Quantum
+ * changes, so this test may need to be updated if the close dispatch changes.
+ */
+class CompletionRunnable final : public Runnable
+{
+public:
+  explicit CompletionRunnable()
+    : mDone(false)
+  {
+  }
+
+  NS_IMETHOD Run() override
+  {
+    mDone = true;
+    return NS_OK;
+  }
+
+  bool mDone;
+};
+
+// Can only run in optimized builds, or it would assert.
+#ifndef DEBUG
+TEST(storage_spinningSynchronousClose, CloseOnAsync)
+{
+  nsCOMPtr<mozIStorageConnection> db(getMemoryDatabase());
+  // Run an async statement.
+  nsCOMPtr<mozIStorageAsyncStatement> stmt;
+  do_check_success(db->CreateAsyncStatement(
+    NS_LITERAL_CSTRING("CREATE TABLE test (id INTEGER PRIMARY KEY)"),
+    getter_AddRefs(stmt)
+  ));
+  nsCOMPtr<mozIStoragePendingStatement> p;
+  do_check_success(stmt->ExecuteAsync(nullptr, getter_AddRefs(p)));
+  do_check_success(stmt->Finalize());
+
+  // Wrongly use Close() instead of AsyncClose().
+  RefPtr<CompletionRunnable> event = new CompletionRunnable();
+  NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
+  do_check_false(NS_SUCCEEDED(db->Close()));
+  do_check_true(event->mDone);
+}
+#endif
+
+TEST(storage_spinningSynchronousClose, spinningSynchronousCloseOnAsync)
+{
+  nsCOMPtr<mozIStorageConnection> db(getMemoryDatabase());
+  // Run an async statement.
+  nsCOMPtr<mozIStorageAsyncStatement> stmt;
+  do_check_success(db->CreateAsyncStatement(
+    NS_LITERAL_CSTRING("CREATE TABLE test (id INTEGER PRIMARY KEY)"),
+    getter_AddRefs(stmt)
+  ));
+  nsCOMPtr<mozIStoragePendingStatement> p;
+  do_check_success(stmt->ExecuteAsync(nullptr, getter_AddRefs(p)));
+  do_check_success(stmt->Finalize());
+
+  // Use the spinning close API.
+  RefPtr<CompletionRunnable> event = new CompletionRunnable();
+  NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
+  do_check_success(db->SpinningSynchronousClose());
+  do_check_true(event->mDone);
+}

storage/test/unit/test_connection_asyncClose.js

@@ -53,23 +53,25 @@ add_task(function* test_asyncClose_does_not_complete_before_statements() {
  * async thread is not available and fall back to invoking Close() which will
  * notice the mDBConn is already gone.
  */
-add_task(function* test_double_asyncClose_throws() {
-  let db = yield openAsyncDatabase(getTestDB());
+if (!AppConstants.DEBUG) {
+  add_task(function* test_double_asyncClose_throws() {
+    let db = yield openAsyncDatabase(getTestDB());
 
-  // (Don't yield control flow yet, save the promise for after we make the
-  // second call.)
-  // Branch coverage: (asyncThread && mDBConn)
-  let realClosePromise = yield asyncClose(db);
-  try {
-    // Branch coverage: (!asyncThread && !mDBConn)
-    db.asyncClose();
-    ok(false, "should have thrown");
-  } catch (e) {
-    equal(e.result, Cr.NS_ERROR_NOT_INITIALIZED);
-  }
+    // (Don't yield control flow yet, save the promise for after we make the
+    // second call.)
+    // Branch coverage: (asyncThread && mDBConn)
+    let realClosePromise = yield asyncClose(db);
+    try {
+      // Branch coverage: (!asyncThread && !mDBConn)
+      db.asyncClose();
+      ok(false, "should have thrown");
+    } catch (e) {
+      equal(e.result, Cr.NS_ERROR_NOT_INITIALIZED);
+    }
 
-  yield realClosePromise;
-});
+    yield realClosePromise;
+  });
+}
 
 /**
  * Create a sync db connection and never take it asynchronous and then call

storage/test/unit/test_storage_connection.js

@@ -265,24 +265,19 @@ if (!AppConstants.DEBUG) {
   });
 }
 
-add_task(function* test_close_fails_with_async_statement_ran() {
-  let deferred = Promise.defer();
-  let stmt = createStatement("SELECT * FROM test");
-  stmt.executeAsync();
-  stmt.finalize();
+// In debug builds this would cause a fatal assertion.
+if (!AppConstants.DEBUG) {
+  add_task(function* test_close_fails_with_async_statement_ran() {
+    let stmt = createStatement("SELECT * FROM test");
+    stmt.executeAsync();
+    stmt.finalize();
 
-  let db = getOpenedDatabase();
-  Assert.throws(() => db.close(), /NS_ERROR_UNEXPECTED/);
-
-  // Clean up after ourselves.
-  db.asyncClose(function() {
+    let db = getOpenedDatabase();
+    Assert.throws(() => db.close(), /NS_ERROR_UNEXPECTED/);
     // Reset gDBConn so that later tests will get a new connection object.
     gDBConn = null;
-    deferred.resolve();
   });
-
-  yield deferred.promise;
-});
+}
 
 add_task(function* test_clone_optional_param() {
   let db1 = getService().openUnsharedDatabase(getTestDB());

toolkit/components/contentprefs/nsContentPrefService.js

@@ -173,7 +173,9 @@ ContentPrefService.prototype = {
     if (this._contentPrefService2)
       this._contentPrefService2.destroy();
 
-    this._dbConnection.asyncClose();
+    this._dbConnection.asyncClose(() => {
+      Services.obs.notifyObservers(null, "content-prefs-db-closed");
+    });
 
     // Delete references to XPCOM components to make sure we don't leak them
     // (although we haven't observed leakage in tests).  Also delete references

toolkit/components/contentprefs/tests/unit/head_contentPrefs.js

@@ -11,6 +11,7 @@ var Cu = Components.utils;
 
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/ContentPrefInstance.jsm");
+Cu.import("resource://testing-common/TestUtils.jsm");
 
 const CONTENT_PREFS_DB_FILENAME = "content-prefs.sqlite";
 const CONTENT_PREFS_BACKUP_DB_FILENAME = "content-prefs.sqlite.corrupt";

toolkit/components/contentprefs/tests/unit/test_contentPrefs.js

@@ -2,39 +2,39 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-function run_test() {
+
+add_task(async function() {
   // Database Creation, Schema Migration, and Backup
 
   // Note: in these tests we use createInstance instead of getService
   // so we can instantiate the service multiple times and make it run
   // its database initialization code each time.
 
-  // Create a new database.
-  {
-    ContentPrefTest.deleteDatabase();
-
-    // Get the service and make sure it has a ready database connection.
-    let cps = Cc["@mozilla.org/content-pref/service;1"].
-              createInstance(Ci.nsIContentPrefService);
-    do_check_true(cps.DBConnection.connectionReady);
-    cps.DBConnection.close();
+  function with_cps_instance(testFn) {
+    let cps = Cc["@mozilla.org/content-pref/service;1"]
+                .createInstance(Ci.nsIContentPrefService)
+                .QueryInterface(Ci.nsIObserver);
+    testFn(cps);
+    let promiseClosed = TestUtils.topicObserved("content-prefs-db-closed");
+    cps.observe(null, "xpcom-shutdown", "");
+    return promiseClosed;
   }
 
+  // Create a new database.
+  ContentPrefTest.deleteDatabase();
+  await with_cps_instance(cps => {
+    do_check_true(cps.DBConnection.connectionReady);
+  });
+
   // Open an existing database.
-  {
-    let dbFile = ContentPrefTest.deleteDatabase();
 
-    let cps = Cc["@mozilla.org/content-pref/service;1"].
-               createInstance(Ci.nsIContentPrefService);
-    cps.DBConnection.close();
-    do_check_true(dbFile.exists());
+  ContentPrefTest.deleteDatabase();
+  await with_cps_instance(cps => {});
 
-    // Get the service and make sure it has a ready database connection.
-    cps = Cc["@mozilla.org/content-pref/service;1"].
-          createInstance(Ci.nsIContentPrefService);
+  // Get the service and make sure it has a ready database connection.
+  await with_cps_instance(cps => {
     do_check_true(cps.DBConnection.connectionReady);
-    cps.DBConnection.close();
-  }
+  });
 
   // Open an empty database.
   {
@@ -49,10 +49,9 @@ function run_test() {
     do_check_true(dbFile.exists());
 
     // Get the service and make sure it has created the schema.
-    let cps = Cc["@mozilla.org/content-pref/service;1"].
-              createInstance(Ci.nsIContentPrefService);
-    do_check_neq(cps.DBConnection.schemaVersion, 0);
-    cps.DBConnection.close();
+    await with_cps_instance(cps => {
+      do_check_neq(cps.DBConnection.schemaVersion, 0);
+    });
   }
 
   // Open a corrupted database.
@@ -69,12 +68,10 @@ function run_test() {
     foStream.close();
 
     // Get the service and make sure it backs up and recreates the database.
-    let cps = Cc["@mozilla.org/content-pref/service;1"].
-              createInstance(Ci.nsIContentPrefService);
-    do_check_true(backupDBFile.exists());
-    do_check_true(cps.DBConnection.connectionReady);
-
-    cps.DBConnection.close();
+    await with_cps_instance(cps => {
+      do_check_true(backupDBFile.exists());
+      do_check_true(cps.DBConnection.connectionReady);
+    });
   }
 
   // Open a database with a corrupted schema.
@@ -92,12 +89,10 @@ function run_test() {
     do_check_true(dbFile.exists());
 
     // Get the service and make sure it backs up and recreates the database.
-    let cps = Cc["@mozilla.org/content-pref/service;1"].
-              createInstance(Ci.nsIContentPrefService);
-    do_check_true(backupDBFile.exists());
-    do_check_true(cps.DBConnection.connectionReady);
-
-    cps.DBConnection.close();
+    await with_cps_instance(cps => {
+      do_check_true(backupDBFile.exists());
+      do_check_true(cps.DBConnection.connectionReady);
+    });
   }
 
 
@@ -108,8 +103,12 @@ function run_test() {
 
   // Make sure disk synchronization checking is turned off by default.
   var statement = cps.DBConnection.createStatement("PRAGMA synchronous");
-  statement.executeStep();
-  do_check_eq(0, statement.getInt32(0));
+  try {
+    statement.executeStep();
+    do_check_eq(0, statement.getInt32(0));
+  } finally {
+    statement.finalize();
+  }
 
   // Nonexistent Pref
 
@@ -460,4 +459,4 @@ function run_test() {
     do_check_true(globalPref.row.groupID == null);
     globalPref.reset();
   }
-}
+});

toolkit/components/places/Database.cpp

@@ -784,7 +784,7 @@ Database::BackupAndReplaceDatabaseFile(nsCOMPtr<mozIStorageService>& aStorage)
 
   // Close database connection if open.
   if (mMainConn) {
-    rv = mMainConn->Close();
+    rv = mMainConn->SpinningSynchronousClose();
     NS_ENSURE_SUCCESS(rv, ForceCrashAndReplaceDatabase(
       NS_LITERAL_CSTRING("Unable to close the corrupt database.")));
   }

toolkit/components/search/tests/xpcshell/test_searchSuggest.js

@@ -32,7 +32,6 @@ function run_test() {
   do_register_cleanup(() => (async function cleanup() {
     // Remove added form history entries
     await updateSearchHistory("remove", null);
-    FormHistory.shutdown();
     Services.prefs.clearUserPref("browser.search.suggest.enabled");
   })());