lib: propagate aborted state to dependent signals before firing events

PR-URL: #54826 Fixes: #54466 Fixes: #54601 Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Oct 4, 2024 · f7c3b03 · f7c3b03
1 parent 341b6d9
commit f7c3b03
Show file tree

Hide file tree

Showing 7 changed files with 131 additions and 6 deletions.
diff --git a/lib/internal/abort_controller.js b/lib/internal/abort_controller.js
@@ -4,9 +4,11 @@
 // in https://github.com/mysticatea/abort-controller (MIT license)
 
 const {
+  ArrayPrototypePush,
   ObjectAssign,
   ObjectDefineProperties,
   ObjectDefineProperty,
+  ObjectSetPrototypeOf,
   PromiseResolve,
   SafeFinalizationRegistry,
   SafeSet,
@@ -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.
   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) => {
+    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 {

diff --git a/test/fixtures/wpt/README.md b/test/fixtures/wpt/README.md
@@ -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

diff --git a/test/fixtures/wpt/dom/abort/WEB_FEATURES.yml b/test/fixtures/wpt/dom/abort/WEB_FEATURES.yml
@@ -0,0 +1,6 @@
+features:
+- name: aborting
+  files: "**"
+- name: abortsignal-any
+  files:
+  - abort-signal-any.any.js
diff --git a/test/fixtures/wpt/dom/abort/abort-signal-any-crash.html b/test/fixtures/wpt/dom/abort/abort-signal-any-crash.html
@@ -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:vhilla@mozilla.com">
+    <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>
diff --git a/test/fixtures/wpt/dom/abort/crashtests/any-on-abort.html b/test/fixtures/wpt/dom/abort/crashtests/any-on-abort.html
@@ -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>
diff --git a/test/fixtures/wpt/dom/abort/resources/abort-signal-any-tests.js b/test/fixtures/wpt/dom/abort/resources/abort-signal-any-tests.js
@@ -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}`);
 }
diff --git a/test/fixtures/wpt/versions.json b/test/fixtures/wpt/versions.json
@@ -12,7 +12,7 @@
     "path": "console"
   },
   "dom/abort": {
-    "commit": "d1f1ecbd52f2eab3b7fe5dc1b20b41174f1341ce",
+    "commit": "0143fe244b3d622441717ce630e0114eb204f9a7",
     "path": "dom/abort"
   },
   "dom/events": {