Bug 1533789 - LSRequestBase::Finish is not called in some edge cases; r=asuth a=pascalc
authorJan Varga <jan.varga@gmail.com>
Sun, 24 Mar 2019 15:02:05 +0100
changeset 525745 33463766bc34c08473b2acb4a05575174ac868f7
parent 525744 c9e5b98d97c973f6d55cea531c776c7dc15c987a
child 525746 56567a8607b5cf0e804c737a9fcd03a422e3fdf4
push id2032
push userffxbld-merge
push dateMon, 13 May 2019 09:36:57 +0000
treeherdermozilla-release@455c1065dcbe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersasuth, pascalc
bugs1533789
milestone67.0
Bug 1533789 - LSRequestBase::Finish is not called in some edge cases; r=asuth a=pascalc Differential Revision: https://phabricator.services.mozilla.com/D24649
dom/localstorage/ActorsParent.cpp
--- a/dom/localstorage/ActorsParent.cpp
+++ b/dom/localstorage/ActorsParent.cpp
@@ -2136,16 +2136,17 @@ class LSRequestBase : public DatastoreOp
     SendingResults,
 
     // All done.
     Completed
   };
 
   nsCOMPtr<nsIEventTarget> mMainEventTarget;
   State mState;
+  bool mWaitingForFinish;
 
  public:
   explicit LSRequestBase(nsIEventTarget* aMainEventTarget);
 
   void Dispatch();
 
  protected:
   ~LSRequestBase() override;
@@ -2156,16 +2157,18 @@ class LSRequestBase : public DatastoreOp
 
   virtual void GetResponse(LSRequestResponse& aResponse) = 0;
 
   virtual void Cleanup() {}
 
  private:
   void SendReadyMessage();
 
+  void Finish();
+
   void SendResults();
 
  protected:
   // Common nsIRunnable implementation that subclasses may not override.
   NS_IMETHOD
   Run() final;
 
   // IPDL methods.
@@ -3868,22 +3871,22 @@ void ConnectionDatastoreOperationBase::R
 }
 
 void ConnectionDatastoreOperationBase::RunOnOwningThread() {
   AssertIsOnOwningThread();
   MOZ_ASSERT(mConnection);
 
   if (!MayProceed()) {
     MaybeSetFailureCode(NS_ERROR_FAILURE);
+  }
+
+  if (NS_SUCCEEDED(ResultCode())) {
+    OnSuccess();
   } else {
-    if (NS_SUCCEEDED(ResultCode())) {
-      OnSuccess();
-    } else {
-      OnFailure(ResultCode());
-    }
+    OnFailure(ResultCode());
   }
 
   Cleanup();
 }
 
 NS_IMETHODIMP
 ConnectionDatastoreOperationBase::Run() {
   if (IsOnConnectionThread()) {
@@ -5601,17 +5604,19 @@ mozilla::ipc::IPCResult Observer::RecvDe
   return IPC_OK();
 }
 
 /*******************************************************************************
  * LSRequestBase
  ******************************************************************************/
 
 LSRequestBase::LSRequestBase(nsIEventTarget* aMainEventTarget)
-    : mMainEventTarget(aMainEventTarget), mState(State::Initial) {}
+    : mMainEventTarget(aMainEventTarget),
+      mState(State::Initial),
+      mWaitingForFinish(false) {}
 
 LSRequestBase::~LSRequestBase() {
   MOZ_ASSERT_IF(MayProceedOnNonOwningThread(),
                 mState == State::Initial || mState == State::Completed);
 }
 
 void LSRequestBase::Dispatch() {
   AssertIsOnOwningThread();
@@ -5631,23 +5636,40 @@ void LSRequestBase::SendReadyMessage() {
       !MayProceed()) {
     MaybeSetFailureCode(NS_ERROR_FAILURE);
   }
 
   if (MayProceed()) {
     Unused << SendReady();
 
     mState = State::WaitingForFinish;
+
+    mWaitingForFinish = true;
   } else {
     Cleanup();
 
     mState = State::Completed;
   }
 }
 
+void LSRequestBase::Finish() {
+  AssertIsOnOwningThread();
+  MOZ_ASSERT(mState == State::WaitingForFinish);
+
+  mState = State::SendingResults;
+
+  mWaitingForFinish = false;
+
+  // This LSRequestBase 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<LSRequestBase> kungFuDeathGrip = this;
+
+  MOZ_ALWAYS_SUCCEEDS(this->Run());
+}
+
 void LSRequestBase::SendResults() {
   AssertIsOnOwningThread();
   MOZ_ASSERT(mState == State::SendingResults);
 
   if (NS_WARN_IF(QuotaClient::IsShuttingDownOnBackgroundThread()) ||
       !MayProceed()) {
     MaybeSetFailureCode(NS_ERROR_FAILURE);
   }
@@ -5711,40 +5733,50 @@ LSRequestBase::Run() {
 
   return NS_OK;
 }
 
 void LSRequestBase::ActorDestroy(ActorDestroyReason aWhy) {
   AssertIsOnOwningThread();
 
   NoteComplete();
+
+  // Assume ActorDestroy can happen at any time, so we can't probe the current
+  // state since mState can be modified on any thread (only one thread at a time
+  // based on the state machine).  However we can use mWaitingForFinish which is
+  // only touched on the owning thread.  If mWaitingForFinisg is true, we can
+  // also modify mState since we are guaranteed that there are no pending
+  // runnables which would probe mState to decide what code needs to run (there
+  // shouldn't be any running runnables on other threads either).
+
+  if (mWaitingForFinish) {
+    Finish();
+  }
+
+  // We don't have to handle the case when mWaitingForFinish 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 LSRequestBase::RecvCancel() {
   AssertIsOnOwningThread();
 
   IProtocol* mgr = Manager();
   if (!PBackgroundLSRequestParent::Send__delete__(this, NS_ERROR_FAILURE)) {
     return IPC_FAIL_NO_REASON(mgr);
   }
 
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult LSRequestBase::RecvFinish() {
   AssertIsOnOwningThread();
-  MOZ_ASSERT(mState == State::WaitingForFinish);
-
-  mState = State::SendingResults;
-
-  // This LSRequestBase 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<LSRequestBase> kungFuDeathGrip = this;
-
-  MOZ_ALWAYS_SUCCEEDS(this->Run());
+
+  Finish();
 
   return IPC_OK();
 }
 
 /*******************************************************************************
  * PrepareDatastoreOp
  ******************************************************************************/