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 draft
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 12 Apr 2017 17:44:39 +0200
changeset 597452 d053e308192f335dfdca089668e56115b068ff8c
parent 597265 2bf3088b500376e58e62e8f078d9950588adc649
child 634244 3680d533e0b6e4afbe6252c6497d79c5e38274d7
push id64946
push usermak77@bonardo.net
push dateTue, 20 Jun 2017 16:24:25 +0000
reviewersasuth
bugs1355561
milestone56.0a1
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
dom/cache/Connection.cpp
storage/mozIStorageAsyncConnection.idl
storage/mozStorageConnection.cpp
storage/mozStorageHelper.h
storage/test/gtest/moz.build
storage/test/gtest/test_spinningSynchronousClose.cpp
storage/test/unit/test_connection_asyncClose.js
storage/test/unit/test_storage_connection.js
toolkit/components/contentprefs/nsContentPrefService.js
toolkit/components/contentprefs/tests/unit/head_contentPrefs.js
toolkit/components/contentprefs/tests/unit/test_contentPrefs.js
toolkit/components/places/Database.cpp
toolkit/components/search/tests/xpcshell/test_searchSuggest.js
--- a/dom/cache/Connection.cpp
+++ b/dom/cache/Connection.cpp
@@ -58,16 +58,23 @@ Connection::Close()
 NS_IMETHODIMP
 Connection::AsyncClose(mozIStorageCompletionCallback*)
 {
   // async methods are not supported
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP
+Connection::SpinningSynchronousClose()
+{
+  // not supported
+  return NS_ERROR_NOT_IMPLEMENTED;
+}
+
+NS_IMETHODIMP
 Connection::AsyncClone(bool, mozIStorageCompletionCallback*)
 {
   // async methods are not supported
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP
 Connection::GetDatabaseFile(nsIFile** aFileOut)
--- a/storage/mozIStorageAsyncConnection.idl
+++ b/storage/mozIStorageAsyncConnection.idl
@@ -37,20 +37,34 @@ interface mozIStorageAsyncConnection : n
    *
    * @throws NS_ERROR_NOT_SAME_THREAD
    *         If called on a thread other than the one that opened it.  The
    *         callback will not be dispatched.
    * @throws NS_ERROR_NOT_INITIALIZED
    *         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.
    *
    * @param aReadOnly
    *        If true, the returned database should be put into read-only mode.
    *
    * @param aCallback
    *        A callback that will be notified when the operation is complete,
--- a/storage/mozStorageConnection.cpp
+++ b/storage/mozStorageConnection.cpp
@@ -364,23 +364,19 @@ WaitForUnlockNotify(sqlite3* aDatabase)
   MOZ_ASSERT(srv == SQLITE_LOCKED || srv == SQLITE_OK);
   if (srv == SQLITE_OK) {
     notification.Wait();
   }
 
   return srv;
 }
 
-} // namespace
-
 ////////////////////////////////////////////////////////////////////////////////
 //// Local Classes
 
-namespace {
-
 class AsyncCloseConnection final: public Runnable
 {
 public:
   AsyncCloseConnection(Connection *aConnection,
                        sqlite3 *aNativeConnection,
                        nsIRunnable *aCallbackEvent)
   : mConnection(aConnection)
   , mNativeConnection(aNativeConnection)
@@ -483,16 +479,42 @@ private:
   }
 
   RefPtr<Connection> mConnection;
   RefPtr<Connection> mClone;
   const bool mReadOnly;
   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
 
 ////////////////////////////////////////////////////////////////////////////////
 //// Connection
 
 Connection::Connection(Service *aService,
                        int aFlags,
                        bool aAsyncOnly,
@@ -512,18 +534,17 @@ Connection::Connection(Service *aService
 {
   MOZ_ASSERT(!mIgnoreLockingMode || mFlags & SQLITE_OPEN_READONLY,
              "Can't ignore locking for a non-readonly connection!");
   mStorageService->registerConnection(this);
 }
 
 Connection::~Connection()
 {
-  (void)Close();
-
+  Unused << Close();
   MOZ_ASSERT(!mAsyncExecutionThread,
              "The async thread has not been shutdown properly!");
 }
 
 NS_IMPL_ADDREF(Connection)
 
 NS_INTERFACE_MAP_BEGIN(Connection)
   NS_INTERFACE_MAP_ENTRY(mozIStorageAsyncConnection)
@@ -1245,32 +1266,69 @@ Connection::Close()
   // We make this check outside of debug code below in setClosedState, but this is
   // here to be explicit.
   bool onOpenerThread = false;
   (void)threadOpenedOn->IsOnCurrentThread(&onOpenerThread);
   MOZ_ASSERT(onOpenerThread);
 #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.
   sqlite3 *nativeConn = mDBConn;
   nsresult rv = setClosedState();
   NS_ENSURE_SUCCESS(rv, rv);
 
   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)
 {
   NS_ENSURE_TRUE(NS_IsMainThread(), NS_ERROR_NOT_SAME_THREAD);
   // Check if AsyncClose or Close were already invoked.
   if (!mDBConn) {
     return NS_ERROR_NOT_INITIALIZED;
   }
 
@@ -1339,17 +1397,20 @@ Connection::AsyncClose(mozIStorageComple
     // not complain about that.  (Close() may, however, complain if the
     // connection is closed, but that's okay.)
     if (completeEvent) {
       // Closing the database is more important than returning an error code
       // about a failure to dispatch, especially because all existing native
       // 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
   // off it, to pass it through the close procedure.
   sqlite3 *nativeConn = mDBConn;
   nsresult rv = setClosedState();
   NS_ENSURE_SUCCESS(rv, rv);
 
--- a/storage/mozStorageHelper.h
+++ b/storage/mozStorageHelper.h
@@ -12,16 +12,17 @@
 #include "nsIConsoleService.h"
 #include "nsIScriptError.h"
 
 #include "mozIStorageAsyncConnection.h"
 #include "mozIStorageConnection.h"
 #include "mozIStorageStatement.h"
 #include "mozIStoragePendingStatement.h"
 #include "nsError.h"
+#include "nsIXPConnect.h"
 
 /**
  * This class wraps a transaction inside a given C++ scope, guaranteeing that
  * the transaction will be completed even if you have an exception or
  * return early.
  *
  * A common use is to create an instance with aCommitOnComplete = false (rollback),
  * then call Commit() on this object manually when your function completes
@@ -222,13 +223,19 @@ protected:
       nsCOMPtr<nsIScriptError> e = do_CreateInstance(NS_SCRIPTERROR_CONTRACTID);   \
       if (e && NS_SUCCEEDED(e->Init(NS_ConvertUTF8toUTF16(msg), EmptyString(),     \
                                     EmptyString(), 0, 0,                           \
                                     nsIScriptError::errorFlag, "Storage"))) {      \
         cs->LogMessage(e);                                                         \
       }                                                                            \
     }                                                                              \
   }                                                                                \
+  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
 
 #endif /* MOZSTORAGEHELPER_H */
--- a/storage/test/gtest/moz.build
+++ b/storage/test/gtest/moz.build
@@ -7,16 +7,17 @@
 UNIFIED_SOURCES += [
     'test_AsXXX_helpers.cpp',
     'test_async_callbacks_with_spun_event_loops.cpp',
     'test_asyncStatementExecution_transaction.cpp',
     'test_binding_params.cpp',
     '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',
     'test_true_async.cpp',
     'test_unlock_notify.cpp',
 ]
 
 if CONFIG['MOZ_DEBUG'] and CONFIG['OS_ARCH'] not in ('WINNT') and CONFIG['OS_TARGET'] != 'Android':
new file mode 100644
--- /dev/null
+++ b/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);
+}
--- a/storage/test/unit/test_connection_asyncClose.js
+++ b/storage/test/unit/test_connection_asyncClose.js
@@ -48,33 +48,35 @@ add_task(function* test_asyncClose_does_
 /**
  * Open an async database (ensures the async thread is created) and then invoke
  * AsyncClose() twice without yielding control flow.  The first will initiate
  * the actual async close after calling setClosedState which synchronously
  * impacts what the second call will observe.  The second call will then see the
  * 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
  * asyncClose on it.  This will bring the async thread to life to perform the
  * shutdown to avoid blocking the main thread, although we won't be able to
  * tell the difference between this happening and the method secretly shunting
  * to close().
  */
--- a/storage/test/unit/test_storage_connection.js
+++ b/storage/test/unit/test_storage_connection.js
@@ -260,34 +260,29 @@ if (!AppConstants.DEBUG) {
     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();
-  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());
   let db2 = db1.clone();
   do_check_true(db2.connectionReady);
 
   // A write statement should not fail here.
   let stmt = db2.createStatement("INSERT INTO test (name) VALUES (:name)");
--- a/toolkit/components/contentprefs/nsContentPrefService.js
+++ b/toolkit/components/contentprefs/nsContentPrefService.js
@@ -168,17 +168,19 @@ ContentPrefService.prototype = {
     if (this.__stmtUpdatePref) {
       this.__stmtUpdatePref.finalize();
       this.__stmtUpdatePref = null;
     }
 
     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
     // in _observers and _genericObservers to avoid cycles with those that
     // refer to us and don't remove themselves from those observer pools.
     delete this._observers;
     delete this._genericObservers;
     delete this.__consoleSvc;
--- a/toolkit/components/contentprefs/tests/unit/head_contentPrefs.js
+++ b/toolkit/components/contentprefs/tests/unit/head_contentPrefs.js
@@ -6,16 +6,17 @@
 
 var Cc = Components.classes;
 var Ci = Components.interfaces;
 var Cr = Components.results;
 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";
 
 var ContentPrefTest = {
   // Convenience Getters
 
   __dirSvc: null,
--- a/toolkit/components/contentprefs/tests/unit/test_contentPrefs.js
+++ b/toolkit/components/contentprefs/tests/unit/test_contentPrefs.js
@@ -1,85 +1,82 @@
 /* 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/. */
 
-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();
+  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;
+  }
 
-    // Get the service and make sure it has a ready database connection.
-    let cps = Cc["@mozilla.org/content-pref/service;1"].
-              createInstance(Ci.nsIContentPrefService);
+  // Create a new database.
+  ContentPrefTest.deleteDatabase();
+  await with_cps_instance(cps => {
     do_check_true(cps.DBConnection.connectionReady);
-    cps.DBConnection.close();
-  }
+  });
 
   // Open an existing database.
-  {
-    let dbFile = ContentPrefTest.deleteDatabase();
+
+  ContentPrefTest.deleteDatabase();
+  await with_cps_instance(cps => {});
 
-    let cps = Cc["@mozilla.org/content-pref/service;1"].
-               createInstance(Ci.nsIContentPrefService);
-    cps.DBConnection.close();
-    do_check_true(dbFile.exists());
-
-    // 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.
   {
     let dbFile = ContentPrefTest.deleteDatabase();
 
     // Create an empty database.
     let dbService = Cc["@mozilla.org/storage/service;1"].
                     getService(Ci.mozIStorageService);
     let dbConnection = dbService.openDatabase(dbFile);
     do_check_eq(dbConnection.schemaVersion, 0);
     dbConnection.close();
     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.
   {
     let dbFile = ContentPrefTest.deleteDatabase();
     let backupDBFile = ContentPrefTest.deleteBackupDatabase();
 
     // Create a corrupted database.
     let foStream = Cc["@mozilla.org/network/file-output-stream;1"].
                    createInstance(Ci.nsIFileOutputStream);
     foStream.init(dbFile, 0x02 | 0x08 | 0x20, 0o666, 0);
     let garbageData = "garbage that makes SQLite think the file is corrupted";
     foStream.write(garbageData, garbageData.length);
     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.
   {
     let dbFile = ContentPrefTest.deleteDatabase();
     let backupDBFile = ContentPrefTest.deleteBackupDatabase();
 
     // Create an empty database and set the schema version to a number
@@ -87,34 +84,36 @@ function run_test() {
     let dbService = Cc["@mozilla.org/storage/service;1"].
                     getService(Ci.mozIStorageService);
     let dbConnection = dbService.openDatabase(dbFile);
     dbConnection.schemaVersion = -1;
     dbConnection.close();
     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);
+    });
   }
 
 
   // Now get the content pref service for real for use by the rest of the tests.
   let cps = new ContentPrefInstance(null);
 
   var uri = ContentPrefTest.getURI("http://www.example.com/");
 
   // 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
 
   do_check_eq(cps.getPref(uri, "test.nonexistent.getPref"), undefined);
   do_check_eq(cps.setPref(uri, "test.nonexistent.setPref", 5), undefined);
   do_check_false(cps.hasPref(uri, "test.nonexistent.hasPref"));
   do_check_eq(cps.removePref(uri, "test.nonexistent.removePref"), undefined);
 
@@ -455,9 +454,9 @@ function run_test() {
     groupCount.executeStep();
     do_check_true(groupCount.row.count == 0);
     groupCount.reset();
     let globalPref = dbConnection.createStatement("SELECT groupID FROM prefs");
     globalPref.executeStep();
     do_check_true(globalPref.row.groupID == null);
     globalPref.reset();
   }
-}
+});
--- a/toolkit/components/places/Database.cpp
+++ b/toolkit/components/places/Database.cpp
@@ -779,17 +779,17 @@ Database::BackupAndReplaceDatabaseFile(n
 
   // If anything fails from this point on, we have a stale connection or
   // database file, and there's not much more we can do.
   // The only thing we can try to do is to replace the database on the next
   // start, and enforce a crash, so it gets reported to us.
 
   // 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.")));
   }
 
   // Remove the broken database.
   rv = databaseFile->Remove(false);
   if (NS_FAILED(rv) && rv != NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) {
     return ForceCrashAndReplaceDatabase(
--- a/toolkit/components/search/tests/xpcshell/test_searchSuggest.js
+++ b/toolkit/components/search/tests/xpcshell/test_searchSuggest.js
@@ -27,17 +27,16 @@ function run_test() {
   removeMetadata();
 
   let server = useHttpServer();
   server.registerContentType("sjs", "sjs");
 
   do_register_cleanup(() => (async function cleanup() {
     // Remove added form history entries
     await updateSearchHistory("remove", null);
-    FormHistory.shutdown();
     Services.prefs.clearUserPref("browser.search.suggest.enabled");
   })());
 
   run_next_test();
 }
 
 add_task(async function add_test_engines() {
   let getEngineData = {