author | Andrew Sutherland <asutherland@asutherland.org> |
Thu, 07 Jan 2016 11:18:00 -0500 | |
changeset 314792 | 42ee91882a503bfb9e216f19111dc935bd400caf |
parent 314791 | d6a85731d2aa518779451ebf8583815151565c56 |
child 314793 | 29a5d0c6ea47ffb17df9bebc7da22df25e7eb451 |
push id | 5703 |
push user | raliiev@mozilla.com |
push date | Mon, 07 Mar 2016 14:18:41 +0000 |
treeherder | mozilla-beta@31e373ad5b5f [default view] [failures only] |
perfherder | [talos] [build metrics] [platform microbench] (compared to previous push) |
reviewers | bkelly |
bugs | 1237601 |
milestone | 46.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
|
--- a/storage/mozIStorageAsyncConnection.idl +++ b/storage/mozIStorageAsyncConnection.idl @@ -31,17 +31,22 @@ interface mozIStorageAsyncConnection : n * * @param aCallback [optional] * A callback that will be notified when the close is completed, * with the following arguments: * - status: the status of the call * - value: |null| * * @throws NS_ERROR_NOT_SAME_THREAD - * If is called on a thread other than the one that opened it. + * 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. */ void asyncClose([optional] in mozIStorageCompletionCallback aCallback); /** * Clone a database and make the clone read only if needed. * * @param aReadOnly * If true, the returned database should be put into read-only mode.
--- a/storage/mozStorageConnection.cpp +++ b/storage/mozStorageConnection.cpp @@ -13,16 +13,17 @@ #include "nsThreadUtils.h" #include "nsIFile.h" #include "nsIFileURL.h" #include "mozilla/Telemetry.h" #include "mozilla/Mutex.h" #include "mozilla/CondVar.h" #include "mozilla/Attributes.h" #include "mozilla/ErrorNames.h" +#include "mozilla/unused.h" #include "mozIStorageAggregateFunction.h" #include "mozIStorageCompletionCallback.h" #include "mozIStorageFunction.h" #include "mozStorageAsyncStatementExecution.h" #include "mozStorageSQLFunctions.h" #include "mozStorageConnection.h" @@ -1195,36 +1196,95 @@ Connection::Close() NS_IMETHODIMP Connection::AsyncClose(mozIStorageCompletionCallback *aCallback) { if (!NS_IsMainThread()) { return NS_ERROR_NOT_SAME_THREAD; } - // It's possible to get here with a null mDBConn but a non-null async - // execution target if OpenAsyncDatabase failed somehow, so don't exit early - // in that case. + // The two relevant factors at this point are whether we have a database + // connection and whether we have an async execution thread. Here's what the + // states mean and how we handle them: + // + // - (mDBConn && asyncThread): The expected case where we are either an + // async connection or a sync connection that has been used asynchronously. + // Either way the caller must call us and not Close(). Nothing surprising + // about this. We'll dispatch AsyncCloseConnection to the already-existing + // async thread. + // + // - (mDBConn && !asyncThread): A somewhat unusual case where the caller + // opened the connection synchronously and was planning to use it + // asynchronously, but never got around to using it asynchronously before + // needing to shutdown. This has been observed to happen for the cookie + // service in a case where Firefox shuts itself down almost immediately + // after startup (for unknown reasons). In the Firefox shutdown case, + // we may also fail to create a new async execution thread if one does not + // already exist. (nsThreadManager will refuse to create new threads when + // it has already been told to shutdown.) As such, we need to handle a + // failure to create the async execution thread by falling back to + // synchronous Close() and also dispatching the completion callback because + // at least Places likes to spin a nested event loop that depends on the + // callback being invoked. + // + // Note that we have considered not trying to spin up the async execution + // thread in this case if it does not already exist, but the overhead of + // thread startup (if successful) is significantly less expensive than the + // worst-case potential I/O hit of synchronously closing a database when we + // could close it asynchronously. + // + // - (!mDBConn && asyncThread): This happens in some but not all cases where + // OpenAsyncDatabase encountered a problem opening the database. If it + // happened in all cases AsyncInitDatabase would just shut down the thread + // directly and we would avoid this case. But it doesn't, so for simplicity + // and consistency AsyncCloseConnection knows how to handle this and we + // act like this was the (mDBConn && asyncThread) case in this method. + // + // - (!mDBConn && !asyncThread): The database was never successfully opened or + // Close() or AsyncClose() has already been called (at least) once. This is + // undeniably a misuse case by the caller. We could optimize for this + // case by adding an additional check of mAsyncExecutionThread without using + // getAsyncExecutionTarget() to avoid wastefully creating a thread just to + // shut it down. But this complicates the method for broken caller code + // whereas we're still correct and safe without the special-case. nsIEventTarget *asyncThread = getAsyncExecutionTarget(); - if (!mDBConn && !asyncThread) - return NS_ERROR_NOT_INITIALIZED; + // Create our callback event if we were given a callback. This will + // eventually be dispatched in all cases, even if we fall back to Close() and + // the database wasn't open and we return an error. The rationale is that + // no existing consumer checks our return value and several of them like to + // spin nested event loops until the callback fires. Given that, it seems + // preferable for us to dispatch the callback in all cases. (Except the + // wrong thread misuse case we bailed on up above. But that's okay because + // that is statically wrong whereas these edge cases are dynamic.) + nsCOMPtr<nsIRunnable> completeEvent; + if (aCallback) { + completeEvent = newCompletionEvent(aCallback); + } + + if (!asyncThread) { + // We were unable to create an async thread, so we need to fall back to + // using normal Close(). Since there is no async thread, Close() will + // 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(); + } // 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); - // Create our callback event if we were given a callback. - nsCOMPtr<nsIRunnable> completeEvent; - if (aCallback) { - completeEvent = newCompletionEvent(aCallback); - } - // Create and dispatch our close event to the background thread. nsCOMPtr<nsIRunnable> closeEvent; { // We need to lock because we're modifying mAsyncExecutionThread MutexAutoLock lockedScope(sharedAsyncExecutionMutex); closeEvent = new AsyncCloseConnection(this, nativeConn, completeEvent, @@ -1523,17 +1583,17 @@ Connection::ExecuteSimpleSQL(const nsACS NS_IMETHODIMP Connection::ExecuteAsync(mozIStorageBaseStatement **aStatements, uint32_t aNumStatements, mozIStorageStatementCallback *aCallback, mozIStoragePendingStatement **_handle) { nsTArray<StatementData> stmts(aNumStatements); for (uint32_t i = 0; i < aNumStatements; i++) { - nsCOMPtr<StorageBaseStatementInternal> stmt = + nsCOMPtr<StorageBaseStatementInternal> stmt = do_QueryInterface(aStatements[i]); // Obtain our StatementData. StatementData data; nsresult rv = stmt->getAsynchronousStatementData(data); NS_ENSURE_SUCCESS(rv, rv); NS_ASSERTION(stmt->getOwner() == this,
--- a/storage/test/unit/head_storage.js +++ b/storage/test/unit/head_storage.js @@ -36,31 +36,39 @@ function getCorruptDB() /** * Obtains a fake (non-SQLite format) database to test against. */ function getFakeDB() { return do_get_file("fakeDB.sqlite"); } +/** + * Delete the test database file. + */ +function deleteTestDB() +{ + print("*** Storage Tests: Trying to remove file!"); + var dbFile = getTestDB(); + if (dbFile.exists()) + try { dbFile.remove(false); } catch (e) { /* stupid windows box */ } +} + function cleanup() { // close the connection print("*** Storage Tests: Trying to close!"); getOpenedDatabase().close(); // we need to null out the database variable to get a new connection the next // time getOpenedDatabase is called gDBConn = null; // removing test db - print("*** Storage Tests: Trying to remove file!"); - var dbFile = getTestDB(); - if (dbFile.exists()) - try { dbFile.remove(false); } catch (e) { /* stupid windows box */ } + deleteTestDB(); } /** * Use asyncClose to cleanup a connection. Synchronous by means of internally * spinning an event loop. */ function asyncCleanup() { @@ -75,20 +83,17 @@ function asyncCleanup() while (!closed) curThread.processNextEvent(true); // we need to null out the database variable to get a new connection the next // time getOpenedDatabase is called gDBConn = null; // removing test db - print("*** Storage Tests: Trying to remove file!"); - var dbFile = getTestDB(); - if (dbFile.exists()) - try { dbFile.remove(false); } catch (e) { /* stupid windows box */ } + deleteTestDB(); } function getService() { return Cc["@mozilla.org/storage/service;1"].getService(Ci.mozIStorageService); } /** @@ -322,16 +327,34 @@ function executeAsync(statement, onResul }, handleCompletion: function (result) { deferred.resolve(result); } }); return deferred.promise; } +function executeMultipleStatementsAsync(db, statements, onResult) { + let deferred = Promise.defer(); + db.executeAsync(statements, statements.length, { + handleError: function (error) { + deferred.reject(error); + }, + handleResult: function (result) { + if (onResult) { + onResult(result); + } + }, + handleCompletion: function (result) { + deferred.resolve(result); + } + }); + return deferred.promise; +} + function executeSimpleSQLAsync(db, query, onResult) { let deferred = Promise.defer(); db.executeSimpleSQLAsync(query, { handleError(error) { deferred.reject(error); }, handleResult(result) { if (onResult) {
new file mode 100644 --- /dev/null +++ b/storage/test/unit/test_connection_asyncClose.js @@ -0,0 +1,125 @@ +/* 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/. */ + +/* + * Thorough branch coverage for asyncClose. + * + * Coverage of asyncClose by connection state at time of AsyncClose invocation: + * - (asyncThread && mDBConn) => AsyncCloseConnection used, actually closes + * - test_asyncClose_does_not_complete_before_statements + * - test_double_asyncClose_throws + * - test_asyncClose_does_not_throw_without_callback + * - (asyncThread && !mDBConn) => AsyncCloseConnection used, although no close + * is required. Note that this is only possible in the event that + * openAsyncDatabase was used and we failed to open the database. + * Additionally, the async connection will never be exposed to the caller and + * AsyncInitDatabase will be the one to (automatically) call AsyncClose. + * - test_asyncClose_failed_open + * - (!asyncThread && mDBConn) => Close() invoked, actually closes + * - test_asyncClose_on_sync_db + * - (!asyncThread && !mDBConn) => Close() invoked, no close needed, errors. + * This happens if the database has already been closed. + * - test_double_asyncClose_throws + */ + + +/** + * Sanity check that our close indeed happens after asynchronously executed + * statements scheduled during the same turn of the event loop. Note that we + * just care that the statement says it completed without error, we're not + * worried that the close will happen and then the statement will magically + * complete. + */ +add_task(function* test_asyncClose_does_not_complete_before_statements() { + let db = getService().openDatabase(getTestDB()); + let stmt = db.createStatement("SELECT * FROM sqlite_master"); + // Issue the executeAsync but don't yield for it... + let asyncStatementPromise = executeAsync(stmt); + stmt.finalize(); + + // Issue the close. (And now the order of yielding doesn't matter.) + // Branch coverage: (asyncThread && mDBConn) + yield asyncClose(db); + equal((yield asyncStatementPromise), + Ci.mozIStorageStatementCallback.REASON_FINISHED); +}); + +/** + * 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()); + + // (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; +}); + +/** + * 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(). + */ +add_task(function* test_asyncClose_on_sync_db() { + let db = getService().openDatabase(getTestDB()); + + // Branch coverage: (!asyncThread && mDBConn) + yield asyncClose(db); + ok(true, 'closed sync connection asynchronously'); +}); + +/** + * Fail to asynchronously open a DB in order to get an async thread existing + * without having an open database and asyncClose invoked. As per the file + * doc-block, note that asyncClose will automatically be invoked by the + * AsyncInitDatabase when it fails to open the database. We will never be + * provided with a reference to the connection and so cannot call AsyncClose on + * it ourselves. + */ +add_task(function* test_asyncClose_failed_open() { + // This will fail and the promise will be rejected. + let openPromise = openAsyncDatabase(getFakeDB()); + yield openPromise.then( + () => { + ok(false, 'we should have failed to open the db; this test is broken!'); + }, + () => { + ok(true, 'correctly failed to open db; bg asyncClose should happen'); + } + ); + // (NB: we are unable to observe the thread shutdown, but since we never open + // a database, this test is not going to interfere with other tests so much.) +}); + +// THE TEST BELOW WANTS TO BE THE LAST TEST WE RUN. DO NOT MAKE IT SAD. +/** + * Verify that asyncClose without a callback does not explode. Without a + * callback the shutdown is not actually observable, so we run this test last + * in order to avoid weird overlaps. + */ +add_task(function test_asyncClose_does_not_throw_without_callback() { + let db = yield openAsyncDatabase(getTestDB()); + // Branch coverage: (asyncThread && mDBConn) + db.asyncClose(); + ok(true, 'if we shutdown cleanly and do not crash, then we succeeded'); +}); +// OBEY SHOUTING UPPER-CASE COMMENTS. +// ADD TESTS ABOVE THE FORMER TEST, NOT BELOW IT.
--- a/storage/test/unit/test_connection_executeAsync.js +++ b/storage/test/unit/test_connection_executeAsync.js @@ -1,106 +1,106 @@ /* 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/. */ /* * This file tests the functionality of mozIStorageConnection::executeAsync for * both mozIStorageStatement and mozIStorageAsyncStatement. + * + * A single database connection is used for the entirety of the test, which is + * a legacy thing, but we otherwise use the modern promise-based driver and + * async helpers. */ const INTEGER = 1; const TEXT = "this is test text"; const REAL = 3.23; const BLOB = [1, 2]; -add_test(function test_create_and_add() { - getOpenedDatabase().executeSimpleSQL( +add_task(function* test_first_create_and_add() { + // synchronously open the database and let gDBConn hold onto it because we + // use this database + let db = getOpenedDatabase(); + // synchronously set up our table *that will be used for the rest of the file* + db.executeSimpleSQL( "CREATE TABLE test (" + "id INTEGER, " + "string TEXT, " + "number REAL, " + "nuller NULL, " + "blober BLOB" + ")" ); let stmts = []; - stmts[0] = getOpenedDatabase().createStatement( + stmts[0] = db.createStatement( "INSERT INTO test (id, string, number, nuller, blober) VALUES (?, ?, ?, ?, ?)" ); stmts[0].bindByIndex(0, INTEGER); stmts[0].bindByIndex(1, TEXT); stmts[0].bindByIndex(2, REAL); stmts[0].bindByIndex(3, null); stmts[0].bindBlobByIndex(4, BLOB, BLOB.length); stmts[1] = getOpenedDatabase().createAsyncStatement( "INSERT INTO test (string, number, nuller, blober) VALUES (?, ?, ?, ?)" ); stmts[1].bindByIndex(0, TEXT); stmts[1].bindByIndex(1, REAL); stmts[1].bindByIndex(2, null); stmts[1].bindBlobByIndex(3, BLOB, BLOB.length); - getOpenedDatabase().executeAsync(stmts, stmts.length, { - handleResult: function (aResultSet) { - dump("handleResult(" + aResultSet + ")\n"); - do_throw("unexpected results obtained!"); - }, - handleError: function (aError) - { - dump("handleError(" + aError.result + ")\n"); - do_throw("unexpected error!"); - }, - handleCompletion: function (aReason) { - dump("handleCompletion(" + aReason + ")\n"); - do_check_eq(Ci.mozIStorageStatementCallback.REASON_FINISHED, aReason); + // asynchronously execute the statements + let execResult = yield executeMultipleStatementsAsync( + db, + stmts, + function(aResultSet) { + ok(false, 'we only did inserts so we should not have gotten results!'); + }); + equal(Ci.mozIStorageStatementCallback.REASON_FINISHED, execResult, + 'execution should have finished successfully.'); - // Check that the result is in the table - let stmt = getOpenedDatabase().createStatement( - "SELECT string, number, nuller, blober FROM test WHERE id = ?" - ); - stmt.bindByIndex(0, INTEGER); - try { - do_check_true(stmt.executeStep()); - do_check_eq(TEXT, stmt.getString(0)); - do_check_eq(REAL, stmt.getDouble(1)); - do_check_true(stmt.getIsNull(2)); - let count = { value: 0 }; - let blob = { value: null }; - stmt.getBlob(3, count, blob); - do_check_eq(BLOB.length, count.value); - for (let i = 0; i < BLOB.length; i++) - do_check_eq(BLOB[i], blob.value[i]); - } - finally { - stmt.finalize(); - } + // Check that the result is in the table + let stmt = db.createStatement( + "SELECT string, number, nuller, blober FROM test WHERE id = ?" + ); + stmt.bindByIndex(0, INTEGER); + try { + do_check_true(stmt.executeStep()); + do_check_eq(TEXT, stmt.getString(0)); + do_check_eq(REAL, stmt.getDouble(1)); + do_check_true(stmt.getIsNull(2)); + let count = { value: 0 }; + let blob = { value: null }; + stmt.getBlob(3, count, blob); + do_check_eq(BLOB.length, count.value); + for (let i = 0; i < BLOB.length; i++) + do_check_eq(BLOB[i], blob.value[i]); + } + finally { + stmt.finalize(); + } - // Make sure we have two rows in the table - stmt = getOpenedDatabase().createStatement( - "SELECT COUNT(1) FROM test" - ); - try { - do_check_true(stmt.executeStep()); - do_check_eq(2, stmt.getInt32(0)); - } - finally { - stmt.finalize(); - } + // Make sure we have two rows in the table + stmt = db.createStatement( + "SELECT COUNT(1) FROM test" + ); + try { + do_check_true(stmt.executeStep()); + do_check_eq(2, stmt.getInt32(0)); + } + finally { + stmt.finalize(); + } - // Run the next test. - run_next_test(); - } - }); stmts[0].finalize(); stmts[1].finalize(); }); -add_test(function test_multiple_bindings_on_statements() { +add_task(function* test_last_multiple_bindings_on_statements() { // This tests to make sure that we pass all the statements multiply bound // parameters when we call executeAsync. const AMOUNT_TO_ADD = 5; const ITERATIONS = 5; let stmts = []; let db = getOpenedDatabase(); let sqlString = "INSERT INTO test (id, string, number, nuller, blober) " + @@ -135,101 +135,37 @@ add_test(function test_multiple_bindings do_check_true(countStmt.executeStep()); currentRows = countStmt.row.count; } finally { countStmt.reset(); } // Execute asynchronously. - getOpenedDatabase().executeAsync(stmts, stmts.length, { - handleResult: function (aResultSet) { - do_throw("Unexpected call to handleResult!"); - }, - handleError: function (aError) { - print("Error code " + aError.result + " with message '" + - aError.message + "' returned."); - do_throw("Unexpected error!"); - }, - handleCompletion: function (aReason) { - print("handleCompletion(" + aReason + - ") for test_multiple_bindings_on_statements"); - do_check_eq(Ci.mozIStorageStatementCallback.REASON_FINISHED, aReason); + let execResult = yield executeMultipleStatementsAsync( + db, + stmts, + function(aResultSet) { + ok(false, 'we only did inserts so we should not have gotten results!'); + }); + equal(Ci.mozIStorageStatementCallback.REASON_FINISHED, execResult, + 'execution should have finished successfully.'); - // Check to make sure we added all of our rows. - try { - do_check_true(countStmt.executeStep()); - do_check_eq(currentRows + (ITERATIONS * AMOUNT_TO_ADD), - countStmt.row.count); - } - finally { - countStmt.finalize(); - } + // Check to make sure we added all of our rows. + try { + do_check_true(countStmt.executeStep()); + do_check_eq(currentRows + (ITERATIONS * AMOUNT_TO_ADD), + countStmt.row.count); + } + finally { + countStmt.finalize(); + } - // Run the next test. - run_next_test(); - } - }); stmts.forEach(stmt => stmt.finalize()); + + // we are the last test using this connection and since it has gone async + // we *must* call asyncClose on it. + yield asyncClose(db); + gDBConn = null; }); -add_test(function test_asyncClose_does_not_complete_before_statements() { - let stmt = createStatement("SELECT * FROM sqlite_master"); - let executed = false; - stmt.executeAsync({ - handleResult(aResultSet) {}, - handleError(aError) { - print("Error code " + aError.result + " with message '" + - aError.message + "' returned."); - do_throw("Unexpected error!"); - }, - handleCompletion(aReason) { - print("handleCompletion(" + aReason + - ") for test_asyncClose_does_not_complete_before_statements"); - do_check_eq(Ci.mozIStorageStatementCallback.REASON_FINISHED, aReason); - executed = true; - } - }); - stmt.finalize(); - - getOpenedDatabase().asyncClose(function () { - // Ensure that the statement executed to completion. - do_check_true(executed); - - // Reset gDBConn so that later tests will get a new connection object. - gDBConn = null; - run_next_test(); - }); -}); - -add_test(function test_asyncClose_does_not_throw_no_callback() { - getOpenedDatabase().asyncClose(); - - // Reset gDBConn so that later tests will get a new connection object. - gDBConn = null; - run_next_test(); -}); - -add_test(function test_double_asyncClose_throws() { - let conn = getOpenedDatabase(); - conn.asyncClose(); - try { - conn.asyncClose(); - do_throw("should have thrown"); - // There is a small race condition here, which can cause either of - // Cr.NS_ERROR_NOT_INITIALIZED or Cr.NS_ERROR_UNEXPECTED to be thrown. - } catch (e) { - if ("result" in e && e.result == Cr.NS_ERROR_NOT_INITIALIZED) { - do_print("NS_ERROR_NOT_INITIALIZED"); - } else if ("result" in e && e.result == Cr.NS_ERROR_UNEXPECTED) { - do_print("NS_ERROR_UNEXPECTED"); - } - } - - // Reset gDBConn so that later tests will get a new connection object. - gDBConn = null; - run_next_test(); -}); - -function run_test() { - cleanup(); - run_next_test(); -} +// If you add a test down here you will need to move the asyncClose or clean +// things up a little more.
--- a/storage/test/unit/xpcshell.ini +++ b/storage/test/unit/xpcshell.ini @@ -12,16 +12,17 @@ support-files = [test_bug-365166.js] [test_bug-393952.js] [test_bug-429521.js] [test_bug-444233.js] [test_cache_size.js] [test_chunk_growth.js] # Bug 676981: test fails consistently on Android fail-if = os == "android" +[test_connection_asyncClose.js] [test_connection_executeAsync.js] [test_connection_executeSimpleSQLAsync.js] [test_js_helpers.js] [test_levenshtein.js] [test_like.js] [test_like_escape.js] [test_locale_collation.js] [test_page_size_is_32k.js]