Skip to content

Bump WebKit to 639550acdcb2: skip preCommitStackMemory before VM entry on Windows - #34669

Merged
Jarred-Sumner merged 2 commits into
mainfrom
farm/0b435ebf/bump-webkit-precommit-fix
Jul 19, 2026
Merged

Bump WebKit to 639550acdcb2: skip preCommitStackMemory before VM entry on Windows#34669
Jarred-Sumner merged 2 commits into
mainfrom
farm/0b435ebf/bump-webkit-precommit-fix

Conversation

@robobun

@robobun robobun commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Bumps WEBKIT_VERSION to oven-sh/WebKit@639550ac to pick up oven-sh/WebKit#310.

Problem

Sentry BUN-2V26 (JSC::preCommitStackMemory): 33k+ events, 100% Windows, 0xC00000FD EXCEPTION_STACK_OVERFLOW at cold process start. 66% are bun build --compile standalone binaries, 64% the baseline dist.

Cause

At VM::VM() time m_stackPointerAtVMEntry is null, so updateStackLimits() computes the soft limit against the full stack reserve (GetCurrentThreadStackLimits). Bun links /STACK:0x1200000 (18MB), so preCommitStackMemory eagerly touches ~17.8MB of stack on every start; on commit-constrained Windows hosts the guard-page grow fails and raises EXCEPTION_STACK_OVERFLOW. The commit is wasted anyway since maxPerThreadStackUsage caps JIT stack usage at 5MB.

Fix

oven-sh/WebKit#310 gates preCommitStackMemory on m_stackPointerAtVMEntry. JSLock::didAcquireLock() sets it and re-runs updateStackLimits() with the 5MB cap applied (and VM::VM() takes a JSLockHolder right after the initial call), so the commit still happens during construction, just bounded to ~5MB instead of ~18MB. Same saving on every Worker thread.

Also in this range (oven-sh/WebKit 4895f45d..639550ac)

  • #306 Drop the ThrowScope from the reifyStaticProperty call sites
  • #304 WTF: use GetSystemTimePreciseAsFileTime for WallTime and QPC for MonotonicTime on Windows
  • #301 Consolidate AsyncLocalStorage save/restore with an RAII AsyncContextSwapScope
  • #298 Dockerfile.windows: enable assertions and disable /GF for the sanitizer build
  • #295 JSMicrotask: keep async context active while settling the async function promise
  • #282 Defer termination at the reifyStaticProperty call sites and propagate builder exceptions

Fixes #27987.


no test proof · iteration 2 · build/CI scripts only; test-proof not applicable

@robobun

robobun commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 7:39 PM PT - Jul 18th, 2026

@robobun, your commit 07cac33 is still building in Build #75592, but has 1 failures so far (All Failures):

@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

WebKit version update

Layer / File(s) Summary
Update WebKit artifact pin
scripts/build/deps/webkit.ts
The exported WEBKIT_VERSION constant now points to a new WebKit commit, changing the prebuilt artifact tag and download identity.

Possibly related PRs

  • oven-sh/bun#34640 — Updates the same WebKit version pin used for prebuilt artifact selection.
  • oven-sh/bun#34655 — Changes the same exported WEBKIT_VERSION constant.
  • oven-sh/bun#34408 — Updates the WebKit prebuilt/autobuild tag through the same constant.

Suggested reviewers: jarred-sumner, sosukesuzuki, dylan-conway

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly matches the main change: bumping WebKit to a specific commit to pick up the Windows stack fix.
Description check ✅ Passed The description covers the purpose, problem, cause, and fix, and includes a verification note, though it lacks the exact template heading.

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The Bun-side diff is a clean one-line WEBKIT_VERSION bump and I found no issues with it, but the pulled-in range carries several non-trivial engine changes (async-context RAII in #301/#295, ThrowScope/termination handling around reifyStaticProperty in #306/#282, Windows WallTime/MonotonicTime source swap in #304) beyond the headline preCommitStackMemory fix — worth a maintainer confirming full-platform CI is green before landing.

What was reviewed:

  • Confirmed the old sha 4895f45d is not duplicated elsewhere in the repo — WEBKIT_VERSION is the single source of truth.
  • New value is a 40-hex merged-main sha (not an autobuild-preview-pr-* tag), so the download.ts preview guard and webkit-prebuilt-url.test.ts format assertion both hold.
  • prebuiltUrl/prebuiltDestDir derive the tag/cache key from the constant, so no other build-script edits are needed.
Extended reasoning...

Overview

Single-line change to scripts/build/deps/webkit.ts bumping WEBKIT_VERSION from 4895f45d… to 639550ac…. The constant feeds prebuiltUrl() (release-tarball download) and prebuiltDestDir() (cache key), and is imported by config.ts and sync-webkit-source.ts. Grep confirms the old sha appears nowhere else, so there are no stale duplicates in Dockerfiles, CI configs, or assertion tables to update.

Security risks

None from the Bun-side diff itself. The upstream range touches JSC internals (exception-scope handling, async-context propagation, Windows time sources), but those were reviewed and merged in oven-sh/WebKit; nothing here changes auth, crypto, or permission surfaces in this repo.

Level of scrutiny

Higher than a typical version bump. While the diff is mechanically trivial, the effective change is a JS-engine upgrade spanning six oven-sh/WebKit PRs, several of which alter runtime-observable behavior (AsyncLocalStorage save/restore consolidation, microtask async-context lifetime, dropping ThrowScope at reifyStaticProperty call sites, QPC-based MonotonicTime on Windows). Per the repo's Dependencies & vendoring guidance, WebKit bumps should be verified against prebuilt-artifact availability for every platform×flavor and full CI before merge — that verification is a maintainer call, not something I can confirm from the diff alone.

Other factors

The new value is a proper 40-hex merged sha, not a preview tag, so the scripts/build/download.ts guard against autobuild-preview-pr-* is satisfied and test/internal/webkit-prebuilt-url.test.ts will continue to pass its format check. No CODEOWNERS-gated paths are touched. No prior claude[bot] review exists on this PR. Deferring so a human can confirm BuildKite #75564 is green across all targets before landing an engine bump of this scope.

@github-actions

Copy link
Copy Markdown
Contributor

Found 6 issues this PR may fix:

  1. Illegal instruction at address 0x7ff6bed2d1a5 #9594 - Windows stack overflow / illegal instruction at cold startup matches the preCommitStackMemory fix that skips eager 18MB stack commit before VM entry
  2. AsyncLocalStorage store is lost during dynamic import() module evaluation #32693 - AsyncLocalStorage store lost during dynamic import() module evaluation, addressed by the JSMicrotask async context fix (Keep async context active while settling the async function promise WebKit#295)
  3. Bun 1.3.14 segfault with Alchemy IaC when AsyncLocalStorage resumes through module top-level await on Linux arm64 #32701 - Segfault when AsyncLocalStorage context resumes through module top-level await, addressed by the async context/microtask fix
  4. Compiling with bytecode will crash when running. #24144 - Bytecode-compiled standalone crash with reifyStaticProperties explicitly in the stack trace, addressed by the ThrowScope fix (Drop the ThrowScope from the reifyStaticProperty call sites WebKit#306, Illegal hardware instruction in wsl2 ubuntu #282)
  5. setInterval() timing way off on Windows #26965 - setInterval() timing way off on Windows (~28ms for 16ms timer), addressed by the Windows timer precision fix (WTF: use GetSystemTimePreciseAsFileTime for WallTime and QPC for MonotonicTime on Windows WebKit#304)
  6. setTimeout(fn, 0) slower to schedule on Windows #16714 - setTimeout(fn, 0) ~15x slower on Windows, references the upstream WebKit bug, addressed by the same timer precision fix

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #9594
Fixes #32693
Fixes #32701
Fixes #24144
Fixes #26965
Fixes #16714

🤖 Generated with Claude Code

@robobun

robobun commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator Author

Re: the issue-finder suggestions above, leaving the PR body at Fixes #27987 only:

@robobun
robobun force-pushed the farm/0b435ebf/bump-webkit-precommit-fix branch from 690ae63 to 80e63a6 Compare July 19, 2026 01:34

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues with the one-line constant change itself, but this bumps the JS engine and pulls in six oven-sh/WebKit PRs (ThrowScope/reifyStaticProperty, AsyncContext, Windows time sources, preCommitStackMemory) whose correctness lives upstream — worth a human confirming CI is green across all platform×flavor combos and that the autobuild-639550ac… prebuilt artifacts exist for every target before merge.

What was reviewed:

  • New value is a 40-hex merged-main SHA, not an autobuild-preview-pr-* tag (download.ts guard would reject those).
  • Grepped for the old hash 4895f45d repo-wide — no stale duplicates in build scripts, Dockerfiles, or assertion tables.
  • WEBKIT_VERSION format satisfies the regex assertion in test/internal/webkit-prebuilt-url.test.ts:104.
Extended reasoning...

Overview

Single-line change to scripts/build/deps/webkit.ts bumping WEBKIT_VERSION from 4895f45d… to 639550ac…. The constant drives the prebuilt tarball URL and the local-mode checkout target. The range picks up six oven-sh/WebKit PRs, headlined by #310 (gate preCommitStackMemory on m_stackPointerAtVMEntry to stop eager ~18MB stack commit on Windows), plus changes to ThrowScope handling at reifyStaticProperty call sites, AsyncContextSwapScope RAII, Windows WallTime/MonotonicTime sources, JSMicrotask async-context lifetime, and termination deferral.

Security risks

None introduced by the diff in this repo. The upstream range does touch exception-scope and termination-deferral machinery in JSC, which are correctness-sensitive, but that review belongs in oven-sh/WebKit.

Level of scrutiny

The in-repo diff is trivially correct as a value substitution: the new hash is a well-formed 40-hex SHA (not a preview tag), WEBKIT_VERSION is defined in exactly one place, and no stale copies of the old hash remain anywhere in the tree. However, per .claude/docs/landing-prs.md (Dependencies & vendoring), engine bumps are gated on verifying prebuilt artifacts exist for every platform × flavor and on green CI across all targets — verification I can't perform from the diff alone. Because this swaps the JS engine underneath the entire runtime and the behavioral surface of the pulled-in changes is broad (async context, exception scopes, Windows timing), a maintainer should sign off after checking BuildKite #75574.

Other factors

The PR description is thorough and correctly scopes Fixes to #27987 only, with an explicit rationale for not claiming the other candidate issues. No test is included, which is expected: the headline fix is a Windows commit-charge-constrained startup crash that isn't reproducible in a hermetic test. The bug-hunting system found nothing (there's essentially nothing to find in a one-line constant).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The one-line pin change itself is fine, but this bump pulls in six oven-sh/WebKit PRs beyond the headline fix — including ThrowScope removal at reifyStaticProperty call sites, the AsyncContextSwapScope refactor, and the Windows WallTime/MonotonicTime clock-source switch. That's engine-level behavior, so a maintainer should confirm CI is green across all platforms (Windows especially) before this lands.

What was reviewed:

  • Verified the new hash is a well-formed 40-hex sha and the constant is the only consumer that derives the prebuilt tag/URL.
  • Checked prebuiltUrl/prebuiltDestDir — no format changes needed for this pin; autobuild- prefixing still applies.
  • No other files reference the old sha 4895f45d… in the repo.
Extended reasoning...

Overview

Single-line change to scripts/build/deps/webkit.ts updating WEBKIT_VERSION from 4895f45d… to 639550ac…. This constant drives the prebuilt WebKit artifact download URL and cache key. No other code in the file changed.

Security risks

None from the diff itself — it's a commit-hash pin to a first-party fork (oven-sh/WebKit). The artifacts are fetched from GitHub Releases on that repo. The pulled-in WebKit changes touch exception handling and stack-limit computation but were reviewed upstream.

Level of scrutiny

The diff is mechanically trivial, but the effective change is a JavaScriptCore engine bump spanning six merged WebKit PRs (preCommitStackMemory gating, ThrowScope removal at reifyStaticProperty, AsyncContextSwapScope RAII refactor, Windows QPC/precise-file-time clock sources, JSMicrotask async-context lifetime, sanitizer Dockerfile tweaks). Several of those affect observable runtime semantics (async context propagation, Windows timer resolution) and exception-scope validation. That's squarely "critical code path" territory — the correctness gate here is the full cross-platform CI matrix, not a code review of a hash literal.

Other factors

  • WebKit bumps are routine (#34640, #34655, #34408 recently did the same), and the substantive review happened in oven-sh/WebKit. Still, a maintainer should be the one to confirm the Windows/macOS/Linux CI is green and pull the trigger — auto-approving an engine bump before CI finishes would be inappropriate.
  • No prior claude[bot] review on this PR, so this is a first-pass deferral, not a repeat.

@robobun

robobun commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator Author

CI status across two runs:

#75574 (278/286 passed): only hard failure was test/js/node/test/parallel/test-net-connect-memleak.js on alpine 3.23 x64 + x64-baseline. It's a FinalizationRegistry timing test (asserts the registry callback fires before the next setImmediate after gc()). Passes 10/10 on a local debug build with this WebKit, and the installed release bun (WebKit c9ad5813, older than main's current pin) also fails it 10/10 locally, so it's timing-sensitive independent of this bump. Did not recur on #75592.

#75592 (retrigger, in progress): only hard failure so far is test/js/node/test/parallel/test-http2-reset-flood.js SIGABRT on one debian x64-asan shard with ExceptionScope::assertNoException. This is the known intermittent from #34414 (pre-existing, surfaced by #32488, fix open). Passed on all 20 asan shards in #75574 with the same WebKit.

The change in this PR (oven-sh/WebKit#310) is gated by #if OS(WINDOWS) and cannot affect either Linux test. The other commits in the bump range (#282/#295/#301/#304/#306) are already merged to oven-sh/WebKit main and were reviewed there. Ready for a maintainer to merge; happy to dig further into either test if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stack overflow crash in JSC::preCommitStackMemory on Windows (v1.3.11)

2 participants