Skip to content

Fix stack overflow when inspecting JSX elements with circular references #29931

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 2 commits into from
Closed

Fix stack overflow when inspecting JSX elements with circular references #29931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c93f76e
Fix stack overflow when inspecting JSX elements with circular references
robobun Apr 29, 2026
1463362
Apply same JSX circular/non-object props fix to pretty_format.zig
robobun Apr 29, 2026
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
7 changes: 3 additions & 4 deletions src/bun.js/ConsoleObject.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1136,7 +1136,7 @@ pub const Formatter = struct {

pub fn canHaveCircularReferences(tag: Tag) bool {
return switch (tag) {
.Function, .Array, .Object, .Map, .Set, .Error, .Class, .Event => true,
.Function, .Array, .Object, .Map, .Set, .Error, .Class, .Event, .JSX => true,
Comment thread
claude[bot] marked this conversation as resolved.
else => false,
};
}
Expand Down Expand Up @@ -3140,13 +3140,12 @@ pub const Formatter = struct {
}
}

if (try value.get(this.globalThis, "props")) |props| {
if (try value.get(this.globalThis, "props")) |props| props: {
const prev_quote_strings = this.quote_strings;
defer this.quote_strings = prev_quote_strings;
this.quote_strings = true;

// SAFETY: JSX props are always objects
const props_obj = props.getObject().?;
const props_obj = props.getObject() orelse break :props;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
var props_iter = try jsc.JSPropertyIterator(.{
.skip_empty_name = true,
.include_value = true,
Expand Down
7 changes: 3 additions & 4 deletions src/bun.js/test/pretty_format.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ pub const JestPrettyFormat = struct {
}

pub inline fn canHaveCircularReferences(tag: Tag) bool {
return tag == .Array or tag == .Object or tag == .Map or tag == .Set;
return tag == .Array or tag == .Object or tag == .Map or tag == .Set or tag == .JSX;
}

const Result = struct {
Expand Down Expand Up @@ -1534,13 +1534,12 @@ pub const JestPrettyFormat = struct {
}
}

if (try value.get(this.globalThis, "props")) |props| {
if (try value.get(this.globalThis, "props")) |props| props: {
const prev_quote_strings = this.quote_strings;
defer this.quote_strings = prev_quote_strings;
this.quote_strings = true;

// SAFETY: JSX props are always an object.
const props_obj = props.getObject().?;
const props_obj = props.getObject() orelse break :props;
var props_iter = try jsc.JSPropertyIterator(.{
.skip_empty_name = true,
.include_value = true,
Expand Down
42 changes: 42 additions & 0 deletions test/js/bun/test/pretty-format-overflow.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,46 @@
// Verify it actually formatted and showed the diff (not just crashed)
expect(stderr).toContain("expect(received).toEqual(expected)");
}, 30000);

test("JSX element with circular or non-object props", async () => {
const dir = tempDirWithFiles("pretty-format-jsx", {
"jsx.test.ts": `
import { test, expect } from "bun:test";

test("circular props", () => {
const el: any = { $$typeof: Symbol.for("react.element"), type: "div", props: {} };
el.props = el;
expect(el).toEqual({});
});

test("circular children", () => {
const el: any = { $$typeof: Symbol.for("react.element"), type: "div", props: {} };
el.props.children = el;
expect(el).toEqual({});
});

test("non-object props", () => {
const el: any = { $$typeof: Symbol.for("react.element"), type: "div", props: 42 };
expect(el).toEqual({});
});
`,
});

const proc = Bun.spawn({
cmd: [bunExe(), "test", "jsx.test.ts"],
env: bunEnv,
cwd: dir,
stderr: "pipe",
stdout: "pipe",
});

const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);

expect(exitCode).toBe(1);
expect(stderr).not.toContain("panic");
expect(stderr).not.toContain("SIGSEGV");
expect(stderr).toContain("[Circular]");
expect(stderr).toContain("expect(received).toEqual(expected)");
expect(stderr).toContain("3 fail");

Check warning on line 93 in test/js/bun/test/pretty-format-overflow.test.ts

View check run for this annotation

Claude / Claude Code Review

Assert stderr content before exitCode (CLAUDE.md guideline) 

nit: per root CLAUDE.md, when spawning processes the test should assert on `stderr` content *before* `exitCode` so that a regression surfaces useful diagnostics. If the spawned `bun test` segfaults again (the very thing this test guards against), `exitCode` will be `null`/signal-coded and the failure will just say "expected null to be 1" without showing what stderr contained. Move `expect(exitCode).toBe(1)` after the stderr assertions (the pre-existing test above has the same ordering and could
Comment on lines +88 to +93

@claude claude Bot Apr 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit: per root CLAUDE.md, when spawning processes the test should assert on stderr content before exitCode so that a regression surfaces useful diagnostics. If the spawned bun test segfaults again (the very thing this test guards against), exitCode will be null/signal-coded and the failure will just say "expected null to be 1" without showing what stderr contained. Move expect(exitCode).toBe(1) after the stderr assertions (the pre-existing test above has the same ordering and could be flipped too).

Extended reasoning...

What the bug is

The new test in test/js/bun/test/pretty-format-overflow.test.ts asserts expect(exitCode).toBe(1) on line 88, before the stderr content assertions on lines 89-93 (not.toContain("panic"), not.toContain("SIGSEGV"), toContain("[Circular]"), etc.). Root CLAUDE.md explicitly says:

When spawning processes, tests should expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0). This gives you a more useful error message on test failure.

How it manifests / step-by-step

This test exists to catch a regression where JestPrettyFormat stack-overflows or panics on circular/non-object JSX props. Suppose that regression is reintroduced:

  1. The spawned bun test jsx.test.ts process hits the native stack overflow / panic.
  2. The child process is killed by a signal (e.g. SIGSEGV / SIGTRAP), so proc.exited resolves to null (or a non-1 signal exit code), and stderr contains the crash banner instead of the expected diff output.
  3. The first assertion executed is expect(exitCode).toBe(1) on line 88. It fails with a message like expected null to be 1.
  4. Execution stops there — none of the subsequent stderr assertions run, so the test report never shows the actual stderr content (which would have made the failure cause obvious: panic: ... or SIGSEGV).

Why existing code doesn't prevent it

expect() failures throw immediately in bun:test, so once the exitCode assertion fails, the more informative stderr checks are skipped. Nothing else in the test prints stderr on failure.

Impact

Purely a test-ergonomics / diagnostics issue — the test still correctly fails on regression, it just produces a much less helpful failure message. Given this PR is specifically guarding against a crash, having the failure surface the crash output is valuable. This is nit-level: it doesn't affect correctness, and it matches the ordering already used by the pre-existing "deeply nested object" test in the same file (lines 46-51).

How to fix

Reorder the assertions so the stderr content checks run first and the exitCode check runs last:

expect(stderr).not.toContain("panic");
expect(stderr).not.toContain("SIGSEGV");
expect(stderr).toContain("[Circular]");
expect(stderr).toContain("expect(received).toEqual(expected)");
expect(stderr).toContain("3 fail");
expect(exitCode).toBe(1);

Optionally apply the same reordering to the pre-existing test above for consistency.

}, 30000);
});
25 changes: 25 additions & 0 deletions test/js/bun/util/inspect.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,3 +772,28 @@ it("CustomEvent", () => {
}"
`);
});

describe("JSX element", () => {
it("handles circular props without crashing", () => {
const el = { $$typeof: Symbol.for("react.element"), type: "div", props: {} };
el.props = el;
expect(Bun.inspect(el)).toContain("[Circular]");
});

it("handles circular children without crashing", () => {
const el = { $$typeof: Symbol.for("react.element"), type: "div", props: {} };
el.props.children = el;
expect(Bun.inspect(el)).toContain("[Circular]");
});

it("handles circular array children without crashing", () => {
const el = { $$typeof: Symbol.for("react.element"), type: "div", props: { children: [] } };
el.props.children.push(el);
expect(Bun.inspect(el)).toContain("[Circular]");
});

it("handles non-object props without crashing", () => {
const el = { $$typeof: Symbol.for("react.element"), type: "div", props: 42 };
expect(Bun.inspect(el)).toBe("<div />");
});
});
Loading