Bug 1903676 - Update the signal abort algorithm r=smaug

Differential Revision: https://phabricator.services.mozilla.com/D220836

dom/abort/AbortFollower.h

@@ -45,6 +45,9 @@ class AbortFollower : public nsISupports {
 };
 
 /*
+ * TODO(krosylight): The only consumer of this is Fetch. It would be nice to
+ * merge this back to AbortSignal as it's quite a duplication right now.
+ *
  * AbortSignalImpl is a minimal implementation without an associated global
  * and without event dispatching, those are added in AbortSignal.
  * See Bug 1478101
@@ -60,7 +63,7 @@ class AbortSignalImpl : public nsISupports, public SupportsWeakPtr {
   // Helper for other DOM code
   JS::Value RawReason() const;
 
-  virtual void SignalAbort(JS::Handle<JS::Value> aReason);
+  void SignalAbort(JS::Handle<JS::Value> aReason);
 
  protected:
   // Subclasses of this class must call these Traverse and Unlink functions
@@ -72,6 +75,10 @@ class AbortSignalImpl : public nsISupports, public SupportsWeakPtr {
 
   virtual ~AbortSignalImpl() { UnlinkFollowers(); }
 
+  virtual void SignalAbortWithDependents();
+
+  virtual void RunAbortSteps();
+
   void SetAborted(JS::Handle<JS::Value> aReason);
 
   JS::Heap<JS::Value> mReason;
@@ -83,7 +90,10 @@ class AbortSignalImpl : public nsISupports, public SupportsWeakPtr {
 
   void UnlinkFollowers();
 
-  // Raw pointers.  |AbortFollower::Follow| adds to this array, and
+  // TODO(krosylight): We should rename all names around the term "Follow". See
+  // bug 1873648.
+  //
+  // |AbortFollower::Follow| adds to this array, and
   // |AbortFollower::Unfollow| (also called by the destructor) will remove
   // from this array.  Finally, calling |SignalAbort()| will (after running all
   // abort algorithms) empty this and make all contained followers |Unfollow()|.

dom/abort/AbortSignal.cpp

@@ -41,17 +41,35 @@ void AbortSignalImpl::GetReason(JSContext* aCx,
 
 JS::Value AbortSignalImpl::RawReason() const { return mReason.get(); }
 
-// https://dom.spec.whatwg.org/#abortsignal-signal-abort steps 1-4
+// https://dom.spec.whatwg.org/#abortsignal-signal-abort
 void AbortSignalImpl::SignalAbort(JS::Handle<JS::Value> aReason) {
-  // Step 1.
+  // Step 1: If signal is aborted, then return.
   if (mAborted) {
     return;
   }
 
-  // Step 2.
+  // Step 2: Set signal’s abort reason to reason if it is given; otherwise to a
+  // new "AbortError" DOMException.
+  //
+  // (But given AbortSignalImpl is supposed to run without JS context, the
+  // DOMException creation is deferred to the getter.)
   SetAborted(aReason);
 
-  // Step 3.
+  // Step 3 - 6
+  SignalAbortWithDependents();
+}
+
+void AbortSignalImpl::SignalAbortWithDependents() {
+  // AbortSignalImpl cannot have dependents, so just run abort steps for itself.
+  RunAbortSteps();
+}
+
+// https://dom.spec.whatwg.org/#run-the-abort-steps
+// This skips event firing as AbortSignalImpl is not supposed to be exposed to
+// JS. It's done instead in AbortSignal::RunAbortSteps.
+void AbortSignalImpl::RunAbortSteps() {
+  // Step 1: For each algorithm of signal’s abort algorithms: run algorithm.
+  //
   // When there are multiple followers, the follower removal algorithm
   // https://dom.spec.whatwg.org/#abortsignal-remove could be invoked in an
   // earlier algorithm to remove a later algorithm, so |mFollowers| must be a
@@ -61,7 +79,7 @@ void AbortSignalImpl::SignalAbort(JS::Handle<JS::Value> aReason) {
     follower->RunAbortAlgorithm();
   }
 
-  // Step 4.
+  // Step 2: Empty signal’s abort algorithms.
   UnlinkFollowers();
 }
 
@@ -277,14 +295,26 @@ already_AddRefed<AbortSignal> AbortSignal::Any(
   RefPtr<AbortSignal> resultSignal =
       new AbortSignal(aGlobal, false, JS::UndefinedHandleValue);
 
-  // Step 2. For each signal of signals: if signal is aborted, then set
-  // resultSignal's abort reason to signal's abort reason and return
-  // resultSignal.
-  for (const auto& signal : aSignals) {
-    if (signal->Aborted()) {
-      JS::Rooted<JS::Value> reason(RootingCx(), signal->RawReason());
-      resultSignal->SetAborted(reason);
-      return resultSignal.forget();
+  if (!aSignals.IsEmpty()) {
+    // (Prepare for step 2 which uses the reason of this. Cannot use
+    // RawReason because that can cause constructing new DOMException for each
+    // dependent signal instead of sharing the single one.)
+    AutoJSAPI jsapi;
+    if (!jsapi.Init(aGlobal)) {
+      return nullptr;
+    }
+    JSContext* cx = jsapi.cx();
+
+    // Step 2. For each signal of signals: if signal is aborted, then set
+    // resultSignal's abort reason to signal's abort reason and return
+    // resultSignal.
+    for (const auto& signal : aSignals) {
+      if (signal->Aborted()) {
+        JS::Rooted<JS::Value> reason(cx);
+        signal->GetReason(cx, &reason);
+        resultSignal->SetAborted(reason);
+        return resultSignal.forget();
+      }
     }
   }
 
@@ -339,17 +369,56 @@ void AbortSignal::ThrowIfAborted(JSContext* aCx, ErrorResult& aRv) {
   }
 }
 
-// https://dom.spec.whatwg.org/#abortsignal-signal-abort
-void AbortSignal::SignalAbort(JS::Handle<JS::Value> aReason) {
-  // Step 1, in case "signal abort" algorithm is called directly
-  if (Aborted()) {
-    return;
+// Step 3 - 6 of https://dom.spec.whatwg.org/#abortsignal-signal-abort
+void AbortSignal::SignalAbortWithDependents() {
+  // Step 3: Let dependentSignalsToAbort be a new list.
+  nsTArray<RefPtr<AbortSignal>> dependentSignalsToAbort;
+
+  // mDependentSignals can go away after this function.
+  nsTArray<RefPtr<AbortSignal>> dependentSignals = std::move(mDependentSignals);
+
+  if (!dependentSignals.IsEmpty()) {
+    // (Prepare for step 4.1.1 which uses the reason of this. Cannot use
+    // RawReason because that can cause constructing new DOMException for each
+    // dependent signal instead of sharing the single one.)
+    AutoJSAPI jsapi;
+    if (!jsapi.Init(GetParentObject())) {
+      return;
+    }
+    JSContext* cx = jsapi.cx();
+    JS::Rooted<JS::Value> reason(cx);
+    GetReason(cx, &reason);
+
+    // Step 4. For each dependentSignal of signal’s dependent signals:
+    for (const auto& dependentSignal : dependentSignals) {
+      MOZ_ASSERT(dependentSignal->mSourceSignals.Contains(this));
+      // Step 4.1: If dependentSignal is not aborted, then:
+      if (!dependentSignal->Aborted()) {
+        // Step 4.1.1: Set dependentSignal’s abort reason to signal’s abort
+        // reason.
+        dependentSignal->SetAborted(reason);
+        // Step 4.1.2: Append dependentSignal to dependentSignalsToAbort.
+        dependentSignalsToAbort.AppendElement(dependentSignal);
+      }
+    }
   }
 
-  // Steps 1-4.
-  AbortSignalImpl::SignalAbort(aReason);
+  // Step 5: Run the abort steps for signal.
+  RunAbortSteps();
 
-  // Step 5. Fire an event named abort at this signal
+  // Step 6: For each dependentSignal of dependentSignalsToAbort, run the abort
+  // steps for dependentSignal.
+  for (const auto& dependentSignal : dependentSignalsToAbort) {
+    dependentSignal->RunAbortSteps();
+  }
+}
+
+// https://dom.spec.whatwg.org/#run-the-abort-steps
+void AbortSignal::RunAbortSteps() {
+  // Step 1 - 2:
+  AbortSignalImpl::RunAbortSteps();
+
+  // Step 3. Fire an event named abort at this signal.
   EventInit init;
   init.mBubbles = false;
   init.mCancelable = false;
@@ -358,14 +427,6 @@ void AbortSignal::SignalAbort(JS::Handle<JS::Value> aReason) {
   event->SetTrusted(true);
 
   DispatchEvent(*event);
-
-  // Step 6. Abort dependentSignals of this signal
-  for (const auto& dependant : mDependentSignals) {
-    MOZ_ASSERT(dependant->mSourceSignals.Contains(this));
-    dependant->SignalAbort(aReason);
-  }
-  // clear dependent signals so that they might be garbage collected
-  mDependentSignals.Clear();
 }
 
 bool AbortSignal::Dependent() const { return mDependent; }

dom/abort/AbortSignal.h

@@ -55,9 +55,6 @@ class AbortSignal : public DOMEventTargetHelper, public AbortSignalImpl {
 
   void ThrowIfAborted(JSContext* aCx, ErrorResult& aRv);
 
-  // AbortSignalImpl
-  void SignalAbort(JS::Handle<JS::Value> aReason) override;
-
   virtual bool IsTaskSignal() const { return false; }
 
   bool Dependent() const;
@@ -67,6 +64,10 @@ class AbortSignal : public DOMEventTargetHelper, public AbortSignalImpl {
 
   void MakeDependentOn(AbortSignal* aSignal);
 
+  void SignalAbortWithDependents() override;
+
+  void RunAbortSteps() override;
+
   nsTArray<WeakPtr<AbortSignal>> mSourceSignals;
   nsTArray<RefPtr<AbortSignal>> mDependentSignals;
 

testing/web-platform/meta/dom/abort/abort-signal-any.any.js.ini

@@ -1,20 +0,0 @@
-[abort-signal-any.any.worker.html]
-  expected:
-    if not debug: ERROR
-    CRASH
-  [Dependent signals for AbortSignal.any() are marked aborted before abort events fire (using AbortController)]
-    expected: FAIL
-
-  [Dependent signals for AbortSignal.any() are aborted correctly for reentrant aborts (using AbortController)]
-    expected: FAIL
-
-
-[abort-signal-any.any.html]
-  expected:
-    if not debug: ERROR
-    CRASH
-  [Dependent signals for AbortSignal.any() are marked aborted before abort events fire (using AbortController)]
-    expected: FAIL
-
-  [Dependent signals for AbortSignal.any() are aborted correctly for reentrant aborts (using AbortController)]
-    expected: FAIL

testing/web-platform/tests/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>

testing/web-platform/tests/dom/abort/resources/abort-signal-any-tests.js

@@ -221,4 +221,20 @@ function abortSignalAnyTests(signalInterface, controllerInterface) {
     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}`);
 }