Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/jsc/ConsoleObject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6012,7 +6012,17 @@
_len: usize,
) {
// TODO: this does an extra JSONStringify and we don't need it to!
let snapshot: [JSValue; 1] = [global_this.generate_heap_snapshot()];
let snapshot: [JSValue; 1] = match global_this.generate_heap_snapshot() {
Ok(snapshot) => [snapshot],
// The exception must not stay pending: JSC's
// `consoleProtoFuncTakeHeapSnapshot` performs no exception check after
// this client call, so report it as uncaught instead (termination just
// stops the print and keeps the VM unwinding).
Err(err) => {
let _ = crate::task::report_error_or_terminate(global_this, err);
return;
}
};

Check warning on line 6025 in src/jsc/ConsoleObject.rs

View check run for this annotation

Claude / Claude Code Review

takeHeapSnapshot: format-step exception still left pending

The new comment at lines 6017-6020 states that this function must not leave an exception pending because `consoleProtoFuncTakeHeapSnapshot` performs no exception check after the client call — but the very next call, `message_with_type_and_level(...)` at line 6028, can also throw (its wrapper explicitly leaves the exception pending on the VM), so the format step still hits the same failure mode the parse step just fixed. Consider calling `message_with_type_and_level_` directly and routing its `Er
Comment thread
robobun marked this conversation as resolved.
// SAFETY: re-entry into our own host shim with a stack-local args slice.
unsafe {
message_with_type_and_level(
Expand Down
4 changes: 2 additions & 2 deletions src/jsc/JSGlobalObject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -966,8 +966,8 @@ impl JSGlobalObject {
})
}

pub(crate) fn generate_heap_snapshot(&self) -> JSValue {
JSC__JSGlobalObject__generateHeapSnapshot(self)
pub(crate) fn generate_heap_snapshot(&self) -> JsResult<JSValue> {
crate::from_js_host_call(self, || JSC__JSGlobalObject__generateHeapSnapshot(self))
}

/// DEPRECATED — use [`TopExceptionScope`](crate::TopExceptionScope) to check for exceptions
Expand Down
11 changes: 8 additions & 3 deletions src/jsc/bindings/bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3930,9 +3930,14 @@ JSC::EncodedJSValue JSC__JSGlobalObject__generateHeapSnapshot(JSC::JSGlobalObjec
snapshotBuilder.buildSnapshot();

WTF::String jsonString = snapshotBuilder.json();
JSC::EncodedJSValue result = JSC::JSValue::encode(JSONParse(globalObject, jsonString));
scope.releaseAssertNoException();
return result;
RETURN_IF_EXCEPTION(scope, {});
if (jsonString.isNull()) [[unlikely]] {
// HeapSnapshotBuilder::json() returns the null string when the snapshot
// overflowed the maximum string length or its allocation failed.
throwOutOfMemoryError(globalObject, scope);
return {};
}
RELEASE_AND_RETURN(scope, JSC::JSValue::encode(JSONParseWithException(globalObject, jsonString)));
}

// One load. always_inline so ThinLTO importers and the inliner never leave
Expand Down
54 changes: 54 additions & 0 deletions test/js/bun/console/console-take-heap-snapshot.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { describe, expect, it } from "bun:test";
import { bunEnv, bunExe } from "harness";

describe.concurrent("console.takeHeapSnapshot", () => {
it("prints the parsed snapshot and survives BUN_JSC_validateExceptionChecks", async () => {
// JSONParse of the snapshot JSON can throw (out of memory building the
// parse tree), so the binding must hand the exception back to the console
// hook instead of releaseAssertNoException()-crashing, and the hook must
// report it rather than leave it pending under JSC's
// consoleProtoFuncTakeHeapSnapshot, whose scope performs no exception
// check after the client call. validateExceptionChecks aborts on debug
// builds if any scope is left unchecked; on release builds the option is
// a no-op and this just exercises the snapshot path.
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", `console.takeHeapSnapshot(); console.takeHeapSnapshot("label"); console.log("done");`],
env: { ...bunEnv, BUN_JSC_validateExceptionChecks: "1" },
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
const uncheckedScopes = stderr
.split("\n")
.map(line => line.trim())
.filter(line => line.startsWith("This scope can throw") || line.startsWith("But the exception was unchecked"));
expect(stdout).toContain(`type: "Inspector"`);
expect(stdout.endsWith("done\n")).toBe(true);
expect(uncheckedScopes).toEqual([]);
expect(exitCode).toBe(0);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});

it("propagates exceptions thrown while coercing the label", async () => {
// The label toString runs before the snapshot is taken; the console
// function checks for the exception and rethrows it.
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`try {
console.takeHeapSnapshot({ toString() { throw new Error("boom"); } });
console.log("did not throw");
} catch (e) {
console.log("caught:", e.message);
}`,
],
env: { ...bunEnv, BUN_JSC_validateExceptionChecks: "1" },
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stdout).toBe("caught: boom\n");
expect(stderr).toBe("");
expect(exitCode).toBe(0);
});
});