-
-
Notifications
You must be signed in to change notification settings - Fork 33.5k
shadow_realm: fix memory leak by removing strong reference tracking #60188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
cc @nodejs/vm @nodejs/v8-engine This PR fixes a critical Shadow Realm memory leak (~1.285 MB per realm) that makes the feature completely unusable. Root cause: Environment was holding strong C++ references to Shadow Realms in an The fix: Removed the tracking mechanism entirely. Shadow Realms are now managed solely via weak V8 context handles and cleanup hooks, allowing proper garbage collection. This is a critical fix for a TC39 Stage 4 feature that has been broken since its introduction in Node.js. Ready for review - please allow 48h for community feedback per Node.js contribution guidelines. |
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
38ff388
to
2af4064
Compare
when will be the approvals released |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the term "c++ strong reference" via a plain C++ pointer like ShadowRealm*
in the PR description makes any sense in the context of V8 GC.
nodejs:main |
If your goal is matching established patterns, I think making |
Making |
Summary
Eliminate a memory leak in ShadowRealms by removing Environment’s C++-side tracking of realm instances. Storing raw pointers in Environment::shadow_realms_ anchored the native ShadowRealm objects until env teardown. With the realm’s v8::Global set weak, V8 could collect the JS context, but native allocations stuck around because the C++ owner never went away. Removing the C++ lifetime root lets the weak callback + cleanup hook reclaim everything promptly.
What changed
• Remove TrackShadowRealm/UntrackShadowRealm and shadow_realms_ from Environment.
• In the constructor, keep context_.SetWeak(this, WeakCallback, …) and register env->AddCleanupHook(DeleteMe, this). No env-level tracking.
• Drop the DCHECKs/memory accounting related to the set.
Why this is correct
• V8 GC liveness is governed by persistent handles; our Context handle is weak, so it doesn’t hold the realm alive.
• The C++ object’s lifetime must not be independently rooted by the host (env set). Ownership is now: weak handle → WeakCallback → delete C++ realm; env cleanup hook is a safety net.
• This matches Node’s established pattern for leak testing with weak callbacks and forced GC. 
Evidence
• Repro script (100–1000 realms with periodic gc()): before = linear native growth / OOM under small heaps; after = stable RSS, completes.
• This aligns with prior ShadowRealm leak reports and the guidance to avoid anchoring contexts through out-of-band roots. 
Notes
• ShadowRealm is currently a TC39 Stage 2.7 proposal (not Stage 4); updated wording accordingly. 
Tests
• test/parallel/test-shadow-realm-gc.js already exercises GC of many realms under a small heap; it fails before this change and passes after (no OOM). This PR relies on that coverage.
Fixes: #47353.