diff --git a/docs/ns-builtin-modules.md b/docs/ns-builtin-modules.md index 24f01c771..fde20fdc7 100644 --- a/docs/ns-builtin-modules.md +++ b/docs/ns-builtin-modules.md @@ -732,12 +732,13 @@ resolvers read the same table differently: Seven rows are public today: `ns:module`, `ns:runtime`, `ns:util`, `node:module`, `node:url`, `node:util`, `node:worker_threads`. - The **internal require** builtins receive (previous section) is the only - thing that can name an internal-only row. Five rows are marked that way: + thing that can name an internal-only row. Six rows are marked that way: `internal/broadcast-channel`, `internal/dom-exception`, `internal/events`, - `internal/message-channel`, `internal/message-event`. Their exports carry - capabilities app code must not hold — listener-accounting hook keys, the - error-reporter setter, base classes that must be the runtime's own rather - than whatever a global currently names. + `internal/message-channel`, `internal/message-event`, + `internal/worker-events`. Their exports carry capabilities app code must not + hold — listener-accounting hook keys, the error-reporter setter, the key a + worker's rebuilt error travels under, base classes that must be the + runtime's own rather than whatever a global currently names. - Builtins with **no row at all** (the intrinsics snapshot, the require factory, the console formatter) are invoked straight from their native call sites. There is no specifier that could reach them and nothing to mark. diff --git a/docs/worker-threads.md b/docs/worker-threads.md index 35078728b..95c23c6ca 100644 --- a/docs/worker-threads.md +++ b/docs/worker-threads.md @@ -54,7 +54,7 @@ means deliberately unsupported. | `threadName` | shim | Always `undefined`. | | `workerData` | shim | Always `null` — see below. | | `parentPort` | shim | `null` on the main isolate. Inside a worker, a `MessagePort`-shaped `EventTarget` over the worker's existing parent channel: `postMessage` forwards to the global `postMessage`, `message`/`messageerror` are re-dispatched from the worker global scope, `start()` and `close()` are no-ops. It is **not** a real port: not transferable, no queue of its own. | -| `Worker` | shim | A class over the runtime's global `Worker` with a small Node-style emitter (`on`/`once`/`off`/`removeListener`) for `message`, `messageerror`, `error`, `online` and `exit`. `postMessage(value, transfer)` and `terminate()` forward. `online` is emitted off a microtask after construction, not from the thread. Unsupported options throw a `TypeError` naming the option: `workerData`, `env`, `eval`, `transferList`, and `stdin`/`stdout`/`stderr` when explicitly truthy. The runtime's own `Worker` options (`androidPriority`) ride along untouched — the native constructor ignores keys it does not know. | +| `Worker` | shim | A class over the runtime's global `Worker` with a small Node-style emitter (`on`/`once`/`off`/`removeListener`) for `message`, `messageerror`, `error`, `online` and `exit`. An `error` listener receives the worker's error rebuilt as an `Error` (see below). `postMessage(value, transfer)` and `terminate()` forward. `online` is emitted off a microtask after construction, not from the thread. Unsupported options throw a `TypeError` naming the option: `workerData`, `env`, `eval`, `transferList`, and `stdin`/`stdout`/`stderr` when explicitly truthy. The runtime's own `Worker` options (`androidPriority`) ride along untouched — the native constructor ignores keys it does not know. | | `postMessageToThread` | throws | `Error: postMessageToThread is not supported in this runtime`. | | `moveMessagePortToContext` | throws | `Error: moveMessagePortToContext is not supported in this runtime`. | | `locks` | absent | Web Locks are not implemented; the property does not exist. | @@ -79,6 +79,22 @@ finished. `terminate()` therefore resolves with `0` and emits `exit` with code `0` on the way, and that is the only path that emits it. A worker that ends by its own `close()` produces no `exit`. +### An `error` listener receives a rebuilt `Error` + +Node hands an `error` listener the worker's error deserialized on the parent. +Only strings cross the isolate boundary here, so the parent rebuilds it from +the thrown value's `name` and `message`, with the worker's stack as its +`stack`. A built-in name (`TypeError`, `RangeError`, …) rebuilds with that +constructor, so `instanceof` holds; any other name, a subclass's or a +`DOMException`'s, is an own `name` on an `Error`. Other properties, such as a +`code` or a `cause`, are not carried. A thrown value with no string `message`, +such as a string or a number, arrives as an `Error` whose message is its string +form, where Node hands over the value itself. + +Once an `error` listener has received it, the error counts as handled and is +not reported on the parent's global scope. With no `error` listener, the +parent's global `error` event carries the same rebuilt `Error`. + ### A worker error carries no `error` object, and the worker scope's `onerror` is not an event An error the worker scope leaves unhandled reaches the parent as a real diff --git a/eslint.config.mjs b/eslint.config.mjs index f467cd4d2..7804b5e45 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -32,7 +32,7 @@ const capturedStatics = [ // Captured constructors. A destructure from `primordials` shadows the global, // so these only fire on the unguarded reference. -const restrictedGlobals = ['Date', 'FinalizationRegistry', 'Map', 'Number', 'Promise', 'Proxy', 'RangeError', 'Set', 'String', 'TypeError', 'Uint8Array', 'Uint32Array', 'WeakRef', 'WeakSet'].map((name) => ({ +const restrictedGlobals = ['Date', 'EvalError', 'FinalizationRegistry', 'Map', 'Number', 'Promise', 'Proxy', 'RangeError', 'ReferenceError', 'Set', 'String', 'SyntaxError', 'TypeError', 'Uint8Array', 'Uint32Array', 'URIError', 'WeakRef', 'WeakSet'].map((name) => ({ name, message: `Destructure ${name} from primordials — builtins must not read intrinsics off globals user code can replace.`, })); diff --git a/test-app/app/src/main/assets/app/tests/messaging/domExceptionThrowingWorker.js b/test-app/app/src/main/assets/app/tests/messaging/domExceptionThrowingWorker.js new file mode 100644 index 000000000..e280aff0c --- /dev/null +++ b/test-app/app/src/main/assets/app/tests/messaging/domExceptionThrowingWorker.js @@ -0,0 +1 @@ +throw new DOMException("aborted in a worker", "AbortError"); diff --git a/test-app/app/src/main/assets/app/tests/messaging/onerrorRethrowingWorker.js b/test-app/app/src/main/assets/app/tests/messaging/onerrorRethrowingWorker.js new file mode 100644 index 000000000..49e26dec8 --- /dev/null +++ b/test-app/app/src/main/assets/app/tests/messaging/onerrorRethrowingWorker.js @@ -0,0 +1,12 @@ +// The scope's onerror throws an error whose message holds a NUL and an +// unpaired surrogate, and whose stack getter throws. +onerror = function () { + var error = new TypeError("before\0after \uD800"); + Object.defineProperty(error, "stack", { + get: function () { throw new RangeError("thrown by the stack getter"); } + }); + throw error; +}; +onmessage = function () { + throw new Error("thrown by onmessage"); +}; diff --git a/test-app/app/src/main/assets/app/tests/messaging/parentPortThrowingWorker.js b/test-app/app/src/main/assets/app/tests/messaging/parentPortThrowingWorker.js new file mode 100644 index 000000000..3505b8e3c --- /dev/null +++ b/test-app/app/src/main/assets/app/tests/messaging/parentPortThrowingWorker.js @@ -0,0 +1,4 @@ +var parentPort = require("node:worker_threads").parentPort; +parentPort.on("message", function () { + throw new TypeError("thrown by a parentPort listener"); +}); diff --git a/test-app/app/src/main/assets/app/tests/testMessaging.js b/test-app/app/src/main/assets/app/tests/testMessaging.js index c06a46037..65188fe03 100644 --- a/test-app/app/src/main/assets/app/tests/testMessaging.js +++ b/test-app/app/src/main/assets/app/tests/testMessaging.js @@ -232,6 +232,7 @@ describe("Messaging runtime edges", function () { expect(seen.length).toBe(1); expect(seen[0].message).toContain("boom from worker"); expect(seen[0].error instanceof Error).toBe(true); + expect(seen[0].error.message).toBe("boom from worker"); worker.terminate(); done(); }; @@ -255,6 +256,94 @@ describe("Messaging runtime edges", function () { } }; }); + + it("lets a node:worker_threads 'error' listener consume the error", function (done) { + var wt = require("node:worker_threads"); + var globalErrors = []; + var listener = function (event) { + globalErrors.push(event.message); + event.preventDefault(); + }; + addEventListener("error", listener); + var worker = new wt.Worker("~/tests/messaging/throwingWorker.js"); + worker.on("error", function (error) { + setTimeout(function () { + removeEventListener("error", listener); + expect(error instanceof Error).toBe(true); + expect(error.name).toBe("Error"); + expect(error.message).toBe("boom from worker"); + expect(error.stack).toContain("boom from worker"); + expect(globalErrors).toEqual([]); + worker.terminate(); + done(); + }, SETTLE); + }); + }); + + it("hands a node:worker_threads 'error' listener a thrown DOMException's name and message", function (done) { + var wt = require("node:worker_threads"); + var worker = new wt.Worker("~/tests/messaging/domExceptionThrowingWorker.js"); + worker.on("error", function (error) { + expect(error instanceof Error).toBe(true); + expect(error.name).toBe("AbortError"); + expect(error.message).toBe("aborted in a worker"); + worker.terminate(); + done(); + }); + }); + + it("rebuilds the error a worker's onerror threw exactly, even when its stack getter throws", function (done) { + var wt = require("node:worker_threads"); + var worker = new wt.Worker("~/tests/messaging/onerrorRethrowingWorker.js"); + worker.on("error", function (error) { + expect(error instanceof TypeError).toBe(true); + expect(error.message).toBe("before\0after \uD800"); + worker.terminate(); + done(); + }); + worker.postMessage("go"); + }); + + it("calls a node:worker_threads once listener once when an earlier listener emits again", function () { + var wt = require("node:worker_threads"); + var worker = new wt.Worker("~/tests/eventLoopEchoWorker.js"); + var calls = 0; + var nested = false; + worker.on("probe", function () { + if (!nested) { + nested = true; + worker.emit("probe"); + } + }); + worker.once("probe", function () { calls++; }); + worker.emit("probe"); + worker.terminate(); + expect(calls).toBe(1); + }); + + it("routes a throw from a parentPort listener to the parent's 'error' listeners", function (done) { + var wt = require("node:worker_threads"); + var worker = new wt.Worker("~/tests/messaging/parentPortThrowingWorker.js"); + var errors = []; + var finish = function () { + expect(errors.length).toBe(1); + expect(errors[0] instanceof TypeError).toBe(true); + expect(errors[0].name).toBe("TypeError"); + expect(errors[0].message).toBe("thrown by a parentPort listener"); + worker.terminate(); + done(); + }; + // Nothing else settles the spec when the error never arrives. + var guard = setTimeout(finish, 10000); + worker.on("error", function (error) { + errors.push(error); + if (errors.length === 1) { + clearTimeout(guard); + setTimeout(finish, SETTLE); + } + }); + worker.postMessage("go"); + }); }); describe("AbortSignal handler attribute accounting", function () { diff --git a/test-app/runtime/src/main/cpp/ArgConverter.h b/test-app/runtime/src/main/cpp/ArgConverter.h index 263a6ba3d..e8d941592 100644 --- a/test-app/runtime/src/main/cpp/ArgConverter.h +++ b/test-app/runtime/src/main/cpp/ArgConverter.h @@ -60,7 +60,7 @@ class ArgConverter { } else { auto isolate = v8::Isolate::GetCurrent(); v8::String::Utf8Value str(isolate, s); - return {*str}; + return {*str, static_cast(str.length())}; } } diff --git a/test-app/runtime/src/main/cpp/CallbackHandlers.cpp b/test-app/runtime/src/main/cpp/CallbackHandlers.cpp index 27e031c07..7fd5d3d7f 100644 --- a/test-app/runtime/src/main/cpp/CallbackHandlers.cpp +++ b/test-app/runtime/src/main/cpp/CallbackHandlers.cpp @@ -1842,6 +1842,9 @@ static void ExtractTryCatchInfo(Isolate *isolate, Local context, TryCat } } + // `stack` may be an accessor. One that throws only costs the stack: caught + // here, its exception cannot replace the one `tc` holds. + TryCatch stackTc(isolate); Local outStackTrace = tc.StackTrace(context).FromMaybe(Local()); if (!outStackTrace.IsEmpty()) { Local stackTraceStr = @@ -1902,14 +1905,16 @@ void CallbackHandlers::CallWorkerScopeOnErrorHandle(Isolate *isolate, TryCatch & // parent sees the handler's own error, and only that one. if (innerTc.HasCaught()) { ExtractTryCatchInfo(isolate, context, innerTc, message, source, stackTrace, lineno); - wrapper->PassUncaughtExceptionFromWorkerToParent(message, source, stackTrace, lineno); + wrapper->PassUncaughtExceptionFromWorkerToParent(message, source, stackTrace, lineno, + innerTc.Exception()); return; } // Unhandled at the worker scope - including when there is no scope // handler at all - so it becomes the parent's error event. ExtractTryCatchInfo(isolate, context, tc, message, source, stackTrace, lineno); - wrapper->PassUncaughtExceptionFromWorkerToParent(message, source, stackTrace, lineno); + wrapper->PassUncaughtExceptionFromWorkerToParent(message, source, stackTrace, lineno, + tc.Exception()); } catch (NativeScriptException &ex) { ex.ReThrowToV8(); } catch (std::exception e) { diff --git a/test-app/runtime/src/main/cpp/NativeScriptException.cpp b/test-app/runtime/src/main/cpp/NativeScriptException.cpp index 16fde7c03..2c4b57ebf 100644 --- a/test-app/runtime/src/main/cpp/NativeScriptException.cpp +++ b/test-app/runtime/src/main/cpp/NativeScriptException.cpp @@ -918,7 +918,9 @@ void PromiseRejectionTracker::Drain() { !workerWrapper->IsTerminating() && !workerWrapper->IsDisposed()) { string forwarded = message; string forwardedStack = stackTrace; + Local forwardedValue = reason; if (!thrown.IsEmpty()) { + forwardedValue = thrown; forwarded = ToDetailString(isolate, thrown); // The handler's own stack replaces the reason's; an accessor // that throws costs the stack, never the forward. @@ -930,7 +932,7 @@ void PromiseRejectionTracker::Drain() { } } workerWrapper->PassUncaughtExceptionFromWorkerToParent( - forwarded, "", forwardedStack, 0); + forwarded, "", forwardedStack, 0, forwardedValue); } } } else { diff --git a/test-app/runtime/src/main/cpp/NsBuiltinModules.cpp b/test-app/runtime/src/main/cpp/NsBuiltinModules.cpp index ee5e81f7a..69c8b5c49 100644 --- a/test-app/runtime/src/main/cpp/NsBuiltinModules.cpp +++ b/test-app/runtime/src/main/cpp/NsBuiltinModules.cpp @@ -68,6 +68,7 @@ constexpr Registration kRegistry[] = { {"internal/events", BuiltinId::kEvents, nullptr, true}, {"internal/message-channel", BuiltinId::kMessageChannel, messaging::CreateBinding, true}, {"internal/message-event", BuiltinId::kMessageEvent, nullptr, true}, + {"internal/worker-events", BuiltinId::kWorkerEvents, nullptr, true}, }; constexpr const char* kDebugKey = "debug"; diff --git a/test-app/runtime/src/main/cpp/WorkerEvents.cpp b/test-app/runtime/src/main/cpp/WorkerEvents.cpp index aea42802a..2b5792e36 100644 --- a/test-app/runtime/src/main/cpp/WorkerEvents.cpp +++ b/test-app/runtime/src/main/cpp/WorkerEvents.cpp @@ -98,28 +98,28 @@ void WorkerEvents::EmitMessage(Isolate* isolate, Local receiver, (void)state->emitMessage.Get(isolate)->Call(context, receiver, 3, args).ToLocal(&result); } -bool WorkerEvents::EmitError(Isolate* isolate, Local receiver, - const std::string& message, const std::string& source, - const std::string& stackTrace, int lineNumber) { +MaybeLocal WorkerEvents::EmitError(Isolate* isolate, Local receiver, + const std::string& message, const std::string& source, + const std::string& stackTrace, int lineNumber, + Local errorName, + Local errorMessage) { auto* state = RuntimeState::For(isolate); if (state == nullptr || state->emitError.IsEmpty()) { - return false; + return MaybeLocal(); } Runtime* runtime = Runtime::TryGetRuntime(isolate); if (runtime == nullptr) { - return false; + return MaybeLocal(); } Local context = runtime->GetContext(); - Local args[4]{ArgConverter::ConvertToV8String(isolate, message), + Local args[6]{ArgConverter::ConvertToV8String(isolate, message), ArgConverter::ConvertToV8String(isolate, source), Number::New(isolate, lineNumber), - ArgConverter::ConvertToV8String(isolate, stackTrace)}; - Local result; - if (!state->emitError.Get(isolate)->Call(context, receiver, 4, args).ToLocal(&result)) { - return false; - } - return result->BooleanValue(isolate); + ArgConverter::ConvertToV8String(isolate, stackTrace), + errorName, + errorMessage}; + return state->emitError.Get(isolate)->Call(context, receiver, 6, args); } } // namespace tns diff --git a/test-app/runtime/src/main/cpp/WorkerEvents.h b/test-app/runtime/src/main/cpp/WorkerEvents.h index 9a011df69..8404370d6 100644 --- a/test-app/runtime/src/main/cpp/WorkerEvents.h +++ b/test-app/runtime/src/main/cpp/WorkerEvents.h @@ -39,16 +39,22 @@ class WorkerEvents { /* * Dispatches a cancelable `error` ErrorEvent on `receiver` (the Worker - * object, on the parent isolate) and returns whether a handler took - * ownership of it - either by returning truthy from the `onerror` - * attribute or by calling preventDefault(). Only primitives cross the - * isolate boundary, so the event carries no error object. A listener that - * throws leaves the exception pending for the caller's TryCatch and - * reports as unhandled. False before Init has run. + * object, on the parent isolate). Only primitives cross the isolate + * boundary, so the event carries no error object; the worker's error is + * rebuilt from `errorName`, `errorMessage` and `stackTrace`. Returns that + * error when no handler took ownership of the event, for the caller to + * report on the parent's global scope, and undefined when one did - either + * by returning truthy from the `onerror` attribute or by calling + * preventDefault(). Empty when a listener threw, which leaves the + * exception pending for the caller's TryCatch, and before Init has run. */ - static bool EmitError(v8::Isolate* isolate, v8::Local receiver, - const std::string& message, const std::string& source, - const std::string& stackTrace, int lineNumber); + static v8::MaybeLocal EmitError(v8::Isolate* isolate, + v8::Local receiver, + const std::string& message, + const std::string& source, + const std::string& stackTrace, int lineNumber, + v8::Local errorName, + v8::Local errorMessage); }; } // namespace tns diff --git a/test-app/runtime/src/main/cpp/WorkerWrapper.cpp b/test-app/runtime/src/main/cpp/WorkerWrapper.cpp index 7bc272b24..ab4d74727 100644 --- a/test-app/runtime/src/main/cpp/WorkerWrapper.cpp +++ b/test-app/runtime/src/main/cpp/WorkerWrapper.cpp @@ -3,6 +3,7 @@ #include #include +#include #include #include "ArgConverter.h" @@ -32,6 +33,60 @@ namespace tns { namespace { +/* + * The `name` and `message` the parent rebuilds the worker's error from. They + * stay UTF-16 on the way, so both arrive exactly as thrown, embedded NULs and + * unpaired surrogates included. + */ +struct ThrownErrorText { + std::u16string name = u"Error"; + std::u16string message; +}; + +std::u16string ToUtf16(Isolate* isolate, Local value) { + std::u16string result(value->Length(), u'\0'); + value->WriteV2(isolate, 0, value->Length(), reinterpret_cast(result.data())); + return result; +} + +Local FromUtf16(Isolate* isolate, const std::u16string& value) { + return String::NewFromTwoByte(isolate, reinterpret_cast(value.data()), + NewStringType::kNormal, static_cast(value.size())) + .ToLocalChecked(); +} + +/* + * Reads the text off the value the worker threw. An object's `name` and + * `message` are taken when they are strings, so an Error or a DOMException + * keeps both; anything else becomes an Error whose message is the value's + * string form. Either property may be a getter, and one that throws leaves the + * default in place. + */ +ThrownErrorText DescribeThrownValue(Isolate* isolate, Local context, Local thrown) { + HandleScope handleScope(isolate); + TryCatch tc(isolate); + ThrownErrorText text; + Local detail; + if (thrown->ToDetailString(context).ToLocal(&detail)) { + text.message = ToUtf16(isolate, detail); + } + if (!thrown->IsObject() || thrown->IsFunction()) { + return text; + } + auto object = thrown.As(); + Local value; + if (object->Get(context, ArgConverter::ConvertToV8String(isolate, "name")).ToLocal(&value) && + value->IsString()) { + text.name = ToUtf16(isolate, value.As()); + } + if (object->Get(context, ArgConverter::ConvertToV8String(isolate, "message")) + .ToLocal(&value) && + value->IsString()) { + text.message = ToUtf16(isolate, value.As()); + } + return text; +} + /* * An uncaught exception from a JS callback the event loop's internal lane * drove. While a pump is on the stack, returning to Java is not the next act @@ -102,12 +157,12 @@ void ReportEntryRejection(Isolate* isolate, Local reason, thrownStack = NativeScriptException::GetErrorStackTrace(stack); } wrapper->PassUncaughtExceptionFromWorkerToParent( - ArgConverter::ToString(isolate, thrown), "", thrownStack, 0); + ArgConverter::ToString(isolate, thrown), "", thrownStack, 0, thrown); return; } } - wrapper->PassUncaughtExceptionFromWorkerToParent(message, "", stackTrace, 0); + wrapper->PassUncaughtExceptionFromWorkerToParent(message, "", stackTrace, 0, reason); } } // namespace @@ -374,34 +429,53 @@ void WorkerWrapper::FireMessageOnParentWorkerObject(int workerId, void WorkerWrapper::PassUncaughtExceptionFromWorkerToParent(const std::string& message, const std::string& filename, const std::string& stackTrace, - int lineno) { + int lineno, Local thrown) { auto parentTasks = parentTasks_.lock(); if (parentTasks == nullptr) { // the parent runtime is gone (e.g. a parent worker that shut down) return; } + // Read here, on the worker's isolate. A report with no thrown value, such + // as the heap-limit one made from inside a GC, touches no v8 handle. + std::optional thrownText; + if (!thrown.IsEmpty()) { + Isolate* workerIsolate = Isolate::GetCurrent(); + thrownText = DescribeThrownValue(workerIsolate, workerIsolate->GetCurrentContext(), thrown); + } + int workerId = workerId_; std::string threadName = threadName_; Isolate* parentIsolate = parentIsolate_; parentTasks->PostInternal([workerId, message, filename, stackTrace, lineno, threadName, - parentIsolate]() { + parentIsolate, thrownText]() { v8::Locker locker(parentIsolate); Isolate::Scope isolate_scope(parentIsolate); HandleScope handle_scope(parentIsolate); auto context = Runtime::GetRuntime(parentIsolate)->GetContext(); Context::Scope context_scope(context); + // Without a thrown value, the error is an Error carrying the report's + // message. + Local errorName = thrownText + ? FromUtf16(parentIsolate, thrownText->name) + : ArgConverter::ConvertToV8String(parentIsolate, "Error"); + Local errorMessage = thrownText + ? FromUtf16(parentIsolate, thrownText->message) + : ArgConverter::ConvertToV8String(parentIsolate, message); WorkerWrapper::FireErrorOnParentWorkerObject(workerId, message, stackTrace, filename, - lineno, threadName); + lineno, threadName, errorName, + errorMessage); }); } void WorkerWrapper::FireErrorOnParentWorkerObject(int workerId, const std::string& message, const std::string& stackTrace, const std::string& filename, int lineno, - const std::string& threadName) { + const std::string& threadName, + Local errorName, + Local errorMessage) { auto wrapper = WorkerWrapper::GetById(workerId); if (wrapper == nullptr) { DEBUG_WRITE("MAIN: no worker instance was found with workerId=%d.", workerId); @@ -419,11 +493,12 @@ void WorkerWrapper::FireErrorOnParentWorkerObject(int workerId, const std::strin } auto worker = Local::New(isolate, *wrapper->poWorker_); - auto context = Runtime::GetRuntime(isolate)->GetContext(); TryCatch tc(isolate); - bool handled = WorkerEvents::EmitError(isolate, worker, message, filename, stackTrace, - lineno); + Local error; + (void)WorkerEvents::EmitError(isolate, worker, message, filename, stackTrace, lineno, + errorName, errorMessage) + .ToLocal(&error); if (tc.HasCaught()) { // A listener that threw replaces the error it was handed; nothing // further is reported for the original. @@ -432,22 +507,14 @@ void WorkerWrapper::FireErrorOnParentWorkerObject(int workerId, const std::strin } return; } - if (handled) { + if (!error.IsEmpty() && error->IsUndefined()) { + // A handler took ownership of it. return; } // HTML: an error the Worker object leaves unhandled is reported to the - // parent's global scope. Only primitives crossed the isolate boundary, - // so the error object is rebuilt from them here. - Local error = - Exception::Error(ArgConverter::ConvertToV8String(isolate, message)); - if (error->IsObject() && !stackTrace.empty()) { - (void)error.As() - ->Set(context, ArgConverter::ConvertToV8String(isolate, "stack"), - ArgConverter::ConvertToV8String(isolate, stackTrace)) - .FromMaybe(false); - } - if (!ErrorEvents::DispatchError(isolate, error, message, stackTrace)) { + // parent's global scope. + if (error.IsEmpty() || !ErrorEvents::DispatchError(isolate, error, message, stackTrace)) { DEBUG_WRITE_FORCE( "Unhandled exception in '%s' thread. file: %s, line %d, message: %s\nStackTrace: %s", threadName.c_str(), filename.c_str(), lineno, message.c_str(), diff --git a/test-app/runtime/src/main/cpp/WorkerWrapper.h b/test-app/runtime/src/main/cpp/WorkerWrapper.h index 5dd3b2fa8..3068d69d2 100644 --- a/test-app/runtime/src/main/cpp/WorkerWrapper.h +++ b/test-app/runtime/src/main/cpp/WorkerWrapper.h @@ -89,12 +89,15 @@ class WorkerWrapper : public std::enable_shared_from_this { /* * Posts the worker object's `onerror` invocation to the parent's thread. - * Strings only - must not hold any v8 handles from the worker isolate. + * `thrown` is the value the worker threw, when there is one: its name and + * message are read here, on the worker's isolate, for the parent to + * rebuild the error from. Without it the parent's error is an Error + * carrying `message`. Strings only cross to the parent's thread. */ - void PassUncaughtExceptionFromWorkerToParent(const std::string& message, - const std::string& filename, - const std::string& stackTrace, - int lineno); + void PassUncaughtExceptionFromWorkerToParent( + const std::string& message, const std::string& filename, + const std::string& stackTrace, int lineno, + v8::Local thrown = v8::Local()); /* * WHATWG parity: the worker's implicit port message queue starts disabled; @@ -172,7 +175,9 @@ class WorkerWrapper : public std::enable_shared_from_this { static void FireErrorOnParentWorkerObject(int workerId, const std::string& message, const std::string& stackTrace, const std::string& filename, int lineno, - const std::string& threadName); + const std::string& threadName, + v8::Local errorName, + v8::Local errorMessage); v8::Isolate* parentIsolate_; // The parent runtime's task queue; weak so a child outliving its parent diff --git a/test-app/runtime/src/main/cpp/js/node-worker-threads.js b/test-app/runtime/src/main/cpp/js/node-worker-threads.js index da60893b3..5ae799101 100644 --- a/test-app/runtime/src/main/cpp/js/node-worker-threads.js +++ b/test-app/runtime/src/main/cpp/js/node-worker-threads.js @@ -44,8 +44,10 @@ const { BroadcastChannel } = require("internal/broadcast-channel"); const { EventTarget, defineEventHandler, + dispatchEventRethrowing, globalEventTarget, } = require("internal/events"); +const { kWorkerError } = require("internal/worker-events"); let MessageEvent; function getMessageEvent() { @@ -63,7 +65,6 @@ const globalPostMessage = g.postMessage; const addEventListener = EventTarget.prototype.addEventListener; const removeEventListener = EventTarget.prototype.removeEventListener; -const dispatchEvent = EventTarget.prototype.dispatchEvent; // Runs `fn` after the caller returns. Node reports 'online' and 'exit' from // the thread's own lifecycle; the runtime's Worker has no equivalent signal, @@ -100,7 +101,7 @@ class WorkerEmitter { } const key = `${type}`; const list = this.#listeners[key] || (this.#listeners[key] = []); - ArrayPrototypePush(list, { listener, once: true }); + ArrayPrototypePush(list, { listener, once: true, fired: false }); return this; } @@ -123,15 +124,23 @@ class WorkerEmitter { return this.removeListener(type, listener); } + // Whether a listener was registered, as Node's EventEmitter reports it. emit(type, arg) { const list = this.#listeners[type]; - if (list === undefined) { - return; + if (list === undefined || list.length === 0) { + return false; } const snapshot = ArrayPrototypeSlice(list); for (let i = 0; i < snapshot.length; i++) { const entry = snapshot[i]; if (entry.once) { + // Node's once wrapper: a registration fires at most once, even when + // an earlier listener emits the same event again and the nested emit + // fires it first. + if (entry.fired) { + continue; + } + entry.fired = true; const index = ArrayPrototypeIndexOf(list, entry); if (index !== -1) { ArrayPrototypeSplice(list, index, 1); @@ -139,6 +148,7 @@ class WorkerEmitter { } FunctionPrototypeCall(entry.listener, this, arg); } + return true; } } @@ -179,8 +189,13 @@ class Worker extends WorkerEmitter { worker.onmessageerror = function (event) { self.emit("messageerror", event.data); }; - worker.onerror = function (error) { - self.emit("error", error); + // An 'error' listener receives the worker's error, as in Node, not the + // event. Once one has, the event is cancelled, so the error is not also + // reported to the parent's global scope. + worker.onerror = function (event) { + if (self.emit("error", event[kWorkerError])) { + event.preventDefault(); + } }; soon(function () { self.emit("online", undefined); @@ -312,9 +327,11 @@ ObjectDefineProperty(ParentPort.prototype, SymbolToStringTag, { let parentPort = null; if (!isMainThread) { parentPort = new ParentPort(); + // Rethrowing, so a listener that throws reaches the worker's error chain + // (the scope's onerror, then the parent's Worker) the way a throwing + // onmessage on the global scope does. const relay = function (event) { - FunctionPrototypeCall( - dispatchEvent, + dispatchEventRethrowing( parentPort, new (getMessageEvent())(event.type, { data: event.data, ports: event.ports }) ); diff --git a/test-app/runtime/src/main/cpp/js/primordials.js b/test-app/runtime/src/main/cpp/js/primordials.js index 53ac8fe94..b8bb6f2dd 100644 --- a/test-app/runtime/src/main/cpp/js/primordials.js +++ b/test-app/runtime/src/main/cpp/js/primordials.js @@ -22,16 +22,20 @@ const intrinsics = { // Constructors. Date, Error, + EvalError, FinalizationRegistry, Map, Number, Promise, RangeError, + ReferenceError, Set, String, + SyntaxError, TypeError, Uint8Array, Uint32Array, + URIError, URL, WeakRef, WeakSet, diff --git a/test-app/runtime/src/main/cpp/js/worker-events.js b/test-app/runtime/src/main/cpp/js/worker-events.js index 73536fc44..674c06aa9 100644 --- a/test-app/runtime/src/main/cpp/js/worker-events.js +++ b/test-app/runtime/src/main/cpp/js/worker-events.js @@ -9,7 +9,18 @@ // Eager, because the handler attributes have to exist before app code assigns // one. MessageEvent itself is pulled in on the first delivery, so a worker // nobody talks to never runs that builtin. -const { ObjectDefineProperty, ObjectSetPrototypeOf } = primordials; +const { + Error, + ErrorCaptureStackTrace, + EvalError, + ObjectDefineProperty, + ObjectSetPrototypeOf, + RangeError, + ReferenceError, + SyntaxError, + TypeError, + URIError, +} = primordials; const { EventTarget, @@ -52,15 +63,58 @@ function emitMessage(data, ports, type) { dispatchEventRethrowing(this, new MessageEventCtor(type, { data, ports })); } +// The key the worker's rebuilt error travels under on the `error` event, for +// node:worker_threads, whose listeners receive the error rather than the +// event. Reached only through require("internal/worker-events"). +const kWorkerError = Symbol("workerError"); + +// A built-in error name rebuilds with its own constructor, as Node's does, so +// `instanceof TypeError` holds on the parent. +const errorConstructors = { + __proto__: null, + Error, + EvalError, + RangeError, + ReferenceError, + SyntaxError, + TypeError, + URIError, +}; + +// The worker's error, rebuilt from the name and message the worker read off +// the thrown value. Any other name, a subclass's or a DOMException's, stays an +// own `name` on an Error. Without the worker's stack, the stack holds the +// header alone rather than the frames that rebuilt it. +function rebuildError(name, message, stack) { + const ErrorConstructor = errorConstructors[name]; + const error = + ErrorConstructor === undefined ? new Error(message) : new ErrorConstructor(message); + if (ErrorConstructor === undefined) { + ObjectDefineProperty(error, "name", { + __proto__: null, + value: name, + writable: true, + configurable: true, + }); + } + if (stack) { + error.stack = stack; + } else { + ErrorCaptureStackTrace(error, emitError); + } + return error; +} + // The parent-side error delivery callout, invoked by native with the Worker // object as `this` once the worker scope has left the error unhandled. Only // primitives cross the isolate boundary, so the event carries no `error` // object; `stackTrace` is this runtime's addition to the ErrorEvent fields. // -// Returns whether the error was handled: a truthy return from the `onerror` -// attribute cancels the event (HTML §8.1.7.3), as does preventDefault() from -// any listener. -function emitError(message, filename, lineno, stackTrace) { +// Returns the rebuilt error when the event was not handled, for native to +// report on the parent's global scope, and undefined when it was: a truthy +// return from the `onerror` attribute cancels the event (HTML §8.1.7.3), as +// does preventDefault() from any listener. +function emitError(message, filename, lineno, stackTrace, errorName, errorMessage) { const ErrorEventCtor = getErrorEvent(); const event = new ErrorEventCtor("error", { message, @@ -69,8 +123,10 @@ function emitError(message, filename, lineno, stackTrace) { cancelable: true, }); event.stackTrace = stackTrace; + const error = rebuildError(errorName, errorMessage, stackTrace); + ObjectDefineProperty(event, kWorkerError, { __proto__: null, value: error }); dispatchEventRethrowing(this, event); - return event.defaultPrevented; + return event.defaultPrevented ? undefined : error; } ObjectSetPrototypeOf(g.Worker.prototype, EventTarget.prototype); @@ -99,4 +155,4 @@ for (const name of ["onmessage", "onmessageerror"]) { }); } -module.exports = { emitMessage, emitError }; +module.exports = { emitMessage, emitError, kWorkerError };