Conversation
|
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)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughChangesMatcher formatting
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: the automated review did not run on this push, so there is no review feedback to act on yet. The change is complete from my side. I am waiting for the CI build. For reviewers, the two hunks are independent. The |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-traced fuzzer crash fix that applies two established patterns.
What was reviewed:
RETURN_IF_EXCEPTIONafteriter(...)inforEachPropertyOrderedmirrors the siblingforEachPropertyImpl(bindings.cpp:5688) exactly, and the early return skipsproperties.releaseData()the same way lines 5608/5688 already do.clear_exception_except_termination()after a failedJestPrettyFormat::formatmatchescreate_error_instance(JSGlobalObject.rs:749); the loop dedupe of the two calls is behavior-preserving on the success path.- Tests drain pipes concurrently, assert combined
{stdout, stderr, exitCode}, and cover the walk, nested walk, received-throws, and the fuzzer's stack-overflow shape.
Extended reasoning...
Overview
Two source hunks fixing a debug-build releaseAssertNoException crash in the expect() failure-message formatter, plus four subprocess regression tests. The C++ hunk adds RETURN_IF_EXCEPTION(scope, void()) after the per-property callback in JSC__JSValue__forEachPropertyOrdered, replacing a TODO comment. The Rust hunk collapses two ignored JestPrettyFormat::format calls into a loop and clears any pending non-termination exception when a call fails, replacing two more TODOs.
Security risks
None. This is exception-scope hygiene in the test runner's diff formatter and the property-enumeration binding. No untrusted-input parsing, auth, crypto, or filesystem changes.
Level of scrutiny
Medium. It touches JSC exception handling in native code, which is a common source of debug assertions and UAFs, but both hunks are direct copies of adjacent established code: the C++ line is identical to forEachPropertyImpl at bindings.cpp:5573/5688 (same file, same loop shape), and the Rust pattern matches four call sites in JSGlobalObject.rs (749, 765, 794, 808). I checked that the early return skipping properties.releaseData() is not a leak concern — the sibling function already returns early past the same call at 5608 and 5688, and PropertyNameArrayBuilder is stack-RAII.
Other factors
The PR description is unusually thorough: it names the exact assertion, traces the backtrace to to_js_host_call, explains why both hunks are needed independently, verifies each test fails on the unfixed build (USE_SYSTEM_BUN=1), and reruns the adjacent test suites. The user-visible behavior change (failure message truncates at the throwing property rather than skipping it, and the matcher throws its own error rather than leaking the getter's) is a strict improvement and only affects the already-pathological case of a getter throwing during failure-message formatting. The tests follow harness conventions (bunEnv/bunExe, describe.concurrent, concurrent pipe draining, combined-object assertions). The one subtlety I considered — a termination exception left pending by clear_exception_except_termination before the second loop iteration — matches how create_error_instance already behaves and is the intended semantics for termination.
|
Thanks for the review. There is nothing to change from it. On the one point it raises, a termination exception that the first CI: the build is still running. So far every failed test passed on retry, and none of them touch |
|
Cross-reference: the bindings.cpp hunk here (RETURN_IF_EXCEPTION after the ordered walk's callback) is the same one-line change as #37331, opened Aug 10, so one of the two should absorb the other. Separately, #34884 adds one field (can_throw_stack_overflow: false) to the FormatOptions literal that sits in the context of this PR's diff_format.rs hunk; whichever lands second gets a one-line conflict there, and the field simply stays in the literal. The two changes are otherwise independent: #34884's stack guard never returns an error on the diff path, so it does not reach the clear_exception branch added here. |
|
Two more triggers reach the same line 63 assertion (
const v = { get [Symbol.toStringTag]() { throw new Error("tag-throws") }, a: 1 };
expect(v).toEqual({ z: 9 });The same holds for
A Stock 1.4.0 reports the getter's error ( |
795fdbb to
2eb640f
Compare
|
Updated 11:38 AM PT - Aug 24th, 2026
✅ @robobun, your commit 884ca9afc0b192eb76e3167848e15401b9b05cd3 passed in 🧪 To try this PR locally: bunx bun-pr 39565That installs a local version of the PR into your bun-39565 --bun |
robobun
changed the title
|
Rebased on main and updated the description. The state of the PR is now:
One decision is open for a maintainer, and the description records it. This PR keeps the matcher's own error and prints the value that threw as far as it got. #34649 keeps the getter's error instead, which is what a release build prints today and what Jest does. If that policy is chosen, close this PR. The tests here then need the expected output changed from the matcher's error to the getter's error. The automated review comments above this one are rate limit notices with no content to act on. |
There was a problem hiding this comment.
Re-reviewed after 64d2c4f — the assertion-ordering nit is addressed and I found no further issues. I'm not auto-approving because the PR itself flags an open maintainer decision: this change picks "clear the exception and throw the matcher's own error" over #34649's "propagate the getter's error" (today's release-build and Jest behavior). That is a user-visible behavior choice, and it uses clear_exception_except_termination(), so a maintainer should confirm the policy before this lands.
What was reviewed:
DiffFormatter::fmtloop: verifiedclear_exception_except_terminationmatches the precedent increate_error_instanceand leaves termination exceptions pending.- Checked that the second loop iteration is safe when a termination exception survives the first (formatter entry points early-return on pending exceptions).
- Eight new subprocess tests: pipes drained concurrently, combined
{stdout, stderr, exitCode}assertions,describe.concurrentfor the ~4s runtime.
Extended reasoning...
Overview
The PR changes src/runtime/test_runner/diff_format.rs to check the return of two JestPrettyFormat::format calls (previously discarded with let _ =) and clear any pending non-termination exception when formatting fails. The two sequential calls are folded into a loop so both sides get identical handling. Eight subprocess regression tests are added to test/js/bun/test/expect-stack-overflow-crash.test.ts covering the fuzzer's stack-overflow shape plus RegExp toString, Proxy get, Symbol.toStringTag, and matcherHint triggers.
Security risks
None. This is the test-runner failure-message formatting path; no untrusted input, no auth/crypto/network surface.
Level of scrutiny
Medium-high. The Rust change is small and mechanically sound, but it deliberately swallows a JS exception via clear_exception_except_termination() — a pattern REVIEW.md explicitly flags ("Never clearException()"). The PR body justifies it by precedent (create_error_instance in JSGlobalObject.rs does the same for other matcher messages) and by necessity (matcher_hint renders DiffFormatter via format!, which panics on fmt::Error, so propagation would require wider changes). The justification is coherent, but it also changes release-build behavior: users who today see the getter's own error will instead see the matcher's error with a truncated diff.
Other factors
The decisive factor against auto-approval is that the author explicitly flags an unresolved policy choice between this PR and #34649, which takes the opposite approach (propagate the getter's error, matching Jest). The PR description says "The two PRs need one decision. If propagate is chosen, this PR should be closed." That is a design decision a maintainer should make, not an automated reviewer.
My earlier nit (assert exit status before JSON.parse) was applied in 64d2c4f. The tests follow harness conventions well: bunEnv/bunExe, concurrent pipe draining, describe.concurrent, combined-object assertions. The stack-overflow test's expected output (Error expect(received)...) locks in the new policy, so it will need updating if #34649 is chosen instead — which the author already noted.
|
Thanks. The re-review asks for nothing further, so the code stays as it is at 64d2c4f. No review thread is open. What is left is the maintainer decision in the description. In short: when a value throws while a failure message is formatted, this PR makes the matcher throw its own error and prints the value as far as it got. #34649 keeps the getter's error instead, which is what a release build prints today and what Jest does. Either choice fixes the debug abort. If #34649's policy is chosen, close this PR. The tests here are then still useful with the expected output changed from the matcher's error to the getter's error. CI on 64d2c4f: the two red tests ( |
…e is formatted DiffFormatter ignored the result of the two JestPrettyFormat::format calls. When the first value threw, the second value was formatted with the exception pending. Debug builds abort on the first native call of that second pass. Release builds then threw that exception from the matcher instead of the matcher's own error. DiffFormatter now drops the exception and diffs what was formatted, so the matcher reports its own failure. This also covers matcherHint, which renders the same DiffFormatter with format!. The ordered property walk that let the exception out of the formatter's callback is fixed on main since #39804.
64d2c4f to
884ca9a
Compare
|
Rebased on main ( On the new base, with main's The open maintainer decision in the description is unchanged. |
|
The automated review of the rebased head (884ca9a) has no comments to act on, and no review thread is open. CI for that head is build 104993, still running. Nothing else changed: the PR waits on the maintainer decision recorded in the description. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs — the clear_exception_except_termination() call after a failed format matches the established pattern in create_error_instance (JSGlobalObject.rs:736), and the eight new subprocess tests cover the crash and the behavior change from multiple angles. My earlier nit about asserting exit status before JSON.parse was addressed in 64d2c4f.
I'm not approving because the description and thread flag an open policy decision for a maintainer: this PR surfaces the matcher's own error with a truncated diff, while #34649 keeps the getter's error (today's release behavior and Jest's). That's a user-visible behavior choice between two competing PRs that a human should make.
Extended reasoning...
Overview
The PR touches two files: src/runtime/test_runner/diff_format.rs (14 lines net) and test/js/bun/test/expect-stack-overflow-crash.test.ts (+162 lines, eight new subprocess tests). In DiffFormatter::fmt, the two JestPrettyFormat::format calls previously discarded their result with let _ = ... // TODO:. When formatting a value ran user code that threw (a $$typeof getter, toString, a Proxy trap, Symbol.toStringTag), the pending exception leaked into the second format call and then into throw_value, tripping ASSERT_NO_PENDING_EXCEPTION in debug builds and replacing the matcher's error with the getter's error in release builds. The fix folds both calls into a loop and calls global_this.clear_exception_except_termination() when format returns Err.
Security risks
None. This is error-message formatting in the test runner's failure path. No untrusted-input parsing, auth, crypto, or filesystem paths are touched.
Level of scrutiny
Medium. The Rust change is small and mirrors the exact pattern already used in create_error_instance, create_type_error_instance, create_syntax_error_instance, and create_range_error_instance (JSGlobalObject.rs:733-799) — clear the non-termination exception and proceed with what was formatted so far. Termination exceptions correctly stay pending. The tests are thorough: they cover the original fuzzer shape (stack overflow at depth), throwing getters on both received and expected sides, nested objects, RegExp toString, array Proxy get, Symbol.toStringTag, and the matcherHint path from expect.extend. Each is a subprocess test asserting the combined {stdout, stderr, exitCode} object per repo convention.
Other factors
The reason I'm deferring rather than approving is the explicit open maintainer decision the author has flagged in the description and in three follow-up comments: this PR changes release-build behavior so that a value which throws during formatting no longer surfaces the getter's error — the matcher throws its own error instead, with the throwing side printed as far as it got (possibly empty). #34649 takes the opposite approach (propagate the getter's error, matching Jest). The author states "The two PRs need one decision" and "If propagate is chosen, this PR should be closed." That is a user-facing API-behavior choice between two competing PRs, not something an automated review should settle.
My prior inline nit (assert exit status before JSON.parse(result.stdout)) was addressed in 64d2c4f and the thread is resolved. The subsequent rebase onto b746c07 only reordered test blocks in the test file; diff_format.rs did not conflict.
|
Thanks. No change is needed from this review, and no review thread is open. The review's summary of the open decision matches the description: a maintainer picks between this PR (the matcher throws its own error) and #34649 (the getter's error is kept). CI for 884ca9a is build 104993, still running, with no red test so far. |
|
The automated review above is the same walkthrough of 884ca9a, re-posted with no comments to act on. No review thread is open. CI build 104993 for that head has 175 of 181 jobs passed and 6 still running. No test is red. The only entries so far passed on retry and none of them touch |
|
Superseded by #40068, which moves the value formatting out of Display::fmt into a fallible DiffFormatter::new and propagates the error from every caller. That settles the open decision in this PR's description: the getter's error is kept, the matcher does not throw its own error. I ran the test file from this PR against main with #40068 merged: no abort in any case. The failing matcher reports the getter's error (Error: boom, or RangeError for the stack overflow case) instead of the matcher's own error, which is the only difference from the expectations here. The coverage moves to #40919, a test-only PR on top of #40068 with the expectations updated to the propagated error. Closing in favor of #40068. |
1 participant
What does this PR do?
Fixes the rest of a fuzzer crash in the failure path of
expect()matchers. Fingerprintc5d217fcf09dae60:The fuzzer program recursed until the stack overflowed. In the frames that caught the overflow it ran
expect(jestObject).toStrictEqual(Bun). The matcher fails and formats both values for its message. At that stack depth every call from native code into JavaScript throws aRangeError. One of these throws happened inside the formatter's property callback, and two places let the pending exception through.The first place was
JSC__JSValue__forEachPropertyOrdered. It went on to the next key with the callback's exception pending, and the native initializer of the nextBunproperty asserted. That is the line 62 assertion in the report. #39804 fixed it on main while this PR was open (the walk now returns after a callback throws), so this PR no longer touchesbindings.cpp. The rebase dropped that hunk, which was the same line.The second place is
DiffFormatter::fmt(src/runtime/test_runner/diff_format.rs), and it is what this PR fixes. It ignored the result of bothJestPrettyFormat::formatcalls. On main today:ASSERT_NO_PENDING_EXCEPTIONinJSC__JSValue__getOwn, reached fromTag::get. This is theExceptionScope.h(63)form of the same assertion.throw_valuesees the pending exception and returns without throwing the matcher's error. The matcher throws the getter's error (or theRangeError) instead ofexpect(received).toEqual(expected).The fix is the
clear_exception_except_termination()after aformatcall fails. The two calls are now one loop, so the same branch runs for both values. The message is built from what was formatted before the throw, and the matcher throws its own error. This is the policycreate_error_instance(src/jsc/JSGlobalObject.rs:749) already applies to every other matcher message when aDisplayimpl fails.DiffFormattercannot returnfmt::Errorinstead:matcher_hint(src/runtime/test_runner/expect.rs:2917) renders it withformat!, which panics on aDisplayerror, andmatcher_hintreaches the same assertion today. A termination exception stays pending, as increate_error_instance.Behavior change, in release builds too: a value that cannot be formatted no longer turns the failure into the getter's error. The matcher reports its own failure, and the side that threw is printed as far as it got (empty when the value itself threw). #34649 proposes the opposite policy for the same crash: keep the getter's error, which is what a release build prints today and what Jest does, by changing
create_error_instance,DiffFormatterandmatcher_hintto propagate, among other changes. The two PRs need one decision. If propagate is chosen, this PR should be closed and the tests below need the matcher's error replaced by the getter's error in their expected output.This PR consolidates #28538 and #29784. Both contain this
diff_format.rschange and nothing else. Their triggers (a RegExp whosetoStringthrows, a Proxy that throws while it is formatted) are tests here now.How did you verify your code works?
test/js/bun/test/expect-stack-overflow-crash.test.tsgets eight subprocess tests. The first four use a$$typeofgetter, which the formatter reads from every object:Bunthat sorts right beforeBun.Archive, and a marker property that sorts after it. The marker must not be in the message.Error Exception: Maximum call stack size exceeded.atExceptionScope.h(62), withDiffFormatter::fmt>JSC__JSValue__forEachPropertyOrdered>getPropertySlot>BunObject_lazyPropCb_Archive>to_js_host_callon the stack.The other four run a RegExp whose
toStringthrows, an array Proxy whosegettrap throws, and an object whoseSymbol.toStringTaggetter throws on both sides oftoEqual, and thetoStringTagcase throughmatcherHintin anexpect.extendmatcher. None of these values is touched by the comparison, only by the message.On a debug build of main (
025570f4b6, which includes #39804) all eight fail: the cases where the received value throws abort with the line 63 assertion, the others print the getter's error. All eight also fail withUSE_SYSTEM_BUN=1 bun test(1.4.0-canary.1). They pass withbun bd test, in about 4 seconds for the file. The original test in the file is unchanged.Also run with the fix on the rebased branch:
test/js/bun/test/expect.test.js,test/js/bun/test/printing/diffexample.test.ts,test/js/bun/util/inspect.test.js. A local probe of ten value types (arrays, Maps, Errors, Dates, class instances, Proxies, nested objects) throughtoEqual,toStrictEqualandtoMatchSnapshotwith a throwing$$typeofgetter runs clean on the debug build.The fuzzer program itself does not crash on main in a fresh process. It needed state left behind by earlier programs in the same long-lived fuzzing process, which is why the report calls it flaky. The tests above set up that state directly.
[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 0
evidence per changed file