diff --git a/dom/serviceworkers/PServiceWorkerRegistration.ipdl b/dom/serviceworkers/PServiceWorkerRegistration.ipdl index 3aea6bd5556e..44453c494e33 100644 --- a/dom/serviceworkers/PServiceWorkerRegistration.ipdl +++ b/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 diff --git a/dom/serviceworkers/ServiceWorkerRegistration.cpp b/dom/serviceworkers/ServiceWorkerRegistration.cpp index 0f86cd831d57..c5833be8f2bf 100644 --- a/dom/serviceworkers/ServiceWorkerRegistration.cpp +++ b/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 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 self = this; if (!mActor) { @@ -281,9 +286,9 @@ already_AddRefed ServiceWorkerRegistration::Update(ErrorResult& aRv) { mActor->SendUpdate( newestWorkerDescriptor.ref().ScriptURL(), - [outer, - self](const IPCServiceWorkerRegistrationDescriptorOrCopyableErrorResult& - aResult) { + [outer, self = std::move(self)]( + const IPCServiceWorkerRegistrationDescriptorOrCopyableErrorResult& + aResult) { AUTO_PROFILER_MARKER_UNTYPED( "ServiceWorkerRegistration::Update (inner)", DOM, {}); @@ -300,25 +305,20 @@ already_AddRefed 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 - // where we use IPC with Promise callbacks, we use - // DOMMozPromiseRequestHolder in order to auto-disconnect the promise - // that would hold these callbacks. However in bug 1466681 we changed - // this call to use (synchronous) callbacks because the use of - // MozPromise introduced an additional runnable scheduling which made - // 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. + // Given that we destroy the actor on DisconnectFromOwner, it should be + // impossible for global to be null here since we should only process + // the reject case below in that case. (And in the event there is an + // in-flight IPC message, it will be discarded.) This assertion will + // help validate this without inconveniencing users. + MOZ_ASSERT_DEBUG_OR_FUZZING(global); 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 ref = global->GetOrCreateServiceWorkerRegistration( ServiceWorkerRegistrationDescriptor(ipcDesc)); @@ -354,8 +354,14 @@ already_AddRefed 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 self = this; mActor->SendUnregister( - [outer](std::tuple&& aResult) { + [self = std::move(self), + outer](std::tuple&& 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 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 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 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 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 self = this; + mActor->SendGetNavigationPreloadState( - [successCB = std::move(aSuccessCB), + [self = std::move(self), successCB = std::move(aSuccessCB), aFailureCB](Maybe&& 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; } } diff --git a/dom/serviceworkers/ServiceWorkerRegistrationChild.cpp b/dom/serviceworkers/ServiceWorkerRegistrationChild.cpp index d3ed8c4cbd98..41bbbd1fe145 100644 --- a/dom/serviceworkers/ServiceWorkerRegistrationChild.cpp +++ b/dom/serviceworkers/ServiceWorkerRegistrationChild.cpp @@ -51,9 +51,9 @@ ServiceWorkerRegistrationChild::Create() { RefPtr> helper = new IPCWorkerRefHelper(actor); - actor->mIPCWorkerRef = IPCWorkerRef::Create( - workerPrivate, "ServiceWorkerRegistrationChild", - [helper] { helper->Actor()->MaybeStartTeardown(); }); + actor->mIPCWorkerRef = + IPCWorkerRef::Create(workerPrivate, "ServiceWorkerRegistrationChild", + [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() { - if (mTeardownStarted) { +void ServiceWorkerRegistrationChild::Shutdown() { + if (!CanSend()) { return; } - mTeardownStarted = true; - Unused << SendTeardown(); + Unused << Send__delete__(this); } } // namespace mozilla::dom diff --git a/dom/serviceworkers/ServiceWorkerRegistrationChild.h b/dom/serviceworkers/ServiceWorkerRegistrationChild.h index a26dfd6de9a4..56cafc943531 100644 --- a/dom/serviceworkers/ServiceWorkerRegistrationChild.h +++ b/dom/serviceworkers/ServiceWorkerRegistrationChild.h @@ -21,7 +21,6 @@ class ServiceWorkerRegistrationChild final : public PServiceWorkerRegistrationChild { RefPtr 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 diff --git a/dom/serviceworkers/ServiceWorkerRegistrationParent.cpp b/dom/serviceworkers/ServiceWorkerRegistrationParent.cpp index e8b9e2c68d96..e97e86e679cf 100644 --- a/dom/serviceworkers/ServiceWorkerRegistrationParent.cpp +++ b/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() - : mDeleteSent(false) {} +ServiceWorkerRegistrationParent::ServiceWorkerRegistrationParent() {} 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); } diff --git a/dom/serviceworkers/ServiceWorkerRegistrationParent.h b/dom/serviceworkers/ServiceWorkerRegistrationParent.h index 7e0ad50983fc..865481dbf05d 100644 --- a/dom/serviceworkers/ServiceWorkerRegistrationParent.h +++ b/dom/serviceworkers/ServiceWorkerRegistrationParent.h @@ -17,15 +17,12 @@ class ServiceWorkerRegistrationProxy; class ServiceWorkerRegistrationParent final : public PServiceWorkerRegistrationParent { RefPtr 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,