fix(heap): don't assume a per-isolate state exists - #384
Open
szegedi wants to merge 2 commits into
Open
Conversation
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>
szegedi
requested review from
IlyasShabi,
nsavoire and
r1viollet
as code owners
August 6, 2026 14:38
Overall package sizeSelf size: 2.46 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 |
|
🔄 Datadog auto-retried 3 jobs - 2 passed on retry 🔗 Commit SHA: 3fafb56 | Docs | Datadog PR Page | Give us feedback! |
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>
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! 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
reviewed
Aug 6, 2026
| } | ||
| std::unique_ptr<v8::AllocationProfile> profile{ | ||
| isolate->GetHeapProfiler()->GetAllocationProfile()}; | ||
| state->profile = TranslateAllocationProfileToCpp(profile->GetRootNode()); |
There was a problem hiding this comment.
Might be worth checking for null profile
nsavoire
reviewed
Aug 6, 2026
| auto state = PerIsolateData::For(isolate)->GetHeapProfilerState(); | ||
|
|
||
| if (!state) { | ||
| // StopSamplingHeapProfiler() resets the state but cannot uninstall this |
There was a problem hiding this comment.
I read the comment several times, but I cannot make sense of it.
the object that tracked its installation is what it just dropped
StopSamplingHeapProfiler resets the state shared_ptr, which destroys it and uninstalls the callback
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.
What
HeapProfiler::GetAllocationProfileandHeapProfiler::MapAllocationProfiledereference the per-isolateHeapProfilerStateafter checking only that V8 returned a profile:A non-null profile only proves V8's sampling heap profiler is running — not that 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 ownStartSamplingHeapProfilercreates the state. The guard then passes with an emptyshared_ptrand we segfault.Why it regressed
Both sites were null-checked through 5.14.4. In 5.15.0
MonitorOutOfMemorychanged from unconditionally replacing the state to reusing an existing one, which made "state already exists" the normal case; the checks were dropped along the way.MapAllocationProfilestill null-checksstateone line above the unguardedOnNewProfile()call, which is a good hint that this was accidental.Restores the pre-5.15.0 behaviour of serving the profile without allocation stats rather than throwing — V8's profiler genuinely is enabled, so
"Heap profiler is not enabled."would be the wrong error.Also fixed
Two pre-existing instances of the same assumption, both reachable because
StopSamplingHeapProfiler()resets the state:NearHeapLimitreadstate->insideCallbackunguarded. The state that recorded the callback's installation is exactly what got dropped, so nothing could uninstall it. Now removes the callback and leaves the heap limit alone so V8 does its normal OOM handling.InterruptCallbackis requested fromNearHeapLimitbut runs later, so the state can disappear in between.Testing
New
ts/test/heap-foreign-sampler.ts, forked fromtest-heap-profiler.tsbecause the failure mode is a SIGSEGV that would otherwise take the whole mocha run down — same pattern as the existingOOMMonitoringtests. It enables V8 heap sampling via the inspector and then calls both entry points.Built the unfixed native code first to confirm the test bites:
Full suite 113 passing / 0 failing,
clang-format -Werrorclean,gts check0 errors.Context
Found while investigating a customer crash on Node.js 20.18.3 / Alpine musl x64 with dd-trace 5.117.0 (
@datadog/pprof5.17.0), reported as appearing in 5.15.0+ but not 5.14.4. This is the only defect in the 5.15.0 delta that produces a SIGSEGV, is reachable on Node 20, and still exists in 5.17.0 —#341's PCP list needs CPED (Node >= 22.9) and#343's allocation profiler needs Node >= 26.It is not confirmed to be that customer's crash: it requires a co-tenant enabling V8 heap sampling, and dd-trace never does so itself. The fix stands on its own regardless.
🤖 Generated with Claude Code