DOM: Observer error handler called with subscribe callback exception

This CL implements the proper Observable error handling when a subscribe
callback throws an error, when their is an existing `Observer#error()`
handler passed in. Observable error handling semantics have the error
being piped through to the user's error handler (when it exists), and
reported to the global otherwise.

This CL also adds another detached global test, where `subscribe()` is
called from within a detached document. We assert that the callback is
never run.

R=masonf@chromium.org

Bug: 1485981
Change-Id: Iccbdbdcbe2492c6850b938bea111ca7e0ab91b01
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4922212
Commit-Queue: Dominic Farolino <dom@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1209426}

third_party/blink/renderer/core/dom/observable.cc

@@ -31,17 +31,44 @@ Observable::Observable(ExecutionContext* execution_context,
   DCHECK(RuntimeEnabledFeatures::ObservableAPIEnabled(execution_context));
 }
 
-void Observable::subscribe(Observer* observer) {
+void Observable::subscribe(ScriptState* script_state, Observer* observer) {
+  // Cannot subscribe to an Observable that was constructed in a detached
+  // context, because this might involve reporting an exception with the global,
+  // which relies on a valid `ScriptState`.
+  if (!script_state->ContextIsValid()) {
+    CHECK(!GetExecutionContext());
+    return;
+  }
+
   // Build and initialize a `Subscriber` with a dictionary of `Observer`
   // callbacks.
   Subscriber* subscriber = MakeGarbageCollected<Subscriber>(
       PassKey(), GetExecutionContext(), observer);
 
   DCHECK(subscribe_callback_);
-  // Exceptions are "reported", per
-  // https://html.spec.whatwg.org/C#report-the-exception, and do not interrupt
-  // the ordinary control flow here.
-  subscribe_callback_->InvokeAndReportException(nullptr, subscriber);
+  // Ordinarily we'd just invoke `subscribe_callback_` with
+  // `InvokeAndReportException()`, so that any exceptions get reported to the
+  // global. However, Observables have special semantics with the error handler
+  // passed in via `observer`. Specifically, if the subscribe callback throws an
+  // exception (that doesn't go through the manual `Subscriber::error()`
+  // pathway), we still give that method a first crack at handling the
+  // exception. This does one of two things:
+  //   1. Lets the provided `Observer#error()` handler run with the thrown
+  //      exception, if such handler was provided
+  //   2. Reports the exception to the global if no such handler was provided.
+  // See `Subscriber::error()` for more details.
+  //
+  // In either case, no exception in this path interrupts the ordinary flow of
+  // control. Therefore, `subscribe()` will never synchronously throw an
+  // exception.
+
+  ScriptState::Scope scope(script_state);
+  v8::TryCatch try_catch(script_state->GetIsolate());
+  std::ignore = subscribe_callback_->Invoke(nullptr, subscriber);
+  if (try_catch.HasCaught()) {
+    subscriber->error(script_state, ScriptValue(script_state->GetIsolate(),
+                                                try_catch.Exception()));
+  }
 }
 
 void Observable::Trace(Visitor* visitor) const {

third_party/blink/renderer/core/dom/observable.h

@@ -30,7 +30,7 @@ class CORE_EXPORT Observable final : public ScriptWrappable,
   Observable(ExecutionContext*, V8SubscribeCallback*);
 
   // API methods:
-  void subscribe(Observer*);
+  void subscribe(ScriptState*, Observer*);
 
   void Trace(Visitor*) const override;
 

third_party/blink/renderer/core/dom/observable.idl

@@ -18,6 +18,6 @@ dictionary Observer {
 [Exposed=(Window,Worker), RuntimeEnabled=ObservableAPI]
 interface Observable {
     [CallWith=ScriptState, MeasureAs=ObservableConstructor] constructor(SubscribeCallback callback);
-    void subscribe(optional Observer observer = {});
+    [CallWith=ScriptState] void subscribe(optional Observer observer = {});
 };
 

third_party/blink/web_tests/external/wpt/dom/observable/tentative/observable-constructor.window.js

@@ -70,3 +70,31 @@ promise_test(async t => {
   `);
 }, "Subscriber.error() does not \"report the exception\" even when an " +
    "`error()` handler is not present, when it is invoked in a detached document");
+
+promise_test(async t => {
+  // Make this available off the global so the child can reach it.
+  window.results = [];
+  const contentWin = await loadIframeAndReturnContentWindow();
+
+  // Set a global error handler on the iframe document's window, and verify that
+  // it is never called (because the thing triggering the error happens when the
+  // document is detached, and "reporting the exception" relies on an attached
+  // document).
+  contentWin.addEventListener("error",
+      t.unreached_func("Error should not be called"), { once: true });
+
+  contentWin.eval(`
+    const parentResults = parent.results;
+    const source = new Observable((subscriber) => {
+      // This should never run.
+      parentResults.push('subscribe');
+    });
+
+    // Detach the iframe and try to subscribe.
+    window.frameElement.remove();
+    parentResults.push('detached');
+    source.subscribe();
+  `);
+
+  assert_array_equals(results, ["detached"], "Subscribe callback is never invoked");
+}, "Cannot subscribe to an Observable in a detached document");