console.takeHeapSnapshot: report snapshot parse failures instead of aborting - #37070
console.takeHeapSnapshot: report snapshot parse failures instead of aborting#37070robobun wants to merge 4 commits into
Conversation
JSC__JSGlobalObject__generateHeapSnapshot ended with JSONParse followed by scope.releaseAssertNoException(), so an exception from JSONParse (out of memory building the parse tree, or one left pending during snapshot generation) aborted the process. A null string from HeapSnapshotBuilder::json(), returned when the snapshot overflows the maximum string length, passed an empty JSValue into the console formatter with no exception at all. The binding now returns empty with the exception pending (throwing an out-of-memory error for the null-string case), and the console.takeHeapSnapshot hook reports that exception through the uncaught-exception path instead of printing the snapshot, because JSC's consoleProtoFuncTakeHeapSnapshot performs no exception check after the client call, so the exception must not stay pending there.
WalkthroughChangesHeap snapshot exception handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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/console/console-take-heap-snapshot.test.ts`:
- Around line 5-28: The heap snapshot test only covers successful parsing;
extend it with a deterministic failure case that exercises the production
JSONParseWithException or null snapshot-string guard. Register an
uncaughtException handler to verify the parsing error is delivered, then run an
unhandled-error variant and assert it exits with code 1 while preserving the
existing successful-path assertions.
🪄 Autofix
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: d8e26f9e-8fd8-4cc6-af4f-55137c53625d
📒 Files selected for processing (4)
src/jsc/ConsoleObject.rssrc/jsc/JSGlobalObject.rssrc/jsc/bindings/bindings.cpptest/js/bun/console/console-take-heap-snapshot.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I traced report_error_or_terminate → take_exception (Task.rs:50) and confirmed it clears the pending exception off the VM before reporting, so the Err arm satisfies the "must not stay pending" invariant the new comment states. Also confirmed from_js_host_call uses the zero_is_throw shape, which matches the C++ side's new empty-iff-thrown contract.
Extended reasoning...
The core change is sound and mirrors sibling PRs #37065/#36857, but it threads through JSC exception-scope machinery across three layers (C++ ThrowScope, Rust FFI boundary, Rust console hook), which is subtle enough to warrant a human look. Deferring rather than approving for that reason; the inline nit is pre-existing and non-blocking.
…pending The takeHeapSnapshot hook called the message_with_type_and_level host shim, which leaves formatter errors pending on the VM for the C++ caller to check. consoleProtoFuncTakeHeapSnapshot performs no such check, so a formatter failure hit the same unchecked-exception state the previous commit fixed for the parse step. Call the inner JsResult-returning function directly and route its error through the same uncaught-exception reporting.
There was a problem hiding this comment.
LGTM — the follow-up commit routes message_with_type_and_level_'s error through report_error_or_terminate as suggested, so the hook now upholds its no-pending-exception invariant on both the parse and format paths.
What was reviewed:
- C++ binding matches the sibling shape in
BunObject.cpp(JSONParseWithException+RELEASE_AND_RETURN) and the null-string OOM guard. from_js_host_callenforces the empty-means-thrown contract at the FFI boundary;report_error_or_terminateclears the exception off the VM and reports it as uncaught, leaving termination to unwind.- Tests follow harness conventions (concurrent pipe drain,
bunEnvspread, stdout asserted before exit code) and pin the reachable paths undervalidateExceptionChecks.
Extended reasoning...
Overview
Replaces releaseAssertNoException() in JSC__JSGlobalObject__generateHeapSnapshot with proper exception propagation across three layers: the C++ binding now checks for a pending exception, throws OOM if HeapSnapshotBuilder::json() returned null, and uses JSONParseWithException + RELEASE_AND_RETURN; JSGlobalObject::generate_heap_snapshot returns JsResult<JSValue> via from_js_host_call; and the takeHeapSnapshot console hook reports both snapshot-generation and formatting failures through report_error_or_terminate rather than leaving an exception pending under JSC's consoleProtoFuncTakeHeapSnapshot (which performs no post-call exception check). A new test file exercises the success path under BUN_JSC_validateExceptionChecks=1 and the label-coercion throw path.
Security risks
None. This is a debug/diagnostic API (console.takeHeapSnapshot); no untrusted input parsing, auth, or data exposure is involved. The change strictly narrows failure modes from abort → reported error.
Level of scrutiny
Low-to-moderate. The change is small (~30 net lines across 3 files plus a test), mechanical, and mirrors two already-merged sibling fixes (#37065, #36857) for the same releaseAssertNoException bug class at adjacent call sites. The C++ shape is copied from BunObject.cpp:845-846. The success path is behaviorally unchanged; only the previously-aborting failure path is affected.
Other factors
- My prior inline finding (the format step could still leave an exception pending) was addressed exactly as suggested in 3562d82: the hook now calls the inner
message_with_type_and_level_directly and routes itsErrthrough the samereport_error_or_terminatehelper, and theunsafere-entry block is gone as a side effect. - CodeRabbit's request for a deterministic failure test was withdrawn after the author explained (consistent with the sibling PRs and #30460) that neither the OOM parse nor the 2 GiB null-string path is reachable in CI without hitting an earlier non-recoverable WTF-side allocation failure. The PR body documents instrumented before/after transcripts covering the
uncaughtExceptionhandler, exit-code, and validator paths. - I verified
report_error_or_terminatetakes the exception off the VM before reporting (so no exception remains pending on return), and that discarding itsErr(JsTerminated)vialet _is correct — termination should keep unwinding, not be re-reported. - Tests use
describe.concurrent, drain stdout/stderr/exited concurrently, spreadbunEnv, and assert output before exit code per repo conventions.
…same hole in Bun.generateHeapSnapshot Self-review follow-ups: - Report failures through JSGlobalObject::report_active_exception_as_unhandled, the method built for exceptions raised in a native context with nowhere to propagate, instead of borrowing the task dispatcher's helper and discarding its termination sentinel. - Add BUN_INTERNAL_FAIL_TAKE_HEAP_SNAPSHOT, a debug-only fault-injection flag (same convention as BUN_INTERNAL_FAIL_PIPE_READER_START) that makes generate_heap_snapshot throw out-of-memory at the seam where a real JSONParse failure would surface. Two new debug-only tests pin the reported behavior: uncaught report with exit 1 and execution continuing, and interception via process.on(uncaughtException), both clean under BUN_JSC_validateExceptionChecks=1. These fail on the previous code. - Give Bun.generateHeapSnapshot the same null-string guard: its JSC branch parsed builder.json() with no isNull() check, so an overflowed snapshot returned an empty value with no exception, which is not a valid host function result.
There was a problem hiding this comment.
LGTM — my earlier note about the format step leaving an exception pending was addressed in 3562d82, and the follow-up commit made the failure path testable via the debug-only fault-injection flag.
What was reviewed:
bindings.cpp/BunObject.cpp: theRETURN_IF_EXCEPTION→ null-string guard →RELEASE_AND_RETURN(JSONParseWithException)shape matches the sibling fixes in #37065/#36857; both snapshot call sites now cover the null-string-returns-empty-without-throwing hole.ConsoleObject.rs: bothgenerate_heap_snapshot()andmessage_with_type_and_level_()errors now route throughreport_active_exception_as_unhandled, which materializesJsError::OutOfMemoryviatake_exceptionbefore reporting — so no unchecked exception can escape the hook.generate_heap_snapshot()'s only caller was updated for theJsResultsignature; the fault-injection flag is#[cfg(debug_assertions)]-gated and follows the existingBUN_INTERNAL_FAIL_PIPE_READER_STARTconvention.
Extended reasoning...
Overview
This PR fixes console.takeHeapSnapshot() aborting via releaseAssertNoException() when JSONParse of the heap snapshot JSON throws (realistically OOM on a large heap), and closes a second hole where HeapSnapshotBuilder::json() returning the null string produced an empty JSValue that flowed into the console formatter (or out of Bun.generateHeapSnapshot() as an invalid host-function result). Six files touched: the C++ binding (bindings.cpp), the sibling Bun.generateHeapSnapshot host function (BunObject.cpp), the Rust FFI wrapper (JSGlobalObject.rs), the console hook (ConsoleObject.rs), a new debug-only fault-injection flag (env_var.rs), and a new test file.
Security risks
None. This is diagnostic tooling (console.takeHeapSnapshot / Bun.generateHeapSnapshot); no user-controlled input reaches new parsing or allocation logic. The change replaces a process abort with a reported uncaught exception, which is strictly less severe.
Level of scrutiny
Moderate. JSC exception-scope handling at the Rust↔C++ boundary is subtle, but the fix follows the exact idiom mandated by the review guidelines (RETURN_IF_EXCEPTION after fallible calls, RELEASE_AND_RETURN on the throwing tail call, throwOutOfMemoryError for the null-string case) and mirrors two recently-merged sibling fixes (#37065 samplingProfilerStackTraces, #36857 generateHeapSnapshotForDebugging) for the same bug class. The Rust side uses the standard from_js_host_call zero-is-throw contract and the documented report_active_exception_as_unhandled pattern for exceptions with nowhere to propagate.
Other factors
- I previously flagged (inline, now resolved) that the format step via
message_with_type_and_level(...)could also leave an exception pending under the sameconsoleProtoFuncTakeHeapSnapshotscope; commit 3562d82 addressed that by callingmessage_with_type_and_level_directly and routing itsErrthrough the same reporter, and dropped theunsafere-entry block as a side effect. - CodeRabbit's request for a deterministic failure test was resolved after the author explained the constraints (WTF-side allocations fail non-recoverably before
JSONParsecan OOM; the null-string path needs a >2 GiB snapshot), then added theBUN_INTERNAL_FAIL_TAKE_HEAP_SNAPSHOTdebug-only injection so the console hook's error path is exercised in CI on debug builds. - Verified
generate_heap_snapshot()has exactly one caller (the console hook), so theJSValue→JsResult<JSValue>signature change is fully covered.Bun.generateHeapSnapshot()is a separate C++ host function that doesn't go through the Rust wrapper, so the fault-injection flag correctly scopes toconsole.takeHeapSnapshot()only. - Verified
take_exception(JsError::OutOfMemory)callsthrow_out_of_memory()before reading the pending exception, soreport_active_exception_as_unhandledhandles the OOM variant thatmessage_with_type_and_level_can return. - The PR body's evidence block shows the new tests fail on main (3 failures under ASAN debug) and pass with the fix; the two fault-injection tests correctly
skipIf(!isDebug)since the flag is compiled out of release builds.
1 participant
Problem
JSC__JSGlobalObject__generateHeapSnapshot(src/jsc/bindings/bindings.cpp, behindconsole.takeHeapSnapshot()) ended with:JSONParsecan throw, realistically an out-of-memory error while building the parse tree for a large heap's snapshot JSON. When it does, the process aborts:via
JSC__JSGlobalObject__generateHeapSnapshot. The fuzzer crash that motivated #30460 hit this same assert with an exception leaked from elsewhere, so the assert also turns unrelated pending exceptions into aborts.There is a second failure mode with no assert at all:
HeapSnapshotBuilder::json()returns the null string when the snapshot JSON overflows the maximum string length or its buffer allocation fails, andJSONParsemaps a null string to an emptyJSValuewithout throwing, so the old code passed an empty value into the console formatter. The same hole existed inBun.generateHeapSnapshot()'s JSC branch (BunObject.cpp), whereJSONParseWithExceptionon a null string returns empty without throwing, which is not a valid host function result. Snapshots of multi-gigabyte heaps are where this lands, matching the crash pattern reported in #23393.This is the same bug class as #37065 (
samplingProfilerStackTraces) and #36857 (generateHeapSnapshotForDebugging). This call site was intentionally excluded from #37065 because the two-line reorder is not enough here: the result feeds the console formatter through the Rust caller, and the exception would then surface under JSC'sconsoleProtoFuncTakeHeapSnapshot, whose throw scope performs no exception check after the client call.Fix
bindings.cpp: check for a pending exception after building the JSON, throw an out-of-memory error whenjson()returned the null string, and parse withJSONParseWithException+RELEASE_AND_RETURN. The function now returns empty if and only if an exception is pending.BunObject.cpp: giveBun.generateHeapSnapshot()'s JSC branch the same null-string guard (its parse-exception handling was already correct).JSGlobalObject.rs:generate_heap_snapshot()returnsJsResult<JSValue>viafrom_js_host_call, so the empty-means-thrown contract is checked at the FFI boundary.ConsoleObject.rs: on error, thetakeHeapSnapshothook reports the exception withreport_active_exception_as_unhandledinstead of printing the snapshot, and routes formatter errors frommessage_with_type_and_level_the same way (the previous code re-entered the host shim, which intentionally leaves formatter errors pending for C++ callers that check). The hook cannot leave an exception pending:consoleProtoFuncTakeHeapSnapshotdoes not check for exceptions after the client call (the console API is void), and withBUN_JSC_validateExceptionChecks=1its unreleased scope would abort inVM::verifyExceptionCheckNeedIsSatisfied.report_active_exception_as_unhandledis the runtime's method for exactly this situation (an exception raised in a native context with nowhere to propagate), keeps the error visible toprocess.on("uncaughtException")and the test runner, and skips reporting when the pending exception is a termination.So a failed snapshot now behaves like an error thrown at the
console.takeHeapSnapshot()call site that nothing caught: reported with a stack, interceptable, exit code 1 by default, instead ofSIGABRT.Test
The real triggers cannot be staged from JS: forcing an OOM inside
JSONParsewith constrained memory makes the earlier WTF-side snapshot allocations fail first (a non-recoverable crash, not a JS exception; #30460 was closed after reaching the same conclusion), and the null-string path needs a snapshot over the 2 GiB string limit. So the failure is injected at the seam where it would surface: a debug-only fault-injection env var,BUN_INTERNAL_FAIL_TAKE_HEAP_SNAPSHOT, makesgenerate_heap_snapshot()throw an out-of-memory error, following the existingBUN_INTERNAL_FAIL_PIPE_READER_STARTconvention.test/js/bun/console/console-take-heap-snapshot.test.ts(this API had no coverage):console.takeHeapSnapshot()and the labeled variant print the parsed snapshot and exit cleanly underBUN_JSC_validateExceptionChecks=1.process.on("uncaughtException")intercepts it and the process exits 0. Both stay clean under the exception-check validator. These two tests fail on the previous code and pass with this change.The unfixed abort itself was reproduced by temporarily forcing a throw at the parse site in a local build:
Transcript of the previous behavior with a simulated parse failure
[review] gate passed · iteration 1 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file