Bug 1375217 - Avoid raw pointers in mozStorageAsyncStatementExecution.cpp. r=asuth draft
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 21 Jun 2017 18:47:26 +0200
changeset 599669 0327588f9cbbfac4e3b5ffdcb62025ee930107aa
parent 598062 e49151136658c0d0f79b522727b807fd6cd8a79e
child 634819 33175905672fb1c42fb1223c4fe3b19d416397cb
push id65569
push usermak77@bonardo.net
push dateFri, 23 Jun 2017 11:43:31 +0000
reviewersasuth
bugs1375217
milestone56.0a1
Bug 1375217 - Avoid raw pointers in mozStorageAsyncStatementExecution.cpp. r=asuth MozReview-Commit-ID: Lshd1HUjXSq
storage/mozStorageAsyncStatementExecution.cpp
storage/mozStorageAsyncStatementExecution.h
--- a/storage/mozStorageAsyncStatementExecution.cpp
+++ b/storage/mozStorageAsyncStatementExecution.cpp
@@ -35,134 +35,16 @@ namespace storage {
  * consumers are trying to avoid blocking their execution thread for long
  * periods of time, and dispatching many small events to the calling thread will
  * end up blocking it.
  */
 #define MAX_MILLISECONDS_BETWEEN_RESULTS 75
 #define MAX_ROWS_PER_RESULT 15
 
 ////////////////////////////////////////////////////////////////////////////////
-//// Local Classes
-
-namespace {
-
-typedef AsyncExecuteStatements::ExecutionState ExecutionState;
-typedef AsyncExecuteStatements::StatementDataArray StatementDataArray;
-
-/**
- * Notifies a callback with a result set.
- */
-class CallbackResultNotifier : public Runnable
-{
-public:
-  CallbackResultNotifier(mozIStorageStatementCallback *aCallback,
-                         mozIStorageResultSet *aResults,
-                         AsyncExecuteStatements *aEventStatus) :
-      Runnable("storage::CallbackResultNotifier")
-    , mCallback(aCallback)
-    , mResults(aResults)
-    , mEventStatus(aEventStatus)
-  {
-  }
-
-  NS_IMETHOD Run() override
-  {
-    NS_ASSERTION(mCallback, "Trying to notify about results without a callback!");
-
-    if (mEventStatus->shouldNotify()) {
-      // Hold a strong reference to the callback while notifying it, so that if
-      // it spins the event loop, the callback won't be released and freed out
-      // from under us.
-      nsCOMPtr<mozIStorageStatementCallback> callback = mCallback;
-
-      (void)callback->HandleResult(mResults);
-    }
-
-    return NS_OK;
-  }
-
-private:
-  mozIStorageStatementCallback *mCallback;
-  nsCOMPtr<mozIStorageResultSet> mResults;
-  RefPtr<AsyncExecuteStatements> mEventStatus;
-};
-
-/**
- * Notifies the calling thread that an error has occurred.
- */
-class ErrorNotifier : public Runnable
-{
-public:
-  ErrorNotifier(mozIStorageStatementCallback *aCallback,
-                mozIStorageError *aErrorObj,
-                AsyncExecuteStatements *aEventStatus) :
-      Runnable("storage::ErrorNotifier")
-    , mCallback(aCallback)
-    , mErrorObj(aErrorObj)
-    , mEventStatus(aEventStatus)
-  {
-  }
-
-  NS_IMETHOD Run() override
-  {
-    if (mEventStatus->shouldNotify() && mCallback) {
-      // Hold a strong reference to the callback while notifying it, so that if
-      // it spins the event loop, the callback won't be released and freed out
-      // from under us.
-      nsCOMPtr<mozIStorageStatementCallback> callback = mCallback;
-
-      (void)callback->HandleError(mErrorObj);
-    }
-
-    return NS_OK;
-  }
-
-private:
-  mozIStorageStatementCallback *mCallback;
-  nsCOMPtr<mozIStorageError> mErrorObj;
-  RefPtr<AsyncExecuteStatements> mEventStatus;
-};
-
-/**
- * Notifies the calling thread that the statement has finished executing.  Takes
- * ownership of the StatementData so it is released on the proper thread.
- */
-class CompletionNotifier : public Runnable
-{
-public:
-  /**
-   * This takes ownership of the callback and the StatementData.  They are
-   * released on the thread this is dispatched to (which should always be the
-   * calling thread).
-   */
-  CompletionNotifier(already_AddRefed<mozIStorageStatementCallback> aCallback,
-                     ExecutionState aReason)
-    : Runnable("storage::CompletionNotifier")
-    , mCallback(aCallback)
-    , mReason(aReason)
-  {
-  }
-
-  NS_IMETHOD Run() override
-  {
-    if (mCallback) {
-      (void)mCallback->HandleCompletion(mReason);
-    }
-
-    return NS_OK;
-  }
-
-private:
-  RefPtr<mozIStorageStatementCallback> mCallback;
-  ExecutionState mReason;
-};
-
-} // namespace
-
-////////////////////////////////////////////////////////////////////////////////
 //// AsyncExecuteStatements
 
 /* static */
 nsresult
 AsyncExecuteStatements::execute(StatementDataArray &aStatements,
                                 Connection *aConnection,
                                 sqlite3 *aNativeConnection,
                                 mozIStorageStatementCallback *aCallback,
@@ -215,16 +97,20 @@ AsyncExecuteStatements::AsyncExecuteStat
   (void)mStatements.SwapElements(aStatements);
   NS_ASSERTION(mStatements.Length(), "We weren't given any statements!");
 }
 
 AsyncExecuteStatements::~AsyncExecuteStatements()
 {
   MOZ_ASSERT(!mCallback, "Never called the Completion callback!");
   MOZ_ASSERT(!mHasTransaction, "There should be no transaction at this point");
+  if (mCallback) {
+    NS_ProxyRelease("AsyncExecuteStatements::mCallback", mCallingThread,
+                    mCallback.forget());
+  }
 }
 
 bool
 AsyncExecuteStatements::shouldNotify()
 {
 #ifdef DEBUG
   mMutex.AssertNotCurrentThreadOwns();
 
@@ -252,17 +138,17 @@ AsyncExecuteStatements::bindExecuteAndPr
   BindingParamsArray *paramsArray(aData);
 
   // Iterate through all of our parameters, bind them, and execute.
   bool continueProcessing = true;
   BindingParamsArray::iterator itr = paramsArray->begin();
   BindingParamsArray::iterator end = paramsArray->end();
   while (itr != end && continueProcessing) {
     // Bind the data to our statement.
-    nsCOMPtr<IStorageBindingParamsInternal> bindingInternal = 
+    nsCOMPtr<IStorageBindingParamsInternal> bindingInternal =
       do_QueryInterface(*itr);
     nsCOMPtr<mozIStorageError> error = bindingInternal->bind(aStatement);
     if (error) {
       // Set our error state.
       mState = ERROR;
 
       // And notify.
       (void)notifyError(error);
@@ -459,25 +345,36 @@ AsyncExecuteStatements::notifyComplete()
     else {
       DebugOnly<nsresult> rv =
         mConnection->rollbackTransactionInternal(mNativeConnection);
       NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Transaction failed to rollback");
     }
     mHasTransaction = false;
   }
 
-  // Always generate a completion notification; it is what guarantees that our
-  // destruction does not happen here on the async thread.
-  RefPtr<CompletionNotifier> completionEvent =
-    new CompletionNotifier(mCallback.forget(), mState);
-  // We no longer own mCallback (the CompletionNotifier takes ownership).
-  // Make sure we don't delete the event on this thread, since it has
-  // other-thread-only members.
-  (void)mCallingThread->Dispatch(completionEvent.forget(), NS_DISPATCH_NORMAL);
+  // This will take ownership of mCallback and make sure its destruction will
+  // happen on the owner thread.
+  Unused << mCallingThread->Dispatch(
+    NewRunnableMethod(this, &AsyncExecuteStatements::notifyCompleteOnCallingThread),
+    NS_DISPATCH_NORMAL);
+
+  return NS_OK;
+}
 
+nsresult
+AsyncExecuteStatements::notifyCompleteOnCallingThread() {
+  MOZ_ASSERT(mCallingThread->IsOnCurrentThread());
+  // Take ownership of mCallback and responsibility for freeing it when we
+  // release it.  Any notifyResultsOnCallingThread and notifyErrorOnCallingThread
+  // calls on the stack spinning the event loop have guaranteed their safety by
+  // creating their own strong reference before invoking the callback.
+  nsCOMPtr<mozIStorageStatementCallback> callback = mCallback.forget();
+  if (callback) {
+    Unused << callback->HandleCompletion(mState);
+  }
   return NS_OK;
 }
 
 nsresult
 AsyncExecuteStatements::notifyError(int32_t aErrorCode,
                                     const char *aMessage)
 {
   mMutex.AssertNotCurrentThreadOwns();
@@ -496,43 +393,65 @@ nsresult
 AsyncExecuteStatements::notifyError(mozIStorageError *aError)
 {
   mMutex.AssertNotCurrentThreadOwns();
   mDBMutex.assertNotCurrentThreadOwns();
 
   if (!mCallback)
     return NS_OK;
 
-  RefPtr<ErrorNotifier> notifier =
-    new ErrorNotifier(mCallback, aError, this);
-  NS_ENSURE_TRUE(notifier, NS_ERROR_OUT_OF_MEMORY);
+  Unused << mCallingThread->Dispatch(
+    NewRunnableMethod<nsCOMPtr<mozIStorageError>>(this, &AsyncExecuteStatements::notifyErrorOnCallingThread, aError),
+    NS_DISPATCH_NORMAL);
+
+  return NS_OK;
+}
 
-  // Make sure we don't delete the event on this thread, since it has
-  // other-thread-only members.
-  return mCallingThread->Dispatch(notifier.forget(), NS_DISPATCH_NORMAL);
+nsresult
+AsyncExecuteStatements::notifyErrorOnCallingThread(mozIStorageError *aError) {
+  MOZ_ASSERT(mCallingThread->IsOnCurrentThread());
+  // Acquire our own strong reference so that if the callback spins a nested
+  // event loop and notifyCompleteOnCallingThread is executed, forgetting
+  // mCallback, we still have a valid/strong reference that won't be freed until
+  // we exit.
+  nsCOMPtr<mozIStorageStatementCallback> callback = mCallback;
+  if (shouldNotify() && callback) {
+    Unused << callback->HandleError(aError);
+  }
+  return NS_OK;
 }
 
 nsresult
 AsyncExecuteStatements::notifyResults()
 {
   mMutex.AssertNotCurrentThreadOwns();
-  NS_ASSERTION(mCallback, "notifyResults called without a callback!");
+  MOZ_ASSERT(mCallback, "notifyResults called without a callback!");
 
-  RefPtr<CallbackResultNotifier> notifier =
-    new CallbackResultNotifier(mCallback, mResultSet, this);
-  NS_ENSURE_TRUE(notifier, NS_ERROR_OUT_OF_MEMORY);
+  // This takes ownership of mResultSet, a new one will be generated in
+  // buildAndNotifyResults() when further results will arrive.
+  Unused << mCallingThread->Dispatch(
+    NewRunnableMethod<RefPtr<ResultSet>>(this, &AsyncExecuteStatements::notifyResultsOnCallingThread, mResultSet.forget()),
+    NS_DISPATCH_NORMAL);
+
+  return NS_OK;
+}
 
-  // Make sure we don't delete the event on this thread, since it has
-  // other-thread-only members.
-  nsresult rv = mCallingThread->Dispatch(notifier.forget(), NS_DISPATCH_NORMAL);
-  if (NS_SUCCEEDED(rv)) {
-    // it may be freed here or on the CallingThread
-    mResultSet = nullptr; // we no longer own it on success
+nsresult
+AsyncExecuteStatements::notifyResultsOnCallingThread(ResultSet *aResultSet)
+{
+  MOZ_ASSERT(mCallingThread->IsOnCurrentThread());
+  // Acquire our own strong reference so that if the callback spins a nested
+  // event loop and notifyCompleteOnCallingThread is executed, forgetting
+  // mCallback, we still have a valid/strong reference that won't be freed until
+  // we exit.
+  nsCOMPtr<mozIStorageStatementCallback> callback = mCallback;
+  if (shouldNotify() && callback) {
+    Unused << callback->HandleResult(aResultSet);
   }
-  return rv;
+  return NS_OK;
 }
 
 NS_IMPL_ISUPPORTS(
   AsyncExecuteStatements,
   nsIRunnable,
   mozIStoragePendingStatement
 )
 
--- a/storage/mozStorageAsyncStatementExecution.h
+++ b/storage/mozStorageAsyncStatementExecution.h
@@ -77,16 +77,24 @@ public:
    * should run or not.
    *
    * @pre mMutex is not held
    *
    * @returns true if the event should notify still, false otherwise.
    */
   bool shouldNotify();
 
+  /**
+   * Used by notifyComplete(), notifyError() and notifyResults() to notify on
+   * the calling thread.
+   */
+  nsresult notifyCompleteOnCallingThread();
+  nsresult notifyErrorOnCallingThread(mozIStorageError *aError);
+  nsresult notifyResultsOnCallingThread(ResultSet *aResultSet);
+
 private:
   AsyncExecuteStatements(StatementDataArray &aStatements,
                          Connection *aConnection,
                          sqlite3 *aNativeConnection,
                          mozIStorageStatementCallback *aCallback);
   ~AsyncExecuteStatements();
 
   /**
@@ -184,17 +192,17 @@ private:
 
   StatementDataArray mStatements;
   RefPtr<Connection> mConnection;
   sqlite3 *mNativeConnection;
   bool mHasTransaction;
   // Note, this may not be a threadsafe object - never addref/release off
   // the calling thread.  We take a reference when this is created, and
   // release it in the CompletionNotifier::Run() call back to this thread.
-  RefPtr<mozIStorageStatementCallback> mCallback;
+  nsCOMPtr<mozIStorageStatementCallback> mCallback;
   nsCOMPtr<nsIThread> mCallingThread;
   RefPtr<ResultSet> mResultSet;
 
   /**
    * The maximum amount of time we want to wait between results.  Defined by
    * MAX_MILLISECONDS_BETWEEN_RESULTS and set at construction.
    */
   const TimeDuration mMaxWait;