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 cc708cd1..1b2af68b 100644 --- a/Core/Node-API-JSI/Source/env.cc +++ b/Core/Node-API-JSI/Source/env.cc @@ -1,22 +1,35 @@ #include +#include "EvalInternal.h" + namespace Napi { - Env Attach(facebook::jsi::Runtime& rt) - { - napi_env__* env_ptr{new napi_env__{rt}}; - return {env_ptr}; - } + 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; - } + 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}; - return {env_ptr, env_ptr->rt.evaluateJavaScript(std::make_shared(string), sourceUrl)}; - } + Napi::Value Eval(Napi::Env env, const char* string, const char* sourceUrl) + { + napi_env__* env_ptr{env}; + try + { + return {env_ptr, env_ptr->rt.evaluateJavaScript(std::make_shared(string), sourceUrl)}; + } + catch (const facebook::jsi::JSError& error) + { + throw Internal::ConvertEvalException(env, error); + } + catch (const facebook::jsi::JSIException& error) + { + throw Internal::ConvertEvalException(env, error); + } + } } diff --git a/Tests/UnitTests/CMakeLists.txt b/Tests/UnitTests/CMakeLists.txt index f3676d7d..1a227951 100644 --- a/Tests/UnitTests/CMakeLists.txt +++ b/Tests/UnitTests/CMakeLists.txt @@ -58,6 +58,8 @@ 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") + 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 new file mode 100644 index 00000000..2c7d2e99 --- /dev/null +++ b/Tests/UnitTests/Shared/JsiEval.cpp @@ -0,0 +1,49 @@ +#include "EvalInternal.h" +#include +#include +#include + +// 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"); + 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); + } +} + +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()); + 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); + } +} + +TEST(JsiEval, ConvertsNativeExceptions) +{ + const auto runtime = v8runtime::makeV8Runtime({}); + 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 1c7e9ff7..fbf796a2 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -20,6 +20,8 @@ #include #include #include +#include +#include #include namespace @@ -829,6 +831,106 @@ 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). + Babylon::AppRuntime runtime{}; + + auto outcome = std::make_shared>(); + auto outcomeFuture = outcome->get_future(); + runtime.Dispatch([outcome](Napi::Env env) { + try + { + bool caughtErrorObject{false}; + try + { + Napi::Eval(env, "var evalError = new Error('boom'); throw evalError;", "eval-throw.js"); + } + catch (const Napi::Error& error) + { + 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 the private conversion helper with an original JSError value. + } + + const auto sum = Napi::Eval(env, "1 + 1", "eval-throw.js"); + EXPECT_TRUE(sum.IsNumber()); + if (sum.IsNumber()) + { + EXPECT_EQ(sum.As().Int32Value(), 2); + } + outcome->set_value(caughtErrorObject); + } + 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');"}) + { + 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-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::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() { testing::InitGoogleTest();