Bug 1539393 - TransactionDatabaseOperationBase::ActorDestroy doesn't check if we are waiting for the Continue message; r=asuth
authorJan Varga <jan.varga@gmail.com>
Sat, 13 Apr 2019 16:52:29 +0200
changeset 469602 a2a538f6c3c90d5241946d7ab071231fcc46fa86
parent 469601 0fd463c6029003cf18b86390f561ba279f1ab09b
child 469603 34062ef2307ca687b8ae5077dd340ce34976c489
push id35875
push userccoroiu@mozilla.com
push dateTue, 16 Apr 2019 04:06:16 +0000
treeherdermozilla-central@a83cab75b00d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersasuth
bugs1539393
milestone68.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 1539393 - TransactionDatabaseOperationBase::ActorDestroy doesn't check if we are waiting for the Continue message; r=asuth Differential Revision: https://phabricator.services.mozilla.com/D27429
dom/indexedDB/ActorsParent.cpp
--- a/dom/indexedDB/ActorsParent.cpp
+++ b/dom/indexedDB/ActorsParent.cpp
@@ -5476,16 +5476,17 @@ class TransactionDatabaseOperationBase :
     WaitingForContinue,
     SendingResults,
     Completed
   };
 
   RefPtr<TransactionBase> mTransaction;
   const int64_t mTransactionLoggingSerialNumber;
   InternalState mInternalState;
+  bool mWaitingForContinue;
   const bool mTransactionIsAborted;
 
  public:
   void AssertIsOnConnectionThread() const
 #ifdef DEBUG
       ;
 #else
   {
@@ -5501,16 +5502,22 @@ class TransactionDatabaseOperationBase :
   void DispatchToConnectionPool();
 
   TransactionBase* Transaction() const {
     MOZ_ASSERT(mTransaction);
 
     return mTransaction;
   }
 
+  bool IsWaitingForContinue() const {
+    AssertIsOnOwningThread();
+
+    return mWaitingForContinue;
+  }
+
   void NoteContinueReceived();
 
   // May be overridden by subclasses if they need to perform work on the
   // background thread before being dispatched. Returning false will kill the
   // child actors and prevent dispatch.
   virtual bool Init(TransactionBase* aTransaction);
 
   // This callback will be called on the background thread before releasing the
@@ -21527,16 +21534,17 @@ nsresult DeleteDatabaseOp::VersionChange
 
 TransactionDatabaseOperationBase::TransactionDatabaseOperationBase(
     TransactionBase* aTransaction)
     : DatabaseOperationBase(aTransaction->GetLoggingInfo()->Id(),
                             aTransaction->GetLoggingInfo()->NextRequestSN()),
       mTransaction(aTransaction),
       mTransactionLoggingSerialNumber(aTransaction->LoggingSerialNumber()),
       mInternalState(InternalState::Initial),
+      mWaitingForContinue(false),
       mTransactionIsAborted(aTransaction->IsAborted()) {
   MOZ_ASSERT(aTransaction);
   MOZ_ASSERT(LoggingSerialNumber());
 }
 
 TransactionDatabaseOperationBase::TransactionDatabaseOperationBase(
     TransactionBase* aTransaction, uint64_t aLoggingSerialNumber)
     : DatabaseOperationBase(aTransaction->GetLoggingInfo()->Id(),
@@ -21671,16 +21679,18 @@ bool TransactionDatabaseOperationBase::H
 nsresult TransactionDatabaseOperationBase::SendPreprocessInfo() {
   return NS_OK;
 }
 
 void TransactionDatabaseOperationBase::NoteContinueReceived() {
   AssertIsOnOwningThread();
   MOZ_ASSERT(mInternalState == InternalState::WaitingForContinue);
 
+  mWaitingForContinue = false;
+
   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();
@@ -21753,16 +21763,18 @@ void TransactionDatabaseOperationBase::S
     if (!SendFailureResult(mResultCode)) {
       // Abort the transaction.
       mTransaction->Abort(mResultCode, /* aForce */ false);
     }
   }
 
   if (aSendPreprocessInfo && NS_SUCCEEDED(mResultCode)) {
     mInternalState = InternalState::WaitingForContinue;
+
+    mWaitingForContinue = true;
   } else {
     if (mLoggingSerialNumber) {
       mTransaction->NoteFinishedRequest();
     }
 
     Cleanup();
 
     mInternalState = InternalState::Completed;
@@ -23721,16 +23733,34 @@ void NormalTransactionOp::Cleanup() {
 
   TransactionDatabaseOperationBase::Cleanup();
 }
 
 void NormalTransactionOp::ActorDestroy(ActorDestroyReason aWhy) {
   AssertIsOnOwningThread();
 
   NoteActorDestroyed();
+
+  // Assume ActorDestroy can happen at any time, so we can't probe the current
+  // state since mInternalState can be modified on any thread (only one thread
+  // at a time based on the state machine).
+  // However we can use mWaitingForContinue which is only touched on the owning
+  // thread.  If mWaitingForContinue is true, we can also modify mInternalState
+  // since we are guaranteed that there are no pending runnables which would
+  // probe mInternalState to decide what code needs to run (there shouldn't be
+  // any running runnables on other threads either).
+
+  if (IsWaitingForContinue()) {
+    NoteContinueReceived();
+  }
+
+  // We don't have to handle the case when mWaitingForContinue is not true since
+  // it means that either nothing has been initialized yet, so nothing to
+  // cleanup or there are pending runnables that will detect that the actor has
+  // been destroyed and cleanup accordingly.
 }
 
 mozilla::ipc::IPCResult NormalTransactionOp::RecvContinue(
     const PreprocessResponse& aResponse) {
   AssertIsOnOwningThread();
 
   switch (aResponse.type()) {
     case PreprocessResponse::Tnsresult: