Mark dependent abort signals as aborted before firing events #1295

shaseley · 2024-07-15T23:11:23Z

The assert in 4.2.1 of "create a dependent abort signal" fails when creating a dependent signal while dispatching abort events or running abort algorithms if abort had not yet been propagated to one of the sources.

This fix splits "signal abort" into two phases: first, set the abort reason on the signal being aborted and all of its unaborted dependents; next, run the abort algorithms and dispatch events for the signal and those same dependents. Note that:

Dependent signals do not themselves have dependent signals, which means it's unnecessary to recursively call "signal abort"
This approach retains the existing event dispatch order, while ensuring the abort state is synced before any JS runs

This fixes #1293.

At least two implementers are interested (and none opposed):
- Chromium
- WebKit
Tests are written and can be reviewed and commented upon at: AbortSignal: Propagate aborted state to dependent signals before firing events web-platform-tests/wpt#47653
Implementation bugs are filed:
- Chromium: https://issues.chromium.org/issues/361129283
- Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1903676
- WebKit: https://bugs.webkit.org/show_bug.cgi?id=278159
- Deno (only for aborting and events): AbortSignal.any: Mark dependent abort signals as aborted before firing events denoland/deno#25118
- Node.js (only for aborting and events): AbortSignal.any: Mark dependent abort signals as aborted before firing events nodejs/node#54466
MDN issue is filed: N/A, bug fix
The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

The assert in 4.2.1 of "create a dependent abort signal" fails when creating a dependent signal while dispatching abort events or running abort algorithms if abort had not yet been propagated to one of the sources. This fix splits "signal abort" into two phases: first, set the abort reason on the signal being aborted and all of its unaborted dependents; next, run the abort algorithms and dispatch events for the signal and those same dependents. Note that: 1. Dependent signals do not themselves have dependent signals, which means it's unnecessary to recursively call "signal abort" 2. This approach retains the existing event dispatch order, while ensuring the abort state is synced before any JS runs This fixes whatwg#1293.

annevk · 2024-08-13T14:07:27Z

I wonder if @domenic / @jakearchibald / @jasnell are willing to review this?

Does this require implementation changes or a test?

jasnell · 2024-08-13T14:30:29Z

Seems fine to me

shaseley · 2024-08-13T21:12:34Z

Does this require implementation changes or a test?

Yes to both. WIP tests are here. I was holding off on landing that and filing bugs until someone had a chance to take a first pass on the PR.

annevk · 2024-08-15T09:42:30Z

Since this is fixing a bug, feel free to count WebKit as interested.

@vinhill thoughts?

maraisr · 2024-08-15T10:00:24Z

Just created related WebKit bug issue: https://bugs.webkit.org/show_bug.cgi?id=278159. Will be ready to land if not landed over the weekend.

vinhill · 2024-08-15T10:38:40Z

I'm no longer employed at Mozilla, but they are likely interested due to Bug 1903676
@saschanaz

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, 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:

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:

…ng events The implementation and spec of dependent signals assumes and asserts that a dependent signal is aborted if any of its sources have been aborted. But this property does not hold during the abort process, since intermediate states of abort propagation can be observed. For example, calling AbortSignal.any([signal]) in an "abort" event listener for one of signal's sources is a way to observe intermediate state, since the "abort" event fires before the source signal's dependents are updated. To fix this, this CL decouples setting the abort state and reacting to abort event: 1. Mark the source signal as aborted 2. Propagate the aborted state to any dependent signals 3. Run abort steps (run algorithms, fire events) for the source signal 4. Run abort steps for each of the dependent signals PR: whatwg/dom#1295 Change-Id: I65a97eb46b01a0071d661e945f64c90e33954088

https://bugs.webkit.org/show_bug.cgi?id=278159 Reviewed by Chris Dumez. 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: Canonical link: https://commits.webkit.org/282387@main

annevk · 2024-08-17T16:55:59Z

@shaseley from my perspective this is good to land, modulo checking the checkboxes in OP.

saschanaz · 2024-08-20T18:12:16Z

LGTM from Mozilla. We can reuse https://bugzilla.mozilla.org/show_bug.cgi?id=1903676 as an implementation bug.

…ng events The implementation and spec of dependent signals assumes and asserts that a dependent signal is aborted if any of its sources have been aborted. But this property does not hold during the abort process, since intermediate states of abort propagation can be observed. For example, calling AbortSignal.any([signal]) in an "abort" event listener for one of signal's sources is a way to observe intermediate state, since the "abort" event fires before the source signal's dependents are updated. To fix this, this CL decouples setting the abort state and reacting to abort event: 1. Mark the source signal as aborted 2. Propagate the aborted state to any dependent signals 3. Run abort steps (run algorithms, fire events) for the source signal 4. Run abort steps for each of the dependent signals PR: whatwg/dom#1295 Change-Id: I65a97eb46b01a0071d661e945f64c90e33954088 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5664649 Commit-Queue: Scott Haseley <[email protected]> Reviewed-by: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1344364}

shaseley · 2024-08-20T20:57:32Z

@annevk bugs filed and checkboxes updated!

…ng events The implementation and spec of dependent signals assumes and asserts that a dependent signal is aborted if any of its sources have been aborted. But this property does not hold during the abort process, since intermediate states of abort propagation can be observed. For example, calling AbortSignal.any([signal]) in an "abort" event listener for one of signal's sources is a way to observe intermediate state, since the "abort" event fires before the source signal's dependents are updated. To fix this, this CL decouples setting the abort state and reacting to abort event: 1. Mark the source signal as aborted 2. Propagate the aborted state to any dependent signals 3. Run abort steps (run algorithms, fire events) for the source signal 4. Run abort steps for each of the dependent signals PR: whatwg/dom#1295 Change-Id: I65a97eb46b01a0071d661e945f64c90e33954088 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5664649 Commit-Queue: Scott Haseley <[email protected]> Reviewed-by: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1344364}

…ing events Ref whatwg/dom#1295 Fixes denoland#25118

…ng events The implementation and spec of dependent signals assumes and asserts that a dependent signal is aborted if any of its sources have been aborted. But this property does not hold during the abort process, since intermediate states of abort propagation can be observed. For example, calling AbortSignal.any([signal]) in an "abort" event listener for one of signal's sources is a way to observe intermediate state, since the "abort" event fires before the source signal's dependents are updated. To fix this, this CL decouples setting the abort state and reacting to abort event: 1. Mark the source signal as aborted 2. Propagate the aborted state to any dependent signals 3. Run abort steps (run algorithms, fire events) for the source signal 4. Run abort steps for each of the dependent signals PR: whatwg/dom#1295 Change-Id: I65a97eb46b01a0071d661e945f64c90e33954088 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5664649 Commit-Queue: Scott Haseley <[email protected]> Reviewed-by: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1344364}

…dependent signals before firing events, a=testonly Automatic update from web-platform-tests AbortSignal: Propagate aborted state to dependent signals before firing events The implementation and spec of dependent signals assumes and asserts that a dependent signal is aborted if any of its sources have been aborted. But this property does not hold during the abort process, since intermediate states of abort propagation can be observed. For example, calling AbortSignal.any([signal]) in an "abort" event listener for one of signal's sources is a way to observe intermediate state, since the "abort" event fires before the source signal's dependents are updated. To fix this, this CL decouples setting the abort state and reacting to abort event: 1. Mark the source signal as aborted 2. Propagate the aborted state to any dependent signals 3. Run abort steps (run algorithms, fire events) for the source signal 4. Run abort steps for each of the dependent signals PR: whatwg/dom#1295 Change-Id: I65a97eb46b01a0071d661e945f64c90e33954088 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5664649 Commit-Queue: Scott Haseley <[email protected]> Reviewed-by: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1344364} -- wpt-commits: 07a9d09a8fbe95c2c2b439b6a88ef2499543133d wpt-pr: 47653

domfarolino · 2024-08-28T10:37:04Z

I just came across this after looking into the AbortSignal algorithms while working on Observables, and I don't understand this part:

Dependent signals do not themselves have dependent signals, which means it's unnecessary to recursively call "signal abort"

Why is this true? In Observables, we were using dependent AbortSignals for a while, for each signal that was used to subscribe to upstream Observables, and I believe we were able to create a long chain of AbortSignal dependencies such that aborting some initial signal led to the recursive invocation of (old version of) the "signal abort" algorithm, on a whole deep chain of signals. Why does this PR say that is not possible?

Note: We are no longer chaining dependent signals like this in Observables, because the timing of aborting parent signals before dependents did not work for us (WICG/observable#154), but still I think it is very possible to do, no?

saschanaz · 2024-08-28T11:26:45Z

Because in https://dom.spec.whatwg.org/#create-a-dependent-abort-signal, a new dependent signal directly attaches to the top level source signal instead of making chains.

shaseley mentioned this pull request Jul 15, 2024

AbortSignal.any() assertion failure #1293

Closed

domenic approved these changes Aug 14, 2024

View reviewed changes

annevk added the topic: aborting AbortController and AbortSignal label Aug 14, 2024

maraisr mentioned this pull request Aug 16, 2024

Propagate aborted state to dependent signals before firing events WebKit/WebKit#32276

Merged

chromium-wpt-export-bot mentioned this pull request Aug 16, 2024

AbortSignal: Propagate aborted state to dependent signals before firing events web-platform-tests/wpt#47653

Merged

jasnell mentioned this pull request Aug 20, 2024

Pending AbortSignal spec update cloudflare/workerd#2561

Open

This was referenced Aug 20, 2024

AbortSignal.any: Mark dependent abort signals as aborted before firing events denoland/deno#25118

Closed

AbortSignal.any: Mark dependent abort signals as aborted before firing events nodejs/node#54466

Closed

annevk merged commit 0b5f3aa into whatwg:main Aug 21, 2024
2 checks passed

littledivy added a commit to littledivy/deno that referenced this pull request Aug 21, 2024

fix(ext/web): propagate aborted state to dependent signals before fir…

18b618a

…ing events Ref whatwg/dom#1295 Fixes denoland#25118

littledivy mentioned this pull request Aug 21, 2024

fix(ext/web): propagate aborted state to dependent signals before firing events denoland/deno#25131

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark dependent abort signals as aborted before firing events #1295

Mark dependent abort signals as aborted before firing events #1295

shaseley commented Jul 15, 2024 •

edited by pr-preview bot

Loading

annevk commented Aug 13, 2024

jasnell commented Aug 13, 2024

shaseley commented Aug 13, 2024

annevk commented Aug 15, 2024

maraisr commented Aug 15, 2024

vinhill commented Aug 15, 2024

annevk commented Aug 17, 2024

saschanaz commented Aug 20, 2024

shaseley commented Aug 20, 2024

domfarolino commented Aug 28, 2024

saschanaz commented Aug 28, 2024

Mark dependent abort signals as aborted before firing events #1295

Mark dependent abort signals as aborted before firing events #1295

Conversation

shaseley commented Jul 15, 2024 • edited by pr-preview bot Loading

annevk commented Aug 13, 2024

jasnell commented Aug 13, 2024

shaseley commented Aug 13, 2024

annevk commented Aug 15, 2024

maraisr commented Aug 15, 2024

vinhill commented Aug 15, 2024

annevk commented Aug 17, 2024

saschanaz commented Aug 20, 2024

shaseley commented Aug 20, 2024

domfarolino commented Aug 28, 2024

saschanaz commented Aug 28, 2024

shaseley commented Jul 15, 2024 •

edited by pr-preview bot

Loading