From d8ffd6abdd2b95a58141c482bddf1e2e2fadc2a7 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Wed, 16 Sep 2026 15:29:17 -0700 Subject: [PATCH 1/5] Node-API (JSI): surface script exceptions from Napi::Eval as Napi::Error The V8JSI shim let facebook::jsi::JSError escape from evaluateJavaScript unconverted. Every other engine throws Napi::Error for a script exception, and AppRuntime's dispatch treats anything else as fatal, so a `throw` reaching a dispatched Eval on JSI aborted the process (exit 3). Convert JSError to Napi::Error carrying the thrown value, and other JSI exceptions to a Napi::Error with their message. --- Core/Node-API-JSI/Source/env.cc | 15 ++++++++++++++- Tests/UnitTests/Shared/Shared.cpp | 29 +++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/Core/Node-API-JSI/Source/env.cc b/Core/Node-API-JSI/Source/env.cc index cc708cd1..085f6049 100644 --- a/Core/Node-API-JSI/Source/env.cc +++ b/Core/Node-API-JSI/Source/env.cc @@ -17,6 +17,19 @@ namespace Napi Napi::Value Eval(Napi::Env env, const char* string, const char* sourceUrl) { napi_env__* env_ptr{env}; - return {env_ptr, env_ptr->rt.evaluateJavaScript(std::make_shared(string), sourceUrl)}; + try + { + return {env_ptr, env_ptr->rt.evaluateJavaScript(std::make_shared(string), sourceUrl)}; + } + catch (const facebook::jsi::JSError& error) + { + // A script exception (any thrown value, primitives included) has to reach callers as the + // Napi::Error the other engines throw; AppRuntime's dispatch treats anything else as fatal. + throw Napi::Error{env_ptr, facebook::jsi::Value{env_ptr->rt, error.value()}}; + } + catch (const facebook::jsi::JSIException& error) + { + throw Napi::Error::New(env, error.what()); + } } } diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 1c7e9ff7..fa285ddd 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -829,6 +829,35 @@ TEST(NodeApi, AdjacentEscapableScopesEscapeIndependently) #endif +TEST(NodeApi, EvalThrowIsCatchable) +{ + // Regression: a script exception has to reach native callers as Napi::Error on every engine. + // The JSI shim let facebook::jsi::JSError escape from Napi::Eval, which AppRuntime's dispatch + // treats as fatal (std::abort). A thrown primitive takes the same path there; it is covered by + // NodeApi.PrimitiveExceptionSurvivesNativeCatch (#239), which cannot run on JavaScriptCore + // before that change lands. + Babylon::AppRuntime runtime{}; + + std::promise outcome; + runtime.Dispatch([&outcome](Napi::Env env) { + bool caught{false}; + std::string message; + try + { + Napi::Eval(env, "throw new Error('boom');", "eval-throw.js"); + } + catch (const Napi::Error& error) + { + caught = true; + message = error.Message(); + } + const auto sum = Napi::Eval(env, "1 + 1", "eval-throw.js"); + outcome.set_value(caught && message == "boom" && sum.IsNumber() && sum.As().Int32Value() == 2); + }); + + EXPECT_TRUE(outcome.get_future().get()); +} + int RunTests() { testing::InitGoogleTest(); From c8293b830cbfab52786c68aa870598bd50994b72 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Thu, 17 Sep 2026 09:42:37 -0700 Subject: [PATCH 2/5] Handle primitive JSI eval exceptions without aborting --- Core/Node-API-JSI/Source/env.cc | 14 ++++++++--- Tests/UnitTests/Shared/Shared.cpp | 40 +++++++++++++++++++++---------- 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/Core/Node-API-JSI/Source/env.cc b/Core/Node-API-JSI/Source/env.cc index 085f6049..46158d90 100644 --- a/Core/Node-API-JSI/Source/env.cc +++ b/Core/Node-API-JSI/Source/env.cc @@ -1,5 +1,7 @@ #include +#include + namespace Napi { Env Attach(facebook::jsi::Runtime& rt) @@ -23,9 +25,15 @@ namespace Napi } catch (const facebook::jsi::JSError& error) { - // A script exception (any thrown value, primitives included) has to reach callers as the - // Napi::Error the other engines throw; AppRuntime's dispatch treats anything else as fatal. - throw Napi::Error{env_ptr, facebook::jsi::Value{env_ptr->rt, error.value()}}; + // Napi::Error is object-backed in this JSI implementation. Preserve thrown objects exactly; + // represent primitive throws with a new Error rather than calling asObject and leaking a + // second JSIException into AppRuntime's fatal catch-all. + auto value = facebook::jsi::Value{env_ptr->rt, error.value()}; + if (value.isObject()) + { + throw Napi::Error{env_ptr, std::move(value)}; + } + throw Napi::Error::New(env, error.what()); } catch (const facebook::jsi::JSIException& error) { diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index fa285ddd..ff12dc2d 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -833,29 +833,45 @@ TEST(NodeApi, EvalThrowIsCatchable) { // Regression: a script exception has to reach native callers as Napi::Error on every engine. // The JSI shim let facebook::jsi::JSError escape from Napi::Eval, which AppRuntime's dispatch - // treats as fatal (std::abort). A thrown primitive takes the same path there; it is covered by - // NodeApi.PrimitiveExceptionSurvivesNativeCatch (#239), which cannot run on JavaScriptCore - // before that change lands. + // treats as fatal (std::abort). Babylon::AppRuntime runtime{}; std::promise outcome; + auto outcomeFuture = outcome.get_future(); runtime.Dispatch([&outcome](Napi::Env env) { - bool caught{false}; - std::string message; try { - Napi::Eval(env, "throw new Error('boom');", "eval-throw.js"); + bool caughtErrorObject{false}; + try + { + Napi::Eval(env, "throw new Error('boom');", "eval-throw.js"); + } + catch (const Napi::Error& error) + { + caughtErrorObject = error.Message() == "boom"; + } + + bool caughtPrimitive{false}; + try + { + Napi::Eval(env, "throw 'primitive boom';", "eval-primitive-throw.js"); + } + catch (const Napi::Error&) + { + caughtPrimitive = true; + } + + const auto sum = Napi::Eval(env, "1 + 1", "eval-throw.js"); + outcome.set_value(caughtErrorObject && caughtPrimitive && sum.IsNumber() && sum.As().Int32Value() == 2); } - catch (const Napi::Error& error) + catch (...) { - caught = true; - message = error.Message(); + outcome.set_exception(std::current_exception()); } - const auto sum = Napi::Eval(env, "1 + 1", "eval-throw.js"); - outcome.set_value(caught && message == "boom" && sum.IsNumber() && sum.As().Int32Value() == 2); }); - EXPECT_TRUE(outcome.get_future().get()); + ASSERT_EQ(outcomeFuture.wait_for(std::chrono::seconds{5}), std::future_status::ready); + EXPECT_TRUE(outcomeFuture.get()); } int RunTests() From 163cba2cc599c392c2487d7dd5f51ffb14b6fcd7 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Thu, 17 Sep 2026 15:21:31 -0700 Subject: [PATCH 3/5] Harden Eval exception regression coverage and callback lifetime Keep primitive-throw coverage specific to JSI until the independent JavaScriptCore fix lands. Cover all supported primitive kinds and preserve Error object identity. Capture test promises by shared ownership, bound completion waits, and copy unexpected exception messages into plain C++ errors on the runtime thread. Apply the repository formatter to the JSI Eval implementation. --- Core/Node-API-JSI/Source/env.cc | 60 ++++++++++++------------ Tests/UnitTests/Shared/Shared.cpp | 77 ++++++++++++++++++++++++------- 2 files changed, 91 insertions(+), 46 deletions(-) diff --git a/Core/Node-API-JSI/Source/env.cc b/Core/Node-API-JSI/Source/env.cc index 46158d90..ba3e56a0 100644 --- a/Core/Node-API-JSI/Source/env.cc +++ b/Core/Node-API-JSI/Source/env.cc @@ -4,40 +4,40 @@ namespace Napi { - Env Attach(facebook::jsi::Runtime& rt) - { - napi_env__* env_ptr{new napi_env__{rt}}; - return {env_ptr}; - } - - void Detach(Env env) - { - napi_env__* env_ptr{env}; - delete env_ptr; - } - - Napi::Value Eval(Napi::Env env, const char* string, const char* sourceUrl) - { - napi_env__* env_ptr{env}; - try + Env Attach(facebook::jsi::Runtime& rt) { - return {env_ptr, env_ptr->rt.evaluateJavaScript(std::make_shared(string), sourceUrl)}; + napi_env__* env_ptr{new napi_env__{rt}}; + return {env_ptr}; } - catch (const facebook::jsi::JSError& error) + + void Detach(Env env) { - // Napi::Error is object-backed in this JSI implementation. Preserve thrown objects exactly; - // represent primitive throws with a new Error rather than calling asObject and leaking a - // second JSIException into AppRuntime's fatal catch-all. - auto value = facebook::jsi::Value{env_ptr->rt, error.value()}; - if (value.isObject()) - { - throw Napi::Error{env_ptr, std::move(value)}; - } - throw Napi::Error::New(env, error.what()); + napi_env__* env_ptr{env}; + delete env_ptr; } - catch (const facebook::jsi::JSIException& error) + + Napi::Value Eval(Napi::Env env, const char* string, const char* sourceUrl) { - throw Napi::Error::New(env, error.what()); + napi_env__* env_ptr{env}; + try + { + return {env_ptr, env_ptr->rt.evaluateJavaScript(std::make_shared(string), sourceUrl)}; + } + catch (const facebook::jsi::JSError& error) + { + // Napi::Error is object-backed in this JSI implementation. Preserve thrown objects exactly; + // represent primitive throws with a new Error rather than calling asObject and leaking a + // second JSIException into AppRuntime's fatal catch-all. + auto value = facebook::jsi::Value{env_ptr->rt, error.value()}; + if (value.isObject()) + { + throw Napi::Error{env_ptr, std::move(value)}; + } + throw Napi::Error::New(env, error.what()); + } + catch (const facebook::jsi::JSIException& error) + { + throw Napi::Error::New(env, error.what()); + } } - } } diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index ff12dc2d..75daac35 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -20,6 +20,8 @@ #include #include #include +#include +#include #include namespace @@ -836,43 +838,86 @@ TEST(NodeApi, EvalThrowIsCatchable) // treats as fatal (std::abort). Babylon::AppRuntime runtime{}; - std::promise outcome; - auto outcomeFuture = outcome.get_future(); - runtime.Dispatch([&outcome](Napi::Env env) { + auto outcome = std::make_shared>(); + auto outcomeFuture = outcome->get_future(); + runtime.Dispatch([outcome](Napi::Env env) { try { bool caughtErrorObject{false}; try { - Napi::Eval(env, "throw new Error('boom');", "eval-throw.js"); + Napi::Eval(env, "var evalError = new Error('boom'); throw evalError;", "eval-throw.js"); } catch (const Napi::Error& error) { - caughtErrorObject = error.Message() == "boom"; + caughtErrorObject = error.Message() == "boom" && error.Value().StrictEquals(env.Global().Get("evalError")); } - bool caughtPrimitive{false}; - try - { - Napi::Eval(env, "throw 'primitive boom';", "eval-primitive-throw.js"); - } - catch (const Napi::Error&) + const auto sum = Napi::Eval(env, "1 + 1", "eval-throw.js"); + outcome->set_value(caughtErrorObject && sum.IsNumber() && sum.As().Int32Value() == 2); + } + catch (const std::exception& error) + { + // Runtime-backed exceptions must be destroyed on this thread, before env is detached. + outcome->set_exception(std::make_exception_ptr(std::runtime_error{error.what()})); + } + catch (...) + { + outcome->set_exception(std::make_exception_ptr(std::runtime_error{"Unexpected non-standard exception during Eval"})); + } + }); + + ASSERT_EQ(outcomeFuture.wait_for(std::chrono::seconds{5}), std::future_status::ready); + EXPECT_TRUE(outcomeFuture.get()); +} + +#if defined(JSRUNTIMEHOST_NAPI_ENGINE_JSI) +// Primitive throws on JavaScriptCore require the separate fix in #239. Here we +// exercise JSI's object-backed Napi::Error conversion without that dependency. +TEST(NodeApi, EvalPrimitiveThrowsAreCatchable) +{ + Babylon::AppRuntime runtime{}; + + auto outcome = std::make_shared>(); + auto outcomeFuture = outcome->get_future(); + runtime.Dispatch([outcome](Napi::Env env) { + try + { + for (const char* script : {"throw 'primitive boom';", "throw 42;", "throw true;", "throw null;", "throw undefined;", "throw Symbol('boom');"}) { - caughtPrimitive = true; - } + bool caughtPrimitive{false}; + try + { + Napi::Eval(env, script, "eval-primitive-throw.js"); + } + catch (const Napi::Error& error) + { + caughtPrimitive = !error.Message().empty(); + } - const auto sum = Napi::Eval(env, "1 + 1", "eval-throw.js"); - outcome.set_value(caughtErrorObject && caughtPrimitive && sum.IsNumber() && sum.As().Int32Value() == 2); + const auto sum = Napi::Eval(env, "1 + 1", "eval-primitive-throw.js"); + if (!caughtPrimitive || !sum.IsNumber() || sum.As().Int32Value() != 2) + { + outcome->set_value(false); + return; + } + } + outcome->set_value(true); + } + catch (const std::exception& error) + { + outcome->set_exception(std::make_exception_ptr(std::runtime_error{error.what()})); } catch (...) { - outcome.set_exception(std::current_exception()); + outcome->set_exception(std::make_exception_ptr(std::runtime_error{"Unexpected non-standard exception during Eval"})); } }); ASSERT_EQ(outcomeFuture.wait_for(std::chrono::seconds{5}), std::future_status::ready); EXPECT_TRUE(outcomeFuture.get()); } +#endif int RunTests() { From b6311e0ac6303a5b47311d07132b5054098651d8 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Thu, 17 Sep 2026 16:11:16 -0700 Subject: [PATCH 4/5] Test Eval conversion independently of V8JSI exception reconstruction V8JSI 0.64.33 reconstructs JSError from message text before Napi::Eval receives it, so original JavaScript object identity is not provided by that adapter. Keep the real Eval message, global visibility, and recovery checks independent, with original-object identity checked on the other engines. Add JSI runtime-decorator tests for forwarding Error/plain-object identity, wrapping actual primitive JSError values, and converting native JSI exceptions. Retain end-to-end primitive coverage and document the narrower object-preservation guarantee. --- Core/Node-API-JSI/Source/env.cc | 2 +- Tests/UnitTests/CMakeLists.txt | 1 + Tests/UnitTests/Shared/JsiEval.cpp | 110 +++++++++++++++++++++++++++++ Tests/UnitTests/Shared/Shared.cpp | 16 ++++- 4 files changed, 126 insertions(+), 3 deletions(-) create mode 100644 Tests/UnitTests/Shared/JsiEval.cpp diff --git a/Core/Node-API-JSI/Source/env.cc b/Core/Node-API-JSI/Source/env.cc index ba3e56a0..e5299d97 100644 --- a/Core/Node-API-JSI/Source/env.cc +++ b/Core/Node-API-JSI/Source/env.cc @@ -25,7 +25,7 @@ namespace Napi } catch (const facebook::jsi::JSError& error) { - // Napi::Error is object-backed in this JSI implementation. Preserve thrown objects exactly; + // Napi::Error is object-backed in this JSI implementation. Preserve JSError objects exactly; // represent primitive throws with a new Error rather than calling asObject and leaking a // second JSIException into AppRuntime's fatal catch-all. auto value = facebook::jsi::Value{env_ptr->rt, error.value()}; diff --git a/Tests/UnitTests/CMakeLists.txt b/Tests/UnitTests/CMakeLists.txt index f3676d7d..d7126a68 100644 --- a/Tests/UnitTests/CMakeLists.txt +++ b/Tests/UnitTests/CMakeLists.txt @@ -58,6 +58,7 @@ target_compile_definitions(UnitTests PRIVATE NAPI_JAVASCRIPT_ENGINE="${NAPI_JAVA # CreateDataViewRejectsOverflowingRange test is compiled out on that backend. if(NAPI_JAVASCRIPT_ENGINE STREQUAL "JSI") target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_JSI) + target_sources(UnitTests PRIVATE "Shared/JsiEval.cpp") endif() target_link_libraries(UnitTests diff --git a/Tests/UnitTests/Shared/JsiEval.cpp b/Tests/UnitTests/Shared/JsiEval.cpp new file mode 100644 index 00000000..c6c49f6c --- /dev/null +++ b/Tests/UnitTests/Shared/JsiEval.cpp @@ -0,0 +1,110 @@ +#include +#include +#include +#include +#include +#include + +namespace +{ + // The pinned V8JSI adapter reconstructs exceptions. Inject the original JSError + // value to exercise our conversion itself, including its primitive fallback. + class ThrowingRuntime : public facebook::jsi::RuntimeDecorator<> + { + public: + ThrowingRuntime(facebook::jsi::Runtime& runtime, const facebook::jsi::Value& value, bool nativeException = false) + : RuntimeDecorator{runtime} + , m_value{runtime, value} + , m_nativeException{nativeException} + { + } + + facebook::jsi::Value evaluateJavaScript(const std::shared_ptr& buffer, const std::string& sourceURL) override + { + if (std::exchange(m_throw, false)) + { + if (m_nativeException) + { + throw facebook::jsi::JSINativeException{"native eval failure"}; + } + throw facebook::jsi::JSError{plain(), facebook::jsi::Value{plain(), m_value}}; + } + return plain().evaluateJavaScript(buffer, sourceURL); + } + + private: + facebook::jsi::Value m_value; + bool m_nativeException; + bool m_throw{true}; + }; +} + +TEST(JsiEval, PreservesJSErrorObjectIdentity) +{ + const auto runtime = v8runtime::makeV8Runtime({}); + for (const char* source : {"new Error('boom')", "({message: 'boom', sentinel: 42})"}) + { + SCOPED_TRACE(source); + const auto original = runtime->evaluateJavaScript(std::make_shared(source), "original.js"); + ThrowingRuntime throwingRuntime{*runtime, original}; + napi_env__ env{throwingRuntime}; + bool caught{false}; + try + { + Napi::Eval(&env, "", "eval-object.js"); + } + catch (const Napi::Error& error) + { + caught = true; + EXPECT_EQ(error.Message(), "boom"); + EXPECT_TRUE(error.Value().StrictEquals(Napi::Value{&env, facebook::jsi::Value{*runtime, original}})); + } + EXPECT_TRUE(caught); + EXPECT_EQ(Napi::Eval(&env, "1 + 1", "recovery.js").As().Int32Value(), 2); + } +} + +TEST(JsiEval, WrapsJSErrorPrimitives) +{ + const auto runtime = v8runtime::makeV8Runtime({}); + for (const char* source : {"'primitive boom'", "42", "true", "null", "undefined", "Symbol('boom')"}) + { + SCOPED_TRACE(source); + const auto original = runtime->evaluateJavaScript(std::make_shared(source), "original.js"); + ASSERT_FALSE(original.isObject()); + ThrowingRuntime throwingRuntime{*runtime, original}; + napi_env__ env{throwingRuntime}; + bool caught{false}; + try + { + Napi::Eval(&env, "", "eval-primitive.js"); + } + catch (const Napi::Error& error) + { + caught = true; + EXPECT_TRUE(error.Value().IsObject()); + EXPECT_FALSE(error.Message().empty()); + } + EXPECT_TRUE(caught); + EXPECT_EQ(Napi::Eval(&env, "1 + 1", "recovery.js").As().Int32Value(), 2); + } +} + +TEST(JsiEval, ConvertsNativeExceptions) +{ + const auto runtime = v8runtime::makeV8Runtime({}); + ThrowingRuntime throwingRuntime{*runtime, facebook::jsi::Value{}, true}; + napi_env__ env{throwingRuntime}; + bool caught{false}; + try + { + Napi::Eval(&env, "", "eval-native.js"); + } + catch (const Napi::Error& error) + { + caught = true; + EXPECT_EQ(error.Message(), "native eval failure"); + } + EXPECT_TRUE(caught); + EXPECT_EQ(Napi::Eval(&env, "1 + 1", "recovery.js").As().Int32Value(), 2); +} diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 75daac35..6bc4e8fc 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -850,11 +850,23 @@ TEST(NodeApi, EvalThrowIsCatchable) } catch (const Napi::Error& error) { - caughtErrorObject = error.Message() == "boom" && error.Value().StrictEquals(env.Global().Get("evalError")); + caughtErrorObject = true; + EXPECT_EQ(error.Message(), "boom"); + EXPECT_TRUE(env.Global().Get("evalError").IsObject()); +#if !defined(JSRUNTIMEHOST_NAPI_ENGINE_JSI) + EXPECT_TRUE(error.Value().StrictEquals(env.Global().Get("evalError"))); +#endif + // V8JSI 0.64.33's ReportException reconstructs the Error before Eval receives it. + // JsiEval tests identity at our conversion boundary with an original JSError value. } const auto sum = Napi::Eval(env, "1 + 1", "eval-throw.js"); - outcome->set_value(caughtErrorObject && sum.IsNumber() && sum.As().Int32Value() == 2); + EXPECT_TRUE(sum.IsNumber()); + if (sum.IsNumber()) + { + EXPECT_EQ(sum.As().Int32Value(), 2); + } + outcome->set_value(caughtErrorObject); } catch (const std::exception& error) { From d0dd17e46abff09795bb98994a6f2f15355130e5 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Thu, 17 Sep 2026 16:30:18 -0700 Subject: [PATCH 5/5] Test JSI exception conversion with packaged V8JSI headers The pinned Windows NuGet package does not ship jsi/decorator.h. Move the existing conversion into a private helper used by Eval and test it with original JSError values on a real V8JSI runtime, without a RuntimeDecorator dependency. Keep end-to-end Eval catch and recovery coverage separately because this adapter reconstructs thrown values before delivering them to Node-API. Validated the JSI source and tests with the headers actually present in ReactNative.V8Jsi.Windows 0.64.33; the macOS JSC suite passes with continuous collection. Windows JSI execution is validated by the fork CI job. --- Core/Node-API-JSI/CMakeLists.txt | 1 + Core/Node-API-JSI/Source/EvalInternal.h | 25 +++++++ Core/Node-API-JSI/Source/env.cc | 14 +--- Tests/UnitTests/CMakeLists.txt | 1 + Tests/UnitTests/Shared/JsiEval.cpp | 95 +++++-------------------- Tests/UnitTests/Shared/Shared.cpp | 2 +- 6 files changed, 48 insertions(+), 90 deletions(-) create mode 100644 Core/Node-API-JSI/Source/EvalInternal.h diff --git a/Core/Node-API-JSI/CMakeLists.txt b/Core/Node-API-JSI/CMakeLists.txt index e8e79a96..c33ce4ae 100644 --- a/Core/Node-API-JSI/CMakeLists.txt +++ b/Core/Node-API-JSI/CMakeLists.txt @@ -2,6 +2,7 @@ set(SOURCES "include/napi/env.h" "include/napi/napi.h" "include/napi/napi-inl.h" + "Source/EvalInternal.h" "source/env.cc") add_library(napi ${SOURCES}) diff --git a/Core/Node-API-JSI/Source/EvalInternal.h b/Core/Node-API-JSI/Source/EvalInternal.h new file mode 100644 index 00000000..c5e34530 --- /dev/null +++ b/Core/Node-API-JSI/Source/EvalInternal.h @@ -0,0 +1,25 @@ +#pragma once + +#include +#include + +namespace Napi::Internal +{ + inline Error ConvertEvalException(Env env, const facebook::jsi::JSError& error) + { + // Napi::Error is object-backed here. Preserve objects supplied by JSError; + // wrap primitives instead of letting asObject throw another JSIException. + napi_env__* env_ptr{env}; + auto value = facebook::jsi::Value{env_ptr->rt, error.value()}; + if (value.isObject()) + { + return Error{env_ptr, std::move(value)}; + } + return Error::New(env, error.what()); + } + + inline Error ConvertEvalException(Env env, const facebook::jsi::JSIException& error) + { + return Error::New(env, error.what()); + } +} diff --git a/Core/Node-API-JSI/Source/env.cc b/Core/Node-API-JSI/Source/env.cc index e5299d97..1b2af68b 100644 --- a/Core/Node-API-JSI/Source/env.cc +++ b/Core/Node-API-JSI/Source/env.cc @@ -1,6 +1,6 @@ #include -#include +#include "EvalInternal.h" namespace Napi { @@ -25,19 +25,11 @@ namespace Napi } catch (const facebook::jsi::JSError& error) { - // Napi::Error is object-backed in this JSI implementation. Preserve JSError objects exactly; - // represent primitive throws with a new Error rather than calling asObject and leaking a - // second JSIException into AppRuntime's fatal catch-all. - auto value = facebook::jsi::Value{env_ptr->rt, error.value()}; - if (value.isObject()) - { - throw Napi::Error{env_ptr, std::move(value)}; - } - throw Napi::Error::New(env, error.what()); + throw Internal::ConvertEvalException(env, error); } catch (const facebook::jsi::JSIException& error) { - throw Napi::Error::New(env, error.what()); + throw Internal::ConvertEvalException(env, error); } } } diff --git a/Tests/UnitTests/CMakeLists.txt b/Tests/UnitTests/CMakeLists.txt index d7126a68..1a227951 100644 --- a/Tests/UnitTests/CMakeLists.txt +++ b/Tests/UnitTests/CMakeLists.txt @@ -59,6 +59,7 @@ target_compile_definitions(UnitTests PRIVATE NAPI_JAVASCRIPT_ENGINE="${NAPI_JAVA if(NAPI_JAVASCRIPT_ENGINE STREQUAL "JSI") target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_JSI) target_sources(UnitTests PRIVATE "Shared/JsiEval.cpp") + target_include_directories(UnitTests PRIVATE "${JsRuntimeHost_SOURCE_DIR}/Core/Node-API-JSI/Source") endif() target_link_libraries(UnitTests diff --git a/Tests/UnitTests/Shared/JsiEval.cpp b/Tests/UnitTests/Shared/JsiEval.cpp index c6c49f6c..2c7d2e99 100644 --- a/Tests/UnitTests/Shared/JsiEval.cpp +++ b/Tests/UnitTests/Shared/JsiEval.cpp @@ -1,65 +1,22 @@ -#include -#include +#include "EvalInternal.h" #include #include #include -#include - -namespace -{ - // The pinned V8JSI adapter reconstructs exceptions. Inject the original JSError - // value to exercise our conversion itself, including its primitive fallback. - class ThrowingRuntime : public facebook::jsi::RuntimeDecorator<> - { - public: - ThrowingRuntime(facebook::jsi::Runtime& runtime, const facebook::jsi::Value& value, bool nativeException = false) - : RuntimeDecorator{runtime} - , m_value{runtime, value} - , m_nativeException{nativeException} - { - } - - facebook::jsi::Value evaluateJavaScript(const std::shared_ptr& buffer, const std::string& sourceURL) override - { - if (std::exchange(m_throw, false)) - { - if (m_nativeException) - { - throw facebook::jsi::JSINativeException{"native eval failure"}; - } - throw facebook::jsi::JSError{plain(), facebook::jsi::Value{plain(), m_value}}; - } - return plain().evaluateJavaScript(buffer, sourceURL); - } - - private: - facebook::jsi::Value m_value; - bool m_nativeException; - bool m_throw{true}; - }; -} +// The pinned V8JSI adapter reconstructs thrown exceptions. Exercise the same +// conversion used by Eval directly to retain the original JSError value. TEST(JsiEval, PreservesJSErrorObjectIdentity) { const auto runtime = v8runtime::makeV8Runtime({}); + napi_env__ env{*runtime}; for (const char* source : {"new Error('boom')", "({message: 'boom', sentinel: 42})"}) { SCOPED_TRACE(source); const auto original = runtime->evaluateJavaScript(std::make_shared(source), "original.js"); - ThrowingRuntime throwingRuntime{*runtime, original}; - napi_env__ env{throwingRuntime}; - bool caught{false}; - try - { - Napi::Eval(&env, "", "eval-object.js"); - } - catch (const Napi::Error& error) - { - caught = true; - EXPECT_EQ(error.Message(), "boom"); - EXPECT_TRUE(error.Value().StrictEquals(Napi::Value{&env, facebook::jsi::Value{*runtime, original}})); - } - EXPECT_TRUE(caught); + const facebook::jsi::JSError exception{*runtime, facebook::jsi::Value{*runtime, original}}; + const auto error = Napi::Internal::ConvertEvalException(&env, exception); + EXPECT_EQ(error.Message(), "boom"); + EXPECT_TRUE(error.Value().StrictEquals(Napi::Value{&env, facebook::jsi::Value{*runtime, original}})); EXPECT_EQ(Napi::Eval(&env, "1 + 1", "recovery.js").As().Int32Value(), 2); } } @@ -67,25 +24,16 @@ TEST(JsiEval, PreservesJSErrorObjectIdentity) TEST(JsiEval, WrapsJSErrorPrimitives) { const auto runtime = v8runtime::makeV8Runtime({}); + napi_env__ env{*runtime}; for (const char* source : {"'primitive boom'", "42", "true", "null", "undefined", "Symbol('boom')"}) { SCOPED_TRACE(source); const auto original = runtime->evaluateJavaScript(std::make_shared(source), "original.js"); ASSERT_FALSE(original.isObject()); - ThrowingRuntime throwingRuntime{*runtime, original}; - napi_env__ env{throwingRuntime}; - bool caught{false}; - try - { - Napi::Eval(&env, "", "eval-primitive.js"); - } - catch (const Napi::Error& error) - { - caught = true; - EXPECT_TRUE(error.Value().IsObject()); - EXPECT_FALSE(error.Message().empty()); - } - EXPECT_TRUE(caught); + const facebook::jsi::JSError exception{*runtime, facebook::jsi::Value{*runtime, original}}; + const auto error = Napi::Internal::ConvertEvalException(&env, exception); + EXPECT_TRUE(error.Value().IsObject()); + EXPECT_FALSE(error.Message().empty()); EXPECT_EQ(Napi::Eval(&env, "1 + 1", "recovery.js").As().Int32Value(), 2); } } @@ -93,18 +41,9 @@ TEST(JsiEval, WrapsJSErrorPrimitives) TEST(JsiEval, ConvertsNativeExceptions) { const auto runtime = v8runtime::makeV8Runtime({}); - ThrowingRuntime throwingRuntime{*runtime, facebook::jsi::Value{}, true}; - napi_env__ env{throwingRuntime}; - bool caught{false}; - try - { - Napi::Eval(&env, "", "eval-native.js"); - } - catch (const Napi::Error& error) - { - caught = true; - EXPECT_EQ(error.Message(), "native eval failure"); - } - EXPECT_TRUE(caught); + napi_env__ env{*runtime}; + const facebook::jsi::JSINativeException exception{"native eval failure"}; + const auto error = Napi::Internal::ConvertEvalException(&env, exception); + EXPECT_EQ(error.Message(), "native eval failure"); EXPECT_EQ(Napi::Eval(&env, "1 + 1", "recovery.js").As().Int32Value(), 2); } diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 6bc4e8fc..fbf796a2 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -857,7 +857,7 @@ TEST(NodeApi, EvalThrowIsCatchable) EXPECT_TRUE(error.Value().StrictEquals(env.Global().Get("evalError"))); #endif // V8JSI 0.64.33's ReportException reconstructs the Error before Eval receives it. - // JsiEval tests identity at our conversion boundary with an original JSError value. + // JsiEval tests the private conversion helper with an original JSError value. } const auto sum = Napi::Eval(env, "1 + 1", "eval-throw.js");