Bug 1356824 - Don't clear mWaitingFactoryOp without holding a self reference in (Open|Delete)DatabaseOp::NoteDatabaseClosed(). r=btseng, a=ritu FIREFOX_54_0b13_BUILD1 FIREFOX_54_0b13_RELEASE
authorJan Varga <jan.varga@gmail.com>
Thu, 01 Jun 2017 13:08:52 -0400
changeset 396472 cb4a4275b365b126ae56b1df5521756b693d8ff9
parent 396471 a85ff8f8399de4eea50c9f3aece921814c98d016
child 396473 4f0bd6c5b03dafa6025621da30e1bc87008d027b
push id1468
push userasasaki@mozilla.com
push dateMon, 05 Jun 2017 19:31:07 +0000
treeherdermozilla-release@0641fc6ee9d1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbtseng, ritu
bugs1356824
milestone54.0
Bug 1356824 - Don't clear mWaitingFactoryOp without holding a self reference in (Open|Delete)DatabaseOp::NoteDatabaseClosed(). r=btseng, a=ritu 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
@@ -7559,16 +7559,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;
@@ -21352,16 +21353,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();
@@ -21436,18 +21442,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()
 {
@@ -21455,18 +21464,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();
@@ -22249,33 +22261,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()
 {
@@ -22385,27 +22406,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;
@@ -23069,33 +23088,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()
 {
@@ -23411,16 +23439,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();
@@ -23625,16 +23655,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);
@@ -23670,32 +23705,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();