Node-API: make the source-URL napi_run_script a C++ overload so Env::RunScript works on Hermes - #244
Open
matthargett wants to merge 1 commit into
Open
matthargett wants to merge 1 commit into
matthargett wants to merge 1 commit into
Conversation
…mplement it for Hermes The shared header replaced Node's three-argument napi_run_script with the Babylon four-argument form (source URL) inside extern "C". Hermes ships its own Node-API and exports the standard three-argument symbol, so Env::RunScript(script, url) bound to it and passed the URL where Hermes expected the result pointer: an access violation on Win32 and a SIGSEGV on Android for any node-addon-api RunScript call. Restore the standard declaration in the C block and declare the source-URL form as a C++ overload after it. The other engines already define both (their three-argument definitions get C linkage, the four-argument ones C++ linkage; nothing else changes), and the Hermes bridge now defines the overload through hermes_run_script, so Env::RunScript works there too. NodeApi.RunScriptWithSourceUrl covers every engine that has Env::RunScript.
This was referenced Sep 16, 2026
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes N-API header linkage/overload behavior across engines and platforms, which is ABI-sensitive and should be validated by a human across the supported toolchains.
Pull request overview
This PR fixes a Node-API linkage/ABI mismatch where Env::RunScript(script, url) could incorrectly bind to Hermes’s 3-argument napi_run_script C symbol, causing crashes by passing the URL in the result-pointer position. The change restores the standard 3-arg C declaration and makes the source-URL form a C++ overload so Hermes can provide it safely without conflicting with its own Node-API export.
Changes:
- Restored the standard 3-argument
napi_run_scriptdeclaration with C linkage and moved the 4-argumentsource_urlform to a C++ overload. - Added a Hermes implementation for the 4-argument overload that routes through
hermes_run_script. - Added a regression unit test covering
Env::RunScript(script, url)on engines that support it.
File summaries
| File | Description |
|---|---|
| Tests/UnitTests/Shared/Shared.cpp | Adds a regression test validating Env::RunScript(script, url) returns expected results without crashing. |
| Core/Node-API/Source/env_hermes.cc | Implements the 4-arg C++ overload of napi_run_script for Hermes via hermes_run_script. |
| Core/Node-API/Include/Shared/napi/js_native_api.h | Splits napi_run_script into a 3-arg C declaration and a 4-arg C++ overload to avoid Hermes symbol binding issues. |
| Core/Node-API/Include/Engine/Hermes/napi/env.h | Updates Hermes Eval/RunScript comments (one comment currently misstates buffer behavior). |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- 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
+18
to
+21
| // `sourceUrl` is attached to stack traces. This calls Hermes's | ||
| // `hermes_run_script` directly with a zero-copy buffer; `Env::RunScript` | ||
| // works too, through the source-URL overload of `napi_run_script` that | ||
| // env_hermes.cc defines. |
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
js_native_api.hreplaces Node's three-argumentnapi_run_scriptwith the Babylon four-argument form (source_url) insideextern "C". Hermes ships its own Node-API implementation and exports the standard three-argument C symbol, so on HermesEnv::RunScript(script, url)links against that symbol and passes the URL where Hermes expects the result pointer — an access violation on Win32 (0xC0000005) and a SIGSEGV on Android for any node-addon-apiRunScriptcall. Found while adding a test that usedenv.RunScript(#239 switched toNapi::Evalto sidestep it).Changes
EXTERN_C_END(it is only ever called from C++ —Env::RunScriptinnapi-inl.h).nm:_napi_run_script+napi_run_script(napi_env__*, napi_value__*, char const*, napi_value__**)).env_hermes.cc): defines the overload throughhermes_run_script, the same pathNapi::Evaluses; comments updated.NodeApi.RunScriptWithSourceUrlexercisesEnv::RunScript(script, url)on every engine that has it (the V8JSI shim has noRunScript).Verified locally on macOS (JavaScriptCore): full
UnitTestsgreen. Fork twin: rebeckerspecialties#28.