Skip to content

Upgrade WebKit to 456cd04b8d5be123bd451f2c007b97dbb3ffed57 #29161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jarred-Sumner merged 4 commits into from
Apr 12, 2026
Merged

Upgrade WebKit to 456cd04b8d5be123bd451f2c007b97dbb3ffed57 #29161

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2650be5
Upgrade WebKit to 456cd04b8d5be123bd451f2c007b97dbb3ffed57
sosukesuzuki Apr 11, 2026
67a210f
[autofix.ci] apply automated fixes
autofix-ci[bot] Apr 11, 2026
4776017
Bump WEBKIT_VERSION to 42f80a684c5df57121a97e20825a3bcab7a0741b
sosukesuzuki Apr 11, 2026
0293909
Retrigger CI: WebKit autobuild release now complete
sosukesuzuki Apr 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/build/deps/webkit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* for local mode. Override via `--webkit-version=<hash>` to test a branch.
* From https://github.com/oven-sh/WebKit releases.
*/
export const WEBKIT_VERSION = "c2010c47d12c525d36adabe3a17b2eb6ec850960";
export const WEBKIT_VERSION = "456cd04b8d5be123bd451f2c007b97dbb3ffed57";

/**
* WebKit (JavaScriptCore) — the JS engine.
Expand Down
8 changes: 4 additions & 4 deletions src/bun.js/bindings/BunHeapProfiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ WTF::String generateHeapProfile(JSC::VM& vm)
// DFS using explicit stack
WTF::Vector<uint32_t> stackNodes(nodeCount);
WTF::Vector<size_t> stackEdgeIdx(nodeCount);
WTF::Vector<uint8_t> visited(nodeCount, 0);
WTF::Vector<uint8_t> visited(WTF::FillWith {}, nodeCount, static_cast<uint8_t>(0));

uint32_t postOrderIndex = 0;
int stackTop = 0;
Expand Down Expand Up @@ -364,9 +364,9 @@ WTF::String generateHeapProfile(JSC::VM& vm)
uint32_t rootPostOrderIndex = nodeCount - 1;
uint32_t noEntry = nodeCount;

WTF::Vector<uint8_t> affected(nodeCount, 0);
WTF::Vector<uint32_t> dominators(nodeCount, noEntry);
WTF::Vector<uint32_t> nodeOrdinalToDominator(nodeCount, 0);
WTF::Vector<uint8_t> affected(WTF::FillWith {}, nodeCount, static_cast<uint8_t>(0));
WTF::Vector<uint32_t> dominators(WTF::FillWith {}, nodeCount, noEntry);
WTF::Vector<uint32_t> nodeOrdinalToDominator(WTF::FillWith {}, nodeCount, 0u);

// Root dominates itself
dominators[rootPostOrderIndex] = rootPostOrderIndex;
Expand Down
2 changes: 1 addition & 1 deletion src/bun.js/bindings/JSBundlerPlugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -673,7 +673,7 @@ extern "C" void JSBundlerPlugin__drainDeferred(Bun::JSBundlerPlugin* pluginObjec
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 {
Comment on lines 673 to 679
Copy link
Copy Markdown
Contributor

@claude claude Bot Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 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:

  1. Build 1: 3 plugins each call defer() → m_list = [WB(P1), WB(P2), WB(P3)] (size 3)
  2. drainDeferred() → each WB.clear() → m_list = [WB(null), WB(null), WB(null)] (still size 3)
  3. Build 2: 3 new promises appended → m_list = [null, null, null, WB(P4), WB(P5), WB(P6)] (size 6)
  4. After drain: m_list = [null×6]
  5. After N builds: m_list has 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.

Expand Down
10 changes: 5 additions & 5 deletions src/bun.js/bindings/bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4359,7 +4359,7 @@ JSC::EncodedJSValue JSC__JSValue__getIfPropertyExistsFromPath(JSC::EncodedJSValu
uint32_t j = 0;

// if "." is the only character, it will search for an empty string twice.
if (pathString.characterAt(0) == '.') {
if (pathString.codeUnitAt(0) == '.') {
auto* currPropObject = currProp.toObject(globalObject);
RETURN_IF_EXCEPTION(scope, {});
currProp = currPropObject->getIfPropertyExists(globalObject, vm.propertyNames->emptyIdentifier);
Expand All @@ -4370,7 +4370,7 @@ JSC::EncodedJSValue JSC__JSValue__getIfPropertyExistsFromPath(JSC::EncodedJSValu
}

while (i < length) {
char16_t ic = pathString.characterAt(i);
char16_t ic = pathString.codeUnitAt(i);
while (ic == '[' || ic == ']' || ic == '.') {
i += 1;
if (i == length) {
Expand All @@ -4392,7 +4392,7 @@ JSC::EncodedJSValue JSC__JSValue__getIfPropertyExistsFromPath(JSC::EncodedJSValu
}

char16_t previous = ic;
ic = pathString.characterAt(i);
ic = pathString.codeUnitAt(i);
if (previous == '.' && ic == '.') {
auto* currPropObject = currProp.toObject(globalObject);
RETURN_IF_EXCEPTION(scope, {});
Expand All @@ -4406,14 +4406,14 @@ JSC::EncodedJSValue JSC__JSValue__getIfPropertyExistsFromPath(JSC::EncodedJSValu
}

j = i;
char16_t jc = pathString.characterAt(j);
char16_t jc = pathString.codeUnitAt(j);
while (!(jc == '[' || jc == ']' || jc == '.')) {
j += 1;
if (j == length) {
// break and search for property
break;
}
jc = pathString.characterAt(j);
jc = pathString.codeUnitAt(j);
}

String propNameStr = pathString.substring(i, j - i);
Expand Down
18 changes: 9 additions & 9 deletions src/bun.js/bindings/node/http/NodeHTTPParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,31 +494,31 @@ int HTTPParser::onHeadersComplete()
} else {
auto headers = createHeaders(globalObject);
RETURN_IF_EXCEPTION(scope, -1);
args.set(A_HEADERS, headers);
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);
}
Comment on lines +497 to 513
Copy link
Copy Markdown
Contributor

@claude claude Bot Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 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.


args.set(A_VERSION_MAJOR, jsNumber(m_parserData.http_major));
args.set(A_VERSION_MINOR, jsNumber(m_parserData.http_minor));
args.at(A_VERSION_MAJOR) = jsNumber(m_parserData.http_major);
args.at(A_VERSION_MINOR) = jsNumber(m_parserData.http_minor);

bool shouldKeepAlive = llhttp_should_keep_alive(&m_parserData);

args.set(A_SHOULD_KEEP_ALIVE, jsBoolean(shouldKeepAlive));
args.set(A_UPGRADE, jsBoolean(m_parserData.upgrade));
args.at(A_SHOULD_KEEP_ALIVE) = jsBoolean(shouldKeepAlive);
args.at(A_UPGRADE) = jsBoolean(m_parserData.upgrade);

CallData callData = getCallData(onHeadersCompleteCallback);

Expand Down
2 changes: 1 addition & 1 deletion src/bun.js/bindings/v8/shim/InternalFieldObject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class InternalFieldObject : public JSC::JSDestructibleObject {
protected:
InternalFieldObject(JSC::VM& vm, JSC::Structure* structure, int internalFieldCount)
: Base(vm, structure)
, m_fields(internalFieldCount, JSC::WriteBarrier<JSC::Unknown>(vm, this, JSC::jsUndefined()))
, m_fields(WTF::FillWith {}, internalFieldCount, JSC::WriteBarrier<JSC::Unknown>(vm, this, JSC::jsUndefined()))
{
}

Expand Down
4 changes: 2 additions & 2 deletions src/bun.js/bindings/webcore/WebSocket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ ExceptionOr<Ref<WebSocket>> WebSocket::create(ScriptExecutionContext& context, c

ExceptionOr<Ref<WebSocket>> WebSocket::create(ScriptExecutionContext& context, const String& url, const String& protocol)
{
return create(context, url, Vector<String> { 1, protocol });
return create(context, url, Vector<String> { protocol });
}

ExceptionOr<void> WebSocket::connect(const String& url)
Expand All @@ -388,7 +388,7 @@ ExceptionOr<void> WebSocket::connect(const String& url)

ExceptionOr<void> WebSocket::connect(const String& url, const String& protocol)
{
return connect(url, Vector<String> { 1, protocol }, std::nullopt);
return connect(url, Vector<String> { protocol }, std::nullopt);
}

static String resourceName(const URL& url)
Expand Down
4 changes: 2 additions & 2 deletions src/bun.js/modules/NodeModuleModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,9 +517,9 @@ JSC::JSValue resolveLookupPaths(JSC::JSGlobalObject* globalObject, String reques
auto scope = DECLARE_THROW_SCOPE(vm);

// Check for node modules paths.
if (request.characterAt(0) != '.' || (request.length() > 1 && request.characterAt(1) != '.' && request.characterAt(1) != '/' &&
if (request.codeUnitAt(0) != '.' || (request.length() > 1 && request.codeUnitAt(1) != '.' && request.codeUnitAt(1) != '/' &&
#if OS(WINDOWS)
request.characterAt(1) != '\\'
request.codeUnitAt(1) != '\\'
#else
true
#endif
Expand Down
Loading