From 1a2d673e75f3a3b39043bc3904c5254278d1f7ca Mon Sep 17 00:00:00 2001 From: Kagami Sascha Rosylight Date: Thu, 5 Sep 2024 16:41:20 +0000 Subject: [PATCH] Bug 1903676 - Update the signal abort algorithm r=smaug Differential Revision: https://phabricator.services.mozilla.com/D220836 --- dom/abort/AbortFollower.h | 14 ++- dom/abort/AbortSignal.cpp | 119 +++++++++++++----- dom/abort/AbortSignal.h | 7 +- .../dom/abort/abort-signal-any.any.js.ini | 20 --- .../dom/abort/crashtests/any-on-abort.html | 11 ++ .../abort/resources/abort-signal-any-tests.js | 16 +++ 6 files changed, 133 insertions(+), 54 deletions(-) delete mode 100644 testing/web-platform/meta/dom/abort/abort-signal-any.any.js.ini create mode 100644 testing/web-platform/tests/dom/abort/crashtests/any-on-abort.html diff --git a/dom/abort/AbortFollower.h b/dom/abort/AbortFollower.h index 8c2509781a1a..4967cac9f10c 100644 --- a/dom/abort/AbortFollower.h +++ b/dom/abort/AbortFollower.h @@ -45,6 +45,9 @@ class AbortFollower : public nsISupports { }; /* + * TODO(krosylight): The only consumer of this is Fetch. It would be nice to + * merge this back to AbortSignal as it's quite a duplication right now. + * * AbortSignalImpl is a minimal implementation without an associated global * and without event dispatching, those are added in AbortSignal. * See Bug 1478101 @@ -60,7 +63,7 @@ class AbortSignalImpl : public nsISupports, public SupportsWeakPtr { // Helper for other DOM code JS::Value RawReason() const; - virtual void SignalAbort(JS::Handle aReason); + void SignalAbort(JS::Handle aReason); protected: // Subclasses of this class must call these Traverse and Unlink functions @@ -72,6 +75,10 @@ class AbortSignalImpl : public nsISupports, public SupportsWeakPtr { virtual ~AbortSignalImpl() { UnlinkFollowers(); } + virtual void SignalAbortWithDependents(); + + virtual void RunAbortSteps(); + void SetAborted(JS::Handle aReason); JS::Heap mReason; @@ -83,7 +90,10 @@ class AbortSignalImpl : public nsISupports, public SupportsWeakPtr { void UnlinkFollowers(); - // Raw pointers. |AbortFollower::Follow| adds to this array, and + // TODO(krosylight): We should rename all names around the term "Follow". See + // bug 1873648. + // + // |AbortFollower::Follow| adds to this array, and // |AbortFollower::Unfollow| (also called by the destructor) will remove // from this array. Finally, calling |SignalAbort()| will (after running all // abort algorithms) empty this and make all contained followers |Unfollow()|. diff --git a/dom/abort/AbortSignal.cpp b/dom/abort/AbortSignal.cpp index 1acfe3ec6948..b0e62b8392d9 100644 --- a/dom/abort/AbortSignal.cpp +++ b/dom/abort/AbortSignal.cpp @@ -41,17 +41,35 @@ void AbortSignalImpl::GetReason(JSContext* aCx, JS::Value AbortSignalImpl::RawReason() const { return mReason.get(); } -// https://dom.spec.whatwg.org/#abortsignal-signal-abort steps 1-4 +// https://dom.spec.whatwg.org/#abortsignal-signal-abort void AbortSignalImpl::SignalAbort(JS::Handle aReason) { - // Step 1. + // Step 1: If signal is aborted, then return. if (mAborted) { return; } - // Step 2. + // Step 2: Set signal’s abort reason to reason if it is given; otherwise to a + // new "AbortError" DOMException. + // + // (But given AbortSignalImpl is supposed to run without JS context, the + // DOMException creation is deferred to the getter.) SetAborted(aReason); - // Step 3. + // Step 3 - 6 + SignalAbortWithDependents(); +} + +void AbortSignalImpl::SignalAbortWithDependents() { + // AbortSignalImpl cannot have dependents, so just run abort steps for itself. + RunAbortSteps(); +} + +// https://dom.spec.whatwg.org/#run-the-abort-steps +// This skips event firing as AbortSignalImpl is not supposed to be exposed to +// JS. It's done instead in AbortSignal::RunAbortSteps. +void AbortSignalImpl::RunAbortSteps() { + // Step 1: For each algorithm of signal’s abort algorithms: run algorithm. + // // When there are multiple followers, the follower removal algorithm // https://dom.spec.whatwg.org/#abortsignal-remove could be invoked in an // earlier algorithm to remove a later algorithm, so |mFollowers| must be a @@ -61,7 +79,7 @@ void AbortSignalImpl::SignalAbort(JS::Handle aReason) { follower->RunAbortAlgorithm(); } - // Step 4. + // Step 2: Empty signal’s abort algorithms. UnlinkFollowers(); } @@ -277,14 +295,26 @@ already_AddRefed AbortSignal::Any( RefPtr resultSignal = new AbortSignal(aGlobal, false, JS::UndefinedHandleValue); - // Step 2. For each signal of signals: if signal is aborted, then set - // resultSignal's abort reason to signal's abort reason and return - // resultSignal. - for (const auto& signal : aSignals) { - if (signal->Aborted()) { - JS::Rooted reason(RootingCx(), signal->RawReason()); - resultSignal->SetAborted(reason); - return resultSignal.forget(); + if (!aSignals.IsEmpty()) { + // (Prepare for step 2 which uses the reason of this. Cannot use + // RawReason because that can cause constructing new DOMException for each + // dependent signal instead of sharing the single one.) + AutoJSAPI jsapi; + if (!jsapi.Init(aGlobal)) { + return nullptr; + } + JSContext* cx = jsapi.cx(); + + // Step 2. For each signal of signals: if signal is aborted, then set + // resultSignal's abort reason to signal's abort reason and return + // resultSignal. + for (const auto& signal : aSignals) { + if (signal->Aborted()) { + JS::Rooted reason(cx); + signal->GetReason(cx, &reason); + resultSignal->SetAborted(reason); + return resultSignal.forget(); + } } } @@ -339,17 +369,56 @@ void AbortSignal::ThrowIfAborted(JSContext* aCx, ErrorResult& aRv) { } } -// https://dom.spec.whatwg.org/#abortsignal-signal-abort -void AbortSignal::SignalAbort(JS::Handle aReason) { - // Step 1, in case "signal abort" algorithm is called directly - if (Aborted()) { - return; +// Step 3 - 6 of https://dom.spec.whatwg.org/#abortsignal-signal-abort +void AbortSignal::SignalAbortWithDependents() { + // Step 3: Let dependentSignalsToAbort be a new list. + nsTArray> dependentSignalsToAbort; + + // mDependentSignals can go away after this function. + nsTArray> dependentSignals = std::move(mDependentSignals); + + if (!dependentSignals.IsEmpty()) { + // (Prepare for step 4.1.1 which uses the reason of this. Cannot use + // RawReason because that can cause constructing new DOMException for each + // dependent signal instead of sharing the single one.) + AutoJSAPI jsapi; + if (!jsapi.Init(GetParentObject())) { + return; + } + JSContext* cx = jsapi.cx(); + JS::Rooted reason(cx); + GetReason(cx, &reason); + + // Step 4. For each dependentSignal of signal’s dependent signals: + for (const auto& dependentSignal : dependentSignals) { + MOZ_ASSERT(dependentSignal->mSourceSignals.Contains(this)); + // Step 4.1: If dependentSignal is not aborted, then: + if (!dependentSignal->Aborted()) { + // Step 4.1.1: Set dependentSignal’s abort reason to signal’s abort + // reason. + dependentSignal->SetAborted(reason); + // Step 4.1.2: Append dependentSignal to dependentSignalsToAbort. + dependentSignalsToAbort.AppendElement(dependentSignal); + } + } } - // Steps 1-4. - AbortSignalImpl::SignalAbort(aReason); + // Step 5: Run the abort steps for signal. + RunAbortSteps(); - // Step 5. Fire an event named abort at this signal + // Step 6: For each dependentSignal of dependentSignalsToAbort, run the abort + // steps for dependentSignal. + for (const auto& dependentSignal : dependentSignalsToAbort) { + dependentSignal->RunAbortSteps(); + } +} + +// https://dom.spec.whatwg.org/#run-the-abort-steps +void AbortSignal::RunAbortSteps() { + // Step 1 - 2: + AbortSignalImpl::RunAbortSteps(); + + // Step 3. Fire an event named abort at this signal. EventInit init; init.mBubbles = false; init.mCancelable = false; @@ -358,14 +427,6 @@ void AbortSignal::SignalAbort(JS::Handle aReason) { event->SetTrusted(true); DispatchEvent(*event); - - // Step 6. Abort dependentSignals of this signal - for (const auto& dependant : mDependentSignals) { - MOZ_ASSERT(dependant->mSourceSignals.Contains(this)); - dependant->SignalAbort(aReason); - } - // clear dependent signals so that they might be garbage collected - mDependentSignals.Clear(); } bool AbortSignal::Dependent() const { return mDependent; } diff --git a/dom/abort/AbortSignal.h b/dom/abort/AbortSignal.h index 05fb6e5d1d32..fef80b9dd2b3 100644 --- a/dom/abort/AbortSignal.h +++ b/dom/abort/AbortSignal.h @@ -55,9 +55,6 @@ class AbortSignal : public DOMEventTargetHelper, public AbortSignalImpl { void ThrowIfAborted(JSContext* aCx, ErrorResult& aRv); - // AbortSignalImpl - void SignalAbort(JS::Handle aReason) override; - virtual bool IsTaskSignal() const { return false; } bool Dependent() const; @@ -67,6 +64,10 @@ class AbortSignal : public DOMEventTargetHelper, public AbortSignalImpl { void MakeDependentOn(AbortSignal* aSignal); + void SignalAbortWithDependents() override; + + void RunAbortSteps() override; + nsTArray> mSourceSignals; nsTArray> mDependentSignals; diff --git a/testing/web-platform/meta/dom/abort/abort-signal-any.any.js.ini b/testing/web-platform/meta/dom/abort/abort-signal-any.any.js.ini deleted file mode 100644 index 505acea1fecf..000000000000 --- a/testing/web-platform/meta/dom/abort/abort-signal-any.any.js.ini +++ /dev/null @@ -1,20 +0,0 @@ -[abort-signal-any.any.worker.html] - expected: - if not debug: ERROR - CRASH - [Dependent signals for AbortSignal.any() are marked aborted before abort events fire (using AbortController)] - expected: FAIL - - [Dependent signals for AbortSignal.any() are aborted correctly for reentrant aborts (using AbortController)] - expected: FAIL - - -[abort-signal-any.any.html] - expected: - if not debug: ERROR - CRASH - [Dependent signals for AbortSignal.any() are marked aborted before abort events fire (using AbortController)] - expected: FAIL - - [Dependent signals for AbortSignal.any() are aborted correctly for reentrant aborts (using AbortController)] - expected: FAIL diff --git a/testing/web-platform/tests/dom/abort/crashtests/any-on-abort.html b/testing/web-platform/tests/dom/abort/crashtests/any-on-abort.html new file mode 100644 index 000000000000..07a0f0bd3c2b --- /dev/null +++ b/testing/web-platform/tests/dom/abort/crashtests/any-on-abort.html @@ -0,0 +1,11 @@ + + + diff --git a/testing/web-platform/tests/dom/abort/resources/abort-signal-any-tests.js b/testing/web-platform/tests/dom/abort/resources/abort-signal-any-tests.js index 929ee8a2e61a..8f897c934e87 100644 --- a/testing/web-platform/tests/dom/abort/resources/abort-signal-any-tests.js +++ b/testing/web-platform/tests/dom/abort/resources/abort-signal-any-tests.js @@ -221,4 +221,20 @@ function abortSignalAnyTests(signalInterface, controllerInterface) { assert_true(signal.aborted); assert_equals(signal.reason, "reason 1"); }, `Dependent signals for ${desc} are aborted correctly for reentrant aborts ${suffix}`); + + test(t => { + const source = signalInterface.abort(); + const dependent = signalInterface.any([source]); + assert_true(source.reason instanceof DOMException); + assert_equals(source.reason, dependent.reason); + }, `Dependent signals for ${desc} should use the same DOMException instance from the already aborted source signal ${suffix}`); + + test(t => { + const controller = new controllerInterface(); + const source = controller.signal; + const dependent = signalInterface.any([source]); + controller.abort(); + assert_true(source.reason instanceof DOMException); + assert_equals(source.reason, dependent.reason); + }, `Dependent signals for ${desc} should use the same DOMException instance from the source signal being aborted later ${suffix}`); }