Skip to content

napi: hand the thrown value to napi_get_and_clear_last_exception after napi_run_script - #41704

Open
robobun wants to merge 2 commits into
mainfrom
robobun/5036e92c/napi-run-script-exception-value
Open

robobun wants to merge 2 commits into
mainfrom
robobun/5036e92c/napi-run-script-exception-value

Conversation

@robobun

@robobun robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • napi_run_script of a script that throws hands the addon the internal JSC::Exception cell instead of the thrown value. After napi_get_and_clear_last_exception, napi_typeof returns napi_generic_failure and napi_get_named_property(e, "message") aborts with panic(main thread): abort() called. The release-asan build reports ASSERTION FAILED: unknown type passed to napi_typeof (napi.cpp:2914).
  • The cause is src/jsc/bindings/napi.cpp:3061. env->scheduleException(returnedException.get()) converts the NakedPtr<JSC::Exception> to a JSValue of the wrapper cell. Every other scheduleException caller passes a real JS value.

Fix

  • Schedule returnedException->value(), the value the script threw.
  • Correct because napi_get_and_clear_last_exception returns the pending value verbatim, so the addon now sees the same RangeError (or primitive) that Node returns. Paths that rethrow the pending exception into JS already unwrapped the cell, so their behavior does not change.
  • Verified: test/napi/napi.test.ts (three new cases under napi_run_script: a thrown RangeError, a SyntaxError, and a thrown primitive). All three crash on stock bun and pass with the fix. The rest of test/napi/napi.test.ts passes.

Background

  • JSC::evaluate reports a throw through a NakedPtr<JSC::Exception>. JSC::Exception is a GC cell that wraps the thrown value plus a captured stack. It is not a JS-visible value.
  • napi_env::scheduleException stores a JSValue in m_pendingException. napi_get_and_clear_last_exception returns it to the addon unchanged.
  • napi/v8: never leave a JS exception on the VM while addon code runs #40249 rewrites napi_run_script as part of a larger exception-handling change and also removes this bug. This PR is the minimal fix for the crash on its own.
Notes

Repro addon and output from the report (bun 1.4.2 and canary):

run st=9 typeof st=9 type=99 is_error=1
panic(main thread): abort() called

Node v26.3.0 prints run st=9 typeof st=0 type=6 is_error=1 and message st=0 'boom'.

The syntax error case only checks bun's output because V8 and JSC word the SyntaxError message differently.

One unrelated test in test/napi/napi.test.ts (a worker's buffers reach the parent as copies ...) timed out at 5 s once under the full-file ASAN run. It passes in isolation with and without this change.


no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/napi/napi.test.ts

…on cell

napi_run_script passed the JSC::Exception wrapper to scheduleException.
napi_get_and_clear_last_exception then returned that internal cell to the
addon, so napi_typeof failed and napi_get_named_property crashed.
Schedule the exception's value instead.
@coderabbitai

coderabbitai Bot commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 1643203d-1c81-4e91-b33f-b35b3988d65c

📥 Commits

Reviewing files that changed from the base of the PR and between d316760 and ad8046d.

📒 Files selected for processing (3)
  • src/jsc/bindings/napi.cpp
  • test/napi/napi-app/standalone_tests.cpp
  • test/napi/napi.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.


Walkthrough

Changes

N-API exception value handling

Layer / File(s) Summary
Schedule thrown JavaScript values
src/jsc/bindings/napi.cpp
napi_run_script now schedules the JavaScript value from Exception::value() when execution throws.
Validate retrieved exception values
test/napi/napi-app/standalone_tests.cpp, test/napi/napi.test.ts
Tests cover thrown Error, syntax-error, and primitive values. The standalone test is registered with the exported test functions.

Suggested reviewers: dylan-conway, jarred-sumner, cirospaciari

Merge Risk: ⚪ Minimal · up to ad804

N-API addons now receive the original JavaScript value thrown by napi_run_script, including errors and primitives, when retrieving the pending exception. The focused implementation change is covered by tests and presents no remaining merge-blocking risk.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly states that napi_run_script now passes the thrown value to napi_get_and_clear_last_exception. It is specific and directly matches the primary change.
Description check ✅ Passed The description explains the problem, fix, implementation reason, verification, test coverage, and known test limitation. It does not use the exact template headings, but it provides the required info…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the claude label Sep 6, 2026
@robobun

robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 1:38 PM PT - Sep 6th, 2026

❌ @autofix-ci[bot], your commit ad8046d has 3 failures in Build #111534 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 41704

That installs a local version of the PR into your bun-41704 executable, so you can run:

bun-41704 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — one-line fix passing the thrown JSValue instead of the JSC::Exception wrapper cell to scheduleException.

What was reviewed:

  • scheduleException(JSC::JSValue) signature in napi.h and the three other call sites — all already pass unwrapped JS values, so this was the only affected site.
  • Test coverage: thrown Error object (compared against Node via checkSameOutput), parse-time SyntaxError, and thrown primitive — the variant matrix is covered and the enum literals asserted (status=9/type=6/type=3) match napi_generic_failure/napi_object/napi_number.
  • No behavior change on the rethrow-into-JS path, which already unwrapped via throwException.
Extended reasoning...

Overview

The PR changes a single line in src/jsc/bindings/napi.cpp inside napi_run_script: when JSC::evaluate returns a NakedPtr<JSC::Exception>, the code now schedules returnedException->value() (the actual thrown JS value) instead of returnedException.get() (the internal JSC::Exception GC cell). It adds a C++ test function test_napi_run_script_exception_value in test/napi/napi-app/standalone_tests.cpp that retrieves and inspects the pending exception, and three test cases in test/napi/napi.test.ts covering a thrown RangeError, a syntax error, and a thrown primitive.

Security risks

None. This is an N-API exception-forwarding correctness fix; no auth, crypto, permissions, or untrusted-input parsing is touched. The change narrows what is exposed to the addon (the user-thrown value rather than an internal wrapper cell), which if anything is safer.

Level of scrutiny

Low-to-moderate. The fix is a one-line change whose correctness is verifiable from the type signature alone: napi_env::scheduleException in napi.h:431 takes a JSC::JSValue, and JSC::Exception::value() is the canonical accessor for the wrapped thrown value. I grepped all scheduleException call sites — the other three (napi.cpp:1075, 1416, 1605) already pass unwrapped JS values (created errors or toJS(napi_value)), confirming this was the only site with the bug and the fix covers the whole class.

Other factors

Tests follow the existing napi.test.ts conventions (checkSameOutput for Node parity, runOn(bunExe(), ...) where JSC/V8 messages diverge, with an inline comment explaining the divergence). The asserted numeric literals match the N-API enum values (napi_generic_failure=9, napi_object=6, napi_number=3, napi_ok=0). CODEOWNERS does not cover the touched paths. No outstanding third-party CHANGES_REQUESTED reviews are visible in the timeline; the only post-open commit is an autofix formatting tweak. Bug-hunt exit reason was dry_streak with no findings.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants