AbortSignal: Add abort reason to DOM's AbortSignal

This CL follows whatwg/dom#1027, which adds an abort reason property[1] to DOM's AbortSignal API (i.e. signal.reason). This change also removes the aborted flag from the AbortSignal, and instead uses the "aborted" predicate[2] to do similar checks. [1] https://dom.spec.whatwg.org/#abortsignal-abort-reason [2] https://dom.spec.whatwg.org/#abortsignal-aborted Intent to Ship: https://groups.google.com/a/chromium.org/g/blink-dev/c/T6B5czAke1I Bug: 1263410 Change-Id: I25beb697d8e73dc73fdce2bc0d24b5e1bd52b78e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3293551 Reviewed-by: Kent Tamura <[email protected]> Reviewed-by: Yutaka Hirano <[email protected]> Reviewed-by: Kentaro Hara <[email protected]> Commit-Queue: Nidhi Jaju <[email protected]> Cr-Commit-Position: refs/heads/main@{#945295} NOKEYCHECK=True GitOrigin-RevId: 155b2353a12c9e0173b8d33b32b115a2406a8131
mjfroman · Nov 25, 2021 · e720725 · e720725
1 parent 0698ce6
commit e720725
Show file tree

Hide file tree

Showing 26 changed files with 121 additions and 76 deletions.
diff --git a/blink/renderer/core/app_history/app_history.cc b/blink/renderer/core/app_history/app_history.cc
@@ -772,7 +772,7 @@ void AppHistory::FinalizeWithAbortedNavigationError(
     ongoing_navigate_event_ = nullptr;
   }
   if (ongoing_navigation_signal_) {
-    ongoing_navigation_signal_->SignalAbort();
+    ongoing_navigation_signal_->SignalAbort(script_state);
     ongoing_navigation_signal_ = nullptr;
   }
 

diff --git a/blink/renderer/core/document_transition/document_transition_test.cc b/blink/renderer/core/document_transition/document_transition_test.cc
@@ -555,7 +555,7 @@ TEST_P(DocumentTransitionTest, AbortSignal) {
       transition->prepare(script_state, &prepare_options, exception_state));
   EXPECT_EQ(GetState(transition), State::kPreparing);
 
-  abort_signal->SignalAbort();
+  abort_signal->SignalAbort(script_state);
   prepare_tester.WaitUntilSettled();
   EXPECT_TRUE(prepare_tester.IsRejected());
   EXPECT_EQ(GetState(transition), State::kIdle);

diff --git a/blink/renderer/core/dom/abort_controller.cc b/blink/renderer/core/dom/abort_controller.cc
@@ -4,7 +4,9 @@
 
 #include "third_party/blink/renderer/core/dom/abort_controller.h"
 
+#include "third_party/blink/renderer/bindings/core/v8/v8_throw_dom_exception.h"
 #include "third_party/blink/renderer/core/dom/abort_signal.h"
+#include "third_party/blink/renderer/platform/bindings/exception_code.h"
 #include "third_party/blink/renderer/platform/heap/visitor.h"
 
 namespace blink {
@@ -18,8 +20,17 @@ AbortController::AbortController(AbortSignal* signal) : signal_(signal) {}
 
 AbortController::~AbortController() = default;
 
-void AbortController::abort() {
-  signal_->SignalAbort();
+void AbortController::abort(ScriptState* script_state) {
+  ScriptValue reason(
+      script_state->GetIsolate(),
+      V8ThrowDOMException::CreateOrEmpty(script_state->GetIsolate(),
+                                         DOMExceptionCode::kAbortError,
+                                         "signal is aborted without reason"));
+  abort(script_state, reason);
+}
+
+void AbortController::abort(ScriptState* script_state, ScriptValue reason) {
+  signal_->SignalAbort(script_state, reason);
 }
 
 void AbortController::Trace(Visitor* visitor) const {

diff --git a/blink/renderer/core/dom/abort_controller.h b/blink/renderer/core/dom/abort_controller.h
@@ -13,6 +13,7 @@ namespace blink {
 
 class AbortSignal;
 class ExecutionContext;
+class ScriptValue;
 
 // Implementation of https://dom.spec.whatwg.org/#interface-abortcontroller
 // See also design doc at
@@ -32,7 +33,8 @@ class CORE_EXPORT AbortController : public ScriptWrappable {
   AbortSignal* signal() const { return signal_.Get(); }
 
   // https://dom.spec.whatwg.org/#dom-abortcontroller-abort
-  void abort();
+  void abort(ScriptState*);
+  void abort(ScriptState*, ScriptValue reason);
 
   void Trace(Visitor*) const override;
 

diff --git a/blink/renderer/core/dom/abort_controller.idl b/blink/renderer/core/dom/abort_controller.idl
@@ -10,5 +10,5 @@
     [CallWith=ExecutionContext, Measure] constructor();
     [SameObject] readonly attribute AbortSignal signal;
 
-    void abort();
+    [CallWith=ScriptState] void abort(optional any reason);
 };
diff --git a/blink/renderer/core/dom/abort_signal.cc b/blink/renderer/core/dom/abort_signal.cc
@@ -7,10 +7,12 @@
 #include <utility>
 
 #include "base/callback.h"
+#include "third_party/blink/renderer/bindings/core/v8/v8_throw_dom_exception.h"
 #include "third_party/blink/renderer/core/dom/events/event.h"
 #include "third_party/blink/renderer/core/event_target_names.h"
 #include "third_party/blink/renderer/core/event_type_names.h"
 #include "third_party/blink/renderer/core/execution_context/execution_context.h"
+#include "third_party/blink/renderer/platform/bindings/exception_code.h"
 #include "third_party/blink/renderer/platform/bindings/script_state.h"
 #include "third_party/blink/renderer/platform/heap/persistent.h"
 #include "third_party/blink/renderer/platform/heap/visitor.h"
@@ -25,12 +27,31 @@ AbortSignal::~AbortSignal() = default;
 
 // static
 AbortSignal* AbortSignal::abort(ScriptState* script_state) {
+  ScriptValue reason(
+      script_state->GetIsolate(),
+      V8ThrowDOMException::CreateOrEmpty(script_state->GetIsolate(),
+                                         DOMExceptionCode::kAbortError,
+                                         "signal is aborted without reason"));
+  return abort(script_state, reason);
+}
+
+AbortSignal* AbortSignal::abort(ScriptState* script_state, ScriptValue reason) {
+  DCHECK(!reason.IsEmpty());
   AbortSignal* signal =
       MakeGarbageCollected<AbortSignal>(ExecutionContext::From(script_state));
-  signal->aborted_flag_ = true;
+  signal->abort_reason_ = reason;
   return signal;
 }
 
+ScriptValue AbortSignal::reason(ScriptState* script_state) {
+  DCHECK(script_state->GetIsolate()->InContext());
+  if (abort_reason_.IsEmpty()) {
+    return ScriptValue(script_state->GetIsolate(),
+                       v8::Undefined(script_state->GetIsolate()));
+  }
+  return abort_reason_;
+}
+
 const AtomicString& AbortSignal::InterfaceName() const {
   return event_target_names::kAbortSignal;
 }
@@ -40,13 +61,14 @@ ExecutionContext* AbortSignal::GetExecutionContext() const {
 }
 
 void AbortSignal::AddAlgorithm(base::OnceClosure algorithm) {
-  if (aborted_flag_)
+  if (aborted())
     return;
   abort_algorithms_.push_back(std::move(algorithm));
 }
 
-void AbortSignal::AddSignalAbortAlgorithm(AbortSignal* dependent_signal) {
-  if (aborted_flag_)
+void AbortSignal::AddSignalAbortAlgorithm(ScriptState* script_state,
+                                          AbortSignal* dependent_signal) {
+  if (aborted())
     return;
 
   // The signal should be kept alive as long as parentSignal is allow chained
@@ -59,14 +81,30 @@ void AbortSignal::AddSignalAbortAlgorithm(AbortSignal* dependent_signal) {
   // that leaks the |execution_context_| as there is no explicit event removing
   // the root anymore.
   abort_algorithms_.emplace_back(WTF::Bind(
-      &AbortSignal::SignalAbort, WrapWeakPersistent(dependent_signal)));
+      &AbortSignal::SignalAbortWithParent, WrapWeakPersistent(dependent_signal),
+      WrapPersistent(script_state), WrapWeakPersistent(this)));
   dependent_signals_.push_back(dependent_signal);
 }
 
-void AbortSignal::SignalAbort() {
-  if (aborted_flag_)
+void AbortSignal::SignalAbortWithParent(ScriptState* script_state,
+                                        AbortSignal* parent_signal) {
+  SignalAbort(script_state, parent_signal->reason(script_state));
+}
+
+void AbortSignal::SignalAbort(ScriptState* script_state) {
+  ScriptValue reason(
+      script_state->GetIsolate(),
+      V8ThrowDOMException::CreateOrEmpty(script_state->GetIsolate(),
+                                         DOMExceptionCode::kAbortError,
+                                         "signal is aborted without reason"));
+  SignalAbort(script_state, reason);
+}
+
+void AbortSignal::SignalAbort(ScriptState* script_state, ScriptValue reason) {
+  DCHECK(!reason.IsEmpty());
+  if (aborted())
     return;
-  aborted_flag_ = true;
+  abort_reason_ = reason;
   for (base::OnceClosure& closure : abort_algorithms_) {
     std::move(closure).Run();
   }
@@ -75,16 +113,17 @@ void AbortSignal::SignalAbort() {
   DispatchEvent(*Event::Create(event_type_names::kAbort));
 }
 
-void AbortSignal::Follow(AbortSignal* parentSignal) {
-  if (aborted_flag_)
+void AbortSignal::Follow(ScriptState* script_state, AbortSignal* parentSignal) {
+  if (aborted())
     return;
-  if (parentSignal->aborted_flag_)
-    SignalAbort();
+  if (parentSignal->aborted())
+    SignalAbort(script_state, parentSignal->reason(script_state));
 
-  parentSignal->AddSignalAbortAlgorithm(this);
+  parentSignal->AddSignalAbortAlgorithm(script_state, this);
 }
 
 void AbortSignal::Trace(Visitor* visitor) const {
+  visitor->Trace(abort_reason_);
   visitor->Trace(execution_context_);
   visitor->Trace(dependent_signals_);
   EventTargetWithInlineData::Trace(visitor);

diff --git a/blink/renderer/core/dom/abort_signal.h b/blink/renderer/core/dom/abort_signal.h
@@ -6,6 +6,7 @@
 #define THIRD_PARTY_BLINK_RENDERER_CORE_DOM_ABORT_SIGNAL_H_
 
 #include "base/callback_forward.h"
+#include "third_party/blink/renderer/bindings/core/v8/script_value.h"
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/dom/events/event_target.h"
 #include "third_party/blink/renderer/platform/heap/collection_support/heap_vector.h"
@@ -27,7 +28,9 @@ class CORE_EXPORT AbortSignal : public EventTargetWithInlineData {
 
   // abort_signal.idl
   static AbortSignal* abort(ScriptState*);
-  bool aborted() const { return aborted_flag_; }
+  static AbortSignal* abort(ScriptState*, ScriptValue reason);
+  ScriptValue reason(ScriptState*);
+  bool aborted() const { return !abort_reason_.IsEmpty(); }
   DEFINE_ATTRIBUTE_EVENT_LISTENER(abort, kAbort)
 
   const AtomicString& InterfaceName() const override;
@@ -54,21 +57,28 @@ class CORE_EXPORT AbortSignal : public EventTargetWithInlineData {
   // https://dom.spec.whatwg.org/#abortsignal-add. Run all algorithms that were
   // added by AddAlgorithm(), in order of addition, then fire an "abort"
   // event. Does nothing if called more than once.
-  void SignalAbort();
+  void SignalAbort(ScriptState*);
+  void SignalAbort(ScriptState*, ScriptValue reason);
 
   // The "follow" algorithm from the standard:
   // https://dom.spec.whatwg.org/#abortsignal-follow
   // |this| is the followingSignal described in the standard.
-  void Follow(AbortSignal* parentSignal);
+  void Follow(ScriptState*, AbortSignal* parentSignal);
 
   virtual bool IsTaskSignal() const { return false; }
 
   void Trace(Visitor*) const override;
 
  private:
-  void AddSignalAbortAlgorithm(AbortSignal*);
-
-  bool aborted_flag_ = false;
+  void SignalAbortWithParent(ScriptState*, AbortSignal* parent_signal);
+  void AddSignalAbortAlgorithm(ScriptState*, AbortSignal* dependent_signal);
+
+  // https://dom.spec.whatwg.org/#abortsignal-abort-reason
+  // There is one difference from the spec. The value is empty instead of
+  // undefined when this signal is not aborted. This is because
+  // ScriptValue::IsUndefined requires callers to enter a V8 context whereas
+  // ScriptValue::IsEmpty does not.
+  ScriptValue abort_reason_;
   Vector<base::OnceClosure> abort_algorithms_;
   HeapVector<Member<AbortSignal>> dependent_signals_;
   Member<ExecutionContext> execution_context_;

diff --git a/blink/renderer/core/dom/abort_signal.idl b/blink/renderer/core/dom/abort_signal.idl
@@ -11,9 +11,10 @@
         CallWith=ScriptState,
         Measure,
         NewObject
-    ] static AbortSignal abort();
+    ] static AbortSignal abort(optional any reason);
 
     readonly attribute boolean aborted;
+    [CallWith=ScriptState] readonly attribute any reason;
 
     attribute EventHandler onabort;
 };
diff --git a/blink/renderer/core/fetch/body_stream_buffer_test.cc b/blink/renderer/core/fetch/body_stream_buffer_test.cc
@@ -697,7 +697,7 @@ TEST_F(BodyStreamBufferTest, AbortSignalMakesAborted) {
       scope.GetScriptState(), src, signal, /*cached_metadata_handler=*/nullptr);
 
   EXPECT_FALSE(buffer->IsAborted());
-  signal->SignalAbort();
+  signal->SignalAbort(scope.GetScriptState());
   EXPECT_TRUE(buffer->IsAborted());
 }
 
@@ -729,7 +729,7 @@ TEST_F(BodyStreamBufferTest,
       scope.GetScriptState(), src, signal, /*cached_metadata_handler=*/nullptr);
 
   checkpoint.Call(1);
-  signal->SignalAbort();
+  signal->SignalAbort(scope.GetScriptState());
 
   checkpoint.Call(2);
   buffer->StartLoading(loader, client, ASSERT_NO_EXCEPTION);
@@ -766,7 +766,7 @@ TEST_F(BodyStreamBufferTest, AbortAfterStartLoadingCallsDataLoaderClientAbort) {
   buffer->StartLoading(loader, client, ASSERT_NO_EXCEPTION);
 
   checkpoint.Call(2);
-  signal->SignalAbort();
+  signal->SignalAbort(scope.GetScriptState());
 
   checkpoint.Call(3);
 }
@@ -802,7 +802,7 @@ TEST_F(BodyStreamBufferTest,
   test::RunPendingTasks();
 
   checkpoint.Call(2);
-  signal->SignalAbort();
+  signal->SignalAbort(scope.GetScriptState());
 
   checkpoint.Call(3);
 }

diff --git a/blink/renderer/core/fetch/request.cc b/blink/renderer/core/fetch/request.cc
@@ -591,7 +591,7 @@ Request* Request::CreateRequestWithRequestOrString(
 
   // "If |signal| is not null, then make |r|’s signal follow |signal|."
   if (signal)
-    r->signal_->Follow(signal);
+    r->signal_->Follow(script_state, signal);
 
   // "If |r|'s request's mode is "no-cors", run these substeps:
   if (r->GetRequest()->Mode() == network::mojom::RequestMode::kNoCors) {
@@ -973,7 +973,7 @@ Request* Request::clone(ScriptState* script_state,
   headers->SetGuard(headers_->GetGuard());
   auto* signal =
       MakeGarbageCollected<AbortSignal>(ExecutionContext::From(script_state));
-  signal->Follow(signal_);
+  signal->Follow(script_state, signal_);
   return MakeGarbageCollected<Request>(script_state, request, headers, signal);
 }
 

diff --git a/blink/renderer/modules/cache_storage/cache.cc b/blink/renderer/modules/cache_storage/cache.cc
@@ -276,7 +276,7 @@ class Cache::BarrierCallbackForPutResponse final
     if (stopped_)
       return;
     if (abort_controller_)
-      abort_controller_->abort();
+      abort_controller_->abort(resolver_->GetScriptState());
     blob_list_.clear();
     stopped_ = true;
   }
@@ -1117,8 +1117,10 @@ ScriptPromise Cache::AddAllImpl(ScriptState* script_state,
   for (wtf_size_t i = 0; i < request_list.size(); ++i) {
     // Chain the AbortSignal objects together so the requests will abort if
     // the |barrier_callback| encounters an error.
-    if (barrier_callback->Signal())
-      request_list[i]->signal()->Follow(barrier_callback->Signal());
+    if (barrier_callback->Signal()) {
+      request_list[i]->signal()->Follow(script_state,
+                                        barrier_callback->Signal());
+    }
 
     V8RequestInfo* info = MakeGarbageCollected<V8RequestInfo>(request_list[i]);
 

diff --git a/blink/renderer/modules/scheduler/dom_scheduler.cc b/blink/renderer/modules/scheduler/dom_scheduler.cc
@@ -96,7 +96,7 @@ ScriptPromise DOMScheduler::postTask(
                                : kDefaultPriority;
     task_signal = CreateTaskSignalFor(priority);
     if (options->hasSignal())
-      task_signal->Follow(options->signal());
+      task_signal->Follow(script_state, options->signal());
   }
 
   DCHECK(task_signal);

diff --git a/blink/web_tests/external/wpt/dom/abort/event.any-expected.txt b/blink/web_tests/external/wpt/dom/abort/event.any-expected.txt
diff --git a/blink/web_tests/external/wpt/dom/abort/event.any.worker-expected.txt b/blink/web_tests/external/wpt/dom/abort/event.any.worker-expected.txt
diff --git a/blink/web_tests/external/wpt/streams/piping/abort.any-expected.txt b/blink/web_tests/external/wpt/streams/piping/abort.any-expected.txt
@@ -6,16 +6,16 @@ PASS a signal argument '-1' should cause pipeTo() to reject
 PASS a signal argument '[object AbortSignal]' should cause pipeTo() to reject
 PASS an aborted signal should cause the writable stream to reject with an AbortError
 FAIL (reason: 'null') all the error objects should be the same object promise_rejects_exactly: pipeTo rejects with abort reason function "function() { throw e }" threw object "AbortError: Pipe aborted." but we expected it to throw null
-FAIL (reason: 'undefined') all the error objects should be the same object assert_equals: signal.reason should be error expected (object) object "AbortError: Pipe aborted." but got (undefined) undefined
+FAIL (reason: 'undefined') all the error objects should be the same object assert_equals: signal.reason should be error expected object "AbortError: Pipe aborted." but got object "AbortError: signal is aborted without reason"
 FAIL (reason: 'error1: error1') all the error objects should be the same object promise_rejects_exactly: pipeTo rejects with abort reason function "function() { throw e }" threw object "AbortError: Pipe aborted." but we expected it to throw object "error1: error1"
 PASS preventCancel should prevent canceling the readable
 PASS preventAbort should prevent aborting the readable
 PASS preventCancel and preventAbort should prevent canceling the readable and aborting the readable
 FAIL (reason: 'null') abort should prevent further reads promise_rejects_exactly: pipeTo rejects with abort reason function "function() { throw e }" threw object "AbortError: Pipe aborted." but we expected it to throw null
-FAIL (reason: 'undefined') abort should prevent further reads assert_equals: signal.reason should be error expected (object) object "AbortError: Pipe aborted." but got (undefined) undefined
+FAIL (reason: 'undefined') abort should prevent further reads assert_equals: signal.reason should be error expected object "AbortError: Pipe aborted." but got object "AbortError: signal is aborted without reason"
 FAIL (reason: 'error1: error1') abort should prevent further reads promise_rejects_exactly: pipeTo rejects with abort reason function "function() { throw e }" threw object "AbortError: Pipe aborted." but we expected it to throw object "error1: error1"
 FAIL (reason: 'null') all pending writes should complete on abort promise_rejects_exactly: pipeTo rejects with abort reason function "function() { throw e }" threw object "AbortError: Pipe aborted." but we expected it to throw null
-FAIL (reason: 'undefined') all pending writes should complete on abort assert_equals: signal.reason should be error expected (object) object "AbortError: Pipe aborted." but got (undefined) undefined
+FAIL (reason: 'undefined') all pending writes should complete on abort assert_equals: signal.reason should be error expected object "AbortError: Pipe aborted." but got object "AbortError: signal is aborted without reason"
 FAIL (reason: 'error1: error1') all pending writes should complete on abort promise_rejects_exactly: pipeTo rejects with abort reason function "function() { throw e }" threw object "AbortError: Pipe aborted." but we expected it to throw object "error1: error1"
 PASS a rejection from underlyingSource.cancel() should be returned by pipeTo()
 PASS a rejection from underlyingSink.abort() should be returned by pipeTo()