Bug 1586315 - LSNG: Finish RequestHelper when LSRequestChild actor is unexpectedly destroyed; r=asuth a=lizzard
authorJan Varga <jan.varga@gmail.com>
Sun, 06 Oct 2019 21:05:45 +0000
changeset 555597 502e28e5b6570ab1374b76aecef223c247c5aaa5
parent 555596 59ef27e0a17ac6805917c0c3e5a6711fd3bb74bc
child 555598 5f0abbc35dd139df5b7c66da12808e9c63ac47a8
push id2165
push userffxbld-merge
push dateMon, 14 Oct 2019 16:30:58 +0000
treeherdermozilla-release@0eae18af659f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersasuth, lizzard
bugs1586315
milestone70.0
Bug 1586315 - LSNG: Finish RequestHelper when LSRequestChild actor is unexpectedly destroyed; r=asuth a=lizzard If LSRequestChild actor is unexpectedly destroyed, the nested event loop runs until the timer fires. This patch improves that by calling the callback in LSRequestChild::ActorDestroyed which immediatelly finishes RequestHelper and finishes the nested event loop in the end too. Differential Revision: https://phabricator.services.mozilla.com/D48194
dom/localstorage/ActorsChild.cpp
dom/localstorage/ActorsChild.h
dom/localstorage/LSObject.cpp
dom/localstorage/LocalStorageManager2.cpp
--- a/dom/localstorage/ActorsChild.cpp
+++ b/dom/localstorage/ActorsChild.cpp
@@ -148,46 +148,65 @@ mozilla::ipc::IPCResult LSObserverChild:
 
   return IPC_OK();
 }
 
 /*******************************************************************************
  * LocalStorageRequestChild
  ******************************************************************************/
 
-LSRequestChild::LSRequestChild(LSRequestChildCallback* aCallback)
-    : mCallback(aCallback), mFinishing(false) {
+LSRequestChild::LSRequestChild() : mFinishing(false) {
   AssertIsOnOwningThread();
 
   MOZ_COUNT_CTOR(LSRequestChild);
 }
 
 LSRequestChild::~LSRequestChild() {
   AssertIsOnOwningThread();
+  MOZ_ASSERT(!mCallback);
 
   MOZ_COUNT_DTOR(LSRequestChild);
 }
 
+void LSRequestChild::SetCallback(LSRequestChildCallback* aCallback) {
+  AssertIsOnOwningThread();
+  MOZ_ASSERT(aCallback);
+  MOZ_ASSERT(!mCallback);
+  MOZ_ASSERT(Manager());
+
+  mCallback = aCallback;
+}
+
 bool LSRequestChild::Finishing() const {
   AssertIsOnOwningThread();
 
   return mFinishing;
 }
 
 void LSRequestChild::ActorDestroy(ActorDestroyReason aWhy) {
   AssertIsOnOwningThread();
+
+  if (mCallback) {
+    MOZ_ASSERT(aWhy != Deletion);
+
+    mCallback->OnResponse(NS_ERROR_FAILURE);
+
+    mCallback = nullptr;
+  }
 }
 
 mozilla::ipc::IPCResult LSRequestChild::Recv__delete__(
     const LSRequestResponse& aResponse) {
   AssertIsOnOwningThread();
   MOZ_ASSERT(mCallback);
 
   mCallback->OnResponse(aResponse);
 
+  mCallback = nullptr;
+
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult LSRequestChild::RecvReady() {
   AssertIsOnOwningThread();
 
   mFinishing = true;
 
@@ -197,41 +216,59 @@ mozilla::ipc::IPCResult LSRequestChild::
 
   return IPC_OK();
 }
 
 /*******************************************************************************
  * LSSimpleRequestChild
  ******************************************************************************/
 
-LSSimpleRequestChild::LSSimpleRequestChild(
-    LSSimpleRequestChildCallback* aCallback)
-    : mCallback(aCallback) {
+LSSimpleRequestChild::LSSimpleRequestChild() {
   AssertIsOnOwningThread();
-  MOZ_ASSERT(aCallback);
 
   MOZ_COUNT_CTOR(LSSimpleRequestChild);
 }
 
 LSSimpleRequestChild::~LSSimpleRequestChild() {
   AssertIsOnOwningThread();
+  MOZ_ASSERT(!mCallback);
 
   MOZ_COUNT_DTOR(LSSimpleRequestChild);
 }
 
+void LSSimpleRequestChild::SetCallback(LSSimpleRequestChildCallback* aCallback) {
+  AssertIsOnOwningThread();
+  MOZ_ASSERT(aCallback);
+  MOZ_ASSERT(!mCallback);
+  MOZ_ASSERT(Manager());
+
+  mCallback = aCallback;
+}
+
 void LSSimpleRequestChild::ActorDestroy(ActorDestroyReason aWhy) {
   AssertIsOnOwningThread();
+
+  if (mCallback) {
+    MOZ_ASSERT(aWhy != Deletion);
+
+    mCallback->OnResponse(NS_ERROR_FAILURE);
+
+    mCallback = nullptr;
+  }
 }
 
 mozilla::ipc::IPCResult LSSimpleRequestChild::Recv__delete__(
     const LSSimpleRequestResponse& aResponse) {
   AssertIsOnOwningThread();
+  MOZ_ASSERT(mCallback);
 
   mCallback->OnResponse(aResponse);
 
+  mCallback = nullptr;
+
   return IPC_OK();
 }
 
 /*******************************************************************************
  * LSSnapshotChild
  ******************************************************************************/
 
 LSSnapshotChild::LSSnapshotChild(LSSnapshot* aSnapshot) : mSnapshot(aSnapshot) {
--- a/dom/localstorage/ActorsChild.h
+++ b/dom/localstorage/ActorsChild.h
@@ -153,21 +153,23 @@ class LSRequestChild final : public PBac
   void AssertIsOnOwningThread() const {
     NS_ASSERT_OWNINGTHREAD(LSReqeustChild);
   }
 
   bool Finishing() const;
 
  private:
   // Only created by LSObject.
-  explicit LSRequestChild(LSRequestChildCallback* aCallback);
+  LSRequestChild();
 
   // Only destroyed by mozilla::ipc::BackgroundChildImpl.
   ~LSRequestChild();
 
+  void SetCallback(LSRequestChildCallback* aCallback);
+
   // IPDL methods are only called by IPDL.
   void ActorDestroy(ActorDestroyReason aWhy) override;
 
   mozilla::ipc::IPCResult Recv__delete__(
       const LSRequestResponse& aResponse) override;
 
   mozilla::ipc::IPCResult RecvReady() override;
 };
@@ -200,17 +202,19 @@ class LSSimpleRequestChild final : publi
 
  public:
   void AssertIsOnOwningThread() const {
     NS_ASSERT_OWNINGTHREAD(LSSimpleReqeustChild);
   }
 
  private:
   // Only created by LocalStorageManager2.
-  explicit LSSimpleRequestChild(LSSimpleRequestChildCallback* aCallback);
+  LSSimpleRequestChild();
+
+  void SetCallback(LSSimpleRequestChildCallback* aCallback);
 
   // Only destroyed by mozilla::ipc::BackgroundChildImpl.
   ~LSSimpleRequestChild();
 
   // IPDL methods are only called by IPDL.
   void ActorDestroy(ActorDestroyReason aWhy) override;
 
   mozilla::ipc::IPCResult Recv__delete__(
--- a/dom/localstorage/LSObject.cpp
+++ b/dom/localstorage/LSObject.cpp
@@ -471,19 +471,26 @@ LSRequestChild* LSObject::StartRequest(n
   PBackgroundChild* backgroundActor =
       XRE_IsParentProcess()
           ? BackgroundChild::GetOrCreateForCurrentThread(aMainEventTarget)
           : BackgroundChild::GetForCurrentThread();
   if (NS_WARN_IF(!backgroundActor)) {
     return nullptr;
   }
 
-  LSRequestChild* actor = new LSRequestChild(aCallback);
+  LSRequestChild* actor = new LSRequestChild();
 
-  backgroundActor->SendPBackgroundLSRequestConstructor(actor, aParams);
+  if (!backgroundActor->SendPBackgroundLSRequestConstructor(actor, aParams)) {
+    return nullptr;
+  }
+
+  // Must set callback after calling SendPBackgroundLSRequestConstructor since
+  // it can be called synchronously when SendPBackgroundLSRequestConstructor
+  // fails.
+  actor->SetCallback(aCallback);
 
   return actor;
 }
 
 Storage::StorageType LSObject::Type() const {
   AssertIsOnOwningThread();
 
   return eLocalStorage;
--- a/dom/localstorage/LocalStorageManager2.cpp
+++ b/dom/localstorage/LocalStorageManager2.cpp
@@ -353,45 +353,55 @@ LSRequestChild* LocalStorageManager2::St
   AssertIsOnDOMFileThread();
 
   PBackgroundChild* backgroundActor =
       BackgroundChild::GetOrCreateForCurrentThread();
   if (NS_WARN_IF(!backgroundActor)) {
     return nullptr;
   }
 
-  auto actor = new LSRequestChild(aCallback);
+  auto actor = new LSRequestChild();
 
   if (!backgroundActor->SendPBackgroundLSRequestConstructor(actor, aParams)) {
     return nullptr;
   }
 
+  // Must set callback after calling SendPBackgroundLSRequestConstructor since
+  // it can be called synchronously when SendPBackgroundLSRequestConstructor
+  // fails.
+  actor->SetCallback(aCallback);
+
   return actor;
 }
 
 nsresult LocalStorageManager2::StartSimpleRequest(
     Promise* aPromise, const LSSimpleRequestParams& aParams) {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(aPromise);
 
   PBackgroundChild* backgroundActor =
       BackgroundChild::GetOrCreateForCurrentThread();
   if (NS_WARN_IF(!backgroundActor)) {
     return NS_ERROR_FAILURE;
   }
 
-  RefPtr<SimpleRequestResolver> resolver = new SimpleRequestResolver(aPromise);
-
-  auto actor = new LSSimpleRequestChild(resolver);
+  auto actor = new LSSimpleRequestChild();
 
   if (!backgroundActor->SendPBackgroundLSSimpleRequestConstructor(actor,
                                                                   aParams)) {
     return NS_ERROR_FAILURE;
   }
 
+  RefPtr<SimpleRequestResolver> resolver = new SimpleRequestResolver(aPromise);
+
+  // Must set callback after calling SendPBackgroundLSRequestConstructor since
+  // it can be called synchronously when SendPBackgroundLSRequestConstructor
+  // fails.
+  actor->SetCallback(resolver);
+
   return NS_OK;
 }
 
 nsresult AsyncRequestHelper::Dispatch() {
   AssertIsOnOwningThread();
 
   nsCOMPtr<nsIEventTarget> domFileThread =
       IPCBlobInputStreamThread::GetOrCreate();