From bc3ae522bdafe9e39998d9ac7807673737559da2 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 20 Aug 2026 11:39:49 -0700 Subject: [PATCH] QuickJS: report success from the napi_throw family napi_throw, napi_throw_error, napi_throw_type_error and napi_throw_range_error returned napi_pending_exception after successfully scheduling the throw. That status means "the call failed because an exception is already pending", so node-addon-api's Error::ThrowAsJavaScriptException treated every native throw as a failed throw and re-threw Error::New(env). That constructor consumes the pending exception via napi_get_and_clear_last_exception, so the C++ exception escaped WrapCallback with no JS exception set. ExternalCallback::Callback then took its fallback path and rebuilt the error from e.what(). By that point the handle scope opened by ThrowAsJavaScriptException had closed, so stringifying the message read freed memory: on Linux this segfaults in js_dup, and elsewhere it silently replaces the error with "InternalError: Uncaught C++ exception: ...". Return napi_ok instead, matching the upstream Node-API implementation, so the exception stays pending and propagates unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09 --- Core/Node-API/Source/js_native_api_quickjs.cc | 18 +++++++++++++---- Tests/UnitTests/Scripts/tests.ts | 20 +++++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_quickjs.cc b/Core/Node-API/Source/js_native_api_quickjs.cc index ba25fefa..6db7f662 100644 --- a/Core/Node-API/Source/js_native_api_quickjs.cc +++ b/Core/Node-API/Source/js_native_api_quickjs.cc @@ -1586,7 +1586,14 @@ napi_status napi_throw(napi_env env, napi_value error) { JSContext* targetCtx = env->current_context ? env->current_context : env->context; JS_Throw(targetCtx, JS_DupValue(targetCtx, jsError)); - return napi_set_last_error(env, napi_pending_exception); + // Returning napi_pending_exception here would report that the throw itself + // failed. node-addon-api's Error::ThrowAsJavaScriptException reacts to that by + // re-throwing `Error::New(env)`, which clears the exception it just set and + // lets the C++ exception escape the callback wrapper. The escaped error is + // then stringified by ExternalCallback::Callback after its handle scope is + // gone, which reads freed memory. + napi_clear_last_error(env); + return napi_ok; } // Throw error @@ -1602,7 +1609,8 @@ napi_status napi_throw_error(napi_env env, const char* code, const char* msg) { } JS_Throw(env->context, error); - return napi_set_last_error(env, napi_pending_exception); + napi_clear_last_error(env); + return napi_ok; } // Throw type error @@ -1610,7 +1618,8 @@ napi_status napi_throw_type_error(napi_env env, const char* code, const char* ms CHECK_ENV(env); JS_ThrowTypeError(env->context, "%s", msg ? msg : ""); - return napi_set_last_error(env, napi_pending_exception); + napi_clear_last_error(env); + return napi_ok; } // Throw range error @@ -1618,7 +1627,8 @@ napi_status napi_throw_range_error(napi_env env, const char* code, const char* m CHECK_ENV(env); JS_ThrowRangeError(env->context, "%s", msg ? msg : ""); - return napi_set_last_error(env, napi_pending_exception); + napi_clear_last_error(env); + return napi_ok; } // Create error diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index cdc9416b..1dc2aa4c 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1359,6 +1359,26 @@ describe("URLSearchParams", function () { expect(() => paramsSet.set()).to.throw(); }); + it("should preserve the type and message of an error thrown from native code", function () { + // A native throw must reach JS unchanged. When napi_throw reported failure, the + // error was replaced by an InternalError reading "Uncaught C++ exception: ...", + // built by stringifying an error whose handle scope had already closed. + let caught: any; + try { + // @ts-expect-error + paramsSet.set(); + } catch (e) { + caught = e; + } + expect(caught).to.be.an.instanceOf(Error); + expect(caught.name).to.equal("Error"); + // Not an equality check: the JSI backend prefixes "Exception in HostFunction: ". + expect(caught.message).to.contain( + "Failed to execute 'set' on 'URLSearchParams': 2 arguments required, but only 0 present" + ); + expect(caught.message).to.not.contain("Uncaught C++ exception"); + }); + it("should add a number and retrieve it as a string from searchParams", function () { // Set Number paramsSet.set("foo", 400 as any);