diff --git a/storage/public/mozIStoragePendingStatement.idl b/storage/public/mozIStoragePendingStatement.idl index d2c74ec21779..be9d34f94a66 100644 --- a/storage/public/mozIStoragePendingStatement.idl +++ b/storage/public/mozIStoragePendingStatement.idl @@ -39,7 +39,7 @@ #include "nsISupports.idl" -[scriptable, uuid(fc3c5fdc-9a87-4757-b01f-4ace2670a3a0)] +[scriptable, uuid(00da7d20-3768-4398-bedc-e310c324b3f0)] interface mozIStoragePendingStatement : nsISupports { /** @@ -48,8 +48,6 @@ interface mozIStoragePendingStatement : nsISupports { * * @note For read statements (such as SELECT), you will no longer receive any * notifications about results once cancel is called. - * - * @returns true if canceled successfully, false otherwise. */ - boolean cancel(); + void cancel(); }; diff --git a/storage/src/mozStorageAsyncStatementExecution.cpp b/storage/src/mozStorageAsyncStatementExecution.cpp index 2e1b2d51a7f4..e1a3e4bdc394 100644 --- a/storage/src/mozStorageAsyncStatementExecution.cpp +++ b/storage/src/mozStorageAsyncStatementExecution.cpp @@ -205,8 +205,7 @@ AsyncExecuteStatements::AsyncExecuteStatements(StatementDataArray &aStatements, , mMaxWait(TimeDuration::FromMilliseconds(MAX_MILLISECONDS_BETWEEN_RESULTS)) , mIntervalStart(TimeStamp::Now()) , mState(PENDING) -, mCancelRequested(PR_FALSE) -, mMutex(aConnection->sharedAsyncExecutionMutex) +, mCancelRequested(0) { (void)mStatements.SwapElements(aStatements); NS_ASSERTION(mStatements.Length(), "We weren't given any statements!"); @@ -222,9 +221,6 @@ AsyncExecuteStatements::shouldNotify() NS_ASSERTION(onCallingThread, "runEvent not running on the calling thread!"); #endif - // We do not need to acquire mMutex here because it can only ever be written - // to on the calling thread, and the only thread that can call us is the - // calling thread, so we know that our access is serialized. return !mCancelRequested; } @@ -232,8 +228,6 @@ bool AsyncExecuteStatements::bindExecuteAndProcessStatement(StatementData &aData, bool aLastStatement) { - mMutex.AssertNotCurrentThreadOwns(); - sqlite3_stmt *stmt(aData); BindingParamsArray *paramsArray(aData); @@ -247,10 +241,7 @@ AsyncExecuteStatements::bindExecuteAndProcessStatement(StatementData &aData, error = (*itr)->bind(stmt); if (error) { // Set our error state. - { - MutexAutoLock mutex(mMutex); - mState = ERROR; - } + (void)::PR_AtomicSet(const_cast(&mState), ERROR); // And notify. (void)notifyError(error); @@ -273,13 +264,6 @@ bool AsyncExecuteStatements::executeAndProcessStatement(sqlite3_stmt *aStatement, bool aLastStatement) { - mMutex.AssertNotCurrentThreadOwns(); - - // We need to hold the mutex for statement execution so we can properly - // reflect state in case we are canceled. We release the mutex in a few areas - // in order to allow for cancelation to occur. - MutexAutoLock lockedScope(mMutex); - // Execute our statement bool hasResults; do { @@ -291,7 +275,7 @@ AsyncExecuteStatements::executeAndProcessStatement(sqlite3_stmt *aStatement, // If we have been canceled, there is no point in going on... if (mCancelRequested) { - mState = CANCELED; + (void)::PR_AtomicSet(const_cast(&mState), CANCELED); return false; } @@ -300,16 +284,11 @@ AsyncExecuteStatements::executeAndProcessStatement(sqlite3_stmt *aStatement, if (mCallback && hasResults && NS_FAILED(buildAndNotifyResults(aStatement))) { // We had an error notifying, so we notify on error and stop processing. - mState = ERROR; + (void)::PR_AtomicSet(const_cast(&mState), ERROR); - { - // Drop our mutex because notifyError doesn't want it held. - MutexAutoUnlock unlockedScope(mMutex); - - // Notify, and stop processing statements. - (void)notifyError(mozIStorageError::ERROR, - "An error occurred while notifying about results"); - } + // Notify, and stop processing statements. + (void)notifyError(mozIStorageError::ERROR, + "An error occurred while notifying about results"); return false; } @@ -320,11 +299,9 @@ AsyncExecuteStatements::executeAndProcessStatement(sqlite3_stmt *aStatement, checkAndLogStatementPerformance(aStatement); #endif - // If we are done, we need to set our state accordingly while we still hold - // our mutex. We would have already returned if we were canceled or had - // an error at this point. + // If we are done, we need to set our state accordingly. if (aLastStatement) - mState = COMPLETED; + (void)::PR_AtomicSet(const_cast(&mState), COMPLETED); return true; } @@ -332,8 +309,6 @@ AsyncExecuteStatements::executeAndProcessStatement(sqlite3_stmt *aStatement, bool AsyncExecuteStatements::executeStatement(sqlite3_stmt *aStatement) { - mMutex.AssertCurrentThreadOwns(); - while (true) { int rc = ::sqlite3_step(aStatement); // Stop if we have no more results. @@ -346,25 +321,17 @@ AsyncExecuteStatements::executeStatement(sqlite3_stmt *aStatement) // Some errors are not fatal, and we can handle them and continue. if (rc == SQLITE_BUSY) { - // We do not want to hold our mutex while we yield. - MutexAutoUnlock cancelationScope(mMutex); - // Yield, and try again (void)::PR_Sleep(PR_INTERVAL_NO_WAIT); continue; } // Set an error state. - mState = ERROR; + (void)::PR_AtomicSet(const_cast(&mState), ERROR); - { - // Drop our mutex because notifyError doesn't want it held. - MutexAutoUnlock unlockedScope(mMutex); - - // And notify. - sqlite3 *db = ::sqlite3_db_handle(aStatement); - (void)notifyError(rc, ::sqlite3_errmsg(db)); - } + // And notify. + sqlite3 *db = ::sqlite3_db_handle(aStatement); + (void)notifyError(rc, ::sqlite3_errmsg(db)); // Finally, indicate that we should stop processing. return false; @@ -375,12 +342,6 @@ nsresult AsyncExecuteStatements::buildAndNotifyResults(sqlite3_stmt *aStatement) { NS_ASSERTION(mCallback, "Trying to dispatch results without a callback!"); - mMutex.AssertCurrentThreadOwns(); - - // At this point, it is safe to not hold the mutex and allow for cancelation. - // We may add an event to the calling thread, but that thread will not end - // up running when it checks back with us to see if it should run. - MutexAutoUnlock cancelationScope(mMutex); // Build result object if we need it. if (!mResultSet) @@ -417,7 +378,6 @@ AsyncExecuteStatements::buildAndNotifyResults(sqlite3_stmt *aStatement) nsresult AsyncExecuteStatements::notifyComplete() { - mMutex.AssertNotCurrentThreadOwns(); NS_ASSERTION(mState != PENDING, "Still in a pending state when calling Complete!"); @@ -432,7 +392,7 @@ AsyncExecuteStatements::notifyComplete() if (mState == COMPLETED) { nsresult rv = mTransactionManager->Commit(); if (NS_FAILED(rv)) { - mState = ERROR; + (void)::PR_AtomicSet(const_cast(&mState), ERROR); (void)notifyError(mozIStorageError::ERROR, "Transaction failed to commit"); } @@ -447,7 +407,7 @@ AsyncExecuteStatements::notifyComplete() // Notify about completion iff we have a callback. if (mCallback) { nsRefPtr completionEvent = - new CompletionNotifier(mCallback, mState); + new CompletionNotifier(mCallback, static_cast(mState)); NS_ENSURE_TRUE(completionEvent, NS_ERROR_OUT_OF_MEMORY); // We no longer own mCallback (the CompletionNotifier takes ownership). @@ -463,8 +423,6 @@ nsresult AsyncExecuteStatements::notifyError(PRInt32 aErrorCode, const char *aMessage) { - mMutex.AssertNotCurrentThreadOwns(); - if (!mCallback) return NS_OK; @@ -477,8 +435,6 @@ AsyncExecuteStatements::notifyError(PRInt32 aErrorCode, nsresult AsyncExecuteStatements::notifyError(mozIStorageError *aError) { - mMutex.AssertNotCurrentThreadOwns(); - if (!mCallback) return NS_OK; @@ -492,7 +448,6 @@ AsyncExecuteStatements::notifyError(mozIStorageError *aError) nsresult AsyncExecuteStatements::notifyResults() { - mMutex.AssertNotCurrentThreadOwns(); NS_ASSERTION(mCallback, "notifyResults called without a callback!"); nsRefPtr notifier = @@ -515,7 +470,7 @@ NS_IMPL_THREADSAFE_ISUPPORTS2( //// mozIStoragePendingStatement NS_IMETHODIMP -AsyncExecuteStatements::Cancel(PRBool *_successful) +AsyncExecuteStatements::Cancel() { #ifdef DEBUG PRBool onCallingThread = PR_FALSE; @@ -527,17 +482,11 @@ AsyncExecuteStatements::Cancel(PRBool *_successful) // we are trying to cancel. NS_ENSURE_FALSE(mCancelRequested, NS_ERROR_UNEXPECTED); - { - MutexAutoLock lockedScope(mMutex); + // Indicate that we want to try and cancel at the next cancelation point. + (void)::PR_AtomicSet(const_cast(&mCancelRequested), 1); - // We need to indicate that we want to try and cancel now. - mCancelRequested = true; - - // Establish if we can cancel - *_successful = (mState == PENDING); - } - - // Note, it is possible for us to return false here, and end up canceling + // Note: While we are requesting to cancel here, it is possible that we will + // not actually be able to. However, we will still suppress handleResult // events that have been dispatched to the calling thread. This is OK, // however, because only read statements (such as SELECT) are going to be // posting events to the calling thread that actually check if they should @@ -553,15 +502,10 @@ NS_IMETHODIMP AsyncExecuteStatements::Run() { // Do not run if we have been canceled. - bool cancelRequested; - { - MutexAutoLock lockedScope(mMutex); - cancelRequested = mCancelRequested; - if (cancelRequested) - mState = CANCELED; - } - if (cancelRequested) + if (mCancelRequested) { + (void)::PR_AtomicSet(const_cast(&mState), CANCELED); return notifyComplete(); + } // If there is more than one statement, run it in a transaction. We assume // that we have been given write statements since getting a batch of read diff --git a/storage/src/mozStorageAsyncStatementExecution.h b/storage/src/mozStorageAsyncStatementExecution.h index 0afa365026e9..00438148f2bb 100644 --- a/storage/src/mozStorageAsyncStatementExecution.h +++ b/storage/src/mozStorageAsyncStatementExecution.h @@ -44,7 +44,6 @@ #include "nsTArray.h" #include "nsAutoPtr.h" #include "nsThreadUtils.h" -#include "mozilla/Mutex.h" #include "mozilla/TimeStamp.h" #include "mozIStoragePendingStatement.h" @@ -118,8 +117,6 @@ private: * occurs, or we are canceled. If aLastStatement is true, we should set * mState accordingly. * - * @pre mMutex is not held - * * @param aData * The StatementData to bind, execute, and then process. * @param aLastStatement @@ -134,8 +131,6 @@ private: * Executes a given statement until completion, an error occurs, or we are * canceled. If aLastStatement is true, we should set mState accordingly. * - * @pre mMutex is not held - * * @param aStatement * The statement to execute and then process. * @param aLastStatement @@ -149,8 +144,6 @@ private: /** * Executes a statement to completion, properly handling any error conditions. * - * @pre mMutex is held - * * @param aStatement * The statement to execute to completion. * @returns true if results were obtained, false otherwise. @@ -161,8 +154,6 @@ private: * Builds a result set up with a row from a given statement. If we meet the * right criteria, go ahead and notify about this results too. * - * @pre mMutex is held - * * @param aStatement * The statement to get the row data from. */ @@ -170,16 +161,12 @@ private: /** * Notifies callback about completion, and does any necessary cleanup. - * - * @pre mMutex is not held */ nsresult notifyComplete(); /** * Notifies callback about an error. * - * @pre mMutex is not held - * * @param aErrorCode * The error code defined in mozIStorageError for the error. * @param aMessage @@ -192,8 +179,6 @@ private: /** * Notifies the callback about a result set. - * - * @pre mMutex is not held */ nsresult notifyResults(); @@ -216,24 +201,20 @@ private: TimeStamp mIntervalStart; /** - * Indicates our state of execution. + * Indicates our state of execution. This is one of the values in the + * ExecutionState enum. + * + * @note This should only be set with PR_Atomic* functions. */ - ExecutionState mState; + volatile PRInt32 mState; /** - * Indicates if we should try to cancel at a cancelation point. + * Indicates if we should try to cancel at a cancelation point or not. This + * is evaluated as a boolean. + * + * @note This should only be set with PR_Atomic* functions. */ - bool mCancelRequested; - - /** - * This is the mutex tat protects our state from changing between threads. - * This includes the following variables: - * - mState - * - mCancelRequested is only set on the calling thread while the lock is - * held. It is always read from within the lock on the background thread, - * but not on the calling thread (see shouldNotify for why). - */ - Mutex &mMutex; + volatile PRInt32 mCancelRequested; }; } // namespace storage diff --git a/storage/src/mozStorageConnection.cpp b/storage/src/mozStorageConnection.cpp index 83e7db3905e7..2103042a4b93 100644 --- a/storage/src/mozStorageConnection.cpp +++ b/storage/src/mozStorageConnection.cpp @@ -245,8 +245,7 @@ aggregateFunctionFinalHelper(sqlite3_context *aCtx) //// Connection Connection::Connection(Service *aService) -: sharedAsyncExecutionMutex("Connection::sharedAsyncExecutionMutex") -, mDBConn(nsnull) +: mDBConn(nsnull) , mAsyncExecutionMutex(nsAutoLock::NewLock("AsyncExecutionMutex")) , mAsyncExecutionThreadShuttingDown(PR_FALSE) , mTransactionMutex(nsAutoLock::NewLock("TransactionMutex")) diff --git a/storage/src/mozStorageConnection.h b/storage/src/mozStorageConnection.h index c47bd90905b5..65afdab32353 100644 --- a/storage/src/mozStorageConnection.h +++ b/storage/src/mozStorageConnection.h @@ -43,7 +43,6 @@ #include "nsAutoPtr.h" #include "nsCOMPtr.h" -#include "mozilla/Mutex.h" #include "nsString.h" #include "nsInterfaceHashtable.h" @@ -93,13 +92,6 @@ public: */ already_AddRefed getAsyncExecutionTarget(); - /** - * Mutex used by asynchronous statements to protect state. The mutex is - * declared on the connection object because there is no contention between - * asynchronous statements (they are serialized on mAsyncExecutionThread). - */ - Mutex sharedAsyncExecutionMutex; - private: ~Connection(); diff --git a/storage/test/unit/test_statement_executeAsync.js b/storage/test/unit/test_statement_executeAsync.js index 6c2fa58aadc1..724d7a3cb919 100644 --- a/storage/test/unit/test_statement_executeAsync.js +++ b/storage/test/unit/test_statement_executeAsync.js @@ -373,7 +373,6 @@ function test_immediate_cancellation() "DELETE FROM test WHERE id = ?" ); stmt.bindInt32Parameter(0, 0); - let reason = Ci.mozIStorageStatementCallback.REASON_CANCELED; var pendingStatement = stmt.executeAsync({ handleResult: function(aResultSet) { @@ -389,7 +388,9 @@ function test_immediate_cancellation() { print("handleCompletion(" + aReason + ") for test_immediate_cancellation"); - do_check_eq(reason, aReason); + // It is possible that we finished before we canceled. + do_check_true(aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED || + aReason == Ci.mozIStorageStatementCallback.REASON_CANCELED); // Run the next test. run_next_test(); @@ -397,10 +398,7 @@ function test_immediate_cancellation() }); // Cancel immediately - if (!pendingStatement.cancel()) { - // It is possible that we finished before we canceled - reason = Ci.mozIStorageStatementCallback.REASON_FINISHED; - } + pendingStatement.cancel() stmt.finalize(); } @@ -411,7 +409,6 @@ function test_double_cancellation() "DELETE FROM test WHERE id = ?" ); stmt.bindInt32Parameter(0, 0); - let reason = Ci.mozIStorageStatementCallback.REASON_CANCELED; var pendingStatement = stmt.executeAsync({ handleResult: function(aResultSet) { @@ -427,7 +424,9 @@ function test_double_cancellation() { print("handleCompletion(" + aReason + ") for test_double_cancellation"); - do_check_eq(reason, aReason); + // It is possible that we finished before we canceled. + do_check_true(aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED || + aReason == Ci.mozIStorageStatementCallback.REASON_CANCELED); // Run the next test. run_next_test(); @@ -435,10 +434,7 @@ function test_double_cancellation() }); // Cancel immediately - if (!pendingStatement.cancel()) { - // It is possible that we finished before we canceled - reason = Ci.mozIStorageStatementCallback.REASON_FINISHED; - } + pendingStatement.cancel() // And cancel again - expect an exception try {