diff --git a/dom/cache/Connection.cpp b/dom/cache/Connection.cpp index 91f4a4154d3c..53a579725725 100644 --- a/dom/cache/Connection.cpp +++ b/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*) { diff --git a/storage/mozIStorageAsyncConnection.idl b/storage/mozIStorageAsyncConnection.idl index aeb2bf1b51ca..e228d9db5935 100644 --- a/storage/mozIStorageAsyncConnection.idl +++ b/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. diff --git a/storage/mozStorageConnection.cpp b/storage/mozStorageConnection.cpp index 2a579d227e6b..87121cdf293e 100644 --- a/storage/mozStorageConnection.cpp +++ b/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 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 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 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 diff --git a/storage/mozStorageHelper.h b/storage/mozStorageHelper.h index f1ae251b1939..d374ff14cb0e 100644 --- a/storage/mozStorageHelper.h +++ b/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 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 diff --git a/storage/test/gtest/moz.build b/storage/test/gtest/moz.build index 357a26e26e19..ba0875de098b 100644 --- a/storage/test/gtest/moz.build +++ b/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', diff --git a/storage/test/gtest/test_spinningSynchronousClose.cpp b/storage/test/gtest/test_spinningSynchronousClose.cpp new file mode 100644 index 000000000000..1e8d599b33e7 --- /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 db(getMemoryDatabase()); + // Run an async statement. + nsCOMPtr stmt; + do_check_success(db->CreateAsyncStatement( + NS_LITERAL_CSTRING("CREATE TABLE test (id INTEGER PRIMARY KEY)"), + getter_AddRefs(stmt) + )); + nsCOMPtr p; + do_check_success(stmt->ExecuteAsync(nullptr, getter_AddRefs(p))); + do_check_success(stmt->Finalize()); + + // Wrongly use Close() instead of AsyncClose(). + RefPtr 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 db(getMemoryDatabase()); + // Run an async statement. + nsCOMPtr stmt; + do_check_success(db->CreateAsyncStatement( + NS_LITERAL_CSTRING("CREATE TABLE test (id INTEGER PRIMARY KEY)"), + getter_AddRefs(stmt) + )); + nsCOMPtr p; + do_check_success(stmt->ExecuteAsync(nullptr, getter_AddRefs(p))); + do_check_success(stmt->Finalize()); + + // Use the spinning close API. + RefPtr event = new CompletionRunnable(); + NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL); + do_check_success(db->SpinningSynchronousClose()); + do_check_true(event->mDone); +} diff --git a/storage/test/unit/test_connection_asyncClose.js b/storage/test/unit/test_connection_asyncClose.js index ca60dc718d3f..ffebf59ccc35 100644 --- a/storage/test/unit/test_connection_asyncClose.js +++ b/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 diff --git a/storage/test/unit/test_storage_connection.js b/storage/test/unit/test_storage_connection.js index 62c05780894e..87427fa5ac59 100644 --- a/storage/test/unit/test_storage_connection.js +++ b/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()); diff --git a/toolkit/components/contentprefs/nsContentPrefService.js b/toolkit/components/contentprefs/nsContentPrefService.js index 62eae4cf6336..8521fa98fc77 100644 --- a/toolkit/components/contentprefs/nsContentPrefService.js +++ b/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 diff --git a/toolkit/components/contentprefs/tests/unit/head_contentPrefs.js b/toolkit/components/contentprefs/tests/unit/head_contentPrefs.js index 177d74b3cb03..a468748312fd 100644 --- a/toolkit/components/contentprefs/tests/unit/head_contentPrefs.js +++ b/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"; diff --git a/toolkit/components/contentprefs/tests/unit/test_contentPrefs.js b/toolkit/components/contentprefs/tests/unit/test_contentPrefs.js index 6a24e07b6247..a2e097b5fdd2 100644 --- a/toolkit/components/contentprefs/tests/unit/test_contentPrefs.js +++ b/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(); } -} +}); diff --git a/toolkit/components/places/Database.cpp b/toolkit/components/places/Database.cpp index 652734ddd750..0a6828f4b5d8 100644 --- a/toolkit/components/places/Database.cpp +++ b/toolkit/components/places/Database.cpp @@ -784,7 +784,7 @@ Database::BackupAndReplaceDatabaseFile(nsCOMPtr& 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."))); } diff --git a/toolkit/components/search/tests/xpcshell/test_searchSuggest.js b/toolkit/components/search/tests/xpcshell/test_searchSuggest.js index 47bae3103568..5db2e3a00e84 100644 --- a/toolkit/components/search/tests/xpcshell/test_searchSuggest.js +++ b/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"); })());