Node-API (JavaScriptCore): only erase the environment registry entry we own - #238
Open
matthargett wants to merge 2 commits into
Open
matthargett wants to merge 2 commits into
matthargett wants to merge 2 commits into
Conversation
…we own The JavaScriptCore backend maps JSGlobalContextRef -> napi_env in a static registry so host callbacks can recover their env from the context. The env destructor erased the entry by context pointer unconditionally. Detach runs after JSGlobalContextRelease (finalizers still need the env), so between the release and the destructor a *new* environment can be handed the same context address and register it; the old destructor then erased the new environment's entry, and its next callback resolved ToNapi() to nullptr and dereferenced it (SIGSEGV inside NativeInfo::Query). With a single environment this never happens, which is why it stayed latent; it surfaced immediately once environments were created and torn down in quick succession (a worker-style create/terminate loop). Erase the entry only if it still maps to this environment.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Concurrent registry access remains unsynchronized and requires mutex protection.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes JavaScriptCore environment registry cleanup so stale environments cannot erase newer registrations.
Changes:
- Makes cleanup ownership-aware.
- Preserves callback resolution during rapid environment recreation.
File summaries
| File | Description |
|---|---|
Core/Node-API/Source/js_native_api_javascriptcore.h |
Adds identity-checked registry cleanup. |
Review details
- Files reviewed: 1/1 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.
Environments live on their own runtime threads, so registration, lookup and the conditional erase all touch one std::unordered_map concurrently.
Author
|
Review addressed in 6f4e7e2 (registry mutex). Fork twin rebeckerspecialties#23 at that head: 24/24 green — https://github.com/rebeckerspecialties/JsRuntimeHost/actions/runs/34841396941 |
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.
The JavaScriptCore backend keeps a static
JSGlobalContextRef -> napi_envregistry so host callbacks (CallAsConstructor,CallAsFunction, …) can recover their env from the context.napi_env__'s destructor erased the entry by context pointer, unconditionally.Detachdeliberately runs afterJSGlobalContextRelease(finalizers still need the env), so between the release and the destructor JavaScriptCore can hand a new environment the very same context address; the new env registers it, and the old destructor then erases the new environment's entry. Its next callback resolvesToNapi()tonullptrand dereferences it — a SIGSEGV insideNativeInfo::Query(Linux shows it as an abort).With one environment per process this is latent, which is why it never showed up; it reproduces immediately with environments created and torn down in quick succession (a worker-style create/terminate loop, which is how we found it). The fix erases the entry only if it still maps to this environment. No behavior change for the single-environment case.
Fork CI twin (full matrix): rebeckerspecialties#23.