Merge pull request #2778 from cloudflare/jsnell/handle-queuemicrotask…

…-reporterror-errors

src/workerd/api/BUILD.bazel

@@ -565,3 +565,9 @@ wd_test(
     args = ["--experimental"],
     data = ["tests/cross-context-promise-test.js"],
 )
+
+wd_test(
+    src = "tests/error-in-error-event-test.wd-test",
+    args = ["--experimental"],
+    data = ["tests/error-in-error-event-test.js"],
+)

src/workerd/api/global-scope.c++

@@ -706,7 +706,28 @@ void ServiceWorkerGlobalScope::queueMicrotask(jsg::Lock& js, v8::Local<v8::Funct
               return jsg::check(
                   function->Call(js.v8Context(), js.v8Null(), argv.size(), argv.data()));
             }, [&](jsg::Value exception) {
-              reportError(js, jsg::JsValue(exception.getHandle(js)));
+              // The reportError call itself can potentially throw errors. Let's catch
+              // and report them as well.
+              js.tryCatch([&] { reportError(js, jsg::JsValue(exception.getHandle(js))); },
+                  [&](jsg::Value exception) {
+                // An error was thrown by the 'error' event handler. That's unfortunate.
+                // Let's log the error and just continue. It won't be possible to actually
+                // catch or handle this error so logging is really the only way to notify
+                // folks about it.
+                auto val = jsg::JsValue(exception.getHandle(js));
+                // If the value is an object that has a stack propery, log that so we get
+                // the stack trace if it is an exception.
+                KJ_IF_SOME(obj, val.tryCast<jsg::JsObject>()) {
+                auto stack = obj.get(js, "stack"_kj);
+                if (!stack.isUndefined()) {
+                js.reportError(stack);
+                return;
+                }
+                } else {
+                }  // Here to avoid a compile warning
+                // Otherwise just log the stringified value generically.
+                js.reportError(val);
+              });
               return js.v8Undefined();
             });
           }));
@@ -798,6 +819,16 @@ void ServiceWorkerGlobalScope::reportError(jsg::Lock& js, jsg::JsValue error) {
         .colno = jsg::check(message->GetStartColumn(js.v8Context())),
         .error = jsg::JsRef(js, error)});
   if (dispatchEventImpl(js, kj::mv(event))) {
+    // If the value is an object that has a stack propery, log that so we get
+    // the stack trace if it is an exception.
+    KJ_IF_SOME(obj, error.tryCast<jsg::JsObject>()) {
+      auto stack = obj.get(js, "stack"_kj);
+      if (!stack.isUndefined()) {
+        js.reportError(stack);
+        return;
+      }
+    }
+    // Otherwise just log the stringified value generically.
     js.reportError(error);
   }
 }

src/workerd/api/tests/error-in-error-event-test.js

@@ -0,0 +1,31 @@
+import { strictEqual } from 'assert';
+
+export const test = {
+  async test(_, env) {
+    const resp = await env.subrequest.fetch('http://example.org');
+    strictEqual(await resp.text(), 'ok');
+  },
+};
+
+export default {
+  async fetch() {
+    // Throwing an error in the error event listener should not be catchable directly
+    // but should cause the IoContext to be aborted with the error.
+    addEventListener(
+      'error',
+      () => {
+        // This error is not going to be catachable. The best we can do is to log it.
+        // Unfortunately, workerd currently does not give us any mechanism to verify
+        // that it was logged in the test. Let's make sure the response is handled
+        // correctly at least.
+        throw new Error('boom (real)');
+      },
+      { once: true }
+    );
+    queueMicrotask(() => {
+      throw new Error('boom (unused)');
+    });
+    await scheduler.wait(10);
+    return new Response('ok');
+  },
+};

src/workerd/api/tests/error-in-error-event-test.wd-test

@@ -0,0 +1,18 @@
+using Workerd = import "/workerd/workerd.capnp";
+
+const unitTests :Workerd.Config = (
+  services = [
+    ( name = "error-in-error-event-test",
+      worker = (
+        modules = [
+          (name = "worker", esModule = embed "error-in-error-event-test.js")
+        ],
+        compatibilityDate = "2024-09-15",
+        compatibilityFlags = ["nodejs_compat_v2"],
+        bindings = [
+          (name = "subrequest", service = "error-in-error-event-test")
+        ],
+      )
+    ),
+  ],
+);

src/workerd/jsg/jsg.h

@@ -2639,6 +2639,8 @@ class Lock {
   void runMicrotasks();
   void terminateExecution();
 
+  // Logs and reports the error to tail workers (if called within an request),
+  // the inspector (if attached), or to KJ_LOG(Info).
   virtual void reportError(const JsValue& value) = 0;
 
 private: