Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: propagate aborted state to dependent signals before firing events #54826

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 34 additions & 4 deletions lib/internal/abort_controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@
// in https://github.com/mysticatea/abort-controller (MIT license)

const {
ArrayPrototypePush,
ObjectAssign,
ObjectDefineProperties,
ObjectDefineProperty,
ObjectSetPrototypeOf,
PromiseResolve,
SafeFinalizationRegistry,
SafeSet,
Expand Down Expand Up @@ -372,18 +374,46 @@ ObjectDefineProperty(AbortSignal.prototype, SymbolToStringTag, {

defineEventHandler(AbortSignal.prototype, 'abort');

// https://dom.spec.whatwg.org/#dom-abortsignal-abort
function abortSignal(signal, reason) {
// 1. If signal is aborted, then return.
jazelly marked this conversation as resolved.
Show resolved Hide resolved
if (signal[kAborted]) return;

// 2. Set signal's abort reason to reason if it is given;
// otherwise to a new "AbortError" DOMException.
signal[kAborted] = true;
signal[kReason] = reason;
// 3. Let dependentSignalsToAbort be a new list.
const dependentSignalsToAbort = ObjectSetPrototypeOf([], null);
// 4. For each dependentSignal of signal's dependent signals:
signal[kDependantSignals]?.forEach((s) => {
jazelly marked this conversation as resolved.
Show resolved Hide resolved
const dependentSignal = s.deref();
// 1. If dependentSignal is not aborted, then:
if (dependentSignal && !dependentSignal[kAborted]) {
// 1. Set dependentSignal's abort reason to signal's abort reason.
dependentSignal[kReason] = reason;
dependentSignal[kAborted] = true;
// 2. Append dependentSignal to dependentSignalsToAbort.
ArrayPrototypePush(dependentSignalsToAbort, dependentSignal);
}
});

// 5. Run the abort steps for signal
runAbort(signal);
// 6. For each dependentSignal of dependentSignalsToAbort,
// run the abort steps for dependentSignal.
for (let i = 0; i < dependentSignalsToAbort.length; i++) {
const dependentSignal = dependentSignalsToAbort[i];
runAbort(dependentSignal);
}
}

// To run the abort steps for an AbortSignal signal
function runAbort(signal) {
const event = new Event('abort', {
[kTrustEvent]: true,
});
signal.dispatchEvent(event);
signal[kDependantSignals]?.forEach((s) => {
const signalRef = s.deref();
if (signalRef) abortSignal(signalRef, reason);
});
}

class AbortController {
Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/wpt/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ Last update:
- common: https://github.com/web-platform-tests/wpt/tree/dbd648158d/common
- compression: https://github.com/web-platform-tests/wpt/tree/5aa50dd415/compression
- console: https://github.com/web-platform-tests/wpt/tree/767ae35464/console
- dom/abort: https://github.com/web-platform-tests/wpt/tree/d1f1ecbd52/dom/abort
- dom/abort: https://github.com/web-platform-tests/wpt/tree/0143fe244b/dom/abort
- dom/events: https://github.com/web-platform-tests/wpt/tree/0a811c5161/dom/events
- encoding: https://github.com/web-platform-tests/wpt/tree/5aa50dd415/encoding
- fetch/data-urls/resources: https://github.com/web-platform-tests/wpt/tree/7c79d998ff/fetch/data-urls/resources
Expand Down
6 changes: 6 additions & 0 deletions test/fixtures/wpt/dom/abort/WEB_FEATURES.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
features:
- name: aborting
files: "**"
- name: abortsignal-any
files:
- abort-signal-any.any.js
23 changes: 23 additions & 0 deletions test/fixtures/wpt/dom/abort/abort-signal-any-crash.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<!DOCTYPE html>
<html class=test-wait>
<head>
<title>AbortSignal::Any when source signal was garbage collected</title>
<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1908466">
<link rel="author" title="Vincent Hilla" href="mailto:[email protected]">
<script src="/common/gc.js"></script>
</head>
<body>
<p>Test passes if the browser does not crash.</p>
<script>
async function test() {
let controller = new AbortController();
let signal = AbortSignal.any([controller.signal]);
controller = undefined;
await garbageCollect();
AbortSignal.any([signal]);
document.documentElement.classList.remove('test-wait');
}
test();
</script>
</body>
</html>
11 changes: 11 additions & 0 deletions test/fixtures/wpt/dom/abort/crashtests/any-on-abort.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<!DOCTYPE html>
<meta charset="utf-8">
<script>
let b;
window.addEventListener("DOMContentLoaded", () => {
let a = new AbortController()
b = AbortSignal.any([a.signal])
a.signal.addEventListener("abort", () => { AbortSignal.any([b]) }, { })
a.abort(undefined)
})
</script>
55 changes: 55 additions & 0 deletions test/fixtures/wpt/dom/abort/resources/abort-signal-any-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -182,4 +182,59 @@ 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}`);

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}`);
}
2 changes: 1 addition & 1 deletion test/fixtures/wpt/versions.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
"path": "console"
},
"dom/abort": {
"commit": "d1f1ecbd52f2eab3b7fe5dc1b20b41174f1341ce",
"commit": "0143fe244b3d622441717ce630e0114eb204f9a7",
"path": "dom/abort"
},
"dom/events": {
Expand Down
Loading