Upgrade WebKit to 456cd04b8d5be123bd451f2c007b97dbb3ffed57#29161
Upgrade WebKit to 456cd04b8d5be123bd451f2c007b97dbb3ffed57#29161sosukesuzuki wants to merge 4 commits intomainfrom
Conversation
Adapt to JSC API changes from upstream d924138e38f4:
- String::characterAt -> codeUnitAt (bindings.cpp, NodeModuleModule.cpp)
- Vector/FixedVector(size, val) -> (FillWith{}, size, val) tag
(BunHeapProfiler.cpp, v8/shim/InternalFieldObject.h, WebSocket.cpp)
- MarkedVector::set() removed -> at() = (NodeHTTPParser.cpp)
- MarkedArgumentBuffer iterator yields JSValue (JSBundlerPlugin.cpp)
JSObject::globalObject() retained as BUN_JSC_ADDITIONS alias for realm()
in the WebKit fork; no mass-rename required here.
|
Updated 9:55 AM PT - Apr 11th, 2026
❌ @sosukesuzuki, your commit 0293909 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29161That installs a local version of the PR into your bun-29161 --bun |
WalkthroughUpdates the pinned WebKit commit and modernizes several C++ bindings: vector initializations use Changes
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| auto& vm = pluginObject->vm(); | ||
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
| for (auto promiseValue : arguments) { | ||
| JSPromise* promise = jsCast<JSPromise*>(JSValue::decode(promiseValue)); | ||
| JSPromise* promise = jsCast<JSPromise*>(promiseValue); | ||
| if (rejected) { | ||
| promise->reject(vm, globalObject, JSC::jsUndefined()); | ||
| } else { |
There was a problem hiding this comment.
🟣 Pre-existing bug: WriteBarrierList::moveTo() (WriteBarrierList.h:44-53) calls value.clear() on each entry but never calls m_list.clear(), leaving null WriteBarrier slots that accumulate over every watch-mode rebuild. The fix is to add m_list.clear() at the end of moveTo().
Extended reasoning...
What the bug is: WriteBarrierList<T>::moveTo() (WriteBarrierList.h:44–53) iterates m_list, appends non-null cells to arguments, and calls value.clear() on each entry — but never removes those entries from m_list itself. After every call, the vector retains all N slots, now holding null WriteBarrier<JSPromise> objects.
How it manifests in watch mode: Each build cycle, JSBundlerPlugin__appendDeferPromise calls deferredPromises.append(), which pushes new JSPromise entries to the end of m_list. After JSBundlerPlugin__drainDeferred calls deferredPromises.moveTo(), those entries are cleared but not removed. The next cycle appends on top of them. Over N build cycles with P deferred promises per build, m_list grows to N × P entries — unboundedly. This is an O(N) slow memory leak in long-running watch sessions.
Why existing code doesn't prevent it: The moveTo() implementation simply does not call m_list.clear() (or any shrink/compact operation) after the loop. The only removal path in this class is takeFirst(), which uses removeAt(0), and removeFirstMatching(). Neither is called during drain.
Secondary breakage — isEmpty() is always false after the first drain: isEmpty() (line 64) returns m_list.isEmpty(), which is never true once entries have been appended and cleared. Any caller checking deferredPromises.isEmpty() to guard work will get a false positive after the first build cycle.
GC overhead: The visit() traversal (line 55–62) iterates every entry in m_list regardless of whether the WriteBarrier holds a live object. After hundreds of builds, the GC visitor walks hundreds of dead null slots on each collection cycle.
Concrete proof — step by step:
- Build 1: 3 plugins each call
defer()→m_list = [WB(P1), WB(P2), WB(P3)](size 3) drainDeferred()→ eachWB.clear()→m_list = [WB(null), WB(null), WB(null)](still size 3)- Build 2: 3 new promises appended →
m_list = [null, null, null, WB(P4), WB(P5), WB(P6)](size 6) - After drain:
m_list = [null×6] - After N builds:
m_listhas 3N null entries
How to fix: Add m_list.clear(); at the end of the moveTo() method (optionally followed by m_list.shrinkToFit() or a capacity cap to avoid keeping large allocations around). This is a three-line change in WriteBarrierList.h.
Relation to this PR: This PR directly modifies JSBundlerPlugin__drainDeferred (the sole caller of deferredPromises.moveTo()) to update the iterator value type from EncodedJSValue to JSValue following the upstream MarkedArgumentBuffer API change. The PR does not introduce or worsen the leak, but it directly touches this code path, making this a good opportunity to fix the underlying issue.
Includes follow-up CI fixes on top of the merge commit: - 6e860391ad34: generate-unified-source-bundles.py Python 3.8 compat - e5adfce303d1: disable NODELETE/annotate_type under clang-cl - 4972e7d6850f + 42f80a684c5d: OFFLINE_ASM_ALIGN_TRAP_512/1024 for Windows ARM64
![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 the current code and only fix it if needed.
Inline comments:
In `@scripts/build/deps/webkit.ts`:
- Line 6: The current WEBKIT_VERSION constant (WEBKIT_VERSION) points to a
release that lacks the LTO assets (bun-webkit-windows-amd64-lto.tar.gz and
bun-webkit-macos-arm64-lto.tar.gz); fix by either rebuilding the referenced
release to include the missing LTO artifacts and republishing with the same
commit hash, or update WEBKIT_VERSION to a prior release hash that contains all
LTO variants so the build picks up complete assets; ensure the chosen
hash/version provides both bun-webkit-windows-amd64-lto.tar.gz and
bun-webkit-macos-arm64-lto.tar.gz before committing the change.
🪄 Autofix (Beta)
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: c13db8d1-0efc-4208-8a26-9fd395040e8e
📒 Files selected for processing (1)
scripts/build/deps/webkit.ts
| * From https://github.com/oven-sh/WebKit releases. | ||
| */ | ||
| export const WEBKIT_VERSION = "c2010c47d12c525d36adabe3a17b2eb6ec850960"; | ||
| export const WEBKIT_VERSION = "42f80a684c5df57121a97e20825a3bcab7a0741b"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TAG="autobuild-42f80a684c5df57121a97e20825a3bcab7a0741b"
API="https://api.github.com/repos/oven-sh/WebKit/releases/tags/${TAG}"
json="$(curl -fsSL "$API")"
echo "Release tag:"
jq -r '.tag_name' <<<"$json"
echo
echo "Published assets:"
jq -r '.assets[].name' <<<"$json" | sort
# Sanity-check common Bun prebuilt targets.
required=(
"bun-webkit-linux-amd64-lto.tar.gz"
"bun-webkit-linux-amd64-musl-lto.tar.gz"
"bun-webkit-linux-arm64-lto.tar.gz"
"bun-webkit-linux-arm64-musl-lto.tar.gz"
"bun-webkit-windows-amd64-lto.tar.gz"
"bun-webkit-macos-arm64-lto.tar.gz"
)
missing=0
for a in "${required[@]}"; do
if ! jq -r '.assets[].name' <<<"$json" | grep -Fxq "$a"; then
echo "MISSING: $a"
missing=1
fi
done
if [ "$missing" -ne 0 ]; then
echo "One or more expected assets are missing."
exit 1
fi
echo "All required assets found."Repository: oven-sh/bun
Length of output: 1411
Remove or rebuild WebKit release with complete LTO assets.
Release autobuild-42f80a684c5df57121a97e20825a3bcab7a0741b exists but is missing:
bun-webkit-windows-amd64-lto.tar.gzbun-webkit-macos-arm64-lto.tar.gz
These prebuilt assets are required for LTO builds on Windows and macOS. Rebuild the release with all LTO variants or revert to a prior version with complete assets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/build/deps/webkit.ts` at line 6, The current WEBKIT_VERSION constant
(WEBKIT_VERSION) points to a release that lacks the LTO assets
(bun-webkit-windows-amd64-lto.tar.gz and bun-webkit-macos-arm64-lto.tar.gz); fix
by either rebuilding the referenced release to include the missing LTO artifacts
and republishing with the same commit hash, or update WEBKIT_VERSION to a prior
release hash that contains all LTO variants so the build picks up complete
assets; ensure the chosen hash/version provides both
bun-webkit-windows-amd64-lto.tar.gz and bun-webkit-macos-arm64-lto.tar.gz before
committing the change.
| args.at(A_HEADERS) = headers; | ||
| if (m_parserData.type == HTTP_REQUEST) { | ||
| args.set(A_URL, m_url.toString(globalObject)); | ||
| args.at(A_URL) = m_url.toString(globalObject); | ||
| } | ||
| } | ||
|
|
||
| m_numFields = 0; | ||
| m_numValues = 0; | ||
|
|
||
| if (m_parserData.type == HTTP_REQUEST) { | ||
| args.set(A_METHOD, jsNumber(m_parserData.method)); | ||
| args.at(A_METHOD) = jsNumber(m_parserData.method); | ||
| } | ||
|
|
||
| if (m_parserData.type == HTTP_RESPONSE) { | ||
| args.set(A_STATUS_CODE, jsNumber(m_parserData.status_code)); | ||
| args.set(A_STATUS_MESSAGE, m_statusMessage.toString(globalObject)); | ||
| args.at(A_STATUS_CODE) = jsNumber(m_parserData.status_code); | ||
| args.at(A_STATUS_MESSAGE) = m_statusMessage.toString(globalObject); | ||
| } |
There was a problem hiding this comment.
🟣 Pre-existing bug: in NodeHTTPParser.cpp onHeadersComplete(), the calls m_url.toString(globalObject) (HTTP_REQUEST path) and m_statusMessage.toString(globalObject) (HTTP_RESPONSE path) lack RETURN_IF_EXCEPTION(scope, -1) guards. If either call throws on OOM while allocating a JSString, execution continues with a pending VM exception and invokes profiledCall() — violating JSC's core invariant. The fix is to add RETURN_IF_EXCEPTION(scope, -1) after each toString() call; this bug predates the PR but the PR touches these exact lines.
Extended reasoning...
What the bug is and how it manifests: In NodeHTTPParser.cpp, onHeadersComplete() uses DECLARE_TOP_EXCEPTION_SCOPE. Two calls — m_url.toString(globalObject) at line 499 (HTTP_REQUEST path) and m_statusMessage.toString(globalObject) at line 512 (HTTP_RESPONSE path) — are not followed by RETURN_IF_EXCEPTION. StringPtr::toString() calls JSC::jsString(vm, WTF::String::fromUTF8(...)), which allocates a JSString on the GC heap. If that allocation fails with OutOfMemory, JSC stores a pending exception in the VM and the call returns a garbage JSValue. Without a guard, execution continues: m_numFields/m_numValues are cleared, further args slots are written, and eventually JSC::profiledCall() is invoked with a pending exception still set — which is undefined behavior in JSC and triggers assertions in debug builds.
The specific code path that triggers it: Under memory pressure, any GC heap allocation can fail with an OutOfMemoryError. In HTTP_REQUEST mode, args.at(A_URL) = m_url.toString(globalObject) stores a garbage value and sets a pending exception. Execution continues through m_numFields = 0 resets, the A_METHOD/A_VERSION_MAJOR/A_VERSION_MINOR/A_SHOULD_KEEP_ALIVE/A_UPGRADE assignments, and then hits JSC::profiledCall() at line 525 with a pending exception still live.
Why existing code does not prevent it: The DECLARE_TOP_EXCEPTION_SCOPE does NOT automatically propagate exceptions — it only asserts in debug builds that no exception is pending at scope boundaries. Protection requires manual RETURN_IF_EXCEPTION calls. Crucially, createHeaders() — called in the same function — uses RETURN_IF_EXCEPTION after every identical toString() call (m_fields[i].toString, m_values[i].toTrimmedString), confirming the codebase treats these as throwable. The omission in onHeadersComplete is an inconsistency, not a deliberate design choice.
Addressing the refutation: One verifier argued that jsString() never sets a JS exception and instead causes a hard crash on OOM. However, JSC's GC heap allocator can and does report OOM as a thrown JavaScript OutOfMemoryError — it does not always crash. The createHeaders() function in the same file guards the same toString() pattern with RETURN_IF_EXCEPTION. If jsString() truly could not throw, those guards would be harmless no-ops; their presence reflects the actual JSC semantics. The flush() function at the bottom of the file appends m_url.toString(globalObject) to args without a guard, which is the same bug.
Step-by-step proof: (1) HTTP request arrives; onHeadersComplete() is called. (2) m_haveFlushed is false, so createHeaders() succeeds and args.at(A_HEADERS) is set. (3) m_parserData.type == HTTP_REQUEST, so args.at(A_URL) = m_url.toString(globalObject) executes. (4) Under OOM, jsString() fails, sets an OutOfMemoryError on the VM, and returns a garbage JSValue — no guard catches this. (5) Execution falls through: m_numFields = 0; m_numValues = 0; args.at(A_METHOD) = jsNumber(...); additional slots filled. (6) JSC::profiledCall() is invoked with a pending exception. Debug builds ASSERT; release builds have undefined behavior.
How to fix it: Add RETURN_IF_EXCEPTION(scope, -1) immediately after args.at(A_URL) = m_url.toString(globalObject) (line 499) and after args.at(A_STATUS_MESSAGE) = m_statusMessage.toString(globalObject) (line 512). The flush() function also appends m_url.toString(globalObject) without a guard and should be fixed in the same pass.
2 participants
WebKit Upgrade: c2010c47d12c → d924138e38f4
Merged 1316 upstream commits (306 touching JSC/WTF/bmalloc).
API Changes Affecting Bun
JSObject::globalObject()→realm()(7d4583947a7b "Realm-less Objects")realm(). Bun'sJSPromise.cppInternalPromise check andJSWebAssembly.cppupdated.MarkedJSValueRefArray→MarkedVector<JSValueRef>JSCell::type()/indexingType*()now defined inline inJSCell.h(6eaeb2be1205)JS_EXPORT_PRIVATEontype()dropped — symbol no longer needed since definition is header-inline.Heap::hasHeapAccess()/worldIsStopped()moved fromHeapInlines.h→Heap.hisMarkedblock kept; duplicate inline defs removed.NODELETEannotations adopted widely (61dfe117a295)JS_EXPORT_PRIVATEadditions where both apply.GCMemoryOperations.hrewritten (7a412e324d10 "wider bulk copy")#if !OS(WINDOWS)guard around.p2align, superseding Bun's separate Windows-ARM64 SEH workaround. Took upstream wholesale.WTF_MAKE_TZONE_ALLOCATED_IMPL(ScheduledTask)added inRunLoopGeneric.cppUSE(BUN_EVENT_LOOP)constructor.OSAllocatorPOSIX.cpp: LinuxPR_SET_VMAmemory tagging (b651d820ecaa)BUN_VM_CHILD_PROCESS_INHERITdefines; dropped Bun's file-wideWTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN(upstream now uses targeted scopes).trunc/isnan→std::trunc/std::isnan(aaf28d392ef7)JSCJSValueInlines.h.JSCell::toThisremoved upstreamJS_EXPORT_PRIVATEdeclaration dropped (no implementation existed).MarkedVector::set(i, v)removedNodeHTTPParser.cppswitched toargs.at(i) = v.MarkedArgumentBufferiterator now yieldsJSValue(notEncodedJSValue) —JSBundlerPlugin.cppupdated.Vector(size, val)/FixedVector(size, val)now requireWTF::FillWith{}tag (22a4dd063a31)BunHeapProfiler.cpp,InternalFieldObject.h,WebSocket.cppupdated.String::characterAt()→codeUnitAt()(e4f1bb7dd2dc)bindings.cpp,NodeModuleModule.cppupdated.RootMarkReason::MarkedJSValueRefArrayenum value removedBunV8HeapSnapshotBuilder.cppswitch case dropped.InlineCacheHandler.hnew transitive include ofInlineCacheCompiler.hPRIVATE_FRAMEWORK_HEADERSin JSCCMakeLists.txt.Compatibility Shim Added (oven-sh/WebKit only)
JSObject::globalObject()andStructure::globalObject()were re-introduced underUSE(BUN_JSC_ADDITIONS)as inline aliases ofrealmMayBeNull()/realm(). This avoids a 300+-call-site rename across Bun's bindings and codegen templates in this PR; the alias preserves the pre-rename nullable semantics. Migration torealm()can be done incrementally.Notable JSC Commits
Performance
ArrayLengthStoreIC forarray.length = Nundefined/true/false/nulltoUpperCaseintrinsicString#indexOfone-char fast path in DFG/FTLmovin await/yield bytecodeCheckPrivateBrand/SetPrivateBrandequalIgnoringASCIICase*makeLatin1Identifierin parseString SIMD fast pathCorrectness / Bugfixes
ToLength(lastIndex)CheckIsConstantisWithinPowerOfTwounsound forBitAndw/ negative maskInstanceFieldEvalContextthrough arrows/nested scopesimport { "*" as x }not treated as namespace importDate.toLocaleString()ICU crash from WorkerNew Features
Iterator.prototype.includes(behind flag)WebAssembly.Suspending/SuspendErroras data propertiesWasm
AboveEqualWTF / bmalloc
characterAt()/characterStartingAt()→codeUnitAt()/codePointAt()WTF::Unexpected→std::unexpected#includes from WTFStringImpl::create(UTF-8) defensive against concurrent mutation= defaultconstructor adoptionroundUpToPowerOfTwo()no longer truncates to uint32_tMerge Conflict Resolutions
28 conflicts. Patterns:
"X.h"↔<JavaScriptCore/X.h>— adopted upstream bracket style (BlockDirectory.h, MarkedBlock.h, StrongInlines.h, Integrity.h, InPlaceInterpreter.h, WasmDebugServerUtilities.h, WebAssemblyGCStructure.h).NODELETE× Bun'sJS_EXPORT_PRIVATE/additions — combined both (MarkingConstraint.h, JSCell.h, VMManager.h, Nodes.h, SourceProvider.h, LiteralParser.h, CachedTypes.cpp, ExceptionHelpers.cpp, NodesCodegen.cpp, Options.cpp).growthMode/maxEdenSizeForRateLimiting(rate-limiting) and de-statics heap-sizing helpers; kept Bun's behavior, dropped upstream'sNODELETEon the removed functions.JSCJSValueInlines.h; kept Bun's relocation, portedstd::qualification fix.realm()rename; kept Bun's InternalPromise short-circuit.WTF_MAKE_TZONE_ALLOCATED_IMPLbefore Bun'sUSE(BUN_EVENT_LOOP)ctor.prctlblock; dropped redundant file-wide unsafe-buffer wrap.platform/mac/StringUtilities.hremoved upstream (moved toplatform/cocoa/).Checks
JSType.h: no upstream changes —src/bun.js/bindings/JSType.zigstill in sync.