Node-API (JSI): surface script exceptions from Napi::Eval as Napi::Error instead of aborting - #246
Open
matthargett wants to merge 1 commit into
Open
matthargett wants to merge 1 commit into
matthargett wants to merge 1 commit into
Conversation
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.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The current JSI exception conversion can still fail for primitive throws (due to Napi::Error(napi_env, jsi::Value) requiring an object), and the new unit test can deadlock if the dispatched lambda exits via an unexpected exception path.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR fixes a backend inconsistency in the Node-API JSI shim by translating script-thrown exceptions from facebook::jsi into Napi::Error, preventing AppRuntime::Dispatch from treating them as fatal and aborting the process.
Changes:
- Update
Napi::Eval(JSI backend) to catchfacebook::jsi::JSError/JSIExceptionand rethrow asNapi::Error. - Add a unit test asserting that
throw new Error('boom')fromNapi::Evalis catchable asNapi::Errorand that the runtime can continue evaluating afterward.
File summaries
| File | Description |
|---|---|
| Tests/UnitTests/Shared/Shared.cpp | Adds regression test ensuring exceptions from Napi::Eval are catchable and do not halt subsequent evaluation. |
| Core/Node-API-JSI/Source/env.cc | Wraps JSI exceptions thrown by evaluateJavaScript into Napi::Error to align behavior with other engines and avoid aborts in dispatch. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+24
to
+29
| 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()}}; | ||
| } |
Comment on lines
+841
to
+859
| std::promise<bool> 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<Napi::Number>().Int32Value() == 2); | ||
| }); | ||
|
|
||
| EXPECT_TRUE(outcome.get_future().get()); | ||
| } |
matthargett
added a commit
to rebeckerspecialties/JsRuntimeHost
that referenced
this pull request
Sep 17, 2026
…alue
Native code throws Napi::Error, a C++ exception carrying the JavaScript
error object. JSI reports any std::exception that escapes a host
function as a fresh Error("Exception in HostFunction: " + what()), so
on the JSI backend a polyfill's TypeError reached scripts as a plain
Error with a prefixed message: `instanceof TypeError` was false and
messages no longer compared equal, unlike every other backend.
Route every host-function trampoline (Function::New callbacks, class
constructors, static and instance methods and accessors) through a
helper that catches Napi::Error and rethrows jsi::JSError with the
original value, the same conversion Napi::Eval already does in the
other direction (BabylonJS#246).
Regression test: "native exceptions reach scripts as the thrown error
object, with its class and message, on every engine".
(cherry picked from commit 94e20fc)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On the V8JSI backend
Napi::EvalcallsRuntime::evaluateJavaScriptdirectly, and a script exception surfaces asfacebook::jsi::JSError. Every other engine throwsNapi::Error, andAppRuntime::Dispatchonly catches that (catch (...)→std::abort), so athrowreaching a dispatchedEvalon JSI aborted the whole process with exit 3 — seen onWin32_x64_JSIwhile adding a test in #239 (which had to gate that test off on JSI).Changes
Napi::Evalconvertsjsi::JSErrorintoNapi::Errorcarrying the thrown value (the JSINapi::Error(napi_env, jsi::Value)constructor), and any otherjsi::JSIExceptioninto aNapi::Errorwith its message.NodeApi.EvalThrowIsCatchable(all engines):throw new Error('boom')fromNapi::Evalis caught asNapi::Error,Message()readsboom, and the runtime keeps evaluating. The primitive-throw variant is covered by JSC Node-API: reference primitives and coerce property receivers (fixes a RELEASE_ASSERT on non-object exceptions) #239'sNodeApi.PrimitiveExceptionSurvivesNativeCatch; once both land its JSI gate can go.Verified locally on macOS (JavaScriptCore, where the test already passed); the JSI path is exercised by the fork twin's Win32/UWP JSI jobs: rebeckerspecialties#30.