From a8b4184a7f3828540b9c490d090166545e3c5c0c Mon Sep 17 00:00:00 2001 From: Marais Rossouw Date: Thu, 15 Aug 2024 14:43:12 -1000 Subject: [PATCH] Propagate aborted state to dependent signals before firing events https://bugs.webkit.org/show_bug.cgi?id=278159 Reviewed by NOBODY (OOPS!). The implementation and spec assert that dependent signals are aborted if any of their sources are aborted. However, the intermediate states do not reflect this during the abort process. Since we iterate through all the dependent signals, updating the state and then firing the event. An updated spec is about to land to address this by first propagating the abort state to any dependent signals, and only then run the abort steps (run algorithms and fire events). Worthy notes: - Dependent signals do not themselves have dependent signals, which means it's unnecessary to call "signal abort recursively". - This approach retains the existing event dispatch order while ensuring the abort state is synced before any JS runs. DOM spec PR: https://github.com/whatwg/dom/pull/1295 * LayoutTests/imported/w3c/web-platform-tests/dom/abort/abort-signal-any.any-expected.txt: Rebaselined. * LayoutTests/imported/w3c/web-platform-tests/dom/abort/abort-signal-any.any.worker-expected.txt: Rebaselined. * LayoutTests/imported/w3c/web-platform-tests/dom/abort/resources/abort-signal-any-tests.js: Ported wpt tests from Chromium's PR addressing this same concern, and added one extra test to assert the event handler timing order. * Source/WebCore/dom/AbortSignal.cpp: (WebCore::AbortSignal::any): `check-webkit-style` did not like the `findIf` with `[whitespace/newline]`. (WebCore::AbortSignal::signalAbort): Collect the dependent signals to abort into a vector, then mark those as aborted, then run the signal's abort steps, and finally run the dependent signals' abort steps. (WebCore::AbortSignal::markAborted): Added this convenience method to set both the `reason` and then `m_aborted` state. (WebCore::AbortSignal::runAbortSteps): * Source/WebCore/dom/AbortSignal.h: --- .../abort/abort-signal-any.any-expected.txt | 3 + .../abort-signal-any.any.worker-expected.txt | 3 + .../abort/resources/abort-signal-any-tests.js | 66 +++++++++++++++++++ Source/WebCore/dom/AbortSignal.cpp | 42 ++++++++++-- Source/WebCore/dom/AbortSignal.h | 6 +- 5 files changed, 111 insertions(+), 9 deletions(-) diff --git a/LayoutTests/imported/w3c/web-platform-tests/dom/abort/abort-signal-any.any-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/dom/abort/abort-signal-any.any-expected.txt index 25877db979fa2..88d765cab544c 100644 --- a/LayoutTests/imported/w3c/web-platform-tests/dom/abort/abort-signal-any.any-expected.txt +++ b/LayoutTests/imported/w3c/web-platform-tests/dom/abort/abort-signal-any.any-expected.txt @@ -9,4 +9,7 @@ PASS AbortSignal.any() signals are composable (using AbortController) PASS AbortSignal.any() works with signals returned by AbortSignal.timeout() (using AbortController) PASS AbortSignal.any() works with intermediate signals (using AbortController) PASS Abort events for AbortSignal.any() signals fire in the right order (using AbortController) +PASS Dependent signals for AbortSignal.any() are marked aborted before abort events fire (using AbortController) +PASS Dependent signals for AbortSignal.any() are aborted correctly for reentrant aborts (using AbortController) +PASS Dependent signals for AbortSignal.any() are aborted with correct timing (using AbortController) diff --git a/LayoutTests/imported/w3c/web-platform-tests/dom/abort/abort-signal-any.any.worker-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/dom/abort/abort-signal-any.any.worker-expected.txt index 25877db979fa2..88d765cab544c 100644 --- a/LayoutTests/imported/w3c/web-platform-tests/dom/abort/abort-signal-any.any.worker-expected.txt +++ b/LayoutTests/imported/w3c/web-platform-tests/dom/abort/abort-signal-any.any.worker-expected.txt @@ -9,4 +9,7 @@ PASS AbortSignal.any() signals are composable (using AbortController) PASS AbortSignal.any() works with signals returned by AbortSignal.timeout() (using AbortController) PASS AbortSignal.any() works with intermediate signals (using AbortController) PASS Abort events for AbortSignal.any() signals fire in the right order (using AbortController) +PASS Dependent signals for AbortSignal.any() are marked aborted before abort events fire (using AbortController) +PASS Dependent signals for AbortSignal.any() are aborted correctly for reentrant aborts (using AbortController) +PASS Dependent signals for AbortSignal.any() are aborted with correct timing (using AbortController) diff --git a/LayoutTests/imported/w3c/web-platform-tests/dom/abort/resources/abort-signal-any-tests.js b/LayoutTests/imported/w3c/web-platform-tests/dom/abort/resources/abort-signal-any-tests.js index 66e4141eaccb0..b0834342581d4 100644 --- a/LayoutTests/imported/w3c/web-platform-tests/dom/abort/resources/abort-signal-any-tests.js +++ b/LayoutTests/imported/w3c/web-platform-tests/dom/abort/resources/abort-signal-any-tests.js @@ -182,4 +182,70 @@ function abortSignalAnyTests(signalInterface, controllerInterface) { controller.abort(); assert_equals(result, "01234"); }, `Abort events for ${desc} signals fire in the right order ${suffix}`); + + test(t => { + const controller = new controllerInterface(); + const signal1 = signalInterface.any([controller.signal]); + const signal2 = signalInterface.any([signal1]); + let eventFired = false; + + controller.signal.addEventListener('abort', () => { + const signal3 = signalInterface.any([signal2]); + assert_true(controller.signal.aborted); + assert_true(signal1.aborted); + assert_true(signal2.aborted); + assert_true(signal3.aborted); + eventFired = true; + }); + + controller.abort(); + assert_true(eventFired, "event fired"); + }, `Dependent signals for ${desc} are marked aborted before abort events fire ${suffix}`); + + test(t => { + const controller1 = new controllerInterface(); + const controller2 = new controllerInterface(); + const signal = signalInterface.any([controller1.signal, controller2.signal]); + let count = 0; + + controller1.signal.addEventListener('abort', () => { + controller2.abort("reason 2"); + }); + + signal.addEventListener('abort', () => { + count++; + }); + + controller1.abort("reason 1"); + assert_equals(count, 1); + assert_true(signal.aborted); + assert_equals(signal.reason, "reason 1"); + }, `Dependent signals for ${desc} are aborted correctly for reentrant aborts ${suffix}`); + + promise_test(async t => { + const controller1 = new controllerInterface(); + const controller2 = new controllerInterface(); + const signal = signalInterface.any([controller1.signal, controller2.signal]); + + const results = []; + + controller1.signal.addEventListener('abort', () => results.push('controller1 aborted')); + controller2.signal.addEventListener('abort', () => results.push('controller2 aborted')); + signal.addEventListener('abort', () => results.push('signal.any aborted')); + + controller1.abort(); + + // After a single task, assert that everything has happened correctly. + await new Promise(resolve => { + t.step_timeout(resolve); + }); + + assert_true(signal.aborted, ".any signal is aborted"); + assert_true(controller1.signal.aborted, "controller1.signal is aborted"); + assert_false(controller2.signal.aborted, "controller2.signal is NOT aborted"); + assert_array_equals(results, [ + "controller1 aborted", + "signal.any aborted", + ], "Events are fired in the right ordered"); + }, `Dependent signals for ${desc} are aborted with correct timing ${suffix}`); } diff --git a/Source/WebCore/dom/AbortSignal.cpp b/Source/WebCore/dom/AbortSignal.cpp index 9c56ddce9335d..f5b39ec9cc725 100644 --- a/Source/WebCore/dom/AbortSignal.cpp +++ b/Source/WebCore/dom/AbortSignal.cpp @@ -79,7 +79,9 @@ Ref AbortSignal::any(ScriptExecutionContext& context, const Vector< { Ref resultSignal = AbortSignal::create(&context); - auto abortedSignalIndex = signals.findIf([](auto& signal) { return signal->aborted(); }); + auto abortedSignalIndex = signals.findIf([](auto& signal) { + return signal->aborted(); + }); if (abortedSignalIndex != notFound) { resultSignal->signalAbort(signals[abortedSignalIndex]->reason().getValue()); return resultSignal; @@ -124,10 +126,31 @@ void AbortSignal::addDependentSignal(AbortSignal& signal) void AbortSignal::signalAbort(JSC::JSValue reason) { // 1. If signal's aborted flag is set, then return. - if (m_aborted) + if (aborted()) return; - + // 2. Set signal’s aborted flag. + markAborted(reason); + + Vector> dependentSignalsToAbort = { }; + + for (Ref dependentSignal : std::exchange(m_dependentSignals, { })) { + if (!dependentSignal->aborted()) { + dependentSignal->markAborted(reason); + dependentSignalsToAbort.append(dependentSignal); + } + } + + // 5. Run the abort steps + runAbortSteps(); + + // 6. For each dependentSignal of dependentSignalsToAbort, run the abort steps for dependentSignal. + for (auto& dependentSignal : dependentSignalsToAbort) + dependentSignal->runAbortSteps(); +} + +void AbortSignal::markAborted(JSC::JSValue reason) +{ m_aborted = true; m_sourceSignals.clear(); @@ -135,16 +158,21 @@ void AbortSignal::signalAbort(JSC::JSValue reason) // https://bugs.webkit.org/show_bug.cgi?id=236353 ASSERT(reason); m_reason.setWeakly(reason); +} +void AbortSignal::runAbortSteps() +{ + auto reason = m_reason.getValue(); + ASSERT(reason); + + // 1. For each algorithm of signal's abort algorithms: run algorithm. + // 2. Empty signal’s abort algorithms. (std::exchange empties) auto algorithms = std::exchange(m_algorithms, { }); for (auto& algorithm : algorithms) algorithm.second(reason); - // 5. Fire an event named abort at signal. + // 3. Fire an event named abort at signal. dispatchEvent(Event::create(eventNames().abortEvent, Event::CanBubble::No, Event::IsCancelable::No)); - - for (Ref dependentSignal : std::exchange(m_dependentSignals, { })) - dependentSignal->signalAbort(reason); } // https://dom.spec.whatwg.org/#abortsignal-follow diff --git a/Source/WebCore/dom/AbortSignal.h b/Source/WebCore/dom/AbortSignal.h index 57930f5045b51..15f8552da836d 100644 --- a/Source/WebCore/dom/AbortSignal.h +++ b/Source/WebCore/dom/AbortSignal.h @@ -88,13 +88,16 @@ class AbortSignal final : public RefCounted, public EventTarget, pr void addSourceSignal(AbortSignal&); void addDependentSignal(AbortSignal&); + void markAborted(JSC::JSValue); + void runAbortSteps(); + // EventTarget. enum EventTargetInterfaceType eventTargetInterface() const final { return EventTargetInterfaceType::AbortSignal; } ScriptExecutionContext* scriptExecutionContext() const final { return ContextDestructionObserver::scriptExecutionContext(); } void refEventTarget() final { ref(); } void derefEventTarget() final { deref(); } void eventListenersDidChange() final; - + Vector> m_algorithms; WeakPtr m_followingSignal; AbortSignalSet m_sourceSignals; @@ -110,4 +113,3 @@ class AbortSignal final : public RefCounted, public EventTarget, pr WebCoreOpaqueRoot root(AbortSignal*); } // namespace WebCore -