Bug 1350752: Cleanup async mozStorage callback management r=layzell
authorRandell Jesup <rjesup@jesup.org>
Tue, 28 Mar 2017 06:54:02 -0400
changeset 400584 9fd2650f5db0f9c28070fb570cc987045d4db9f3
parent 400583 4fd1e9b734b931691a5a849482bff0b05fe1ef4f
child 400585 13eb53195041d2adcd3f7ddf467480b1a94d1f53
push id1490
push usermtabara@mozilla.com
push dateMon, 31 Jul 2017 14:08:16 +0000
treeherdermozilla-release@70e32e6bf15e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslayzell
bugs1350752
milestone55.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 1350752: Cleanup async mozStorage callback management r=layzell MozReview-Commit-ID: 6LS1Strqu6s
storage/mozStorageAsyncStatementExecution.cpp
storage/mozStorageAsyncStatementExecution.h
--- a/storage/mozStorageAsyncStatementExecution.cpp
+++ b/storage/mozStorageAsyncStatementExecution.cpp
@@ -128,36 +128,35 @@ private:
 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(mozIStorageStatementCallback *aCallback,
+  CompletionNotifier(already_AddRefed<mozIStorageStatementCallback> aCallback,
                      ExecutionState aReason)
     : Runnable("storage::CompletionNotifier")
     , mCallback(aCallback)
     , mReason(aReason)
   {
   }
 
   NS_IMETHOD Run() override
   {
     if (mCallback) {
       (void)mCallback->HandleCompletion(mReason);
-      NS_RELEASE(mCallback);
     }
 
     return NS_OK;
   }
 
 private:
-  mozIStorageStatementCallback *mCallback;
+  RefPtr<mozIStorageStatementCallback> mCallback;
   ExecutionState mReason;
 };
 
 } // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 //// AsyncExecuteStatements
 
@@ -206,25 +205,25 @@ AsyncExecuteStatements::AsyncExecuteStat
 , mCallback(aCallback)
 , mCallingThread(::do_GetCurrentThread())
 , mMaxWait(TimeDuration::FromMilliseconds(MAX_MILLISECONDS_BETWEEN_RESULTS))
 , mIntervalStart(TimeStamp::Now())
 , mState(PENDING)
 , mCancelRequested(false)
 , mMutex(aConnection->sharedAsyncExecutionMutex)
 , mDBMutex(aConnection->sharedDBMutex)
-  , mRequestStartDate(TimeStamp::Now())
+, mRequestStartDate(TimeStamp::Now())
 {
   (void)mStatements.SwapElements(aStatements);
   NS_ASSERTION(mStatements.Length(), "We weren't given any statements!");
-  NS_IF_ADDREF(mCallback);
 }
 
 AsyncExecuteStatements::~AsyncExecuteStatements()
 {
+  MOZ_ASSERT(!mCallback, "Never called the Completion callback!");
   MOZ_ASSERT(!mHasTransaction, "There should be no transaction at this point");
 }
 
 bool
 AsyncExecuteStatements::shouldNotify()
 {
 #ifdef DEBUG
   mMutex.AssertNotCurrentThreadOwns();
@@ -463,22 +462,21 @@ AsyncExecuteStatements::notifyComplete()
       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, mState);
-
+    new CompletionNotifier(mCallback.forget(), mState);
   // We no longer own mCallback (the CompletionNotifier takes ownership).
-  mCallback = nullptr;
-
-  (void)mCallingThread->Dispatch(completionEvent, NS_DISPATCH_NORMAL);
+  // 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);
 
   return NS_OK;
 }
 
 nsresult
 AsyncExecuteStatements::notifyError(int32_t aErrorCode,
                                     const char *aMessage)
 {
@@ -502,32 +500,38 @@ AsyncExecuteStatements::notifyError(mozI
 
   if (!mCallback)
     return NS_OK;
 
   RefPtr<ErrorNotifier> notifier =
     new ErrorNotifier(mCallback, aError, this);
   NS_ENSURE_TRUE(notifier, NS_ERROR_OUT_OF_MEMORY);
 
-  return mCallingThread->Dispatch(notifier, NS_DISPATCH_NORMAL);
+  // 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::notifyResults()
 {
   mMutex.AssertNotCurrentThreadOwns();
   NS_ASSERTION(mCallback, "notifyResults called without a callback!");
 
   RefPtr<CallbackResultNotifier> notifier =
     new CallbackResultNotifier(mCallback, mResultSet, this);
   NS_ENSURE_TRUE(notifier, NS_ERROR_OUT_OF_MEMORY);
 
-  nsresult rv = mCallingThread->Dispatch(notifier, NS_DISPATCH_NORMAL);
-  if (NS_SUCCEEDED(rv))
+  // 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
+  }
   return rv;
 }
 
 NS_IMPL_ISUPPORTS(
   AsyncExecuteStatements,
   nsIRunnable,
   mozIStoragePendingStatement
 )
--- a/storage/mozStorageAsyncStatementExecution.h
+++ b/storage/mozStorageAsyncStatementExecution.h
@@ -181,17 +181,20 @@ private:
    * @return true if an explicit transaction is needed, false otherwise.
    */
   bool statementsNeedTransaction();
 
   StatementDataArray mStatements;
   RefPtr<Connection> mConnection;
   sqlite3 *mNativeConnection;
   bool mHasTransaction;
-  mozIStorageStatementCallback *mCallback;
+  // 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<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;