Bug 1781395 - Prevent deletion of ServiceWorkerRegistration objects while async IPC calls are in flight. r=dom-worker-reviewers,jstutte

Ensure that all async IPC calls made on ServiceWorkerRegistration (which
includes calls made on behalf of NavigationPreloadManager) hold a strong
reference to the ServiceWorkerRegistration for the duration of the async
IPC calls so that the ServiceWorkerRegistration is not cycle-collected
if the caller is only then-ing on the promise and has not retained a strong
reference to the ServiceWorkerRegistration.

This is accomplished by holding a strong self-reference in the IPC-resolved
callback.

In order to ensure that these strong references are properly cleaned up on
the main thread, we:
- Add a call to Shutdown during DisconnectFromOwner to shutdown IPC when the
  global is being torn down.
- Modernize PServiceWorkerRegistration to allow the child to directly invoke
  `Send__delete__` rather than having to ask the parent to call that method
  via `SendTeardown`.  This ensures the actor is destroyed in a more timely
  fashion and generally makes things easier to reason about.
- Eliminate use of boolean flags that are redundant with what `CanSend()`
  indicates (as long as we call it on the thread that owns the actor).

The Shutdown call isn't technically needed on the worker thread because worker
shutdown tears down PBackground and thereby all PServiceWorkerRegistration
instances, although these cleanups make that cleanup happen in a more timely
fashion.  Additionally, ServiceWorkerRegistrationChild does use IPCWorkerRef
with the callback triggering eager shutdown of the actor on workers
(notify(Canceling) can occur prior to DisconnectFromOwner when the interrupt
is used), but the more notable impact is IPCWorkerRef causes the actor to
not be counted as something that should prevent GC of the worker (and these
changes do not alter that).

Differential Revision: https://phabricator.services.mozilla.com/D242885

dom/serviceworkers/PServiceWorkerRegistration.ipdl

@@ -20,8 +20,6 @@ protocol PServiceWorkerRegistration
   manager PBackground;
 
 parent:
-  async Teardown();
-
   async Unregister() returns (bool aSuccess, CopyableErrorResult aRv);
   async Update(nsCString aNewestWorkerScriptUrl) returns (
     IPCServiceWorkerRegistrationDescriptorOrCopyableErrorResult aResult);
@@ -33,10 +31,11 @@ parent:
   async GetNavigationPreloadState() returns (IPCNavigationPreloadState? aState);
 
 child:
-  async __delete__();
-
   async UpdateState(IPCServiceWorkerRegistrationDescriptor aDescriptor);
   async FireUpdateFound();
+
+both:
+  async __delete__();
 };
 
 } // namespace dom

dom/serviceworkers/ServiceWorkerRegistration.cpp

@@ -133,6 +133,7 @@ ServiceWorkerRegistration::CreateForWorker(
 
 void ServiceWorkerRegistration::DisconnectFromOwner() {
   DOMEventTargetHelper::DisconnectFromOwner();
+  Shutdown();
 }
 
 void ServiceWorkerRegistration::RegistrationCleared() {
@@ -272,6 +273,10 @@ already_AddRefed<Promise> ServiceWorkerRegistration::Update(ErrorResult& aRv) {
     }
   }
 
+  // Keep the SWR and thereby its actor live throughout the IPC call (unless
+  // the global is torn down and DisconnectFromOwner is called which will cause
+  // us to call Shutdown() which will shutdown the actor and reject the IPC
+  // calls).
   RefPtr<ServiceWorkerRegistration> self = this;
 
   if (!mActor) {
@@ -281,9 +286,9 @@ already_AddRefed<Promise> ServiceWorkerRegistration::Update(ErrorResult& aRv) {
 
   mActor->SendUpdate(
       newestWorkerDescriptor.ref().ScriptURL(),
-      [outer,
+      [outer, self = std::move(self)](
-       self](const IPCServiceWorkerRegistrationDescriptorOrCopyableErrorResult&
+          const IPCServiceWorkerRegistrationDescriptorOrCopyableErrorResult&
-                 aResult) {
+              aResult) {
         AUTO_PROFILER_MARKER_UNTYPED(
             "ServiceWorkerRegistration::Update (inner)", DOM, {});
 
@@ -300,25 +305,20 @@ already_AddRefed<Promise> ServiceWorkerRegistration::Update(ErrorResult& aRv) {
         const auto& ipcDesc =
             aResult.get_IPCServiceWorkerRegistrationDescriptor();
         nsIGlobalObject* global = self->GetParentObject();
-        // It's possible this binding was detached from the global.  In cases
+        // Given that we destroy the actor on DisconnectFromOwner, it should be
-        // where we use IPC with Promise callbacks, we use
+        // impossible for global to be null here since we should only process
-        // DOMMozPromiseRequestHolder in order to auto-disconnect the promise
+        // the reject case below in that case.  (And in the event there is an
-        // that would hold these callbacks.  However in bug 1466681 we changed
+        // in-flight IPC message, it will be discarded.)  This assertion will
-        // this call to use (synchronous) callbacks because the use of
+        // help validate this without inconveniencing users.
-        // MozPromise introduced an additional runnable scheduling which made
+        MOZ_ASSERT_DEBUG_OR_FUZZING(global);
-        // it very difficult to maintain ordering required by the standard.
-        //
-        // If we were to delete this actor at the time of DETH detaching, we
-        // would not need to do this check because the IPC callback of the
-        // RemoteServiceWorkerRegistrationImpl lambdas would never occur.
-        // However, its actors currently depend on asking the parent to delete
-        // the actor for us.  Given relaxations in the IPC lifecyle, we could
-        // potentially issue a direct termination, but that requires additional
-        // evaluation.
         if (!global) {
           outer->MaybeReject(NS_ERROR_DOM_INVALID_STATE_ERR);
           return;
         }
+        // TODO: Given that we are keeping this registration alive through the
+        // call, it's not clear how `ref` could be anything but this instance.
+        // Consider just returning `self` after doing the code archaeology to
+        // ensure there isn't some still-valid reason.
         RefPtr<ServiceWorkerRegistration> ref =
             global->GetOrCreateServiceWorkerRegistration(
                 ServiceWorkerRegistrationDescriptor(ipcDesc));
@@ -354,8 +354,14 @@ already_AddRefed<Promise> ServiceWorkerRegistration::Unregister(
     return outer.forget();
   }
 
+  // Keep the SWR and thereby its actor live throughout the IPC call (unless
+  // the global is torn down and DisconnectFromOwner is called which will cause
+  // us to call Shutdown() which will shutdown the actor and reject the IPC
+  // calls).
+  RefPtr<ServiceWorkerRegistration> self = this;
   mActor->SendUnregister(
-      [outer](std::tuple<bool, CopyableErrorResult>&& aResult) {
+      [self = std::move(self),
+       outer](std::tuple<bool, CopyableErrorResult>&& aResult) {
         if (std::get<1>(aResult).Failed()) {
           // application layer error
           // register() should be resilient and resolve false instead of
@@ -465,11 +471,18 @@ already_AddRefed<Promise> ServiceWorkerRegistration::GetNotifications(
     return promise.forget();
   }
 
+  // Keep the SWR and thereby its actor live throughout the IPC call (unless
+  // the global is torn down and DisconnectFromOwner is called which will cause
+  // us to call Shutdown() which will shutdown the actor and reject the IPC
+  // calls).
+  RefPtr<ServiceWorkerRegistration> self = this;
+
   // Step 5.3: Queue a global task on the DOM manipulation task source
   // given global to run these steps:
   mActor->SendGetNotifications(aOptions.mTag)
       ->Then(GetCurrentSerialEventTarget(), __func__,
-             [promise, scope = NS_ConvertUTF8toUTF16(mDescriptor.Scope())](
+             [self = std::move(self), promise,
+              scope = NS_ConvertUTF8toUTF16(mDescriptor.Scope())](
                  const PServiceWorkerRegistrationChild::
                      GetNotificationsPromise::ResolveOrRejectValue&& aValue) {
                if (aValue.IsReject()) {
@@ -521,9 +534,16 @@ void ServiceWorkerRegistration::SetNavigationPreloadEnabled(
     return;
   }
 
+  // Keep the SWR and thereby its actor live throughout the IPC call (unless
+  // the global is torn down and DisconnectFromOwner is called which will cause
+  // us to call Shutdown() which will shutdown the actor and reject the IPC
+  // calls).
+  RefPtr<ServiceWorkerRegistration> self = this;
+
   mActor->SendSetNavigationPreloadEnabled(
       aEnabled,
-      [successCB = std::move(aSuccessCB), aFailureCB](bool aResult) {
+      [self = std::move(self), successCB = std::move(aSuccessCB),
+       aFailureCB](bool aResult) {
         if (!aResult) {
           aFailureCB(CopyableErrorResult(NS_ERROR_DOM_INVALID_STATE_ERR));
           return;
@@ -543,9 +563,16 @@ void ServiceWorkerRegistration::SetNavigationPreloadHeader(
     return;
   }
 
+  // Keep the SWR and thereby its actor live throughout the IPC call (unless
+  // the global is torn down and DisconnectFromOwner is called which will cause
+  // us to call Shutdown() which will shutdown the actor and reject the IPC
+  // calls).
+  RefPtr<ServiceWorkerRegistration> self = this;
+
   mActor->SendSetNavigationPreloadHeader(
       aHeader,
-      [successCB = std::move(aSuccessCB), aFailureCB](bool aResult) {
+      [self = std::move(self), successCB = std::move(aSuccessCB),
+       aFailureCB](bool aResult) {
         if (!aResult) {
           aFailureCB(CopyableErrorResult(NS_ERROR_DOM_INVALID_STATE_ERR));
           return;
@@ -565,8 +592,14 @@ void ServiceWorkerRegistration::GetNavigationPreloadState(
     return;
   }
 
+  // Keep the SWR and thereby its actor live throughout the IPC call (unless
+  // the global is torn down and DisconnectFromOwner is called which will cause
+  // us to call Shutdown() which will shutdown the actor and reject the IPC
+  // calls).
+  RefPtr<ServiceWorkerRegistration> self = this;
+
   mActor->SendGetNavigationPreloadState(
-      [successCB = std::move(aSuccessCB),
+      [self = std::move(self), successCB = std::move(aSuccessCB),
        aFailureCB](Maybe<IPCNavigationPreloadState>&& aState) {
         if (NS_WARN_IF(!aState)) {
           aFailureCB(CopyableErrorResult(NS_ERROR_DOM_INVALID_STATE_ERR));
@@ -757,7 +790,7 @@ void ServiceWorkerRegistration::Shutdown() {
 
   if (mActor) {
     mActor->RevokeOwner(this);
-    mActor->MaybeStartTeardown();
+    mActor->Shutdown();
     mActor = nullptr;
   }
 }

dom/serviceworkers/ServiceWorkerRegistrationChild.cpp

@@ -51,9 +51,9 @@ ServiceWorkerRegistrationChild::Create() {
     RefPtr<IPCWorkerRefHelper<ServiceWorkerRegistrationChild>> helper =
         new IPCWorkerRefHelper<ServiceWorkerRegistrationChild>(actor);
 
-    actor->mIPCWorkerRef = IPCWorkerRef::Create(
+    actor->mIPCWorkerRef =
-        workerPrivate, "ServiceWorkerRegistrationChild",
+        IPCWorkerRef::Create(workerPrivate, "ServiceWorkerRegistrationChild",
-        [helper] { helper->Actor()->MaybeStartTeardown(); });
+                             [helper] { helper->Actor()->Shutdown(); });
 
     if (NS_WARN_IF(!actor->mIPCWorkerRef)) {
       return nullptr;
@@ -64,7 +64,7 @@ ServiceWorkerRegistrationChild::Create() {
 }
 
 ServiceWorkerRegistrationChild::ServiceWorkerRegistrationChild()
-    : mOwner(nullptr), mTeardownStarted(false) {}
+    : mOwner(nullptr) {}
 
 void ServiceWorkerRegistrationChild::SetOwner(
     ServiceWorkerRegistration* aOwner) {
@@ -80,12 +80,11 @@ void ServiceWorkerRegistrationChild::RevokeOwner(
   mOwner = nullptr;
 }
 
-void ServiceWorkerRegistrationChild::MaybeStartTeardown() {
+void ServiceWorkerRegistrationChild::Shutdown() {
-  if (mTeardownStarted) {
+  if (!CanSend()) {
     return;
   }
-  mTeardownStarted = true;
+  Unused << Send__delete__(this);
-  Unused << SendTeardown();
 }
 
 }  // namespace mozilla::dom

dom/serviceworkers/ServiceWorkerRegistrationChild.h

@@ -21,7 +21,6 @@ class ServiceWorkerRegistrationChild final
     : public PServiceWorkerRegistrationChild {
   RefPtr<IPCWorkerRef> mIPCWorkerRef;
   ServiceWorkerRegistration* mOwner;
-  bool mTeardownStarted;
 
   ServiceWorkerRegistrationChild();
 
@@ -44,7 +43,8 @@ class ServiceWorkerRegistrationChild final
 
   void RevokeOwner(ServiceWorkerRegistration* aOwner);
 
-  void MaybeStartTeardown();
+  // Idempotently delete the actor.
+  void Shutdown();
 };
 
 }  // namespace mozilla::dom

dom/serviceworkers/ServiceWorkerRegistrationParent.cpp

@@ -21,11 +21,6 @@ void ServiceWorkerRegistrationParent::ActorDestroy(ActorDestroyReason aReason) {
   }
 }
 
-IPCResult ServiceWorkerRegistrationParent::RecvTeardown() {
-  MaybeSendDelete();
-  return IPC_OK();
-}
-
 namespace {
 
 void ResolveUnregister(
@@ -143,8 +138,7 @@ IPCResult ServiceWorkerRegistrationParent::RecvGetNotifications(
   return IPC_OK();
 }
 
-ServiceWorkerRegistrationParent::ServiceWorkerRegistrationParent()
+ServiceWorkerRegistrationParent::ServiceWorkerRegistrationParent() {}
-    : mDeleteSent(false) {}
 
 ServiceWorkerRegistrationParent::~ServiceWorkerRegistrationParent() {
   MOZ_DIAGNOSTIC_ASSERT(!mProxy);
@@ -160,10 +154,9 @@ void ServiceWorkerRegistrationParent::Init(
 }
 
 void ServiceWorkerRegistrationParent::MaybeSendDelete() {
-  if (mDeleteSent) {
+  if (!CanSend()) {
     return;
   }
-  mDeleteSent = true;
   Unused << Send__delete__(this);
 }
 

dom/serviceworkers/ServiceWorkerRegistrationParent.h

@@ -17,15 +17,12 @@ class ServiceWorkerRegistrationProxy;
 class ServiceWorkerRegistrationParent final
     : public PServiceWorkerRegistrationParent {
   RefPtr<ServiceWorkerRegistrationProxy> mProxy;
-  bool mDeleteSent;
 
   ~ServiceWorkerRegistrationParent();
 
   // PServiceWorkerRegistrationParent
   void ActorDestroy(ActorDestroyReason aReason) override;
 
-  mozilla::ipc::IPCResult RecvTeardown() override;
-
   mozilla::ipc::IPCResult RecvUnregister(
       UnregisterResolver&& aResolver) override;
 
@@ -49,6 +46,7 @@ class ServiceWorkerRegistrationParent final
  public:
   NS_INLINE_DECL_REFCOUNTING(ServiceWorkerRegistrationParent, override);
 
+  // If we default this we have to fully define ServiceWorkerRegistrationProxy.
   ServiceWorkerRegistrationParent();
 
   void Init(const IPCServiceWorkerRegistrationDescriptor& aDescriptor,