Upgrade WebKit to b5ba38a21e17#32414
Conversation
- Bump WEBKIT_VERSION to oven-sh/WebKit@09f04cd5a489 (merges upstream b5ba38a21e17) - NodeVM: compare against vm.propertyNames->globalThis instead of allocating utf8(); the new CString==ASCIILiteral overload made the old comparison ambiguous
|
Updated 2:32 PM PT - Jun 16th, 2026
✅ @Jarred-Sumner, your commit 0e5f9a062baa7f16c9c171252a9827ca677bd902 passed in 🧪 To try this PR locally: bunx bun-pr 32414That installs a local version of the PR into your bun-32414 --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)
WalkthroughThe PR bumps the default ChangesWebKit Version Bump
NodeVM globalThis Identifier Comparison
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The Bun-side changes look correct (the vm.propertyNames->globalThis comparison is equivalent and avoids the allocation), but since this is a JSC engine bump it should get a human sign-off once CI is green on the new autobuild.
Extended reasoning...
Overview
Two files changed in this repo: a WEBKIT_VERSION hash bump in scripts/build/deps/webkit.ts, and two one-line edits in src/jsc/bindings/NodeVM.cpp swapping propertyName.uid()->utf8() == "globalThis" for propertyName == vm.propertyNames->globalThis. The NodeVM change is semantically equivalent (interned-identifier pointer compare vs. allocating a CString), avoids the new upstream operator==(CString, ASCIILiteral) ambiguity, and is arguably safer since it can't accidentally match a Symbol whose description happens to be "globalThis".
Security risks
None introduced by the Bun-side diff. The upstream merge does include security-relevant JSC fixes (Yarr heap overflow, corrupted JSString from \\B+unicode), but those are fixes flowing in, not new exposure.
Level of scrutiny
The in-repo diff is trivial, but the effective change is a JavaScriptCore engine upgrade — the most critical code path in the runtime. The PR description itself notes CI will fail until the autobuild-09f04cd5… release finishes building, so there's no green signal yet that the new engine passes Bun's test suite. That alone makes it inappropriate for bot auto-approval.
Other factors
No prior human reviews and no outstanding comments beyond the build-bot status. The bug-hunting pass found nothing in the Bun-side changes, which matches my read. Routine WebKit bumps like this are typically gated on CI rather than line-by-line review, so deferring until a human can confirm the build/test results is the right call.
|
Found 8 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
The Bun-side changes look correct (the PropertyName == vm.propertyNames->globalThis comparison is the idiomatic JSC pattern and strictly better than the old uid()->utf8() allocation), but deferring since a JSC engine upgrade is inherently high-impact and should land with human sign-off once CI is green across platforms.
Extended reasoning...
Overview
This PR bumps WEBKIT_VERSION in scripts/build/deps/webkit.ts to pull in a new oven-sh/WebKit build (merging upstream WebKit b5ba38a21e17), and adjusts two property-name comparisons in src/jsc/bindings/NodeVM.cpp from propertyName.uid()->utf8() == "globalThis" to propertyName == vm.propertyNames->globalThis. The Bun-side diff is 3 lines.
Correctness of the NodeVM.cpp change
The new form is the standard JSC idiom: PropertyName::operator==(const Identifier&) compares interned UniquedStringImpl* pointers, so this is a cheap pointer-equality check against the CommonIdentifiers entry rather than allocating a CString. It is semantically equivalent to the old string comparison for the public "globalThis" identifier, and it resolves the overload ambiguity introduced by upstream's new operator==(CString, ASCIILiteral). No behavior change expected here.
Security risks
The Bun-side diff itself introduces no new security surface. However, the WebKit bump pulls in upstream changes that are security-relevant (e.g., the Yarr heap-overflow fix for duplicate named capture groups, and the Yarr JIT \B+unicode corruption fix). Those are fixes rather than regressions, but they underscore that this is a JS-engine-level change.
Level of scrutiny
Although the visible diff is tiny and mechanical, the effective change is a JavaScriptCore engine upgrade spanning JIT/DFG optimizer changes, Yarr, Temporal/ICU caching, libpas, and Wasm BBQ JIT. That is core, production-critical code where regressions surface as crashes or subtle JS semantics changes. WebKit-bump PRs in this repo are routine, but they're routinely gated on full multi-platform CI and a maintainer's eye — not something a bot should rubber-stamp, especially while the autobuild release (and therefore CI) is still pending.
Other factors
- No bugs were found by the bug-hunting system.
- No prior
claude[bot]reviews on this PR. - No outstanding human reviewer comments.
- CI is expected to fail until the
autobuild-09f04cd5...WebKit release finishes; merging should wait on green CI regardless.
The WebKit upgrade in #32414 changed JSPromise::reject to the 2-arg reject(VM&, JSValue) form. Update the unsupported-import-attribute rejection site to match. Still a plain reject (not rejectAsHandled), so unhandledRejection keeps firing per the regression test.
The WebKit upgrade in #32414 changed JSPromise::reject to the 2-arg reject(VM&, JSValue) form. Update the unsupported-import-attribute rejection site to match. Still a plain reject (not rejectAsHandled), so unhandledRejection keeps firing per the regression test.
The WebKit upgrade in #32414 changed JSPromise::reject to the 2-arg reject(VM&, JSValue) form. Update the unsupported-import-attribute rejection site to match. Still a plain reject (not rejectAsHandled), so unhandledRejection keeps firing per the regression test.
The WebKit upgrade in #32414 changed JSPromise::reject to the 2-arg reject(VM&, JSValue) form. Update the unsupported-import-attribute rejection site to match. Still a plain reject (not rejectAsHandled), so unhandledRejection keeps firing per the regression test.
3 participants
Merges upstream WebKit b5ba38a21e17 into oven-sh/WebKit@09f04cd5a489.
Bun-side changes
NodeVM.cpp: compare againstvm.propertyNames->globalThis(added toCommonIdentifiersin the fork) instead of allocatinguid()->utf8(). Upstream's newoperator==(CString, ASCIILiteral)overload made the old comparison ambiguous.Note
CI will fail until the
autobuild-09f04cd5a489b7c0b44aed255bfafce2a316eadarelease finishes building.Notable JSC changes
Spec compliance / correctness
Promise.resolveis no longer folded to identity for Promise subclasses WebKit/WebKit@0f5acb567be0Object.freeze/Object.sealnow reify lazy/special properties before freezing WebKit/WebKit@357d3881fd8dString#replace/replaceAll/splitRegExp paths now perform required observable side effects WebKit/WebKit@eff90f74db57RegExpSearchconstant folding no longer drops theTypeErrorfor non-writablelastIndexWebKit/WebKit@21b46f50939eToBoolean/TypeOfIsUndefined/TypeOfIsFunction/TypeOf/CompareEqdef value no longer keyed on semantic origin (CSE correctness) WebKit/WebKit@c7fe05eb98f6RegExp / Yarr
RegularExpressionwith duplicate named capture groups WebKit/WebKit@e5368156542a\B+ unicode + surrogate producing corruptedJSStringand crashing WebKit/WebKit@116394de195bRegExp.prototype[Symbol.split]moved from builtin JS to C++ WebKit/WebKit@0c18eb164fcaJIT / optimizer
Spreadnow uses a runtime structure check for the fast path instead of requiring a proven AbstractValue WebKit/WebKit@4b8ee8d96bd1StringIteratorPrototype.nextWebKit/WebKit@93518ed198a1for-ofover a closure-capturedconstbinding can now reach DFG/FTL WebKit/WebKit@85309cfe91e8??(nullish coalescing) in condition context WebKit/WebKit@0dde5cddb126Temporal
UCalendarper TimeZoneID, dropucal_clone, useUCAL_TZ_TRANSITION_PREVIOUS_INCLUSIVE, and optimize several Temporal date operations WebKit/WebKit@7830cc6341b5 WebKit/WebKit@ba459bae8fa7 WebKit/WebKit@82b8665ef1c3 WebKit/WebKit@33ca7b8504a9Wasm
OMGIRGenerator::RootBlockWebKit/WebKit@b29dbe254948Runtime / memory
ArrayBufferboolean fields to remove padding WebKit/WebKit@660e785c5128usedRegistersonly onRepatchingPropertyInlineCacheWebKit/WebKit@3429f34cbda2WTF / bmalloc
tagged_bmalloc_heapWebKit/WebKit@3f04665f12e8Borrow<T>constructor is nowlifetimeboundWebKit/WebKit@44c202a8ebfbWTF::FormattedLogging(std::format-based logging helpers, requires GCC ≥ 13) WebKit/WebKit@a2495fb672e2Other
InlineCacheHandlerstatic_assert fix for Windows ARM64#includes, IWYU, inlines moved out of headers)std::optionalinitializers in JSC; libpas xctest timeout bumpobject-view-box, Close Watcher, partitioned cookies, Spatial Backdrop removal, etc.) — not relevant to Bun