test: add JSC stress fixture for the DFG fast ArrayIterator entries path - #33260
test: add JSC stress fixture for the DFG fast ArrayIterator entries path#33260robobun wants to merge 2 commits into
Conversation
Ports WebKit's JSTests/stress/array-iterator-fast-entries-double-array-fixup-exit-ok.js into the jsc-stress suite. Iterating doubleArray.entries() through the DFG's fast array-iterator path produced a use-before-def graph: the bytecode parser emitted GetByVal (which clobbers exit state) directly followed by NewArray([index, value]) with no ExitOK in between, so FixupPhase inserted the ValueRep that re-boxes the double element one slot before the GetByVal that defines it. In release builds the malformed graph reached virtual register allocation and aborted the JIT worker thread: WTF::Vector<unsigned int, 64, CrashOnOverflow>::expandCapacity JSC::DFG::performVirtualRegisterAllocation Introduced by WebKit/WebKit@3d1bebd7930d, which reached bun with the WebKit bump in #32437, and fixed upstream by WebKit/WebKit@ae69ef2312c0, which is in the current pin (picked up by #33133). The fixture runs with useConcurrentJIT=false and validateGraphAtEachPhase=true so a future WebKit bump that loses the fix fails deterministically at the fixup phase instead of probabilistically on a concurrent compiler thread.
|
Updated 1:09 PM PT - Jul 2nd, 2026
❌ @robobun, your commit 0dbac99 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33260That installs a local version of the PR into your bun-33260 --bun |
|
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)
WalkthroughAdds a new Bun/JSC stress test fixture that reproduces an ArrayIterator ChangesNew stress fixture and registration
Sequence Diagram(s)Not applicable; the change is a test fixture addition with no interaction flow worth diagramming. Estimated code review effort: 1 (trivial addition of a test fixture and its registration) Suggested labels: test, javascript-jit Suggested reviewers: N/A Poem 🚥 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 — test-only port of an upstream WebKit JSC stress fixture, following the existing pattern in this directory.
Extended reasoning...
Overview
This PR adds a single JSC stress-test fixture (array-iterator-fast-entries-double-array-fixup-exit-ok.js, 31 lines) ported directly from WebKit's JSTests/stress/, and appends its filename to the jsFixtures array in test/js/bun/jsc-stress/jsc-stress.test.ts. No production code is touched.
Security risks
None. The change is entirely within the test tree, adds a self-contained JS fixture that only iterates an array in a hot loop, and is executed via the existing spawn harness with bunEnv. There is no network, filesystem, auth, or crypto surface.
Level of scrutiny
Low. This is a mechanical addition to an established fixture-list pattern: the //@ runDefault("--useConcurrentJIT=false", "--validateGraphAtEachPhase=true") directive is already handled by parseJSCFlags, testLoopCount is provided by the existing preload.js, and the test driver loop is identical to the ~60 other fixtures already in the list. The fixture is a near-verbatim upstream port with a clear provenance comment.
Other factors
The PR description thoroughly documents the root cause, the upstream fix commit, and verification (debug build pass, DFG graph excerpt confirming the intended shape is reached). The bug-hunting system found no issues. There are no outstanding reviewer comments and no prior reviews from me on this PR.
There was a problem hiding this comment.
LGTM — straightforward port of an upstream JSC stress fixture into the existing harness.
Extended reasoning...
Overview
This PR adds a single JSC stress-test fixture (array-iterator-fast-entries-double-array-fixup-exit-ok.js) ported directly from WebKit's JSTests/stress/, and appends its filename to the jsFixtures array in test/js/bun/jsc-stress/jsc-stress.test.ts. No runtime, native, or production code is touched.
Security risks
None. The change is entirely test-only: a new fixture file executed in a spawned subprocess by the existing test runner, plus a one-line array entry. No untrusted input handling, no auth/crypto/permissions surface.
Level of scrutiny
Low. This follows the exact established pattern for the ~70 other fixtures in this directory: a // @bun marker, a //@ runDefault(...) directive that parseJSCFlags translates into BUN_JSC_useConcurrentJIT=false / BUN_JSC_validateGraphAtEachPhase=true, and a loop over the preload-defined testLoopCount global. The fixture body is a near-verbatim copy of the upstream WebKit regression test.
Other factors
I verified that preload.js defines testLoopCount and that the //@ runDefault(...) directive parses correctly with the existing regex (both flags are captured before the parser breaks on the following comment lines). The PR description documents that the test was verified to pass under both bun bd and USE_SYSTEM_BUN=1, and explains precisely why the fixture is deterministic (--validateGraphAtEachPhase fails right after fixup rather than probabilistically at register allocation). The bug-hunting system found no issues, and there are no outstanding reviewer comments.
|
CI status across both runs (67993, 67998): the new fixture,
One retrigger already spent on this, so leaving it as is. Ready for review. |
1 participant
What
Ports WebKit's regression test
JSTests/stress/array-iterator-fast-entries-double-array-fixup-exit-ok.js(added by WebKit/WebKit@ae69ef2) intotest/js/bun/jsc-stress/, and runs it with--useConcurrentJIT=false --validateGraphAtEachPhase=true.Why
Bun builds whose WebKit pin was in the range
cd821fecca0d(#32437, Jun 16) throughf72c0151d39e(#32184, Jun 27) could abort on the JIT worker thread while DFG-compiling any hot function containing:Native stack of the abort:
Root cause (upstream WebKit/WebKit@3d1bebd): the fast path for
ArrayIteratorwith kindentriesemitsGetByVal(which clobbers exit state) immediately followed byNewArray([index, value])in the same bytecode origin, with noExitOKin between, so the NewArray is exit-invalid. When the array has double storage, FixupPhase gives the GetByVal an unboxed Double result and must insert aValueRepto re-box it for NewArray's edge. Fixup inserts such conversions at the most recent exit-OK node, which here is the GetByVal itself, so the ValueRep lands one slot above the node that defines its operand. The use-before-def graph then crashes virtual register allocation: the child's VirtualRegister is still the invalid sentinel, andtoLocal()of it indexes the ScoreBoard'sVector<uint32_t, 64>at about 3.2e9, which tripsCrashOnOverflow.Upstream fixed this in WebKit/WebKit@ae69ef2 ("entries" ArrayIterator should emit ExitOK before NewArray, webkit.org/b/317327). That commit is in bun's current WebKit pin (
d81bcc3d833c, picked up by #33133), so bun main is not affected today, and no tagged release was ever affected (1.3.14 predates the regression). Only canary/main builds between Jun 16 and Jun 29 were.This fixture keeps the shape covered by bun's own CI so a future WebKit upgrade that drops or reintroduces the bug fails deterministically (graph validation aborts right after fixup) instead of as a probabilistic SIGABRT on a concurrent compiler thread.
Verification
bun bd test test/js/bun/jsc-stress/jsc-stress.test.ts -t array-iterator-fast-entriespasses (debug build).USE_SYSTEM_BUN=1(release).reportDFGCompileTimes=1,inneris DFG-compiled, and the post-fixup graph shows the boxing conversion being required and correctly placed after its operand.post-fixup DFG excerpt (block 5 of inner)
Without the upstream fix the
ExitOKat index 11 does not exist and theValueRepis inserted at the GetByVal's own index, above its operand, which--validateGraphAtEachPhaserejects withvalidation failed: block->isInPhis(child) || seenNodes.contains(child).