Skip to content

Commit

Permalink
Implements the web platform standard reportError API (#1979)
Browse files Browse the repository at this point in the history
  • Loading branch information
jasnell authored and garrettgu10 committed May 13, 2024
1 parent 3b3c4c7 commit 8828214
Show file tree
Hide file tree
Showing 16 changed files with 272 additions and 59 deletions.
30 changes: 18 additions & 12 deletions src/workerd/api/basics.c++
Original file line number Diff line number Diff line change
Expand Up @@ -458,19 +458,25 @@ bool EventTarget::dispatchEventImpl(jsg::Lock& js, jsg::Ref<Event> event) {
// consistency, we should probably trigger fallback behavior if any handler throws, so
// again it doesn't matter. For other types of handlers, e.g. WebSocket 'message', it's
// not clear why one would ever register multiple handlers.
if (warnOnHandlerReturn) KJ_IF_SOME(r, ret) {
warnOnHandlerReturn = false;
// To help make debugging easier, let's tailor the warning a bit if it was a promise.
KJ_IF_SOME(r, ret) {
auto handle = r.getHandle(js);
if (handle->IsPromise()) {
js.logWarning(
kj::str("An event handler returned a promise that will be ignored. Event handlers "
"should not have a return value and should not be async functions."));
} else {
js.logWarning(
kj::str("An event handler returned a value of type \"",
handle->TypeOf(js.v8Isolate),
"\" that will be ignored. Event handlers should not have a return value."));
// Returning true is the same as calling preventDefault() on the event.
if (handle->IsTrue()) {
event->preventDefault();
}
if (warnOnHandlerReturn && !handle->IsBoolean()) {
warnOnHandlerReturn = false;
// To help make debugging easier, let's tailor the warning a bit if it was a promise.
if (handle->IsPromise()) {
js.logWarning(
kj::str("An event handler returned a promise that will be ignored. Event handlers "
"should not have a return value and should not be async functions."));
} else {
js.logWarning(
kj::str("An event handler returned a value of type \"",
handle->TypeOf(js.v8Isolate),
"\" that will be ignored. Event handlers should not have a return value."));
}
}
}
}
Expand Down
49 changes: 49 additions & 0 deletions src/workerd/api/events.c++
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
#include "events.h"

namespace workerd::api {

ErrorEvent::ErrorEvent(kj::String type, ErrorEventInit init)
: Event(kj::mv(type)), init(kj::mv(init)) {}

jsg::Ref<ErrorEvent> ErrorEvent::constructor(
jsg::Lock& js,
kj::String type,
jsg::Optional<ErrorEventInit> init) {
return jsg::alloc<ErrorEvent>(kj::mv(type), kj::mv(init).orDefault({}));
}

kj::StringPtr ErrorEvent::getFilename() {
return init.filename.orDefault(nullptr);
}

kj::StringPtr ErrorEvent::getMessage() {
return init.message.orDefault(nullptr);
}

int ErrorEvent::getLineno() {
return init.lineno.orDefault(0);
}

int ErrorEvent::getColno() {
return init.colno.orDefault(0);
}

jsg::JsValue ErrorEvent::getError(jsg::Lock& js) {
KJ_IF_SOME(error, init.error) {
return error.getHandle(js);
} else {
return js.undefined();
}
}

void ErrorEvent::visitForMemoryInfo(jsg::MemoryTracker& tracker) const {
tracker.trackField("message", init.message);
tracker.trackField("filename", init.filename);
tracker.trackField("error", init.error);
}

void ErrorEvent::visitForGc(jsg::GcVisitor& visitor) {
visitor.visit(init.error);
}

} // namespace workerd::api
56 changes: 56 additions & 0 deletions src/workerd/api/events.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
#pragma once

#include <workerd/jsg/jsg.h>
#include "basics.h"

namespace workerd::api {

class ErrorEvent: public Event {
public:
struct ErrorEventInit {
jsg::Optional<kj::String> message;
jsg::Optional<kj::String> filename;
jsg::Optional<int32_t> lineno;
jsg::Optional<int32_t> colno;
jsg::Optional<jsg::JsRef<jsg::JsValue>> error;
JSG_STRUCT(message, filename, lineno, colno, error);
};

ErrorEvent(kj::String type, ErrorEventInit init);

static jsg::Ref<ErrorEvent> constructor(
jsg::Lock& js,
kj::String type,
jsg::Optional<ErrorEventInit> init);

kj::StringPtr getFilename();
kj::StringPtr getMessage();
int getLineno();
int getColno();
jsg::JsValue getError(jsg::Lock& js);

JSG_RESOURCE_TYPE(ErrorEvent) {
JSG_INHERIT(Event);

JSG_READONLY_PROTOTYPE_PROPERTY(filename, getFilename);
JSG_READONLY_PROTOTYPE_PROPERTY(message, getMessage);
JSG_READONLY_PROTOTYPE_PROPERTY(lineno, getLineno);
JSG_READONLY_PROTOTYPE_PROPERTY(colno, getColno);
JSG_READONLY_PROTOTYPE_PROPERTY(error, getError);

JSG_TS_ROOT();
}

void visitForMemoryInfo(jsg::MemoryTracker& tracker) const;

private:
ErrorEventInit init;

void visitForGc(jsg::GcVisitor& visitor);
};

#define EW_EVENTS_ISOLATE_TYPES \
api::ErrorEvent, \
api::ErrorEvent::ErrorEventInit

} // namespace workerd::api
19 changes: 19 additions & 0 deletions src/workerd/api/global-scope.c++
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include <workerd/api/cache.h>
#include <workerd/api/crypto.h>
#include <workerd/api/events.h>
#include <workerd/api/scheduled.h>
#include <workerd/api/system-streams.h>
#include <workerd/api/trace.h>
Expand Down Expand Up @@ -758,6 +759,24 @@ jsg::Promise<jsg::Ref<Response>> ServiceWorkerGlobalScope::fetch(
return fetchImpl(js, kj::none, kj::mv(requestOrUrl), kj::mv(requestInit));
}

void ServiceWorkerGlobalScope::reportError(jsg::Lock& js, jsg::JsValue error) {
// Per the spec, we are going to first emit an error event on the global object.
// If that event is not prevented, we will log the error to the console. Note
// that we do not throw the error at all.
auto message = v8::Exception::CreateMessage(js.v8Isolate, error);
auto event = jsg::alloc<ErrorEvent>(kj::str("error"),
ErrorEvent::ErrorEventInit {
.message = kj::str(message->Get()),
.filename = kj::str(message->GetScriptResourceName()),
.lineno = jsg::check(message->GetLineNumber(js.v8Context())),
.colno = jsg::check(message->GetStartColumn(js.v8Context())),
.error = jsg::JsRef(js, error)
});
if (dispatchEventImpl(js, kj::mv(event))) {
js.reportError(error);
}
}

double Performance::now() {
// We define performance.now() for compatibility purposes, but due to spectre concerns it
// returns exactly what Date.now() returns.
Expand Down
5 changes: 5 additions & 0 deletions src/workerd/api/global-scope.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include <workerd/jsg/jsg.h>
#include "basics.h"
#include "events.h"
#include "http.h"
#include "hibernation-event-params.h"
#include <workerd/io/io-timers.h>
Expand Down Expand Up @@ -451,6 +452,8 @@ class ServiceWorkerGlobalScope: public WorkerGlobalScope {

jsg::Ref<CacheStorage> getCaches();

void reportError(jsg::Lock& js, jsg::JsValue error);

JSG_RESOURCE_TYPE(ServiceWorkerGlobalScope, CompatibilityFlags::Reader flags) {
JSG_INHERIT(WorkerGlobalScope);

Expand All @@ -466,6 +469,7 @@ class ServiceWorkerGlobalScope: public WorkerGlobalScope {
JSG_METHOD(clearInterval);
JSG_METHOD(queueMicrotask);
JSG_METHOD(structuredClone);
JSG_METHOD(reportError);

JSG_METHOD(fetch);

Expand Down Expand Up @@ -513,6 +517,7 @@ class ServiceWorkerGlobalScope: public WorkerGlobalScope {
JSG_NESTED_TYPE(TransformStream);
JSG_NESTED_TYPE(ByteLengthQueuingStrategy);
JSG_NESTED_TYPE(CountQueuingStrategy);
JSG_NESTED_TYPE(ErrorEvent);

if (flags.getStreamsJavaScriptControllers()) {
JSG_NESTED_TYPE(ReadableStreamBYOBRequest);
Expand Down
2 changes: 2 additions & 0 deletions src/workerd/api/rtti.c++
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <workerd/api/cache.h>
#include <workerd/api/crypto.h>
#include <workerd/api/encoding.h>
#include <workerd/api/events.h>
#include <workerd/api/global-scope.h>
#include <workerd/api/html-rewriter.h>
#include <workerd/api/kv.h>
Expand Down Expand Up @@ -48,6 +49,7 @@
F("cache", EW_CACHE_ISOLATE_TYPES) \
F("crypto", EW_CRYPTO_ISOLATE_TYPES) \
F("encoding", EW_ENCODING_ISOLATE_TYPES) \
F("events", EW_EVENTS_ISOLATE_TYPES) \
F("form-data", EW_FORMDATA_ISOLATE_TYPES) \
F("html-rewriter", EW_HTML_REWRITER_ISOLATE_TYPES) \
F("http", EW_HTTP_ISOLATE_TYPES) \
Expand Down
41 changes: 41 additions & 0 deletions src/workerd/api/tests/reporterror-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { mock } from 'node:test';
import { strictEqual, throws } from 'node:assert';

const boom = new Error('boom');

const handler = mock.fn((event) => {
if (event.error instanceof Error) {
strictEqual(event.message, 'Uncaught Error: boom');
strictEqual(event.colno, 13);
strictEqual(event.lineno, 4);
strictEqual(event.filename, 'worker');
strictEqual(event.error, boom);
} else {
strictEqual(event.message, 'Uncaught boom');
strictEqual(event.colno, 0);
strictEqual(event.lineno, 25);
strictEqual(event.filename, 'worker');
strictEqual(event.error, 'boom');
}
return true;
});

addEventListener('error', handler);

reportError('boom');

throws(() => reportError(), {
message: "Failed to execute 'reportError' on 'ServiceWorkerGlobalScope': " +
"parameter 1 is not of type 'JsValue'."
});

export const reportErrorTest = {
test() {
// TODO(soon): We are limited in what we can test here because we cannot
// inspect the log output and workerd does not implement that WorkerTracer
// used for collecting data for tail workers. The best we can currently do
// is make sure the basic API is working and that the mock fn was called.
reportError(boom);
strictEqual(handler.mock.calls.length, 2);
}
};
15 changes: 15 additions & 0 deletions src/workerd/api/tests/reporterror-test.wd-test
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
using Workerd = import "/workerd/workerd.capnp";

const unitTests :Workerd.Config = (
services = [
( name = "reporterror-test",
worker = (
modules = [
(name = "worker", esModule = embed "reporterror-test.js")
],
compatibilityDate = "2023-01-15",
compatibilityFlags = ["nodejs_compat"],
)
),
],
);
11 changes: 6 additions & 5 deletions src/workerd/api/web-socket.c++
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// https://opensource.org/licenses/Apache-2.0

#include "web-socket.h"
#include "events.h"
#include <workerd/jsg/jsg.h>
#include <workerd/jsg/ser.h>
#include <workerd/io/features.h>
Expand Down Expand Up @@ -972,10 +973,6 @@ jsg::Ref<WebSocketPair> WebSocketPair::constructor() {
return kj::mv(pair);
}

void ErrorEvent::visitForGc(jsg::GcVisitor& visitor) {
visitor.visit(error);
}

void WebSocket::reportError(jsg::Lock& js, kj::Exception&& e) {
reportError(js, js.exceptionToJsValue(kj::cp(e)));
}
Expand All @@ -986,7 +983,11 @@ void WebSocket::reportError(jsg::Lock& js, jsg::JsRef<jsg::JsValue> err) {
auto msg = kj::str(v8::Exception::CreateMessage(js.v8Isolate, err.getHandle(js))->Get());
error = err.addRef(js);

dispatchEventImpl(js, jsg::alloc<ErrorEvent>(js, kj::mv(msg), kj::mv(err)));
dispatchEventImpl(js, jsg::alloc<ErrorEvent>(kj::str("error"),
ErrorEvent::ErrorEventInit {
.message = kj::mv(msg),
.error = kj::mv(err)
}));

// After an error we don't allow further send()s. If the receive loop has also ended then we
// can destroy the connection. Note that we don't set closedOutgoing = true because that flag
Expand Down
42 changes: 0 additions & 42 deletions src/workerd/api/web-socket.h
Original file line number Diff line number Diff line change
Expand Up @@ -128,47 +128,6 @@ class CloseEvent: public Event {
bool clean;
};

class ErrorEvent: public Event {
public:
ErrorEvent(jsg::Lock& js, kj::String&& message, jsg::JsRef<jsg::JsValue> error)
: Event("error"), message(kj::mv(message)), error(kj::mv(error)) {}

static jsg::Ref<ErrorEvent> constructor() = delete;

// Due to the context in which we use this ErrorEvent class (internal errors), the getters for
// filename, lineNo, and colNo are all falsy.
kj::String getFilename() { return nullptr; }
kj::StringPtr getMessage() { return message; }
int getLineno() { return 0; }
int getColno() { return 0; }
jsg::JsValue getError(jsg::Lock& js) { return error.getHandle(js); }


JSG_RESOURCE_TYPE(ErrorEvent) {
JSG_INHERIT(Event);

JSG_READONLY_INSTANCE_PROPERTY(filename, getFilename);
JSG_READONLY_INSTANCE_PROPERTY(message, getMessage);
JSG_READONLY_INSTANCE_PROPERTY(lineno, getLineno);
JSG_READONLY_INSTANCE_PROPERTY(colno, getColno);
JSG_READONLY_INSTANCE_PROPERTY(error, getError);

JSG_TS_ROOT();
// ErrorEvent will be referenced from the `WebSocketEventMap` define
}

void visitForMemoryInfo(jsg::MemoryTracker& tracker) const {
tracker.trackField("message", message);
tracker.trackField("error", error);
}

private:
kj::String message;
jsg::JsRef<jsg::JsValue> error;

void visitForGc(jsg::GcVisitor& visitor);
};

// The forward declaration is necessary so we can make some
// WebSocket methods accessible to WebSocketPair via friend declaration.
class WebSocket;
Expand Down Expand Up @@ -674,7 +633,6 @@ class WebSocket: public EventTarget {
api::CloseEvent::Initializer, \
api::MessageEvent, \
api::MessageEvent::Initializer, \
api::ErrorEvent, \
api::WebSocket, \
api::WebSocketPair
// The list of websocket.h types that are added to worker.c++'s JSG_DECLARE_ISOLATE_TYPE
Expand Down
20 changes: 20 additions & 0 deletions src/workerd/io/worker.c++
Original file line number Diff line number Diff line change
Expand Up @@ -1025,6 +1025,26 @@ Worker::Isolate::Isolate(kj::Own<Api> apiParam,
}
KJ_LOG(INFO, "console warning", message);
});
lock->setErrorReporterCallback([this](jsg::Lock& js, kj::String desc,
const jsg::JsValue& error,
const jsg::JsMessage& message) {
// Only add exception to trace when running within an I/O context with a tracer.
if (IoContext::hasCurrent()) {
auto& ioContext = IoContext::current();
KJ_IF_SOME(tracer, ioContext.getWorkerTracer()) {
addExceptionToTrace(js, ioContext, tracer,
UncaughtExceptionSource::REQUEST_HANDLER, error,
api->getErrorInterfaceTypeHandler(js));
}
}

KJ_IF_SOME(i, impl->inspector) {
jsg::sendExceptionToInspector(js, *i.get(), kj::str(desc), error, message);
}

// Run with --verbose to log JS exceptions to stderr. Useful when running tests.
KJ_LOG(INFO, "uncaught exception", desc);
});
}

// By default, V8's memory pressure level is "none". This tells V8 that no one else on the
Expand Down
Loading

0 comments on commit 8828214

Please sign in to comment.