console.takeHeapSnapshot: clear prior snapshots before building#30817
console.takeHeapSnapshot: clear prior snapshots before building#30817robobun wants to merge 1 commit into
Conversation
|
Updated 3:43 AM PT - May 17th, 2026
✅ @robobun, your commit f197807c2bda83c0102cd040f6fad38e714ca6dd passed in 🧪 To try this PR locally: bunx bun-pr 30817That installs a local version of the PR into your bun-30817 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughHeap snapshot generation refactored to ensure profiler lifecycle safety. C++ binding adds explicit heap profiler initialization, snapshot cleanup, and exception handling. Rust wrapper changes return type to ChangesHeap Snapshot Error 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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/console-takeHeapSnapshot.test.ts`:
- Line 20: The test couples to a validator-specific env toggle
(BUN_JSC_validateExceptionChecks) and asserts the absence of an exception-log
string; remove the brittle parts so the test only checks observable behavior.
Delete the BUN_JSC_validateExceptionChecks environment entry and remove the
assertion that looks for/against the exception-log string, leaving the existing
checks that the process prints "OK" and exits with the expected code (keep the
exit code/"OK" assertions intact in this test file).
🪄 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: 51c0a67d-c704-4814-8840-9313c758eec1
📒 Files selected for processing (6)
src/jsc/ConsoleObject.rssrc/jsc/ConsoleObject.zigsrc/jsc/JSGlobalObject.rssrc/jsc/JSGlobalObject.zigsrc/jsc/bindings/bindings.cpptest/js/bun/util/console-takeHeapSnapshot.test.ts
| ], | ||
| env: { | ||
| ...bunEnv, | ||
| BUN_JSC_validateExceptionChecks: "1", |
There was a problem hiding this comment.
Remove validator/log-string coupling from this test.
Line 28 asserts absence of an exception-log string, which is a brittle/non-actionable signal for CI. Keep the test anchored to observable behavior ("OK" + exit code), and drop the validator-specific env toggle at Line 20.
♻️ Proposed fix
await using proc = Bun.spawn({
@@
env: {
...bunEnv,
- BUN_JSC_validateExceptionChecks: "1",
},
@@
- expect(stderr).not.toContain("Unchecked JS exception");
expect(stderr).toContain("OK");
expect(exitCode).toBe(0);Also applies to: 28-28
🤖 Prompt for 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.
In `@test/js/bun/util/console-takeHeapSnapshot.test.ts` at line 20, The test
couples to a validator-specific env toggle (BUN_JSC_validateExceptionChecks) and
asserts the absence of an exception-log string; remove the brittle parts so the
test only checks observable behavior. Delete the BUN_JSC_validateExceptionChecks
environment entry and remove the assertion that looks for/against the
exception-log string, leaving the existing checks that the process prints "OK"
and exits with the expected code (keep the exit code/"OK" assertions intact in
this test file).
There was a problem hiding this comment.
LGTM — the latest commit addresses both prior concerns (wraps the FFI in from_js_host_call/fromJSHostCall and switches to RETURN_IF_EXCEPTION), and the test now explicitly exercises validateExceptionChecks=1.
Extended reasoning...
Overview
This PR fixes a fuzzer-found SIGSEGV in console.takeHeapSnapshot() by clearing prior heap snapshots before building a new one, and makes the FFI boundary exception-aware. It touches:
bindings.cpp(~10 lines): addsHeapProfiler.hinclude, callsensureHeapProfiler()+clearSnapshots()before constructing the builder, and replacesscope.releaseAssertNoException()withRETURN_IF_EXCEPTION(scope, {}).JSGlobalObject.{rs,zig}/ConsoleObject.{rs,zig}(~8 lines): wraps the extern call infrom_js_host_call/fromJSHostCall(returningJsResult/bun.JSError!) and early-returns on error in the only caller.- New regression test that runs
require()-fail →takeHeapSnapshot()→Bun.gc(true)5× in a subprocess withBUN_JSC_validateExceptionChecks=1forced on, asserting no "Unchecked JS exception" in stderr. - 10 Rust files with pure
#[cfg(...)]/#[cfg_attr(...)]whitespace reflow from autofix.ci — zero semantic change.
Security risks
None. This is a debugging/profiling API (console.takeHeapSnapshot); no auth, crypto, permissions, or untrusted input handling involved. The change narrows a UAF/crash window rather than introducing one.
Level of scrutiny
Low–moderate. The core fix is a 4-line application of an established pattern — ensureHeapProfiler() + clearSnapshots() is used identically in BunObject.cpp:808-810, BunHeapProfiler.cpp:116-118 and :941-943, and JSWorker.cpp:701-703. The exception-propagation change follows the same from_js_host_call / fromJSHostCall convention used by the immediately-adjacent create_aggregate_error_with_array (JSGlobalObject.rs:945) and dozens of other bindings in the same file. The rustfmt hunks are mechanical.
Other factors
I left two inline comments on earlier revisions of this PR (a 🟡 nit on the bare-JSValue return and a 🔴 on the x64-asan unchecked exception failure). Commit 2afdc95 implements the "best" fix I recommended for both: the Rust/Zig callers are now exception-aware via from_js_host_call, the C++ side checks the exception in-scope with RETURN_IF_EXCEPTION, and the test was updated to explicitly force BUN_JSC_validateExceptionChecks: "1" and assert stderr does not contain "Unchecked JS exception" — directly validating the scenario that previously failed CI. The bug-hunting system found no issues on the current revision.
robobun
force-pushed
the
farm/0042a3ec/console-takeHeapSnapshot-clear-prior
branch
from
3eccbc0 to
714ffb7
Compare
…se exceptions JSC__JSGlobalObject__generateHeapSnapshot now calls heapProfiler.clearSnapshots() before building, so repeated console.takeHeapSnapshot() calls do not accumulate stale snapshot state, and it returns any JSONParse exception to the caller instead of release-asserting. On the Rust side, generate_heap_snapshot is wrapped in from_js_host_call so the pending exception is observed, and Bun__ConsoleObject__takeHeapSnapshot bails out early on Err instead of logging a zero JSValue.
robobun
force-pushed
the
farm/0042a3ec/console-takeHeapSnapshot-clear-prior
branch
from
714ffb7 to
f197807
Compare
|
Rebased onto
The fix is now just |
There was a problem hiding this comment.
LGTM — both earlier concerns are addressed (Rust caller now uses from_js_host_call, C++ uses RETURN_IF_EXCEPTION); remaining inline comments are test-organization nits.
Extended reasoning...
Overview
Four files: bindings.cpp (JSC__JSGlobalObject__generateHeapSnapshot now calls heapProfiler.clearSnapshots() before building and uses RETURN_IF_EXCEPTION(scope, {}) instead of releaseAssertNoException()), JSGlobalObject.rs (generate_heap_snapshot now returns JsResult<JSValue> via from_js_host_call), ConsoleObject.rs (early-return on Err instead of forwarding JSValue::ZERO into the formatter), and a new regression test. The branch was rebased/squashed on 2026-05-17 to drop unrelated .zig mirror edits and cargo fmt reflow noise.
Security risks
None. This is a heap-snapshot debugging code path with no auth, crypto, parsing of untrusted input, or permission checks involved. The change replaces a releaseAssertNoException() (process abort on OOM) with proper exception propagation, which is strictly less crash-prone.
Level of scrutiny
Low–medium. Although it touches the C++/Rust FFI boundary, the C++ hunk is a near-verbatim copy of the pattern already used at BunObject.cpp:808-810, BunHeapProfiler.cpp:116-118 and :941-943, and JSWorker.cpp:701-703 — console.takeHeapSnapshot() was the lone outlier that didn't clear prior snapshots. The Rust side follows the from_js_host_call convention used throughout src/jsc/ (e.g. create_aggregate_error_with_array two functions above). No novel design decisions.
Other factors
I left two inline comments on earlier revisions (3248206952 on RELEASE_AND_RETURN leaving the Rust caller exception-unaware, 3248506528 on the test-file move not fixing the asan failure); both are now resolved by this revision exactly as suggested. The two new inline findings on this run are [Nit]-grade (test file location per CLAUDE.md, and a redundant .not.toContain("Unchecked JS exception") assertion subsumed by the exit-code check) — neither affects the fix's correctness or CI signal, so they don't block. No CODEOWNERS cover these paths.
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe } from "harness"; | ||
|
|
||
| test("console.takeHeapSnapshot can be called repeatedly after a failed require", async () => { |
There was a problem hiding this comment.
🟡 nit: per CLAUDE.md ("add tests to the existing test file … do not create a new file"), this should live in test/js/bun/util/heap-snapshot.test.ts rather than its own file. The timeline shows it was originally there and was split out only to dodge the x64-asan unchecked-exception assert — but that assert fired in the spawned subprocess, so moving files couldn't help, and the real fix (from_js_host_call + RETURN_IF_EXCEPTION) is now in this PR. With no remaining reason for the separation, it'd be cleaner to fold it back.
Extended reasoning...
What the guideline says
Both CLAUDE.md:55 and test/CLAUDE.md:152 state: "Default: add your test to the existing test file for the code you're changing. Do not create a new file." An existing test/js/bun/util/heap-snapshot.test.ts (201 lines) already covers heap-snapshot functionality and is the natural home for a console.takeHeapSnapshot() regression test.
Why the separate file exists, and why that reason is gone
Per the PR timeline:
- Commit
1a74a6daadded this test insideheap-snapshot.test.ts. - Build #54778 reported
unchecked exception thrown from JSC__JSGlobalObject__generateHeapSnapshoton thex64-asanrunner. - Commit
d7bc668bresponded by moving the test, byte-for-byte, into a newconsole-takeHeapSnapshot.test.ts.
As the (now-resolved) inline comment 3248506528 explained, moving the file could not have fixed that failure: the assert fired inside the spawned subprocess, which inherits BUN_JSC_validateExceptionChecks=1 via bunEnv regardless of which test file does the spawning. Neither file is listed in test/no-validate-exceptions.txt, so the runner sets the flag on both.
The actual fix is now in this PR — JSC__JSGlobalObject__generateHeapSnapshot uses RETURN_IF_EXCEPTION(scope, {}) and the Rust caller wraps the FFI in from_js_host_call so the exception scope is satisfied. That resolves the asan failure independently of which file the test lives in. So the only rationale for a separate file (a workaround that didn't work) no longer applies.
Why nothing prevents folding it back
The test is fully self-contained: it spawns its own subprocess with its own env: { ...bunEnv, BUN_JSC_validateExceptionChecks: "1" }, pipes its own stderr, and makes no assumptions about sibling tests. heap-snapshot.test.ts is not in test/no-validate-exceptions.txt, so there is no opt-out conflict. Dropping this test(...) block into heap-snapshot.test.ts and adding bunEnv, bunExe to its harness import is all that's required.
Step-by-step
- CLAUDE.md:55 says: don't create a new test file; use the existing one.
test/js/bun/util/heap-snapshot.test.tsexists and covers this area.- The test was originally placed there (commit
1a74a6da). - It was moved out (commit
d7bc668b) only as a workaround for an asan failure. - That workaround was ineffective by construction (subprocess-side assert), and the underlying bug is now actually fixed in
bindings.cpp/JSGlobalObject.rs/ConsoleObject.rs. - Therefore the separate file has no remaining justification and contradicts the repo guideline.
Impact and fix
Purely organizational — no functional impact, hence nit. The fix is to move the test("console.takeHeapSnapshot can be called repeatedly after a failed require", ...) block into test/js/bun/util/heap-snapshot.test.ts and delete console-takeHeapSnapshot.test.ts. (One could argue console.takeHeapSnapshot is a distinct-enough API to merit its own file, but the CLAUDE.md default is explicit and there's no stated exception here.)
|
|
||
| const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stderr).not.toContain("Unchecked JS exception"); |
There was a problem hiding this comment.
🟡 Line 28's expect(stderr).not.toContain("Unchecked JS exception") violates CLAUDE.md:126 ("NEVER write tests that check for no 'panic' or 'uncaught exception' or similar in the test output") and is redundant — with BUN_JSC_validateExceptionChecks: "1" set on line 20, an unchecked-exception hit aborts the subprocess, so expect(stderr).toContain("OK") and expect(exitCode).toBe(0) already cover it. Drop line 28; but unlike CodeRabbit's suggestion above, keep BUN_JSC_validateExceptionChecks: "1" on line 20 — that env var is load-bearing (without it the test would not fail on main).
Extended reasoning...
What the guideline says
CLAUDE.md:126 reads verbatim:
NEVER write tests that check for no "panic" or "uncaught exception" or similar in the test output. These tests will never fail in CI.
Line 28 — expect(stderr).not.toContain("Unchecked JS exception") — is exactly this prohibited pattern: a negative assertion against a diagnostic-log substring.
Why the assertion is redundant
The test sets BUN_JSC_validateExceptionChecks: "1" on line 20. Per the PR description, on main this causes the subprocess to abort with an "unchecked-exception" diagnostic when console.takeHeapSnapshot()'s JSONParse simulated-throw goes unchecked. An abort means:
- the subprocess exits non-zero →
expect(exitCode).toBe(0)on line 30 fails, and console.error("OK")(line 15 of the-escript) never runs →expect(stderr).toContain("OK")on line 29 fails.
So lines 29–30 already detect the regression that line 28 is trying to guard against. Line 28 adds no signal and falls afoul of the repo guideline.
Why CodeRabbit's broader fix is wrong
CodeRabbit's unresolved inline comment (id 3249331869) suggests dropping both line 28 and the BUN_JSC_validateExceptionChecks: "1" env var on line 20. That second deletion would gut the test: the PR description states this regression test "runs the fuzzer's minimized sequence under BUN_JSC_validateExceptionChecks=1; it fails on main (unchecked-exception abort) and passes with this change." Without the env var, the validator is disabled in non-asan local runs and the test would pass on main too — i.e. it would no longer be a regression test for fix #2 (the from_js_host_call / RETURN_IF_EXCEPTION change). The env var is load-bearing; only the .not.toContain assertion should go.
Step-by-step proof
- On
main(pre-fix), spawnbun -e '...console.takeHeapSnapshot()...'withBUN_JSC_validateExceptionChecks=1. JSC__JSGlobalObject__generateHeapSnapshotdeclares aThrowScope;JSONParseruns under simulation and leavesm_needExceptionCheckset; the Rust caller (pre-fix) has no scope, so the nextDECLARE_THROW_SCOPEin the formatter asserts →ERROR: Unchecked JS exception:printed to stderr → process aborts.- Subprocess exit code ≠ 0 → line 30 (
expect(exitCode).toBe(0)) fails. console.error("OK")never executed → stderr lacks"OK"→ line 29 (expect(stderr).toContain("OK")) fails.- Line 28 would also fail, but it is strictly subsumed by 3–4. With this PR's fix applied, the subprocess exits 0 with
"OK"on stderr and no diagnostic — all three assertions pass, so removing line 28 changes nothing.
Suggested fix
- expect(stderr).not.toContain("Unchecked JS exception");
expect(stderr).toContain("OK");
expect(exitCode).toBe(0);Keep line 20 (BUN_JSC_validateExceptionChecks: "1") as-is.
1 participant
Fuzzilli hit a flaky SIGSEGV (fingerprint
a00ace4a94349590) on:Two fixes in
console.takeHeapSnapshot()→JSC__JSGlobalObject__generateHeapSnapshot:1. Clear prior snapshots before building. The function created an
InspectorSnapshot-typeHeapSnapshotBuilderwithout clearing the profiler's existing snapshots. In a long-lived process (the fuzzer's REPRL loop reuses the VM for up to 1000 scripts), snapshots chain together andHeapSnapshotBuilder::json()walks every retained snapshot, dereferencing eachnode.cell->classInfo().Bun.generateHeapSnapshot()already callsheapProfiler.clearSnapshots(); this makesconsole.takeHeapSnapshot()do the same.2. Check exceptions from
JSONParseand make the callers exception-aware. The C++ side had aThrowScopewhose destructor simulates a throw for the caller to check, butJSGlobalObject::generate_heap_snapshot(Rust/Zig) returned the bareJSValuestraight into the console formatter without an exception scope — trippingvalidateExceptionCheckswithUnchecked JS exception: ... JSC__JSGlobalObject__generateHeapSnapshot. Now:RETURN_IF_EXCEPTION(scope, {})so it returns encoded zero on a real throw (matching thezero_is_throwcontract)from_js_host_call/fromJSHostCalland early-return onErr, so a realJSONParseexception propagates to JS instead of printingundefinedfirstThe regression test runs the fuzzer's minimized sequence under
BUN_JSC_validateExceptionChecks=1; it fails on main (unchecked-exception abort) and passes with this change.