Bug 1533789 - LSRequestBase::Finish is not called in some edge cases; r=asuth
authorJan Varga <jan.varga@gmail.com>
Sun, 24 Mar 2019 15:02:05 +0100
changeset 524895 d1bf0d89acc3560fdf12785ebcdd342c0a2c0ffd
parent 524780 482787f1c704049f287b922f6da05fedaec1ba1e
child 524896 8b7ef93b6cb4c55de687e0d2181bb39f49d477cc
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersasuth
bugs1533789
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 1533789 - LSRequestBase::Finish is not called in some edge cases; r=asuth Differential Revision: https://phabricator.services.mozilla.com/D24649
dom/localstorage/ActorsParent.cpp
--- a/dom/localstorage/ActorsParent.cpp
+++ b/dom/localstorage/ActorsParent.cpp
@@ -2140,16 +2140,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;
@@ -2164,16 +2165,18 @@ class LSRequestBase : public DatastoreOp
 
   void LogState();
 
   virtual void LogNestedState() {}
 
  private:
   void SendReadyMessage();
 
+  void Finish();
+
   void SendResults();
 
  protected:
   // Common nsIRunnable implementation that subclasses may not override.
   NS_IMETHOD
   Run() final;
 
   // IPDL methods.
@@ -3879,22 +3882,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()) {
@@ -5612,17 +5615,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();
@@ -5697,23 +5702,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);
   }
@@ -5777,16 +5799,33 @@ 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();
 
   LogState();
 
   const char* crashOnCancel = PR_GetEnv("LSNG_CRASH_ON_CANCEL");
@@ -5799,25 +5838,18 @@ mozilla::ipc::IPCResult LSRequestBase::R
     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
  ******************************************************************************/