Repository navigation
Conversation
…iterals Bumps WebKit to the preview build carrying oven-sh/WebKit#297, which makes failIfStackOverflow() in the JSC parser consult the sticky m_hasStackOverflow flag. Without it, save-point backtracking clears the error at every nesting level and retries the same input as a destructuring pattern and again as a member expression, so a 10KB source with ~2600 levels of {a:{a:...}} reparses exponentially and never terminates (observed as 99% CPU and ~500MB/s RSS growth). The regression test drives this via eval() so it exercises JSC directly on every build; the file-load path now also throws RangeError cleanly instead of hanging.
|
Status: ready for review. Build #73871 finished 283/286 green. The new The three red lanes are two pre-existing main breaks, both already owned by separate fix sessions and both present on all five of the most recently merged PRs' builds:
WebKit preview build is The fix is entirely in the WebKit dependency (Parser.cpp |
|
Updated 6:48 AM PT - Jul 16th, 2026
❌ @robobun, your commit ba687bb has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34339That installs a local version of the PR into your bun-34339 --bun |
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
There was a problem hiding this comment.
Beyond the inline nit, I also checked the stderr: "" assertion (bunEnv sets BUN_DEBUG_QUIET_LOGS=1, so it holds on debug lanes) and the explicit 30s test timeout (bounded by the 20s spawn timeout, needed so the pre-fix hang fails fast rather than stalling CI). The WEBKIT_VERSION bump to a preview tag is handled correctly by prebuiltUrl/prebuiltDestDir (both branch on the autobuild- prefix).
Extended reasoning...
The reported finding is a test-fixture arithmetic nit; the object-literal case still exercises ~16.6k levels, well past the ~3k threshold, so the test is not broken today. Separately verified: the autobuild-preview-* tag flows through scripts/build/deps/webkit.ts without issue (both the release URL and cache-dir key already special-case the autobuild- prefix), and the test's stderr/timeout patterns that CLAUDE.md usually flags were examined and found acceptable in this context. Not approving because this pins the JS engine to an unmerged WebKit PR's preview build — a maintainer should confirm the landing sequence with oven-sh/WebKit#297.
…00 deep
Buffer.alloc(n, fill) fills n bytes, so a 3-byte fill only produced
~16667 '{a:' copies with a truncated tail while the close side had
50000 '}', leaving the object-literal fixture unbalanced and shallower
than intended.
There was a problem hiding this comment.
Thanks — ba687bb addresses the Buffer.alloc sizing issue; both fixtures are now balanced 50000-level nestings. I'm deferring to a human on the merge itself: this pins WEBKIT_VERSION to a preview tag for the still-open oven-sh/WebKit#297, and a maintainer should decide whether to land against the preview build or wait for that PR to merge and bump to the resulting sha.
Checked: the test now generates syntactically valid balanced input for both variants; prebuiltUrl/prebuiltDestDir already special-case autobuild--prefixed versions so the tag resolves correctly; the spawn drains stdout/stderr concurrently and asserts signalCode: null so a hang surfaces as a real failure.
Extended reasoning...
Overview
Two files: a one-line WEBKIT_VERSION bump in scripts/build/deps/webkit.ts (from sha 4895f45d… to preview tag autobuild-preview-pr-297-3946a08b), and a new 45-line regression test at test/js/bun/jsc/parser-deep-nesting.test.ts. The behavioral fix itself lives entirely in oven-sh/WebKit#297 (JSC Parser.cpp — making failIfStackOverflow() consult the sticky m_hasStackOverflow flag so save-point backtracking can't clear the overflow error and reparse exponentially).
Since my last review
I previously flagged that Buffer.alloc(50000, "{a:") fills 50000 bytes, not 50000 repetitions, leaving the object-literal fixture unbalanced at ~16667 levels. ba687bb changes the byte count to depth * open.length / depth * close.length, which produces exactly 50000 balanced repetitions for both the {a:/} and [/] variants. That resolves the concern; the thread is marked resolved.
Security risks
None identified. The change tightens behavior (a hang becomes a thrown RangeError) rather than relaxing any check. The test spawns bunExe() with bunEnv and a fixed inline script — no external input, network, or filesystem writes.
Level of scrutiny
High, because the substantive change is a JavaScriptCore parser modification pulled in via a dependency bump. The diff in this repo is trivial, but the linked WebKit change alters stack-overflow handling on a code path shared by destructuring patterns, arrow-function heuristics, and assignment expressions. That review belongs on oven-sh/WebKit#297; here a maintainer needs to sign off on shipping Bun against a preview WebKit build.
Other factors
WEBKIT_VERSIONhas never been merged to main pointing at anautobuild-preview-pr-*tag before (theautobuild-handling inprebuiltUrl/prebuiltDestDirlanded in #34143 but main still pins a plain sha). Whether to land against a preview tag or wait for WebKit#297 to merge is a process call for a maintainer.- The PR description references tag
…-8b31a3bcwhile the code pins…-3946a08b; the robobun status comment confirms3946a08bis the published one, so the code is correct and the description is just stale. - The test itself looks solid post-fix: it drains both pipes concurrently with
proc.exited, asserts the full{stdout, stderr, exitCode, signalCode}object so a timeout kill (SIGTERM) fails loudly, and driveseval()in a subprocess so it exercises JSC directly regardless of Bun's transpiler stack-frame size. Finder concerns aboutstderr: ""and the 30s per-test timeout were examined and ruled out by verifiers this run.
|
This also fixes the assignment-context variant that was separately reported as a "codegen memory explosion": const N = 3000;
new Function(`let q; (${"[".repeat(N)}q${"]".repeat(N)} = []);`)();
// peak RSS ~21 GB then SIGABRT on a release buildThat report assumed the parser was fine because The existing array-literal test here ( |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-16, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
Depends on oven-sh/WebKit#297. This PR bumps
WEBKIT_VERSIONto pick up that fix and adds a regression test; the preview release isautobuild-preview-pr-297-3946a08b.Reproduction
1.4: 99% CPU, RSS climbs unboundedly (~500MB/s), never terminates.
1.3.14:
error: Maximum call stack size exceededin ~25ms (Bun's parser had larger stack frames and rejected before handing to JSC).Node:
RangeError: Maximum call stack size exceededimmediately.The same hang is reachable directly via
eval()on both 1.3.14 and 1.4, bypassing Bun's transpiler entirely.Cause
The hang is in JavaScriptCore's parser, not the bytecode compiler. When
failIfStackOverflow()trips insideparseAssignmentExpressionfor a nested{a:{a:...}}, the failure propagates back to each enclosing level. At each levelmaybeAssignmentPatternis true (the token is{), soswapSavePointForErrorrewinds the lexer and clearsm_errorMessage, then the same input is retried viatryParseDestructuringPatternExpression. That path reachesparseAssignmentElement, which on failure of itsparseDestructuringPatternattempt callsrestoreSavePoint(clearing the error again) and retries viaparseMemberExpression->parseObjectLiteral->parseAssignmentExpression. Two alternate-production retries per nesting level multiply to exponential work; the process spins inParser::logError/StringPrintStream::vprintfallocating error strings forever.Sampled backtrace of the hang
Fix
m_hasStackOverflowis already set byfailWithStackOverflow()and is not touched byrestoreSavePoint. oven-sh/WebKit#297 makesfailIfStackOverflow()consult it, so the first overflow short-circuits every subsequent recursion entry (parseAssignmentExpression,parsePrimaryExpression,parseDestructuringPattern) and the failure propagates in O(depth).Verification
With a local WebKit build carrying the fix:
Destructuring, arrow functions, assignment patterns and array destructuring continue to parse normally. The new test drives the eval path so it exercises JSC directly regardless of Bun's transpiler stack-frame size; before the fix the spawned child is killed by the 20s timeout and the assertion fails on
signalCode: "SIGTERM".[decide:webkit] gate passed · iteration 0 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file