Fix V8 N-API leaking every promise ever created - #227
Conversation
v8impl::Persistent aliased v8::Persistent<T>, whose traits set kResetInDestructor = false, so destroying one never releases the underlying global handle. ConcludeDeferred() disposes the deferred with `delete deferred_ref`, which therefore leaked the handle and pinned the resolved promise -- and everything reachable from its fulfilled value -- for the lifetime of the isolate. Alias v8::Global<T> instead, as upstream Node.js does. Global resets in its destructor, so the existing `delete` now releases the handle. This also covers napi_env__::context_persistent, which relies on the same destructor. Found via a heap snapshot of a long-running Babylon Native session: 769 fulfilled promises, each retained solely by a global handle, holding 404 MB of decoded-image ArrayBuffers. Peak RSS over a full validation sweep drops from >13 GB (OOM crash) to ~2.4 GB. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
There was a problem hiding this comment.
Pull request overview
This PR fixes a V8 N-API memory leak where N-API-created promises (and related persistent handles) could be pinned for the lifetime of the isolate due to v8::Persistent not resetting in its destructor under the default traits.
Changes:
- Switch
v8impl::Persistent<T>fromv8::Persistent<T>tov8::Global<T>so persistent handles are released on destruction. - Aligns this internal N-API V8 handle type with upstream Node.js behavior.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bghgary
left a comment
There was a problem hiding this comment.
[Reviewed by Copilot on behalf of @bghgary]
This was my mistake from the original fork, not later drift — the file was hand-written to drop Node's internal dependencies, and the alias came out v8::Persistent.
Fixing it properly means syncing this file from upstream rather than patching it, which is separate work. This change is fine on its own.
…escape UAF, BabylonJS#225 QuickJS throw) Takes upstream's escapable-handle-scope implementation (BabylonJS#223) over shotgun's earlier 111efc5 attempt. Upstream's is strictly more correct: it keys scopes by a monotonic counter rather than a position in handle_scope_stack (two scopes opened with no handle allocated between them share a position, so a position-derived token cannot tell them apart), holds the escaped handle beside the scope instead of inserting it into the middle of the stack (which shifted every entry above it and invalidated the recorded start of any still-open nested scope), and rejects out-of-LIFO closes rather than corrupting the stack. Shared.cpp is resolved hunk-by-hunk rather than with `git checkout --theirs`: the two quickjs files carry only the superseded 111efc5, but Shared.cpp also carries shotgun's unhandled-promise-rejection tests (still unlanded upstream as BabylonJS#204), which taking the whole file would have silently dropped. The merged file is exactly the union of both sides' test cases, 9 + 10 -> 12 with 7 shared. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Problem
v8impl::Persistent<T>was aliased tov8::Persistent<T>, whose default traits setkResetInDestructor = false— the destructor does not dispose the global handle.ConcludeDeferred()(behindnapi_resolve_deferred/napi_reject_deferred) relies onthat destructor:
v8impl::Persistent<v8::Value>* deferred_ref = NodePersistentFromJsDeferred(deferred); ... delete deferred_ref;So the
deletefrees the wrapper but leaks the handle, pinning every promise created vianapi_create_promise— and everything its settled value retains — for the life of theisolate.
napi_env__::context_persistentleaks the same way.v8impl::Referenceisunaffected; it calls
persistent_.Reset()explicitly.Fix
Alias
v8::Global<T>, which always resets in its destructor. This is what upstreamNode.js uses in
src/js_native_api_v8_internals.h;the divergence came from vendoring, where the original
node::Persistent<T>(av8::PersistentwithkResetInDestructor = true) was swapped for the plain V8 type.v8::Globalis move-only like the non-copyablev8::Persistentit replaces, and everyother member used (
Reset,SetWeak,ClearWeak,IsEmpty,Local::New) lives onv8::PersistentBase, so no call sites change.Testing
Static finding from the source, the V8 headers, and upstream parity; no measured
before/after attached. To confirm at runtime, run a long-lived host that settles many
promises (e.g. an image decode returning one promise per image) and watch external
memory / global handle count. Happy to add that before merging.