fix(worker): keep a forwarded worker error exact when its stack getter throws

Reading the stack of the error a worker's onerror threw could run a `stack` getter, and a getter that threw replaced that error in the TryCatch holding it, so the parent rebuilt the getter's error instead. The stack read now runs under its own TryCatch.

The thrown value's name and message travel to the parent as UTF-16, so an unpaired surrogate arrives as thrown rather than as U+FFFD. ArgConverter::ConvertToString copies by length, so an embedded NUL no longer cuts a converted string short.

Author: adrian-niculescu

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");
+};

test-app/app/src/main/assets/app/tests/testMessaging.js

@@ -292,6 +292,18 @@ describe("Messaging runtime edges", function () {
             });
         });
 
+        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");

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<size_t>(str.length())};
                 }
         }
 

test-app/runtime/src/main/cpp/CallbackHandlers.cpp

@@ -1842,6 +1842,9 @@ static void ExtractTryCatchInfo(Isolate *isolate, Local<Context> 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<Value> outStackTrace = tc.StackTrace(context).FromMaybe(Local<Value>());
     if (!outStackTrace.IsEmpty()) {
         Local<String> stackTraceStr =

test-app/runtime/src/main/cpp/WorkerEvents.cpp

@@ -101,8 +101,8 @@ void WorkerEvents::EmitMessage(Isolate* isolate, Local<Object> receiver,
 MaybeLocal<Value> WorkerEvents::EmitError(Isolate* isolate, Local<Object> receiver,
                                           const std::string& message, const std::string& source,
                                           const std::string& stackTrace, int lineNumber,
-                                          const std::string& errorName,
-                                          const std::string& errorMessage) {
+                                          Local<String> errorName,
+                                          Local<String> errorMessage) {
     auto* state = RuntimeState::For<WorkerEventsState>(isolate);
     if (state == nullptr || state->emitError.IsEmpty()) {
         return MaybeLocal<Value>();
@@ -117,8 +117,8 @@ MaybeLocal<Value> WorkerEvents::EmitError(Isolate* isolate, Local<Object> receiv
                          ArgConverter::ConvertToV8String(isolate, source),
                          Number::New(isolate, lineNumber),
                          ArgConverter::ConvertToV8String(isolate, stackTrace),
-                         ArgConverter::ConvertToV8String(isolate, errorName),
-                         ArgConverter::ConvertToV8String(isolate, errorMessage)};
+                         errorName,
+                         errorMessage};
     return state->emitError.Get(isolate)->Call(context, receiver, 6, args);
 }
 

test-app/runtime/src/main/cpp/WorkerEvents.h

@@ -53,8 +53,8 @@ class WorkerEvents {
                                                const std::string& message,
                                                const std::string& source,
                                                const std::string& stackTrace, int lineNumber,
-                                               const std::string& errorName,
-                                               const std::string& errorMessage);
+                                               v8::Local<v8::String> errorName,
+                                               v8::Local<v8::String> errorMessage);
 };
 
 }  // namespace tns

test-app/runtime/src/main/cpp/WorkerWrapper.cpp

@@ -3,6 +3,7 @@
 #include <android/looper.h>
 #include <pthread.h>
 
+#include <optional>
 #include <thread>
 
 #include "ArgConverter.h"
@@ -33,36 +34,57 @@ namespace tns {
 namespace {
 
 /*
- * The `name` and `message` the parent rebuilds the worker's error from, read
- * 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.
+ * 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.
  */
-void DescribeThrownValue(Isolate* isolate, Local<Context> context, Local<Value> thrown,
-                         std::string& name, std::string& message) {
+struct ThrownErrorText {
+    std::u16string name = u"Error";
+    std::u16string message;
+};
+
+std::u16string ToUtf16(Isolate* isolate, Local<String> value) {
+    std::u16string result(value->Length(), u'\0');
+    value->WriteV2(isolate, 0, value->Length(), reinterpret_cast<uint16_t*>(result.data()));
+    return result;
+}
+
+Local<String> FromUtf16(Isolate* isolate, const std::u16string& value) {
+    return String::NewFromTwoByte(isolate, reinterpret_cast<const uint16_t*>(value.data()),
+                                  NewStringType::kNormal, static_cast<int>(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> context, Local<Value> thrown) {
     HandleScope handleScope(isolate);
     TryCatch tc(isolate);
-    name = "Error";
-    message.clear();
+    ThrownErrorText text;
     Local<String> detail;
     if (thrown->ToDetailString(context).ToLocal(&detail)) {
-        message = ArgConverter::ConvertToString(detail);
+        text.message = ToUtf16(isolate, detail);
     }
     if (!thrown->IsObject() || thrown->IsFunction()) {
-        return;
+        return text;
     }
     auto object = thrown.As<Object>();
     Local<Value> value;
     if (object->Get(context, ArgConverter::ConvertToV8String(isolate, "name")).ToLocal(&value) &&
         value->IsString()) {
-        name = ArgConverter::ConvertToString(value.As<String>());
+        text.name = ToUtf16(isolate, value.As<String>());
     }
     if (object->Get(context, ArgConverter::ConvertToV8String(isolate, "message"))
                 .ToLocal(&value) &&
         value->IsString()) {
-        message = ArgConverter::ConvertToString(value.As<String>());
+        text.message = ToUtf16(isolate, value.As<String>());
     }
+    return text;
 }
 
 /*
@@ -416,26 +438,32 @@ void WorkerWrapper::PassUncaughtExceptionFromWorkerToParent(const std::string& m
 
     // 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::string errorName = "Error";
-    std::string errorMessage = message;
+    std::optional<ThrownErrorText> thrownText;
     if (!thrown.IsEmpty()) {
         Isolate* workerIsolate = Isolate::GetCurrent();
-        DescribeThrownValue(workerIsolate, workerIsolate->GetCurrentContext(), thrown, errorName,
-                            errorMessage);
+        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, errorName, errorMessage]() {
+                       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<String> errorName = thrownText
+                ? FromUtf16(parentIsolate, thrownText->name)
+                : ArgConverter::ConvertToV8String(parentIsolate, "Error");
+        Local<String> errorMessage = thrownText
+                ? FromUtf16(parentIsolate, thrownText->message)
+                : ArgConverter::ConvertToV8String(parentIsolate, message);
         WorkerWrapper::FireErrorOnParentWorkerObject(workerId, message, stackTrace, filename,
                                                      lineno, threadName, errorName,
                                                      errorMessage);
@@ -446,8 +474,8 @@ void WorkerWrapper::FireErrorOnParentWorkerObject(int workerId, const std::strin
                                                 const std::string& stackTrace,
                                                 const std::string& filename, int lineno,
                                                 const std::string& threadName,
-                                                const std::string& errorName,
-                                                const std::string& errorMessage) {
+                                                Local<String> errorName,
+                                                Local<String> errorMessage) {
     auto wrapper = WorkerWrapper::GetById(workerId);
     if (wrapper == nullptr) {
         DEBUG_WRITE("MAIN: no worker instance was found with workerId=%d.", workerId);

test-app/runtime/src/main/cpp/WorkerWrapper.h

@@ -176,8 +176,8 @@ class WorkerWrapper : public std::enable_shared_from_this<WorkerWrapper> {
                                             const std::string& stackTrace,
                                             const std::string& filename, int lineno,
                                             const std::string& threadName,
-                                            const std::string& errorName,
-                                            const std::string& errorMessage);
+                                            v8::Local<v8::String> errorName,
+                                            v8::Local<v8::String> errorMessage);
 
     v8::Isolate* parentIsolate_;
     // The parent runtime's task queue; weak so a child outliving its parent