Bug 1356824 - Don't clear mWaitingFactoryOp without holding a self reference in (Open|Delete)DatabaseOp::NoteDatabaseClosed(). r=btseng
authorJan Varga <jan.varga@gmail.com>
Thu, 01 Jun 2017 13:08:52 -0400
changeset 361851 665f1887a112dac916caac5e659764a0f62db27b
parent 361850 71e0ff5a6c76a811fb7fcc0684578727754dab17
child 361852 15e32469eb048957223bac458140776a30251c6b
push id31945
push userryanvm@gmail.com
push dateThu, 01 Jun 2017 20:42:17 +0000
treeherdermozilla-central@15e32469eb04 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbtseng
bugs1356824
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 1356824 - Don't clear mWaitingFactoryOp without holding a self reference in (Open|Delete)DatabaseOp::NoteDatabaseClosed(). r=btseng Remove now useless self reference in OpenDatabaseOp::SendResults(). Add comments for how to call FactoryOp::Run() safely and also add comments to all the call sites. This prevents a self destruction while Run() (or a method that is called by Run()) is still being executed. Also fix TransactionDatabaseOperationBase which also calls Run() directly in NoteContinueReceived().
dom/indexedDB/ActorsParent.cpp
--- a/dom/indexedDB/ActorsParent.cpp
+++ b/dom/indexedDB/ActorsParent.cpp
@@ -7626,16 +7626,17 @@ protected:
   DoDatabaseWork() = 0;
 
   virtual nsresult
   BeginVersionChange() = 0;
 
   virtual nsresult
   DispatchToWorkThread() = 0;
 
+  // Should only be called by Run().
   virtual void
   SendResults() = 0;
 
   NS_DECL_ISUPPORTS_INHERITED
 
   // Common nsIRunnable implementation that subclasses may not override.
   NS_IMETHOD
   Run() final;
@@ -21574,16 +21575,21 @@ FactoryOp::NoteDatabaseBlocked(Database*
 
   if (sendBlockedEvent) {
     SendBlockedNotification();
   }
 }
 
 NS_IMPL_ISUPPORTS_INHERITED0(FactoryOp, DatabaseOperationBase)
 
+// Run() assumes that the caller holds a strong reference to the object that
+// can't be cleared while Run() is being executed.
+// So if you call Run() directly (as opposed to dispatching to an event queue)
+// you need to make sure there's such a reference.
+// See bug 1356824 for more details.
 NS_IMETHODIMP
 FactoryOp::Run()
 {
   nsresult rv;
 
   switch (mState) {
     case State::Initial:
       rv = Open();
@@ -21658,18 +21664,21 @@ FactoryOp::DirectoryLockAcquired(Directo
   mDirectoryLock = aLock;
 
   nsresult rv = DirectoryOpen();
   if (NS_WARN_IF(NS_FAILED(rv))) {
     if (NS_SUCCEEDED(mResultCode)) {
       mResultCode = rv;
     }
 
+    // The caller holds a strong reference to us, no need for a self reference
+    // before calling Run().
+
     mState = State::SendingResults;
-    SendResults();
+    MOZ_ALWAYS_SUCCEEDS(Run());
 
     return;
   }
 }
 
 void
 FactoryOp::DirectoryLockFailed()
 {
@@ -21677,18 +21686,21 @@ FactoryOp::DirectoryLockFailed()
   MOZ_ASSERT(mState == State::DirectoryOpenPending);
   MOZ_ASSERT(!mDirectoryLock);
 
   if (NS_SUCCEEDED(mResultCode)) {
     IDB_REPORT_INTERNAL_ERR();
     mResultCode = NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
   }
 
+  // The caller holds a strong reference to us, no need for a self reference
+  // before calling Run().
+
   mState = State::SendingResults;
-  SendResults();
+  MOZ_ALWAYS_SUCCEEDS(Run());
 }
 
 void
 FactoryOp::ActorDestroy(ActorDestroyReason aWhy)
 {
   AssertIsOnBackgroundThread();
 
   NoteActorDestroyed();
@@ -22470,33 +22482,42 @@ OpenDatabaseOp::NoteDatabaseClosed(Datab
   nsresult rv;
   if (actorDestroyed) {
     IDB_REPORT_INTERNAL_ERR();
     rv = NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
   } else {
     rv = NS_OK;
   }
 
+  // We are being called with an assuption that mWaitingFactoryOp holds a strong
+  // reference to us.
+  RefPtr<OpenDatabaseOp> kungFuDeathGrip;
+
   if (mMaybeBlockedDatabases.RemoveElement(aDatabase) &&
       mMaybeBlockedDatabases.IsEmpty()) {
     if (actorDestroyed) {
       DatabaseActorInfo* info;
       MOZ_ALWAYS_TRUE(gLiveDatabaseHashtable->Get(mDatabaseId, &info));
       MOZ_ASSERT(info->mWaitingFactoryOp == this);
+      kungFuDeathGrip =
+        static_cast<OpenDatabaseOp*>(info->mWaitingFactoryOp.get());
       info->mWaitingFactoryOp = nullptr;
     } else {
       WaitForTransactions();
     }
   }
 
   if (NS_WARN_IF(NS_FAILED(rv))) {
     if (NS_SUCCEEDED(mResultCode)) {
       mResultCode = rv;
     }
 
+    // A strong reference is held in kungFuDeathGrip, so it's safe to call Run()
+    // directly.
+
     mState = State::SendingResults;
     MOZ_ALWAYS_SUCCEEDS(Run());
   }
 }
 
 void
 OpenDatabaseOp::SendBlockedNotification()
 {
@@ -22606,27 +22627,25 @@ OpenDatabaseOp::SendResults()
 {
   AssertIsOnOwningThread();
   MOZ_ASSERT(mState == State::SendingResults);
   MOZ_ASSERT_IF(NS_SUCCEEDED(mResultCode), mMaybeBlockedDatabases.IsEmpty());
   MOZ_ASSERT_IF(NS_SUCCEEDED(mResultCode), !mVersionChangeTransaction);
 
   mMaybeBlockedDatabases.Clear();
 
-  // Only needed if we're being called from within NoteDatabaseDone() since this
-  // OpenDatabaseOp is only held alive by the gLiveDatabaseHashtable.
-  RefPtr<OpenDatabaseOp> kungFuDeathGrip;
-
   DatabaseActorInfo* info;
   if (gLiveDatabaseHashtable &&
       gLiveDatabaseHashtable->Get(mDatabaseId, &info) &&
       info->mWaitingFactoryOp) {
     MOZ_ASSERT(info->mWaitingFactoryOp == this);
-    kungFuDeathGrip =
-      static_cast<OpenDatabaseOp*>(info->mWaitingFactoryOp.get());
+    // SendResults() should only be called by Run() and Run() should only be
+    // called if there's a strong reference to the object that can't be cleared
+    // here, so it's safe to clear mWaitingFactoryOp without adding additional
+    // strong reference.
     info->mWaitingFactoryOp = nullptr;
   }
 
   if (mVersionChangeTransaction) {
     MOZ_ASSERT(NS_FAILED(mResultCode));
 
     mVersionChangeTransaction->Abort(mResultCode, /* aForce */ true);
     mVersionChangeTransaction = nullptr;
@@ -23290,33 +23309,42 @@ DeleteDatabaseOp::NoteDatabaseClosed(Dat
   nsresult rv;
   if (actorDestroyed) {
     IDB_REPORT_INTERNAL_ERR();
     rv = NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
   } else {
     rv = NS_OK;
   }
 
+  // We are being called with an assuption that mWaitingFactoryOp holds a strong
+  // reference to us.
+  RefPtr<OpenDatabaseOp> kungFuDeathGrip;
+
   if (mMaybeBlockedDatabases.RemoveElement(aDatabase) &&
       mMaybeBlockedDatabases.IsEmpty()) {
     if (actorDestroyed) {
       DatabaseActorInfo* info;
       MOZ_ALWAYS_TRUE(gLiveDatabaseHashtable->Get(mDatabaseId, &info));
       MOZ_ASSERT(info->mWaitingFactoryOp == this);
+      kungFuDeathGrip =
+        static_cast<OpenDatabaseOp*>(info->mWaitingFactoryOp.get());
       info->mWaitingFactoryOp = nullptr;
     } else {
       WaitForTransactions();
     }
   }
 
   if (NS_WARN_IF(NS_FAILED(rv))) {
     if (NS_SUCCEEDED(mResultCode)) {
       mResultCode = rv;
     }
 
+    // A strong reference is held in kungFuDeathGrip, so it's safe to call Run()
+    // directly.
+
     mState = State::SendingResults;
     MOZ_ALWAYS_SUCCEEDS(Run());
   }
 }
 
 void
 DeleteDatabaseOp::SendBlockedNotification()
 {
@@ -23632,16 +23660,18 @@ VersionChangeOp::RunOnOwningThread()
           }
 
           MOZ_ASSERT(!gLiveDatabaseHashtable->Get(deleteOp->mDatabaseId));
         }
       }
     }
   }
 
+  // We hold a strong ref to the deleteOp, so it's safe to call Run() directly.
+
   deleteOp->mState = State::SendingResults;
   MOZ_ALWAYS_SUCCEEDS(deleteOp->Run());
 
 #ifdef DEBUG
   // A bit hacky but the DeleteDatabaseOp::VersionChangeOp is not really a
   // normal database operation that is tied to an actor. Do this to make our
   // assertions happy.
   NoteActorDestroyed();
@@ -23846,16 +23876,21 @@ TransactionDatabaseOperationBase::SendPr
 void
 TransactionDatabaseOperationBase::NoteContinueReceived()
 {
   AssertIsOnOwningThread();
   MOZ_ASSERT(mInternalState == InternalState::WaitingForContinue);
 
   mInternalState = InternalState::SendingResults;
 
+  // This TransactionDatabaseOperationBase can only be held alive by the IPDL.
+  // Run() can end up with clearing that last reference. So we need to add
+  // a self reference here.
+  RefPtr<TransactionDatabaseOperationBase> kungFuDeathGrip = this;
+
   Unused << this->Run();
 }
 
 void
 TransactionDatabaseOperationBase::SendToConnectionPool()
 {
   AssertIsOnOwningThread();
   MOZ_ASSERT(mInternalState == InternalState::Initial);
@@ -23891,32 +23926,23 @@ void
 TransactionDatabaseOperationBase::SendPreprocessInfoOrResults(
                                                        bool aSendPreprocessInfo)
 {
   AssertIsOnOwningThread();
   MOZ_ASSERT(mInternalState == InternalState::SendingPreprocess ||
              mInternalState == InternalState::SendingResults);
   MOZ_ASSERT(mTransaction);
 
-  // Only needed if we're being called from within NoteContinueReceived() since
-  // this TransactionDatabaseOperationBase is only held alive by the IPDL.
-  // SendSuccessResult/SendFailureResult releases that last reference.
-  RefPtr<TransactionDatabaseOperationBase> kungFuDeathGrip;
-
   if (NS_WARN_IF(IsActorDestroyed())) {
     // Don't send any notifications if the actor was destroyed already.
     if (NS_SUCCEEDED(mResultCode)) {
       IDB_REPORT_INTERNAL_ERR();
       mResultCode = NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
     }
   } else {
-    if (!aSendPreprocessInfo) {
-      kungFuDeathGrip = this;
-    }
-
     if (mTransaction->IsInvalidated() || mTransaction->IsAborted()) {
       // Aborted transactions always see their requests fail with ABORT_ERR,
       // even if the request succeeded or failed with another error.
       mResultCode = NS_ERROR_DOM_INDEXEDDB_ABORT_ERR;
     } else if (NS_SUCCEEDED(mResultCode)) {
       if (aSendPreprocessInfo) {
         // This should not release the IPDL reference.
         mResultCode = SendPreprocessInfo();