Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughThe change pins Bun to a WebKit preview build and adds a JSC stress fixture for RegExp values passed as both the receiver and argument to ChangesRegExp validation
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No actionable issue was established from the available evidence. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides detailed problem, fix, background, and verification information. However, it states that the PR does not change the WebKit pin, while the changeset updates Resolution Update the description to document the
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
|
Updated 4:27 AM PT - Sep 27th, 2026
✅ @robobun, your commit 67a0d50ac4ee1f1ff7f5aa5af3e69f496fb21c68 passed in 🧪 To try this PR locally: bunx bun-pr 42694That installs a local version of the PR into your bun-42694 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
The finding on The preview pin is there on purpose for now. oven-sh/WebKit#654 is still open, and CI needs a build of that branch to prove the fix. After oven-sh/WebKit#654 merges, I will move |
|
The fuzzer reported this assertion a second time, about 8 hours after the first report, with a different script. The core of the script is the same: const v10 = /za\S\p{gc=Nd}a|bqja*b|c/g;
function f11() {
// ... object setup that does not matter here ...
v10.test(v10);
return f11;
}
v10.valueOf = f11;
function f31() { return v10; }
v10.toString = f31;
try { v32.slice(v10); } catch (e) {}It behaves like the first script. On a debug build with WebKit So the fuzzer will keep reporting |
71b86df to
07303bb
Compare
|
I put the commit of oven-sh/WebKit#654 on top of Checked with a debug build of the rebased branch: the new fixture passes, all 117 tests of The earlier review finding still applies. This PR must not merge with a preview pin. I will move the pin to the |
07303bb to
616336c
Compare
|
I put the commit of oven-sh/WebKit#654 on top of Checked with a debug build of the rebased branch: the new fixture passes, all 117 tests of Each pin move on |
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 1 finding from earlier reviews is still open above.
Still open from earlier reviews (1):
- Unresolved: 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
|
I will not rebuild the preview for each pin move any more. Each round needs a new WebKit preview build and a full CI run here, and this PR cannot merge on a preview pin anyway. So this PR stays as it is until oven-sh/WebKit#654 merges. After that merge, the next WebKit upgrade on oven-sh/WebKit#654 is not affected. It is mergeable and its CI is green. The fuzzer reported this assertion a sixth time today. The new script has the RegExp literal inside a recursive constructor: function F0(a2, a3) {
if (!new.target) { throw 'must be called with new'; }
const v4 = /P(?:ab|cde)(x)(x)(x)\2/iv;
v4.test(v4);
const t7 = this.constructor;
new t7(F0);
}
new F0();With WebKit |
|
Upstream WebKit landed the same change on 2026-09-15: WebKit/WebKit#74030 (https://commits.webkit.org/321146@main). oven-sh/WebKit#654 is now a cherry-pick of that commit on the Nothing changes for this PR. It still waits for the fix to reach the WebKit pin on |
The DFG gives the RegExp and the argument of RegExpTest one register when both are the same value. Before WebKit 321146@main the minimum-length filter of RegExp#test asserted that the two registers differ, so a debug or ASAN build aborted in the DFG compiler. The fixture is JSTests/stress/regexp-test-regexp-is-also-the-argument.js from that WebKit commit.
616336c to
3637080
Compare
robobun
changed the title
regExp.test(regExp) passes the DFG register assertion in debug and ASAN buildsregExp.test(regExp) in the DFG
|
The fixture body is now the upstream test file, byte for byte. What I ran on the rebased branch (
This closes the review finding about the preview pin. I close oven-sh/WebKit#654, because the upstream merge brought the same commit to the fork. |
There was a problem hiding this comment.
LGTM — the diff is now test-only (the preview-tag pin in scripts/build/deps/webkit.ts is gone), and the new jsc-stress fixture is a faithful port that the harness runs as-is.
What was reviewed:
- Traced every
shouldBein the fixture by hand againstregExp.test("/source/flags")semantics (non-anchored,^...$,.{20}longer than its own source,/glastIndex 0→4→0,/\/ab/ylastIndex 0→0 and 2→5): all expected values are right, so no assertion is vacuous. - Checked the
// @ bun+//@ runDefault("--useConcurrentJIT=0")header againstparseJSCFlagsin jsc-stress.test.ts andnoInline/testLoopCountagainst preload.js — all supported; entry sits in the DFG group ofjsFixtures. - Could not confirm from this offline checkout that the base pin
f20ce774…already contains oven-sh/WebKit#654; if it does not, the debug/ASAN jsc-stress lanes fail loudly rather than pass silently, so CI settles it.
Extended reasoning...
The change adds one 57-line JSC stress fixture under test/js/bun/jsc-stress/fixtures and a one-line registration in the jsFixtures array; no src/, native, or build-config files are touched anymore, so it touches no security-sensitive surface. The earlier blocking finding about pinning WEBKIT_VERSION to an ephemeral preview tag is addressed by the pin change being dropped from the diff. Every assertion in the fixture was verified by hand and the harness directive parsing and preload globals it relies on exist in the checked-in harness. No CODEOWNERS entry covers the changed paths and no third-party change request is visible in the timeline. The only residual uncertainty, whether the base WebKit pin already includes the register-overlap fix, is one that CI's debug and ASAN lanes would surface as a hard failure rather than something a human reviewer could settle by reading the diff.
|
A note on the automated summary above: its description warning refers to the previous head ( The current head,
|
Behaviour change: none
Problem
regExp.test(regExp):ASSERTION FAILED: noOverlap(baseGPR, argumentGPR, scratch1GPR, scratch2GPR)inemitRegExpMinimumLengthFilterGuards(dfg/DFGSpeculativeJIT64.cpp).RegExpTestthe same register. The assertion did not permit that.mainhas the fix since Upgrade WebKit to 7b485a76e9 #43882. It is upstream WebKitd22fa241f1(https://commits.webkit.org/321146@main, https://bugs.webkit.org/show_bug.cgi?id=324211). Bun has no test for it.Fix
regexp-test-regexp-is-also-the-argument.jsintest/js/bun/jsc-stress, and its line injsc-stress.test.ts.src/, and it does not change the WebKit pin.f20ce7744553, the pin onmain): the fixture passes, and all 117 tests ofjsc-stress.test.tspass.37da174d50, the commit onmainbefore Upgrade WebKit to 7b485a76e9 #43882 (WebKit35e8970dfd92), fails the fixture with exit code 134 and the assertion above.Background
x.test(x)gives theRegExpTestnode two edges to the same node, so both operands read the same register.RegExpTestasserts when the RegExp is also the argument (cherry-pick WebKit/WebKit#74030) WebKit#654, which had the same change. Upstream landed the change on 2026-09-15, andmaingot it with the upstream merge in Upgrade WebKit to 7b485a76e9 #43882. So the pin change is gone from this PR.Notes
Smallest repro for a debug build without the fix:
How it was found: fuzzer runs of the debug build. The fuzzer reported this assertion six times, with three shapes of script.
The RegExp is a constant of the function. This shape was flaky:
RegExpTestfails and emitted no code for the node.RegExpTestasserts when the RegExp is also the argument (cherry-pick WebKit/WebKit#74030) WebKit#654 has the details.The RegExp is a closure variable, or a literal in the function. These shapes failed on every run in a fresh process:
On the debug build of this branch, all three scripts exit cleanly, 3 of 3 runs each. The first script also passes 3 of 3 runs after the earlier script. The last script ends with the expected
RangeError: Maximum call stack size exceeded.The fixture:
JSTests/stress/regexp-test-regexp-is-also-the-argument.jsfrom the upstream commit, byte for byte. The file adds the// @bunline and a--useConcurrentJIT=0directive above it.^, and a constant sticky RegExp. The first two fail on a build without the fix.About the fail-before proof:
USE_SYSTEM_BUN=1cannot show it. A release build has no assertions, so the fixture passes there before and after the fix.bun bd --webkit-version=35e8970dfd92on this branch does not compile, because the bindings onmainuse a WebKit API that the older commit does not have. So the fail-before build is the commit before Upgrade WebKit to 7b485a76e9 #43882 with this PR's two files on top.jscshells of the two WebKit releases agree: the one of35e8970dfd92aborts on the upstream test file with the assertion, and the one off20ce7744553passes it.[auto-merge] gate passed · iteration 2 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 0 rejected · iteration 2
evidence per changed file