Skip to content

Fix crash when inspecting JSX elements with circular or non-object props #30630

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 1 commit into from
Closed

Fix crash when inspecting JSX elements with circular or non-object props #30630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 3 additions & 5 deletions src/jsc/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,
else => false,
};
}
Expand Down Expand Up @@ -3140,13 +3140,11 @@ pub const Formatter = struct {
}
}

if (try value.get(this.globalThis, "props")) |props| {
if (try value.get(this.globalThis, "props")) |props| if (props.getObject()) |props_obj| {
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().?;
var props_iter = try jsc.JSPropertyIterator(.{
.skip_empty_name = true,
.include_value = true,
Expand Down Expand Up @@ -3303,7 +3301,7 @@ pub const Formatter = struct {
}
}
}
}
};

writer.writeAll(" />");
},
Expand Down
8 changes: 3 additions & 5 deletions src/test_runner/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;
}
Comment on lines +329 to 330

@coderabbitai coderabbitai Bot May 13, 2026 •
edited
Loading

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.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Checking JSX symbol handling parity between inspect and pretty formatter..."
rg -n -C2 'react\.element|react\.transitional\.element|react\.fragment|\$\$typeof' src/jsc/ConsoleObject.zig src/test_runner/pretty_format.zig

echo
echo "Checking tests for transitional symbol coverage..."
rg -n -C2 'react\.transitional\.element|react\.element|\\$\\$typeof' test

Repository: oven-sh/bun

Length of output: 8294

Add react.transitional.element support to pretty formatter for React 19 parity

pretty_format.zig doesn't recognize Symbol.for("react.transitional.element"), creating a parity gap with ConsoleObject.zig which already handles it. This prevents React 19 transitional elements from being tagged as .JSX in Jest pretty formatting, bypassing circular reference handling for these elements.

Mirror the approach in ConsoleObject.zig (lines 1305-1310) by adding the transitional symbol check:

Fix 
                         var reactElement = ZigString.init("react.element");
+                        var react_element_transitional = ZigString.init("react.transitional.element");
                         var react_fragment = ZigString.init("react.fragment");
 
-                        if (try typeof_symbol.isSameValue(.symbolFor(globalThis, &reactElement), globalThis) or try typeof_symbol.isSameValue(.symbolFor(globalThis, &react_fragment), globalThis)) {
+                        if (try typeof_symbol.isSameValue(.symbolFor(globalThis, &reactElement), globalThis) or
+                            try typeof_symbol.isSameValue(.symbolFor(globalThis, &react_element_transitional), globalThis) or
+                            try typeof_symbol.isSameValue(.symbolFor(globalThis, &react_fragment), globalThis))
+                        {
                             return .{ .tag = .JSX, .cell = js_type };
                         }
🤖 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 `@src/test_runner/pretty_format.zig` around lines 329 - 330, The pretty
formatter currently doesn't treat React 19 transitional elements as JSX because
the JSX detection condition only checks tags (.Array, .Object, .Map, .Set,
.JSX); update the JSX detection logic in src/test_runner/pretty_format.zig to
also recognize Symbol.for("react.transitional.element") the same way
ConsoleObject.zig does (mirror the transitional symbol check used around the
ConsoleObject handling) so that when the inspected value matches the
transitional element symbol it will return tag == .JSX and thus participate in
the pretty printer's circular reference/JSX handling; locate the
JSX/tag-checking function (the block that returns tag == .Array or tag ==
.Object or tag == .Map or tag == .Set or tag == .JSX) and add the transitional
symbol check analogous to ConsoleObject.zig.

@robobun robobun May 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is a pre-existing parity gap unrelated to the crash being fixed here. When pretty_format.zig doesn't recognize react.transitional.element, it falls through to .Object, which already participates in circular reference detection — so there's no crash risk for that case. Keeping this PR scoped to the crash fix.

@coderabbitai coderabbitai Bot May 13, 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.

@robobun, that makes sense — since unrecognized react.transitional.element objects fall through to .Object, which is already in canHaveCircularReferences(), there's no crash exposure for that case. Keeping the PR scoped to the crash fix is the right call. Thanks for the clarification!

🧠 Learnings used 
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.

Learnt from: theshadow27
Repo: oven-sh/bun PR: 26310
File: src/cache.zig:123-127
Timestamp: 2026-01-20T21:51:21.482Z
Learning: Ensure readFileShared in Zig files follows the same FD lifecycle as readFileWithAllocator: compute will_close = rfs.needToCloseFiles() and opened_file; use it in defer; and return bun.invalid_fd (not the closed handle) when will_close is true to avoid exposing closed FDs. This pattern should be applied to similar readFile* helpers across the Zig codebase to maintain correct resource lifecycle and avoid leaking or returning invalid file descriptors.

Learnt from: jimmywarting
Repo: oven-sh/bun PR: 26595
File: src/bun.js/webcore/Blob.zig:3839-3841
Timestamp: 2026-02-12T22:09:40.373Z
Learning: In Zig source files implementing Blob (e.g., Blob constructors in src/bun.js/webcore/Blob.zig), ensure that Blob construction does not synchronously read file contents. When building a Blob from parts that include file-backed blobs (e.g., Bun.file() results), create references to those blobs that preserve offset/size metadata instead of eagerly loading bytes into memory. Only the read methods (text(), arrayBuffer(), stream(), etc.) should perform actual file I/O. This aligns with Web API semantics (Blob constructor is synchronous but non-blocking) and common runtime patterns (Node.js, Deno, fetch-blob). The Blob should internally store an array of BlobParts (ArrayBuffer, file-backed Blob with offset/size, other Blob references) and materialize bytes only when read methods are invoked.

Learnt from: alii
Repo: oven-sh/bun PR: 28128
File: src/cli/test/Scanner.zig:170-178
Timestamp: 2026-03-15T05:22:35.951Z
Learning: In oven-sh/bun's Zig code, avoid flagging small fixed-size buffers as needing path_buffer_pool. Use path_buffer_pool.get()/put() only when you actually need a MAX_PATH_BYTES-sized buffer (around ~96KB on Windows). For small buffers (e.g., [4096]u8) used for short relative paths under ~1KB, allocate on the stack and avoid pool overhead on hot paths. Do not apply path_buffer_pool guidance unless a full MAX_PATH_BYTES-sized buffer is required.

Learnt from: robobun
Repo: oven-sh/bun PR: 28299
File: src/bun.js/api/cron.zig:311-315
Timestamp: 2026-03-20T16:27:36.818Z
Learning: In oven-sh/bun Zig code, treat these as equivalent and acceptable: `bun.FD.cwd().makePath(u8, path)` and `bun.makePath(bun.FD.cwd().stdDir(), path)`. The underlying implementation (`FD.makePath` around src/fd.zig) is a thin wrapper that delegates to `bun.makePath`, so reviewers should not flag `bun.FD.cwd().makePath(u8, ...)` as non-standard or incorrect when it’s used for path creation.

Learnt from: cirospaciari
Repo: oven-sh/bun PR: 28611
File: src/http.zig:565-620
Timestamp: 2026-03-30T19:17:01.758Z
Learning: In this oven-sh/bun Zig codebase, treat `bun.hash()` as a one-shot helper: it hashes a single `[]const u8` buffer and is appropriate only for single-buffer, non-incremental hashing. For incremental hashing across multiple chunks (e.g., building a hash in a loop over header name/value pairs), use the wyhash streaming API directly: `std.hash.Wyhash.init(0)`, then call `.update(chunk)` for each piece, and finally `.final()` to produce the result. Do not apply the CLAUDE.md “use `bun.hash()` for 64-bit wyhash” guideline to incremental/multi-buffer cases—use `Wyhash.init/update/final` instead even though both ultimately rely on wyhash.

Learnt from: robobun
Repo: oven-sh/bun PR: 29137
File: src/bun.js/node/node_util_binding.zig:145-164
Timestamp: 2026-04-11T03:00:34.188Z
Learning: In this repo’s Zig code (oven-sh/bun), it’s acceptable to call `std.posix.isatty(fd_int)` directly when you already have a file descriptor as a raw `i32`. Do not treat direct `std.posix.isatty(fd_int)` calls as inconsistent just because `bun.sys.File.isTty()` exists—`bun.sys.File.isTty()` is essentially `std.posix.isatty(self.handle.cast())` and would require constructing a `bun.sys.File` first. Review should not flag direct `std.posix.isatty(fd_int)` usage as a `bun.sys` pattern violation.

Learnt from: robobun
Repo: oven-sh/bun PR: 29154
File: src/interchange/xml.zig:443-445
Timestamp: 2026-04-13T17:55:27.597Z
Learning: When reviewing Zig code in oven-sh/bun, note that `bun.strings.eqlLong(a, b, comptime check_len: bool)` performs a case-sensitive equality check. The `check_len` comptime bool only controls whether a length pre-check happens before the SIMD byte comparison, and is not related to case sensitivity. Therefore, do not treat `eqlLong(a, b, true)` (or any `check_len` value) as a case-insensitive comparison; only flag for case-insensitivity if the code actually uses a case-folding/comparison API intended for that purpose.

Learnt from: robobun
Repo: oven-sh/bun PR: 29422
File: src/bun.js/api/JSBundler.zig:0-0
Timestamp: 2026-04-18T04:44:25.893Z
Learning: In oven-sh/bun Zig code, follow the established OutOfMemory handling pattern: when you have an exhaustive `switch (err)` over the error union (enumerating every error variant), keep using `error.OutOfMemory => bun.outOfMemory()` inside the exhaustive switch arm. Do NOT recommend replacing `bun.outOfMemory()` with `bun.handleOom()` within those exhaustive switch arms. Only flag bare `catch bun.outOfMemory()` calls (i.e., non-exhaustive/non-switch handling), since `bun.handleOom()` is intended to wrap whole error-union expressions with `catch` to avoid accidentally swallowing non-OOM errors.

Learnt from: robobun
Repo: oven-sh/bun PR: 30147
File: src/bun.js/webcore/Body.zig:1777-1804
Timestamp: 2026-05-03T06:53:51.543Z
Learning: In this repo’s Zig code, avoid tail-calling (or otherwise delegating) between two mutually recursive functions when both rely on inferred error sets. Zig can fail with errors like “unable to resolve inferred error set” for the recursive cycle. If such mutually recursive structure is needed, apply the workaround used in Body.zig: inline the terminal-state handling directly (e.g., via an exhaustive switch) at the call site rather than tail-calling the other function that participates in the mutual recursion.

Learnt from: robobun
Repo: oven-sh/bun PR: 30219
File: src/resolver/package_json.zig:1731-1732
Timestamp: 2026-05-04T03:21:09.821Z
Learning: When reviewing this repo’s Zig code, avoid flagging repeated calls to `module_bufs.get()` (or other `bun.ThreadlocalBuffers(T).get()` instances) as redundant if they occur within the same function/scope. In `oven-sh/bun`’s `bun.ThreadlocalBuffers(T)` implementation, `get()` is `inline`, so after the first call the compiler typically reuses the same TLS value/register within that scope. For readability, it’s acceptable (and preferred) to call `get()` multiple times in the same scope rather than caching the value in a local variable and reusing it.

Learnt from: robobun
Repo: oven-sh/bun PR: 30314
File: src/cli/Arguments.zig:1046-1056
Timestamp: 2026-05-06T11:21:18.669Z
Learning: Boolean env var handling in Zig projects: In oven-sh/bun, env vars declared with kind.boolean (for example New(kind.boolean, \"NODE_USE_SYSTEM_CA\", .{ .default = false })) return a plain bool from .get(). The internal stringIsTruthy conversion is applied before returning the value, so callers should receive a bool, not an optional string. In code reviews, do not suggest capturing the result as ?[]const u8 (e.g., using |value|) or performing raw string comparisons like == \"0\" to infer truth for boolean-kind env vars. Rely on the .get() result and its boolean semantics. This guideline applies to files that read boolean env vars (e.g., src/cli/Arguments.zig) and related env_var usage (src/bun_core/env_var.zig).

Learnt from: robobun
Repo: oven-sh/bun PR: 30357
File: src/jsc/JSGlobalObject.zig:335-343
Timestamp: 2026-05-07T07:17:05.327Z
Learning: In this repo’s Zig code (e.g., Bun’s usage of `std.Io.Writer`), `std.Io.Writer.Error` is a single-variant set containing only `error{WriteFailed}`. Allocation/OOM failures are internally mapped to `error.WriteFailed` by `std.Io.Writer.Allocating.drain` (vendored `vendor/zig/lib/std/Io/Writer.zig`), so `error.OutOfMemory` is not catchable/distinguishable via `std.Io.Writer` error handling. Therefore, when reviewing `std.Io.Writer` operations, do not recommend splitting `catch` logic to handle `error.OutOfMemory` separately; a bare `catch` or `catch |err|` that only covers `error.WriteFailed` is correct.

Learnt from: ig-ant
Repo: oven-sh/bun PR: 30403
File: src/aio/MemoryPressureWatcher.zig:367-373
Timestamp: 2026-05-08T16:28:02.004Z
Learning: When reviewing Zig code, do not treat “accumulator-style” holdoff/elapsed-time loops as potentially wrong due to EINTR if the code measures elapsed time using repeated slices of `std.Thread.sleep` (e.g., `slept += N; std.Thread.sleep(N)`). On POSIX, `std.Thread.sleep` handles EINTR by restarting `nanosleep` with the remaining time (`req = rem; continue`), so each sleep slice consumes its full intended duration even if signals are delivered. In this case, an `slept += N` accumulator correctly tracks elapsed time without needing a deadline/`clock`-based approach. Only flag under-counting concerns if the code uses a sleep primitive that does NOT perform this EINTR retry behavior.

Learnt from: mchv
Repo: oven-sh/bun PR: 30625
File: src/test_runner/ScopeFunctions.zig:162-167
Timestamp: 2026-05-13T16:37:25.279Z
Learning: When calling `jsc.JSObject.createStructure(globalThis, owner, ...)` in Bun’s Zig bindings, ensure the `owner` argument is the JSValue that will actually hold references to the objects created from that structure (because it’s used only for the GC write barrier via `vm.writeBarrier(owner, structure)`). Do not pass a static context like `globalThis` or `vm` as the owner. Instead, pass a stable container such as `this.js_value` when callers cache the structure on a statement object, or pass a transient container like the result array JSValue when the structure is only needed for that array’s lifetime.

Learnt from: mchv
Repo: oven-sh/bun PR: 30625
File: src/test_runner/ScopeFunctions.zig:162-167
Timestamp: 2026-05-13T16:37:25.279Z
Learning: In this repo’s Zig code (oven-sh/bun), treat `jsc.JSObject.createStructure` as infallible: it internally performs GC-triggering heap allocations (`emptyObjectStructureForPrototype` + `addPropertyTransition`) but does not return a failure value (never returns null/invalid). During code review, do not flag the absence of validity/error checking on the returned `JSValue` from `createStructure` as a bug. Only apply this exemption to `jsc.JSObject.createStructure` (not other APIs).


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

if (try value.get(this.globalThis, "props")) |props| {
if (try value.get(this.globalThis, "props")) |props| if (props.getObject()) |props_obj| {
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().?;
var props_iter = try jsc.JSPropertyIterator(.{
.skip_empty_name = true,
.include_value = true,
Expand Down Expand Up @@ -1691,7 +1689,7 @@ pub const JestPrettyFormat = struct {
}
}
}
}
};

writer.writeAll(" />");
},
Expand Down
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 @@ -316,6 +316,31 @@ it("jsx with fragment", () => {
expect(input).toBe(output);
});

it("jsx with circular props does not crash", () => {
const el = { $$typeof: Symbol.for("react.element"), type: "div", key: null, ref: null, props: {} };
el.props = el;
expect(Bun.inspect(el)).toContain("[Circular]");
});

it("jsx with circular key does not crash", () => {
const el = { $$typeof: Symbol.for("react.element"), type: "div" };
el.key = el;
expect(Bun.inspect(el)).toContain("[Circular]");
});

it("jsx with circular children does not crash", () => {
const el = { $$typeof: Symbol.for("react.element"), type: "div", key: null, ref: null, props: {} };
el.props = { children: el };
expect(Bun.inspect(el)).toContain("[Circular]");
});

it("jsx with non-object props does not crash", () => {
for (const props of [123, "str", true, null, Symbol("x")]) {
const el = { $$typeof: Symbol.for("react.element"), type: "div", key: null, ref: null, props };
expect(() => Bun.inspect(el)).not.toThrow();
}
});

it("inspect", () => {
expect(Bun.inspect(new TypeError("what")).includes("TypeError: what")).toBe(true);
expect(Bun.inspect("hi")).toBe('"hi"');
Expand Down
Loading