Skip to content

Commit

Permalink
Propagate aborted state to dependent signals before firing events
Browse files Browse the repository at this point in the history
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: whatwg/dom#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. The
  tests:
  - `Dependent signals for ${desc} are marked aborted before abort events fire ${suffix}` and
  - `Dependent signals for ${desc} are aborted correctly for reentrant aborts ${suffix}`.
* 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:
  • Loading branch information
maraisr committed Aug 16, 2024
1 parent 3137d42 commit c757769
Show file tree
Hide file tree
Showing 5 changed files with 81 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,6 @@ 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)

Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,6 @@ 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)

Original file line number Diff line number Diff line change
Expand Up @@ -182,4 +182,43 @@ 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}`);
}
40 changes: 34 additions & 6 deletions Source/WebCore/dom/AbortSignal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ Ref<AbortSignal> 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;
Expand Down Expand Up @@ -126,25 +128,51 @@ void AbortSignal::signalAbort(JSC::JSValue reason)
// 1. If signal's aborted flag is set, then return.
if (m_aborted)
return;

// 2. Set signal’s aborted flag.
markAborted(reason);

Vector<Ref<AbortSignal>> 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();

// FIXME: This code is wrong: we should emit a write-barrier. Otherwise, GC can collect it.
// 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
Expand Down
6 changes: 4 additions & 2 deletions Source/WebCore/dom/AbortSignal.h
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,16 @@ class AbortSignal final : public RefCounted<AbortSignal>, 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<std::pair<uint32_t, Algorithm>> m_algorithms;
WeakPtr<AbortSignal, WeakPtrImplWithEventTargetData> m_followingSignal;
AbortSignalSet m_sourceSignals;
Expand All @@ -110,4 +113,3 @@ class AbortSignal final : public RefCounted<AbortSignal>, public EventTarget, pr
WebCoreOpaqueRoot root(AbortSignal*);

} // namespace WebCore

0 comments on commit c757769

Please sign in to comment.