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
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 12 Apr 2017 17:44:39 +0200
changeset 365069 73edd4e1acef4e708346600b81a337022e450475
parent 365068 41c595eb1a0b587b054dad69b7bcf99a76e3876b
child 365070 9d23ec0ef50a71aa635cfa3b9652ae6924bf495f
push id32058
push userkwierso@gmail.com
push dateWed, 21 Jun 2017 01:24:44 +0000
treeherdermozilla-central@c55e582aee5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersasuth
bugs1355561
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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 = {