Repository navigation
Conversation
JSC__JSGlobalObject__generateHeapSnapshot (used by console.takeHeapSnapshot) called scope.releaseAssertNoException() after building the snapshot and parsing its JSON. If any exception ended up pending during that sequence the process would abort with a release assertion instead of surfacing the error. Also apply the same treatment to the two bun:jsc helpers that used the same pattern, teach console.takeHeapSnapshot to bail out cleanly if the snapshot call threw, and capture exceptions that surface after waitForPromise/unhandledRejection in HTMLRewriter handler callbacks so they do not escape as an unchecked pending exception.
|
Updated 10:22 AM PT - May 10th, 2026
❌ @robobun, your commit 6905aa4 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 30460That installs a local version of the PR into your bun-30460 --bun |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR hardens heap snapshot generation by adopting consistent exception macros in C++ JSC bindings, adding early-return validation in Zig's takeHeapSnapshot, capturing exceptions thrown during unhandledRejection handling, and adding a test that verifies snapshotting with exotic heap objects. ChangesHeap Snapshot Generation and Exception Handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/util/take-heap-snapshot.test.ts`:
- Around line 26-28: The test currently asserts raw stderr equals "ok" which
fails when ASAN emits "WARNING: ASAN interferes with JSC signal handlers";
update the post-processing of the proc.stderr.text() result (the stderr variable
used with Promise.all([... proc.stderr.text(), proc.exited])) to filter out any
ASAN warning lines (e.g., remove lines that include "WARNING: ASAN interferes
with JSC signal handlers" or match that regex), then trim and assert the
filtered stderr equals "ok" while leaving the exitCode assertion (exitCode
variable) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f12d11db-8de5-49fd-b70e-d4147489fad1
📒 Files selected for processing (1)
test/js/bun/util/take-heap-snapshot.test.ts
|
Closing — the fuzzer crash that motivated this change is a flaky The code changes are straightforward defensive hardening (swap |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/js/bun/util/take-heap-snapshot.test.ts:1-4— nit: per CLAUDE.md / test/CLAUDE.md ("add your test to the existing test file for the code you're changing — do not create a new file"), this regression test could live in the existingtest/js/bun/util/heap-snapshot.test.ts, which already exercises the sameJSC__JSGlobalObject__generateHeapSnapshotbinding. Alternatively, since this fixes a real reported crash (#23393),test/regression/issue/23393.test.tswould also fit. Not blocking — just flagging since d809def moved it out ofheap-snapshot.test.tswithout a stated reason.Extended reasoning...
What this is
This is a minor test-organization convention concern, not a functional bug.
Both the root
CLAUDE.md(§ Test Organization) andtest/CLAUDE.mdstate:Default: add your test to the existing test file for the code you're changing. Do not create a new file.
and list the only exceptions as: a real GitHub-issue regression (→
test/regression/issue/NNNNN.test.ts), an integration test spanning multiple features, or a genuinely new feature with no existing test file.Why it applies here
There is already an existing
test/js/bun/util/heap-snapshot.test.tsthat exercisesBun.generateHeapSnapshot(), which calls the exact same C++ binding (JSC__JSGlobalObject__generateHeapSnapshotinbindings.cpp) that this PR modifies.console.takeHeapSnapshot()is just the ZigConsoleObject.takeHeapSnapshotwrapper around that same call. So by the "existing test file for the code you're changing" rule, the new test belongs inheap-snapshot.test.ts.The git history shows the test was in fact originally added to
heap-snapshot.test.tsin commit16cba24, then moved out to its own file in commitd809def("Move regression test to its own file") with no rationale in the commit message or PR description.Why colocating is safe
The new test spawns a subprocess (
Bun.spawn({ cmd: [bunExe(), "-e", code], ... })) and only asserts on the child's stderr/exit code, so it shares no in-process heap state with the existing FormData/Blob/SharedArrayBuffer tests inheap-snapshot.test.ts. There is no isolation reason it couldn't live alongside them.Alternative location
The github-actions bot identified this as fixing #23393 ("
getHeapSnapshotcauses crash on a large real-world application"). If that linkage is accepted,test/regression/issue/23393.test.tswould also satisfy the convention, since CLAUDE.md explicitly carves outtest/regression/issue/<number>.test.tsfor fixes tied to a real GitHub issue.Step-by-step
- PR modifies
JSC__JSGlobalObject__generateHeapSnapshotinsrc/jsc/bindings/bindings.cpp. - Existing
test/js/bun/util/heap-snapshot.test.tsalready testsBun.generateHeapSnapshot(), which is the JS entry point for that exact function. - CLAUDE.md says: add to the existing test file for the code you're changing.
- PR instead creates
test/js/bun/util/take-heap-snapshot.test.ts(after first adding it to the existing file in16cba24, then moving it out ind809def). - No documented reason for the move; the subprocess test has no state coupling that would require isolation.
Impact & fix
Zero functional impact — purely organizational. To address: either move the test back into
test/js/bun/util/heap-snapshot.test.ts, or rename it totest/regression/issue/23393.test.tsand addFixes #23393to the PR description. If there is a deliberate reason for the separate file (e.g. wanting it to run in its own process group for the snapshot), a brief note would resolve this. - PR modifies
Fuzzilli found a flaky crash where
console.takeHeapSnapshot()aborted with:JSC__JSGlobalObject__generateHeapSnapshot(backingconsole.takeHeapSnapshot) calledscope.releaseAssertNoException()after building the snapshot and runningJSONParse. Any exception that ended up pending at that point — e.g. from property enumeration of exotic objects duringHeapSnapshotBuilder::json(), or an exception left on the VM by other work running while the snapshot is taken — would abort the process rather than propagate.Changes
bindings.cpp/BunJSCModule.h: replacereleaseAssertNoException()withRETURN_IF_EXCEPTION+RELEASE_AND_RETURNinJSC__JSGlobalObject__generateHeapSnapshot,functionGenerateHeapSnapshotForDebugging, andfunctionSamplingProfilerStackTracesso exceptions propagate instead of crashing.ConsoleObject.zig: bail out oftakeHeapSnapshotif the snapshot call threw so we do not pass a zero value (and a pending exception) intomessageWithTypeAndLevel.html_rewriter.zig: afterwaitForPromise/unhandledRejectioninHandlerCallback, capture and clear any exception that surfaced while draining the event loop so it does not escape as an unchecked pending exception on the outer host-call scope.Test
Added a subprocess test that populates the heap with Proxy / ShadowRealm / node:vm contexts (objects whose
constructorlookup may throw) and callsconsole.takeHeapSnapshot(), asserting a clean exit.