Node-API (JSI): surface script exceptions from Napi::Eval as Napi::Error instead of aborting

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})

Core/Node-API-JSI/Source/EvalInternal.h

@@ -0,0 +1,25 @@
+#pragma once
+
+#include <napi/env.h>
+#include <utility>
+
+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());
+    }
+}

Core/Node-API-JSI/Source/env.cc

@@ -1,22 +1,35 @@
 #include <napi/env.h>
 
+#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<facebook::jsi::StringBuffer>(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<facebook::jsi::StringBuffer>(string), sourceUrl)};
+        }
+        catch (const facebook::jsi::JSError& error)
+        {
+            throw Internal::ConvertEvalException(env, error);
+        }
+        catch (const facebook::jsi::JSIException& error)
+        {
+            throw Internal::ConvertEvalException(env, error);
+        }
+    }
 }

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

Tests/UnitTests/Shared/JsiEval.cpp

@@ -0,0 +1,49 @@
+#include "EvalInternal.h"
+#include <V8JsiRuntime.h>
+#include <ScriptStore.h>
+#include <gtest/gtest.h>
+
+// 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<facebook::jsi::StringBuffer>(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<Napi::Number>().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<facebook::jsi::StringBuffer>(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<Napi::Number>().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<Napi::Number>().Int32Value(), 2);
+}

Tests/UnitTests/Shared/Shared.cpp

@@ -20,6 +20,8 @@
 #include <cstdint>
 #include <future>
 #include <iostream>
+#include <memory>
+#include <stdexcept>
 #include <thread>
 
 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<std::promise<bool>>();
+    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<Napi::Number>().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<std::promise<bool>>();
+    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<Napi::Number>().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();