Conversation
…azy builders JSC__JSValue__forEachPropertyImpl skipped its exception clear when getPropertySlot reported the property as not found, which happens when a lazy static property builder throws (for example Bun.$ evaluating shell.ts after the global Symbol was clobbered). The walk then entered the next lazy builder with that exception still pending and failed the exception scope assertion in debug builds. Clear the exception before the continue, matching JSC__JSValue__forEachPropertyOrdered. The Bun.sql and Bun.SQL builders also called reportUncaughtExceptionAtEventLoop in debug builds while the module load exception was still pending, running the uncaughtException machinery (process._fatalException lookup) with a pending exception and tripping a stale structure assertion. Drop the report and let RETURN_IF_EXCEPTION propagate the error to whoever touched the property.
WalkthroughThe change removes debug exception reporting from Bun SQL lazy helpers, makes property enumeration skip failed lookups after clearing exceptions, and adds a regression test for ChangesLazy property exception handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped exception-handling fix that mirrors the existing forEachPropertyOrdered pattern. The one inline nit (undrained stderr pipe in the test) is minor and doesn't block.
Extended reasoning...
Overview
Three changes: (1) bindings.cpp reorders CLEAR_IF_EXCEPTION in JSC__JSValue__forEachPropertyImpl so it runs before the continue when getPropertySlot returns false — the ordered sibling JSC__JSValue__forEachPropertyOrdered at line 5764-5766 already does exactly this. (2) BunObject.cpp deletes six lines of #if BUN_DEBUG-only reportUncaughtExceptionAtEventLoop calls in the Bun.sql/Bun.SQL lazy builders that ran with a pending exception; the RETURN_IF_EXCEPTION immediately following already propagates correctly. (3) A subprocess regression test in inspect.test.js.
Security risks
None. The change only affects how pending exceptions are cleared/propagated during property enumeration for inspect. No new user-controlled input paths, no auth/crypto/permissions.
Level of scrutiny
Native JSC bindings warrant care, but both native edits are minimal and mechanical: one is a pure reorder to match its sibling function's already-correct pattern (strictly widening when the exception clear fires — no case that previously cleared now skips it), and the other deletes debug-only code that was actively violating the "never enter JSC with a pending exception" invariant. Release-build behavior of BunObject.cpp is byte-identical. The PR description traces the exact assertion failures and references the prior PR #27308 that attempted the same Bun.sql fix.
Other factors
The bug hunter flagged one test-hygiene nit: the new test pipes stderr but only drains stdout in the Promise.all. That's worth fixing to match the neighboring test, but it's not a correctness blocker — the passing child writes nothing to stderr, and on regression the child is SIGABRT'd. I also confirmed forEachPropertyOrdered uses the identical hasProperty = ...; clear; if (!hasProperty) continue; sequence, so this is applying an established fix to its twin. No prior human reviews or outstanding comments on the PR.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Ignore exceptions from "Get" proxy traps and lazy static | ||
| // property builders, including when the slot was not found. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
![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/util/inspect.test.js`:
- Around line 958-961: Update the subprocess result handling around the
Promise.all call to retain stderr instead of discarding it, then assert the
fixture’s stable stderr contract alongside stdout and exitCode. Avoid asserting
stderr is universally empty when debug or sanitizer diagnostics may be
permitted.
🪄 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: 0ef0bbc2-ef84-40d3-a6ff-83ae3a589455
📒 Files selected for processing (3)
src/jsc/bindings/BunObject.cppsrc/jsc/bindings/bindings.cpptest/js/bun/util/inspect.test.js
💤 Files with no reviewable changes (1)
- src/jsc/bindings/BunObject.cpp
| const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout).toBe(["inspect: true", "sql: TypeError", "SQL: TypeError", ""].join("\n")); | ||
| expect(exitCode).toBe(0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the subprocess stderr contract.
Line 958 drains stderr but discards it. A lazy-builder debug report or another child diagnostic can appear while stdout and exitCode still match, so the regression test can pass after a regression. Retain stderr and assert the stable contract for this fixture together with stdout and exitCode. Do not use a blanket empty-string assertion if debug or sanitizer diagnostics are allowed.
🤖 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 `@test/js/bun/util/inspect.test.js` around lines 958 - 961, Update the
subprocess result handling around the Promise.all call to retain stderr instead
of discarding it, then assert the fixture’s stable stderr contract alongside
stdout and exitCode. Avoid asserting stderr is universally empty when debug or
sanitizer diagnostics may be permitted.
Sources: Coding guidelines, Learnings
|
Closing this as a duplicate. #37256 (opened earlier) carries the same two native changes verbatim: the clear-before-continue reorder in The Windows lane failures on this PR's test (empty child stdout on On the review notes: the stderr drain was added in b54e964, and I'll skip the suggested stderr content assertion since the repo's test guidance rules out asserting on crash-report text; with this closed as a duplicate it's moot anyway. |
There was a problem hiding this comment.
LGTM — the stderr-drain nit from the earlier pass is addressed, and this run found nothing further.
What was reviewed:
forEachPropertyImplnow clears the exception beforecontinue, matchingforEachPropertyOrderedat bindings.cpp:5763-5768 exactly.- The removed
#if BUN_DEBUGblocks indefaultBunSQLObject/constructBunSQLObjectwere the only debug-only hooks betweenrequireIdandRETURN_IF_EXCEPTION; the propagation path is unchanged for release. - The comment-cop flag on bindings.cpp:5601 looks like a false positive — it's a one-sentence note naming what
CLEAR_IF_EXCEPTIONcovers, not a workaround justification.
Extended reasoning...
Overview
Three files: src/jsc/bindings/bindings.cpp reorders CLEAR_IF_EXCEPTION to run before the if (!hasProperty) continue; in JSC__JSValue__forEachPropertyImpl, so a lazy static property builder that throws (and makes getPropertySlot return false with a pending exception) no longer carries that exception into the next iteration. src/jsc/bindings/BunObject.cpp deletes two #if BUN_DEBUG blocks that called reportUncaughtExceptionAtEventLoop while an exception was still pending — the immediately-following RETURN_IF_EXCEPTION already propagates correctly. test/js/bun/util/inspect.test.js adds a spawned-child regression test.
Security risks
None. This is exception-handling ordering inside the inspect property walk and removal of debug-only diagnostic code. No new inputs are parsed, no permissions/auth touched.
Level of scrutiny
Moderate — JSC exception discipline is sensitive, but the change is ~10 production lines. The bindings.cpp reorder brings the unordered walk into exact parity with its sibling JSC__JSValue__forEachPropertyOrdered (bindings.cpp:5763-5768), which already stores hasProperty, clears, then checks. The BunObject.cpp change is pure deletion of debug-gated code that violated the "never enter JS with a pending exception" invariant; release behavior is byte-identical.
Other factors
The prior review's stderr-drain nit was addressed in commit b54e964 — the test now reads proc.stderr.text() in the same Promise.all. The github-actions comment-cop flag on the two-line comment at bindings.cpp:5601 appears to be a heuristic false positive: the comment is a single wrapped sentence naming which exceptions are cleared (proxy traps and lazy builders), extending the pre-existing one-liner rather than justifying a workaround. The test asserts exact stdout, checks exit code last, isolates the Symbol = NaN global clobber in a subprocess, and drains both pipes. The bug-hunting system found no issues this run.
1 participant
Fixes a deterministic Fuzzilli crash (fingerprint
e42c73f70c7e7925, debug assertion abort).Repro
The fuzzer hit this through
new CompressionStream(globalThis): the invalidformaterror message inspects the received value, the walk reaches theBunobject, and materializing the lazy$property evaluatesshell.ts, which throwsSymbol is not a functionat module scope.What was wrong
Two pending-exception bugs, both only observable as aborts in debug/ASAN builds:
JSC__JSValue__forEachPropertyImplcleared getter exceptions only whengetPropertySlotreported the slot as found. A throwing lazy static property builder makesgetPropertySlotreturn false with the exception left pending, so thecontinueskipped the clear and the walk entered the next lazy builder (Bun.Archive) with a pending exception, failingreleaseAssertNoException.JSC__JSValue__forEachPropertyOrderedalready clears before itscontinue; this applies the same order to the unordered walk.The
Bun.sql/Bun.SQLbuilders calledreportUncaughtExceptionAtEventLoop(debug builds only) while the module-load exception was still pending. That runs the uncaughtException machinery, and itsprocess._fatalExceptionlookup reifies static properties with a pending exception, which makes the lookup loop see a staleStructure*and fail theobject->structure() == thisassertion inStructure::storedPrototype. Reachable with justSymbol = NaN; Bun.sql. The report is gone;RETURN_IF_EXCEPTIONpropagates the real module error to whoever touched the property (PR Fix null pointer dereference in Bun.sql when module fails to load #27308 fixed an earlier version of this but was closed after the repo restructure made it unmergeable).After the fix
Bun.inspect(Bun)completes, skipping properties whose builder threw.Bun.sql/Bun.SQLthrow the underlying module error instead of aborting.TypeError.Testing
Added a regression test in
test/js/bun/util/inspect.test.jsthat spawns a child which clobbersSymbol, inspectsBun, and touchesBun.sql/Bun.SQL. It fails on an unfixed debug build (child dies with SIGABRT) and passes with this change. Release builds compile these assertions out, so the crash does not reproduce underUSE_SYSTEM_BUN=1; the fail-before was verified against an unfixed debug binary.Pre-existing local failures in
inspect-error.test.js(extraat requirestack frame) reproduce identically on an unfixed binary and are unrelated.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/inspect.test.js