fix(wall): don't derive PersistentContextPtr from node::ObjectWrap - #385
Merged
Conversation
node::ObjectWrap registers a per-instance environment cleanup hook in
its constructor and calls RemoveEnvironmentCleanupHook from its
destructor. That teardown path CHECKs the Environment is still alive:
node[650]: void node::RemoveEnvironmentCleanupHook(
v8::Isolate*, CleanupHook, void*) at ../src/api/hooks.cc:142
Assertion failed: (env) != nullptr
3: node::RemoveEnvironmentCleanupHook(...)
4: node::ObjectWrap::RemoveCleanupHook()
5: node::ObjectWrap::~ObjectWrap()
6: dd::PersistentContextPtr::~PersistentContextPtr()
8: node::ObjectWrap::WeakCallback(...)
A PCP is owned by a weak V8 handle, so V8 decides when it dies — and V8
runs weak callbacks during isolate teardown, after the Environment is
gone. The CHECK then aborts the process with SIGABRT.
The wrapper only ever needed two things from the base class: the
internal-field pointer that GetContextPtrSignalSafe reads, and a weak
handle to hang the object's lifetime on. Neither needs a cleanup hook —
~WallProfiler already walks the live list and deletes any PCP V8 has not
collected, which is what keeps LSAN quiet at exit. So hold the weak
Persistent directly and drop the base class.
~PersistentContextPtr resets the handle, which cancels the weak callback
when ~WallProfiler is the one doing the deleting and is a no-op when we
arrived from the callback itself.
Reproduced on main under ASAN (which perturbs GC timing enough to make
it deterministic) as an abort during teardown after the Time Profiler
tests; the full ASAN suite goes from exit 134 to 158 passing with no
leaks reported.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
szegedi
requested review from
IlyasShabi,
nsavoire and
r1viollet
as code owners
August 6, 2026 15:06
Overall package sizeSelf size: 2.44 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | pprof-format | 2.3.0 | 503.97 kB | 503.97 kB | | source-map | 0.8.0 | 185.66 kB | 185.66 kB | | node-gyp-build | 4.8.4 | 13.86 kB | 13.86 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
|
Node 26 requires an EmbedderDataTypeTag on
Object::SetAlignedPointerInInternalField:
error: no matching function for call to
'v8::Object::SetAlignedPointerInInternalField(int, dd::PersistentContextPtr*)'
note: candidate: 'void v8::Object::SetAlignedPointerInInternalField(
int, void*, v8::EmbedderDataTypeTag)'
note: candidate expects 3 arguments, 2 provided
node::ObjectWrap::Wrap hid this: its header handles the tag internally,
so taking over the store exposed the version difference. Add the setter
counterpart to the existing GetAlignedPointerFromInternalField helper and
use it, so both ends of the internal-field access agree on
kEmbedderDataTypeTagDefault.
Verified on Node 20, 24 and 26 (the last is where AsyncContextFrame is on
by default, so it actually exercises the PCP path): builds clean, 158
passing, ASAN exit 0 with no leaks or aborts.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
nsavoire
approved these changes
Aug 6, 2026
szegedi
enabled auto-merge (squash)
August 7, 2026 06:59
szegedi
added a commit
that referenced
this pull request
Aug 7, 2026
Addresses review feedback on #384. Check GetAllocationProfile for null before dereferencing it. It returns null when V8's sampling heap profiler isn't running, and that is reachable with this callback still installed: HeapProfilerCleanupHook stops V8's sampler without touching our state, so between that hook running and the isolate going away we stay registered with nothing to sample. The heap-limit bookkeeping still has to happen in that case, so only the profile-dependent work is skipped. Also remove the null-state check this branch had added to NearHeapLimit. Its justification was simply wrong: it claimed StopSamplingHeapProfiler could not uninstall the callback, but resetting the state shared_ptr destroys HeapProfilerState, whose destructor calls UninstallNearHeapLimitCallback. The callback cannot fire after the state is gone, so the check was dead code resting on a false premise. The one hole in that argument was ordering inside ~HeapProfilerState: it called V8's StopSamplingHeapProfiler before uninstalling, and by then the shared_ptr in PerIsolateData is already empty, so a GC in that window would have reached NearHeapLimit with no state. Fixed at the source by uninstalling first, which is where the invariant belongs. Node 20 ASAN: exit 0, 99 passing, no leaks, both OOM tests green. Node 24 still aborts on the pre-existing ~PersistentContextPtr teardown CHECK (#385), unrelated to this file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nsavoire
reviewed
Aug 7, 2026
| return object->GetAlignedPointerFromInternalField(index); | ||
| #endif | ||
| // Cancels the weak callback when we're deleted by ~WallProfiler rather than | ||
| // by V8; a no-op when we got here from WeakCallback itself. The holder |
There was a problem hiding this comment.
nit: "a no-op when we got here from WeakCallback itself" is misleading -> v8 docs state that the callback is supposed to reset the handle.
nsavoire
reviewed
Aug 7, 2026
|
|
||
| // Weak handle on the holder object. Owns this PCP: when V8 collects the | ||
| // holder, WeakCallback deletes us. | ||
| v8::Persistent<v8::Object> handle_; |
There was a problem hiding this comment.
nit: not sure if this is relevant but v8 docs warn about using Persistent and suggest using Global instead:
CAVEAT: Persistent objects do not have proper destruction behavior by default and as such will leak the object without explicit clear. Consider using v8::Global instead which has proper destruction and move semantics.
Author
There was a problem hiding this comment.
dang, I actually knew this. Thanks, I'll look into it. It might be okay here as we're specifically only using this as a weak handle so it doesn't leak the object IMO.
Author
There was a problem hiding this comment.
Okay, so I think we are actually okay here as we're explicitly used Persistent here anyway for very long time before 7bf5cc6 (the class name is kind of a clue :-) ).
Still, we could idiomatically switch to Global since we're using that everywhere else anyway, and then we don't need to call handle_.Reset() anymore in the PersistentContextPtr destructor.
szegedi
added a commit
that referenced
this pull request
Aug 7, 2026
Review follow-up on #385. v8::Persistent has no destruction behaviour: the handle leaks unless every path clears it by hand. v8::Global releases it in its own destructor, and is what every other handle in this file already uses — ContextPtr, cpedKey_, wrapObjectTemplate_, jsArray_. The Persistent introduced in #385 was the odd one out. Nothing was leaking in practice, since ~PersistentContextPtr always reset the handle explicitly, but relying on that is exactly the footgun the V8 docs warn about. Switching to Global makes the release structural, so the explicit Reset goes away with it. Historically the manual handle was justified: before #261 removed instance reuse, PersistentContextPtr recycled itself through a freelist and needed ClearWeak/Reset to unregister and re-register the same object. With reuse gone a handle lives exactly as long as its PCP, so there is nothing left for Persistent's manual semantics to buy. Also correct the destructor comment. It claimed the reset was "a no-op when we got here from WeakCallback itself", which is backwards: V8 requires a weak callback to reset the handle, so that path is precisely where the release is load-bearing. Verified on Node 20, 24 and 26 — the last is where AsyncContextFrame is on by default and PCPs are actually created. 163 passing, ASAN exit 0 with no leaks and no aborts on 20 and 24.
nsavoire
reviewed
Aug 7, 2026
There was a problem hiding this comment.
Would using nan::ObjectWrap instead of node::ObjectWrap have solved the issue ?
Can the same issue occur for CtxWrap ?
Author
There was a problem hiding this comment.
I kinda don't want to rely too much on nan, I honestly feel a bit more comfortable handling this on my own. I'll look into CtxWrap. My gut feeling is that it's okay because we aren't doing live instance tracking like WallProfiler does but I'll unleash Claude on it to confirm. FWIW it feels dodgy to me that node::ObjectWrap would be a public API if it had this dangerous of an edge case.
Author
There was a problem hiding this comment.
huh, interesting. I can reproduce this with CtxWrap too on current main 😢:
node[339]: void node::RemoveEnvironmentCleanupHook(v8::Isolate*, CleanupHook, void*) at ../src/api/hooks.cc:142
Assertion failed: (env) != nullptr
2: node::RemoveEnvironmentCleanupHook(...)
3: [/src/build/Release/dd_pprof.node] ← ~CtxWrap → ~ObjectWrap → RemoveCleanupHook
4-8: [node] ← GC / weak callback
This happens deterministically by creating 3000 ThreadContexts, some in ALS, some abandoned.
There's 3 GC contexts in which the path can be invoked:
- collected during ordinary GC: a context is entered; it's fine
- still alive at env teardown: deleted by ObjectWrap's own cleanup hook, while the env is alive; also fine
- the problem is the third: objects that become garbage late and in bulk, so V8 only collects them during the teardown GC, after the Environment is gone. That needs enough garbage to provoke that GC at all hence it appears at around 3000 objects on my machine.
I was wondering why this doesn't come out for thousands of other addons that use ObjectWrap, but I guess most of them wrap a handful of long-lived objects and never enter this situation where we create thousands and thousands of objects.
Both PCP and CtxWrap have exactly this unusual profile: many short-lived instances whose lifetime is tied to async contexts, retained in a CPED maps until the very end of execution.
Now I have been digging into this deeper, basically why does node::ObjectWrap need that CHECK at all, and it's because it registers per-instance hooks to guarantee that the C++ object is deleted at env teardown even if V8 never collects it. With PCP we replaced that guarantee with ~WallProfiler's list walk over liveContextPtrHead_.
We don't have anything similar for CtxWrap at the moment, but we do have a PerIsolateData environment cleanup hook:
per_isolate_data_.emplace(...);
node::AddEnvironmentCleanupHook(isolate,
[](void* data) { per_isolate_data_.erase(static_cast<Isolate*>(data)); },
isolate);
where we could add this teardown, but then CtxWrap class also needs to become an intrusive linked list; I think I'm fine with that. Some things would actually become better :-) like, if we remove ObjectWrap as ancestor, its virtual destructor goes away, so instances no longer have a vptr and we can get rid of the -Winvalid-offsetof pragma. Even better, NATIVE_WRAP_FIELDS_OFFSET changes from sizeof(node::ObjectWrap) which is a foreign type we don't control to offsetof(CtxWrap, record_), which becomes 0 and stays correct through any future layout change.
I'd say let's merge #387 and then I'll work on this separately.
szegedi
added a commit
that referenced
this pull request
Aug 7, 2026
Review follow-up on #385. v8::Persistent has no destruction behaviour: the handle leaks unless every path clears it by hand. v8::Global releases it in its own destructor, and is what every other handle in this file already uses — ContextPtr, cpedKey_, wrapObjectTemplate_, jsArray_. The Persistent introduced in #385 was the odd one out. Nothing was leaking in practice, since ~PersistentContextPtr always reset the handle explicitly, but relying on that is exactly the footgun the V8 docs warn about. Switching to Global makes the release structural, so the explicit Reset goes away with it. Historically the manual handle was justified: before #261 removed instance reuse, PersistentContextPtr recycled itself through a freelist and needed ClearWeak/Reset to unregister and re-register the same object. With reuse gone a handle lives exactly as long as its PCP, so there is nothing left for Persistent's manual semantics to buy. Verified on Node 20, 24 and 26 — the last is where AsyncContextFrame is on by default and PCPs are actually created. 163 passing, ASAN exit 0 with no leaks and no aborts on 20 and 24.
szegedi
added a commit
that referenced
this pull request
Aug 7, 2026
CtxWrap has the same defect #385 fixed in the wall profiler's PersistentContextPtr. node::ObjectWrap registers a per-instance environment cleanup hook in its constructor and calls RemoveEnvironmentCleanupHook from its destructor, which CHECKs that an Environment is current. A CtxWrap is owned by a weak V8 handle, so V8 picks the moment it dies, and weak callbacks run during isolate teardown with no context entered: Assertion failed: (env) != nullptr 2: node::RemoveEnvironmentCleanupHook(...) 3: otel_thread_ctx_nodejs::CtxWrap::~CtxWrap() This one is not subtle: create a few thousand ThreadContexts and exit normally and it aborts every time, on a plain release build. No ASAN needed, unlike the PCP case. Nothing below ~1000 instances reproduces it — V8 has to still have some left to collect at teardown. Note the CHECK guards something real, so it must not be worked around by skipping the removal. Environment::GetCurrent(isolate) returns null on `!isolate->InContext()` alone, so the Environment may well still be alive; leaving a hook behind whose arg is a freed pointer would turn the abort into a use-after-free when CleanupQueue::Drain later calls it. The fix is to not register the per-instance hook at all. Dropping the base loses what that hook provided: deletion at teardown even when V8 never collects the object. PCP could rely on ~WallProfiler walking its live list; CtxWrap has no such owner and owns a malloc'd record, so without a replacement this would trade an abort for a leak. Add the equivalent: a thread-local list of live CtxWraps drained by a single per-isolate cleanup hook, registered from Wrap() — inside a JS constructor call, where a context is entered, so AddEnvironmentCleanupHook is satisfied honestly — and never removed, since it fires once at teardown while the Environment is alive. One hook per isolate instead of one per instance, with removal timing we control rather than V8. With no base class, `record_` becomes CtxWrap's first member, so the published threadlocal.native_wrap_fields_offset goes from 24 to 0 and is now computed with offsetof rather than sizeof() of a foreign type. That is a reader-contract change, made now because no readers exist yet. Losing the base also makes CtxWrap standard-layout — no base subobject, no virtuals, all data members in one access section — so offsetof on it is now unconditionally valid and the two -Winvalid-offsetof suppressions the inheriting version needed are gone. A static_assert on is_standard_layout keeps it that way, since the reader contract depends on offsetof(record_) being well-defined. The two internal-field accessors move to a new internal-field.hh: Node 26 requires an EmbedderDataTypeTag on both the get and the set, and having the pair in one place stops them drifting when only one is exercised on the version you build against. wall.cc keeps its own copies for now to avoid conflicting with in-flight work there; folding those in is a follow-up. Verified on Node 20, 24 and 26, with both clang and gcc. New regression test fails with signal=SIGABRT against the pre-fix binding and passes after; ASAN exit 0 with zero leaks on 20 and 24, which is the check that the drain hook really does replace what ObjectWrap was doing.
szegedi
added a commit
that referenced
this pull request
Aug 7, 2026
Addresses review feedback on #384. Check GetAllocationProfile for null before dereferencing it. It returns null when V8's sampling heap profiler isn't running, and that is reachable with this callback still installed: HeapProfilerCleanupHook stops V8's sampler without touching our state, so between that hook running and the isolate going away we stay registered with nothing to sample. The heap-limit bookkeeping still has to happen in that case, so only the profile-dependent work is skipped. Also remove the null-state check this branch had added to NearHeapLimit. Its justification was simply wrong: it claimed StopSamplingHeapProfiler could not uninstall the callback, but resetting the state shared_ptr destroys HeapProfilerState, whose destructor calls UninstallNearHeapLimitCallback. The callback cannot fire after the state is gone, so the check was dead code resting on a false premise. The one hole in that argument was ordering inside ~HeapProfilerState: it called V8's StopSamplingHeapProfiler before uninstalling, and by then the shared_ptr in PerIsolateData is already empty, so a GC in that window would have reached NearHeapLimit with no state. Fixed at the source by uninstalling first, which is where the invariant belongs. Node 20 ASAN: exit 0, 99 passing, no leaks, both OOM tests green. Node 24 still aborts on the pre-existing ~PersistentContextPtr teardown CHECK (#385), unrelated to this file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
szegedi
added a commit
that referenced
this pull request
Aug 7, 2026
CtxWrap has the same defect #385 fixed in the wall profiler's PersistentContextPtr. node::ObjectWrap registers a per-instance environment cleanup hook in its constructor and calls RemoveEnvironmentCleanupHook from its destructor, which CHECKs that an Environment is current. A CtxWrap is owned by a weak V8 handle, so V8 picks the moment it dies, and weak callbacks run during isolate teardown with no context entered: Assertion failed: (env) != nullptr 2: node::RemoveEnvironmentCleanupHook(...) 3: otel_thread_ctx_nodejs::CtxWrap::~CtxWrap() This one is not subtle: create a few thousand ThreadContexts and exit normally and it aborts every time, on a plain release build. No ASAN needed, unlike the PCP case. Nothing below ~1000 instances reproduces it — V8 has to still have some left to collect at teardown. Note the CHECK guards something real, so it must not be worked around by skipping the removal. Environment::GetCurrent(isolate) returns null on `!isolate->InContext()` alone, so the Environment may well still be alive; leaving a hook behind whose arg is a freed pointer would turn the abort into a use-after-free when CleanupQueue::Drain later calls it. The fix is to not register the per-instance hook at all. Dropping the base loses what that hook provided: deletion at teardown even when V8 never collects the object. PCP could rely on ~WallProfiler walking its live list; CtxWrap has no such owner and owns a malloc'd record, so without a replacement this would trade an abort for a leak. Add the equivalent: a thread-local list of live CtxWraps drained by a single per-isolate cleanup hook, registered from Wrap() — inside a JS constructor call, where a context is entered, so AddEnvironmentCleanupHook is satisfied honestly — and never removed, since it fires once at teardown while the Environment is alive. One hook per isolate instead of one per instance, with removal timing we control rather than V8. With no base class, `record_` becomes CtxWrap's first member, so the published threadlocal.native_wrap_fields_offset goes from 24 to 0 and is now computed with offsetof rather than sizeof() of a foreign type. That is a reader-contract change, made now because no readers exist yet. Losing the base also makes CtxWrap standard-layout — no base subobject, no virtuals, all data members in one access section — so offsetof on it is now unconditionally valid and the two -Winvalid-offsetof suppressions the inheriting version needed are gone. A static_assert on is_standard_layout keeps it that way, since the reader contract depends on offsetof(record_) being well-defined. The two internal-field accessors move to a new internal-field.hh: Node 26 requires an EmbedderDataTypeTag on both the get and the set, and having the pair in one place stops them drifting when only one is exercised on the version you build against. wall.cc keeps its own copies for now to avoid conflicting with in-flight work there; folding those in is a follow-up. Verified on Node 20, 24 and 26, with both clang and gcc. New regression test fails with signal=SIGABRT against the pre-fix binding and passes after; ASAN exit 0 with zero leaks on 20 and 24, which is the check that the drain hook really does replace what ObjectWrap was doing.
szegedi
added a commit
that referenced
this pull request
Aug 7, 2026
CtxWrap has the same defect #385 fixed in the wall profiler's PersistentContextPtr. node::ObjectWrap registers a per-instance environment cleanup hook in its constructor and calls RemoveEnvironmentCleanupHook from its destructor, which CHECKs that an Environment is current. A CtxWrap is owned by a weak V8 handle, so V8 picks the moment it dies, and weak callbacks run during isolate teardown with no context entered: Assertion failed: (env) != nullptr 2: node::RemoveEnvironmentCleanupHook(...) 3: otel_thread_ctx_nodejs::CtxWrap::~CtxWrap() This one is not subtle: create a few thousand ThreadContexts and exit normally and it aborts every time, on a plain release build. No ASAN needed, unlike the PCP case. Nothing below ~1000 instances reproduces it — V8 has to still have some left to collect at teardown. Note the CHECK guards something real, so it must not be worked around by skipping the removal. Environment::GetCurrent(isolate) returns null on `!isolate->InContext()` alone, so the Environment may well still be alive; leaving a hook behind whose arg is a freed pointer would turn the abort into a use-after-free when CleanupQueue::Drain later calls it. The fix is to not register the per-instance hook at all. Dropping the base loses what that hook provided: deletion at teardown even when V8 never collects the object. PCP could rely on ~WallProfiler walking its live list; CtxWrap has no such owner and owns a malloc'd record, so without a replacement this would trade an abort for a leak. Add the equivalent: a thread-local list of live CtxWraps drained by a single per-isolate cleanup hook, registered from Wrap() — inside a JS constructor call, where a context is entered, so AddEnvironmentCleanupHook is satisfied honestly — and never removed, since it fires once at teardown while the Environment is alive. One hook per isolate instead of one per instance, with removal timing we control rather than V8. With no base class, `record_` becomes CtxWrap's first member, so the published threadlocal.native_wrap_fields_offset goes from 24 to 0 and is now computed with offsetof rather than sizeof() of a foreign type. That is a reader-contract change, made now because no readers exist yet. Losing the base also makes CtxWrap standard-layout — no base subobject, no virtuals, all data members in one access section — so offsetof on it is now unconditionally valid and the two -Winvalid-offsetof suppressions the inheriting version needed are gone. A static_assert on is_standard_layout keeps it that way, since the reader contract depends on offsetof(record_) being well-defined. The two internal-field accessors move to a new internal-field.hh: Node 26 requires an EmbedderDataTypeTag on both the get and the set, and having the pair in one place stops them drifting when only one is exercised on the version you build against. wall.cc keeps its own copies for now to avoid conflicting with in-flight work there; folding those in is a follow-up. Verified on Node 20, 24 and 26, with both clang and gcc. New regression test fails with signal=SIGABRT against the pre-fix binding and passes after; ASAN exit 0 with zero leaks on 20 and 24, which is the check that the drain hook really does replace what ObjectWrap was doing.
szegedi
added a commit
that referenced
this pull request
Aug 7, 2026
* fix(heap): don't assume a per-isolate state exists
GetAllocationProfile and MapAllocationProfile dereference the
per-isolate HeapProfilerState after only checking that V8 returned a
profile:
auto& state = PerIsolateData::For(isolate)->GetHeapProfilerState();
std::unique_ptr<v8::AllocationProfile> profile(
isolate->GetHeapProfiler()->GetAllocationProfile());
if (!profile) {
return Nan::ThrowError("Heap profiler is not enabled.");
}
const bool allocations = state->allocations; // <- state may be null
A non-null profile only proves V8's sampling heap profiler is running.
It does not prove we started it: anything else in the process can enable
it out of band — the inspector's HeapProfiler.startSampling, DevTools, a
second agent — and only our own StartSamplingHeapProfiler creates the
state. In that case the guard passes and we dereference an empty
shared_ptr, which segfaults.
Both call sites were null-checked until 5.15.0, when MonitorOutOfMemory
switched from unconditionally replacing the state to reusing an existing
one. That made "state already exists" the normal case and the checks
were dropped along the way — MapAllocationProfile still null-checks
`state` one line above the unguarded OnNewProfile() call.
Restore the checks, keeping the pre-5.15.0 behaviour of serving the
profile without allocation stats rather than throwing: V8's profiler
really is enabled, so "Heap profiler is not enabled." would be wrong.
Fix two pre-existing instances of the same assumption while here, both
reachable because StopSamplingHeapProfiler() resets the state:
- NearHeapLimit ran `state->insideCallback` unguarded. The state that
recorded the callback's installation is the one that was dropped, so
nothing could uninstall it. Remove the callback and leave the heap
limit alone so V8 does its normal OOM handling.
- InterruptCallback is requested from NearHeapLimit but runs later, so
the state can disappear in between.
The regression test forks a child process, since the failure mode is a
SIGSEGV that would otherwise take the whole mocha run down with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(heap): keep the forked child out of LeakSanitizer's reach
Under the asan CI job the forked child inherits LD_PRELOAD=libasan and
LSAN_OPTIONS, so LeakSanitizer runs when it exits. The child ends via
process.exit(), which skips V8 heap teardown, so every live object is
reported as leaked and the child exits non-zero — failing the test for a
reason unrelated to what it checks. Seen on asan (20):
1) foreign heap sampler
should not crash when V8 heap sampling was enabled outside of
pprof:
Error: heap-foreign-sampler exited with code=1 signal=null
Pass LSAN_OPTIONS=detect_leaks=0 to the child. ASAN itself stays active,
so a real memory error in the code under test is still caught; only the
exit-time leak sweep is suppressed, and only for this child.
Two things made this harder to diagnose than it should have been, both
fixed here:
- The failure message came through empty because the promise settled
on 'exit', which can fire before the piped stdio has drained. Settle
on 'close' instead, so the captured output is complete.
- Drop the retained allocation from 200k objects to 20k and keep it
function-scoped rather than parking it on globalThis. The profile
only needs a non-empty sample set.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(heap): guard NearHeapLimit's profile, drop its bogus state check
Addresses review feedback on #384.
Check GetAllocationProfile for null before dereferencing it. It returns
null when V8's sampling heap profiler isn't running, and that is reachable
with this callback still installed: HeapProfilerCleanupHook stops V8's
sampler without touching our state, so between that hook running and the
isolate going away we stay registered with nothing to sample. The
heap-limit bookkeeping still has to happen in that case, so only the
profile-dependent work is skipped.
Also remove the null-state check this branch had added to NearHeapLimit.
Its justification was simply wrong: it claimed StopSamplingHeapProfiler
could not uninstall the callback, but resetting the state shared_ptr
destroys HeapProfilerState, whose destructor calls
UninstallNearHeapLimitCallback. The callback cannot fire after the state
is gone, so the check was dead code resting on a false premise.
The one hole in that argument was ordering inside ~HeapProfilerState: it
called V8's StopSamplingHeapProfiler before uninstalling, and by then the
shared_ptr in PerIsolateData is already empty, so a GC in that window
would have reached NearHeapLimit with no state. Fixed at the source by
uninstalling first, which is where the invariant belongs.
Node 20 ASAN: exit 0, 99 passing, no leaks, both OOM tests green. Node 24
still aborts on the pre-existing ~PersistentContextPtr teardown CHECK
(#385), unrelated to this file.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(heap): uninstall the near-heap-limit callback before dropping the state
Two review nits from #384.
StopSamplingHeapProfiler relied on ~HeapProfilerState to uninstall the
near-heap-limit callback, but reset() only destroys the state when it holds
the last reference — and it need not. Both NearHeapLimit and
InterruptCallback take a shared_ptr copy for the duration of the call, so a
stop() reached from inside one of them (the near-heap-limit JS callback
calling heapProfiler.stop(), say) leaves the state alive, the destructor
unrun, and the callback still registered with V8 while the per-isolate slot
is already empty. The next near-heap-limit GC would then enter
NearHeapLimit with no state at all — exactly the crash this branch is
about. Uninstall explicitly instead; it is idempotent, clearing
callbackInstalled.
Also keep clearing state->profile when GetAllocationProfile returns null.
Any profile retained from an earlier invocation is stale at that point and
nothing below is going to consume or replace it.
* fix(heap): re-add NearHeapLimit's null-state guard, with a real reason
The version of this check removed earlier on this branch rested on a false
premise — that StopSamplingHeapProfiler could not uninstall the callback —
and deserved to go. There is a genuine reason for it, though, which only
became apparent from the shared_ptr-copy problem in the previous commit.
StopSamplingHeapProfiler now uninstalls before dropping the state, so that
path is covered. The other destruction path is not: a shared_ptr copy taken
by an in-flight NearHeapLimit or InterruptCallback can outlive the
per-isolate slot. If the OOM JS callback calls process.exit(),
PerIsolateData is erased while InterruptCallback still holds a reference,
~HeapProfilerState never runs, and the callback stays registered with an
empty slot behind it. A teardown GC reaching the heap limit then enters
NearHeapLimit with no state and dereferences null.
Decline and let V8 do its normal OOM handling. Deliberately no
RemoveNearHeapLimitCallback: the state that tracked the installation is
already unreachable, so callbackInstalled cannot be cleared, and the only
way to reach this is a process on its way out.
Kept as its own commit because it partially reverses a change made earlier
on this branch, and because it is defence in depth rather than a fix for
anything reproducible — the trigger needs process.exit() from inside the OOM
callback plus a teardown GC that hits the limit, which I could not turn into
a non-flaky test. The branch it adds is therefore uncovered.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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 abort
node::ObjectWrapregisters a per-instance environment cleanup hook in its constructor, and its destructor callsRemoveEnvironmentCleanupHook, whichCHECKs that the Environment is still alive:A
PersistentContextPtris owned by a weak V8 handle, so V8 decides when it dies — and V8 runs weak callbacks during isolate teardown, after the Environment is gone. The CHECK then aborts the process (SIGABRT, exit 134).The fix
The wrapper only ever needed two things from the base class: the internal-field pointer that
GetContextPtrSignalSafereads, and a weak handle to hang the object's lifetime on. Neither requires a cleanup hook —~WallProfileralready walks the live list and deletes any PCP V8 hasn't collected, which is what keeps LSAN quiet at exit.So hold the weak
Persistentdirectly and drop the base class.~PersistentContextPtrresets the handle, which cancels the weak callback when~WallProfileris the one deleting, and is a no-op when we arrived from the callback itself.Verification
Reproducible on unmodified main — ASAN perturbs GC timing enough to make it deterministic. It surfaces as an abort during teardown right after the Time Profiler tests, so mocha never prints its summary:
npm run test:js-asannpm testThe zero leak count matters: it confirms
~WallProfilerreally is sufficient and the per-instance cleanup hooks were not load-bearing.Found while CI was red on #384; it is unrelated to that PR's heap-profiler change, which is why it is split out here.
Jira: PROF-15673