Node-API (JSI): surface script exceptions from Napi::Eval as Napi::Error instead of aborting - #246
Open
matthargett wants to merge 5 commits into
Open
matthargett wants to merge 5 commits into
matthargett wants to merge 5 commits 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.
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)
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.
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.
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.
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. Script exceptions escape asfacebook::jsi::JSError, butAppRuntime::DispatchcatchesNapi::Errorand treats other exception types as fatal. A script throw can therefore abort the process.Changes
JSErrorobjects intoNapi::Errorwithout reconstructing those objects. Wrap primitive values in a new Error with diagnostic text; their original type and identity are not preserved.JSIExceptioninstances intoNapi::Errorwith their message.Adapter Limitation
V8JSI 0.64.33's ReportException constructs
JSErrorfrom text; its constructor creates another JavaScript Error. Original thrown-value identity is already lost before our conversion. Identity is tested on other engines and at our JSI conversion boundary, not claimed for this pinned adapter.Validation
ReactNative.V8Jsi.Windows0.64.33. This is compile-only, not local JSI execution.git diff --checkpass.d0dd17e: 22 native tests and 227 JavaScript cases, with 3 existing skips. Both end-to-end Eval tests and all three conversion tests pass. UWP x64/arm64 JSI builds also pass. This revision removes the missingjsi/decorator.hdependency that broke the previous run; that failed build is not counted as validation.