From 5def06e77f8ff791726654fac53f5bc95dea5d3b Mon Sep 17 00:00:00 2001 From: Yaron Tausky Date: Thu, 29 Oct 2020 13:50:44 +0000 Subject: [PATCH] Bug 1617822 - Implement FetchEvent.handled r=dom-workers-and-storage-reviewers,asuth Differential Revision: https://phabricator.services.mozilla.com/D93483 --- dom/serviceworkers/ServiceWorkerEvents.cpp | 8 +++++ dom/serviceworkers/ServiceWorkerEvents.h | 3 ++ dom/serviceworkers/ServiceWorkerOp.cpp | 36 +++++++++++++++++-- dom/serviceworkers/ServiceWorkerOp.h | 4 +++ dom/webidl/FetchEvent.webidl | 1 + .../fetch-event-handled.https.html.ini | 20 ----------- 6 files changed, 49 insertions(+), 23 deletions(-) delete mode 100644 testing/web-platform/meta/service-workers/service-worker/fetch-event-handled.https.html.ini diff --git a/dom/serviceworkers/ServiceWorkerEvents.cpp b/dom/serviceworkers/ServiceWorkerEvents.cpp index eecb612c8fc7..81e37713d6c1 100644 --- a/dom/serviceworkers/ServiceWorkerEvents.cpp +++ b/dom/serviceworkers/ServiceWorkerEvents.cpp @@ -153,6 +153,14 @@ already_AddRefed FetchEvent::Constructor( e->mRequest = aOptions.mRequest; e->mClientId = aOptions.mClientId; e->mResultingClientId = aOptions.mResultingClientId; + RefPtr global = do_QueryObject(aGlobal.GetAsSupports()); + MOZ_ASSERT(global); + ErrorResult rv; + e->mHandled = Promise::Create(global, rv); + if (rv.Failed()) { + rv.SuppressException(); + return nullptr; + } return e.forget(); } diff --git a/dom/serviceworkers/ServiceWorkerEvents.h b/dom/serviceworkers/ServiceWorkerEvents.h index 617f332934be..3a0fbf5ef3aa 100644 --- a/dom/serviceworkers/ServiceWorkerEvents.h +++ b/dom/serviceworkers/ServiceWorkerEvents.h @@ -138,6 +138,7 @@ class FetchEvent final : public ExtendableEvent { nsMainThreadPtrHandle mChannel; nsMainThreadPtrHandle mRegistration; RefPtr mRequest; + RefPtr mHandled; nsCString mScriptSpec; nsCString mPreventDefaultScriptSpec; nsString mClientId; @@ -184,6 +185,8 @@ class FetchEvent final : public ExtendableEvent { aResultingClientId = mResultingClientId; } + Promise* Handled() const { return mHandled; } + void RespondWith(JSContext* aCx, Promise& aArg, ErrorResult& aRv); // Pull in the Event version of PreventDefault so we don't get diff --git a/dom/serviceworkers/ServiceWorkerOp.cpp b/dom/serviceworkers/ServiceWorkerOp.cpp index 7fc1e23097b3..448924f2dfe1 100644 --- a/dom/serviceworkers/ServiceWorkerOp.cpp +++ b/dom/serviceworkers/ServiceWorkerOp.cpp @@ -131,6 +131,7 @@ class ExtendableEventKeepAliveHandler final if (mCallback) { mCallback->FinishedWithResult(mRejected ? Rejected : Resolved); + mCallback = nullptr; } Cleanup(); @@ -167,6 +168,10 @@ class ExtendableEventKeepAliveHandler final void Cleanup() { MOZ_ASSERT(IsCurrentThreadRunningWorker()); + if (mCallback) { + mCallback->FinishedWithResult(Rejected); + } + mSelfRef = nullptr; mWorkerRef = nullptr; mCallback = nullptr; @@ -1059,6 +1064,7 @@ class MOZ_STACK_CLASS FetchEventOp::AutoCancel { } MOZ_ASSERT(!mOwner->mRespondWithPromiseHolder.IsEmpty()); + mOwner->mHandled->MaybeRejectWithNetworkError("AutoCancel"_ns); mOwner->mRespondWithPromiseHolder.Reject(NS_ERROR_INTERCEPTION_FAILED, __func__); } @@ -1230,9 +1236,10 @@ void FetchEventOp::MaybeFinished() { MOZ_ASSERT(!mPromiseHolder.IsEmpty()); if (mResult) { - // mRespondWithPromiseHolder should have been settled in - // {Resolve,Reject}Callback by now. - MOZ_DIAGNOSTIC_ASSERT(mRespondWithPromiseHolder.IsEmpty()); + // It's possible that mRespondWithPromiseHolder wasn't settled. That happens + // if the worker was terminated before the respondWith promise settled. + + mHandled = nullptr; ServiceWorkerFetchEventOpResult result( mResult.value() == Resolved ? NS_OK : NS_ERROR_FAILURE); @@ -1468,6 +1475,14 @@ void FetchEventOp::ResolvedCallback(JSContext* aCx, autoCancel.Reset(); + // https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm Step 26: If + // eventHandled is not null, then resolve eventHandled. + // + // mRespondWithPromiseHolder will resolve a MozPromise that will resolve on + // the worker owner's thread, so it's fine to resolve the mHandled promise now + // because content will not interfere with respondWith getting the Response to + // where it's going. + mHandled->MaybeResolveWithUndefined(); mRespondWithPromiseHolder.Resolve( FetchEventRespondWithResult( SynthesizeResponseArgs(ir, mRespondWithClosure.ref())), @@ -1497,6 +1512,11 @@ void FetchEventOp::RejectedCallback(JSContext* aCx, AsyncLog(sourceSpec, line, column, "InterceptionRejectedResponseWithURL"_ns, {std::move(requestURL), valueString}); + // https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm Step 25.1: + // If eventHandled is not null, then reject eventHandled with a "NetworkError" + // DOMException in workerRealm. + mHandled->MaybeRejectWithNetworkError( + "FetchEvent.respondWith() Promise rejected"_ns); mRespondWithPromiseHolder.Resolve( FetchEventRespondWithResult( CancelInterceptionArgs(NS_ERROR_INTERCEPTION_FAILED)), @@ -1581,6 +1601,7 @@ nsresult FetchEventOp::DispatchFetchEvent(JSContext* aCx, FetchEvent::Constructor(globalObject, u"fetch"_ns, fetchEventInit); fetchEvent->SetTrusted(true); fetchEvent->PostInit(args.workerScriptSpec(), this); + mHandled = fetchEvent->Handled(); /** * Step 5: Dispatch the FetchEvent to the worker's global object @@ -1590,6 +1611,7 @@ nsresult FetchEventOp::DispatchFetchEvent(JSContext* aCx, bool dispatchFailed = NS_FAILED(rv) && rv != NS_ERROR_XPC_JS_THREW_EXCEPTION; if (NS_WARN_IF(dispatchFailed)) { + mHandled = nullptr; return rv; } @@ -1629,11 +1651,19 @@ nsresult FetchEventOp::DispatchFetchEvent(JSContext* aCx, "We don't support system-principal serviceworkers"); if (fetchEvent->DefaultPrevented(CallerType::NonSystem)) { + // https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm + // Step 24.1.1: If eventHandled is not null, then reject eventHandled with + // a "NetworkError" DOMException in workerRealm. + mHandled->MaybeRejectWithNetworkError( + "FetchEvent.preventDefault() called"_ns); mRespondWithPromiseHolder.Resolve( FetchEventRespondWithResult( CancelInterceptionArgs(NS_ERROR_INTERCEPTION_FAILED)), __func__); } else { + // https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm + // Step 24.2: If eventHandled is not null, then resolve eventHandled. + mHandled->MaybeResolveWithUndefined(); mRespondWithPromiseHolder.Resolve( FetchEventRespondWithResult(ResetInterceptionArgs()), __func__); } diff --git a/dom/serviceworkers/ServiceWorkerOp.h b/dom/serviceworkers/ServiceWorkerOp.h index 00ac79073332..b553154f1229 100644 --- a/dom/serviceworkers/ServiceWorkerOp.h +++ b/dom/serviceworkers/ServiceWorkerOp.h @@ -168,6 +168,10 @@ class FetchEventOp final : public ExtendableEventOp, // Worker thread only; set when `FetchEvent::RespondWith()` is called. Maybe mRespondWithClosure; + + // Must be set to `nullptr` on the worker thread because `Promise`'s + // destructor must be called on the worker thread. + RefPtr mHandled; }; } // namespace dom diff --git a/dom/webidl/FetchEvent.webidl b/dom/webidl/FetchEvent.webidl index ef805ffe4810..6cbcd9903e82 100644 --- a/dom/webidl/FetchEvent.webidl +++ b/dom/webidl/FetchEvent.webidl @@ -15,6 +15,7 @@ interface FetchEvent : ExtendableEvent { [SameObject, BinaryName="request_"] readonly attribute Request request; readonly attribute DOMString clientId; readonly attribute DOMString resultingClientId; + readonly attribute Promise handled; [Throws] void respondWith(Promise r); diff --git a/testing/web-platform/meta/service-workers/service-worker/fetch-event-handled.https.html.ini b/testing/web-platform/meta/service-workers/service-worker/fetch-event-handled.https.html.ini deleted file mode 100644 index 662d8fe2af68..000000000000 --- a/testing/web-platform/meta/service-workers/service-worker/fetch-event-handled.https.html.ini +++ /dev/null @@ -1,20 +0,0 @@ -[fetch-event-handled.https.html] - max-asserts: 2 - [FetchEvent.handled should reject when the promise provided to respondWith() is rejected] - expected: FAIL - - [FetchEvent.handled should resolve when the promise provided to respondWith() is resolved] - expected: FAIL - - [FetchEvent.handled should reject when the promise provided to respondWith() is resolved to an invalid response] - expected: FAIL - - [FetchEvent.handled should reject when respondWith() is not called and the event is canceled] - expected: FAIL - - [FetchEvent.handled should resolve when respondWith() is not called for a sub-resource request] - expected: FAIL - - [FetchEvent.handled should resolve when respondWith() is not called for a navigation request] - expected: FAIL -