Repository navigation
[fork twin] Node-API (JSI): script exceptions from Napi::Eval as Napi::Error - #30
Open
matthargett wants to merge 6 commits into
Open
matthargett wants to merge 6 commits into
matthargett wants to merge 6 commits into
Conversation
## Summary Follow-up to BabylonJS#230 (comment). The UTF-16 decoder previously dropped a trailing odd byte and created JavaScript strings from unpaired surrogates. Replacement mode in the Encoding Standard requires U+FFFD for both, in either endianness. `DecodeUtf16` now: - emits U+FFFD for a leftover odd byte - emits U+FFFD for a lone lead or trail surrogate - treats a lead followed by a non-trail as one replacement, then reprocesses the second unit - treats an unpaired lead plus a leftover odd byte as a single end-of-queue replacement The README describes this behavior and the still-unsupported `fatal` option. ## Timer cleanup after rebase Correction to the original description, per [Gary's review](BabylonJS#235 (review)): `Dispatch` always passed `0`, so the `id == 0` branch was **always taken**. The **nonzero ID-reuse path** was dead. The latest `main` already includes the timer refactor from BabylonJS#232, which removes `DispatchImpl` and guards against duplicate-ID insertion. The rebase keeps that implementation unchanged and drops this PR's superseded timer changes. The remaining diff contains only TextDecoder, its documentation, and the UTF-16 regression cases. ## Regression coverage - Trailing odd byte, both endiannesses - Lone lead / lone trail, both endiannesses - Lead + BMP (`U+D800` then `'A'`) produces `"\uFFFDA"` - Lead + leftover odd byte produces a single `"\uFFFD"` Existing valid-pair and BOM cases are unchanged. --------- Co-authored-by: Branimir Karadzic <branimirkaradzic@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da74bc94-a7dc-4817-bd81-59b5c6b123fc
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.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8ffd6abdd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| // 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()}}; |
There was a problem hiding this comment.
When evaluated code throws a primitive such as throw 42, error.value() is not an object, but Napi::Error(napi_env, jsi::Value) immediately calls value.asObject(). That raises another JSIException from inside this catch handler, which is not handled by the following sibling catch, so it still reaches AppRuntime::Dispatch's catch-all and aborts the process. The conversion must support arbitrary JavaScript values rather than passing primitives to the object-only constructor.
Useful? React with 👍 / 👎.
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.
Fork CI twin of BabylonJS#246.
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 BabylonJS#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) BabylonJS/JsRuntimeHost#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/JsRuntimeHost#TWIN.