Skip to content

build: WebKit → preview-pr-181-ebf11e28 #29250

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

Closed
Jarred-Sumner wants to merge 4 commits into from
Closed

build: WebKit → preview-pr-181-ebf11e28 #29250

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2c2b1ca
build: WebKit → preview-pr-181-e38dd2fc (Windows libpas/WTF resource …
Jarred-Sumner Apr 13, 2026
6f011fb
build: WebKit → preview-pr-181-68e2ebfa (restore do_mprotect gate in …
Jarred-Sumner Apr 13, 2026
c63706b
build: WebKit → preview-pr-181-4596a6f6 (is_exiting flag for TLC tear…
Jarred-Sumner Apr 13, 2026
7d5ca23
build: WebKit → preview-pr-181-ebf11e28 (fix pthread_once on Windows)
Jarred-Sumner Apr 13, 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
@@ -1,10 +1,10 @@
/**
* WebKit commit — determines prebuilt download URL + what to checkout
* for local mode. Override via `--webkit-version=<hash>` to test a branch.
* From https://github.com/oven-sh/WebKit releases.
*/
export const WEBKIT_VERSION = "42f80a684c5df57121a97e20825a3bcab7a0741b";
export const WEBKIT_VERSION = "preview-pr-181-e38dd2fc";

Check warning on line 6 in scripts/build/deps/webkit.ts

View check run for this annotation

Claude / Claude Code Review

Stale comment: WEBKIT_VERSION is a preview tag, not a commit hash 

The JSDoc comment at the top of `webkit.ts` says "WebKit commit" and `--webkit-version=<hash>`, but `WEBKIT_VERSION` is now `"preview-pr-181-e38dd2fc"` — a named preview tag, not a git commit SHA. The `<hash>` placeholder in the override hint is misleading since preview-style strings are equally valid.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Verify which refs actually exist in oven-sh/WebKit
repo="https://github.com/oven-sh/WebKit"

echo "Checking candidate refs..."
git ls-remote --heads --tags "$repo" \
  "refs/heads/preview-pr-181-e38dd2fc" \
  "refs/tags/preview-pr-181-e38dd2fc" \
  "refs/heads/autobuild-preview-pr-181-e38dd2fc" \
  "refs/tags/autobuild-preview-pr-181-e38dd2fc"

echo
echo "Expected:"
echo "- At least one ref must match what sync-webkit-source.ts passes to 'git checkout'."
echo "- If only autobuild-* exists, current WEBKIT_VERSION may break local sync."

Repository: oven-sh/bun

Length of output: 335

🏁 Script executed:

cat -n scripts/sync-webkit-source.ts | head -30

Repository: oven-sh/bun

Length of output: 1177

🏁 Script executed:

rg -A 10 "function prebuiltUrl\|const prebuiltUrl" scripts/build/deps/webkit.ts

Repository: oven-sh/bun

Length of output: 37

🏁 Script executed:

rg -B 3 -A 3 "git checkout" scripts/sync-webkit-source.ts

Repository: oven-sh/bun

Length of output: 354

🏁 Script executed:

rg "prebuiltUrl" scripts/build/deps/webkit.ts -A 15

Repository: oven-sh/bun

Length of output: 1474

🏁 Script executed:

rg "export.*WEBKIT_VERSION\|autobuild" scripts/build/deps/webkit.ts | head -20

Repository: oven-sh/bun

Length of output: 37

git checkout in sync-webkit-source.ts will fail: only autobuild-preview-pr-181-e38dd2fc exists upstream.

WEBKIT_VERSION = "preview-pr-181-e38dd2fc" works for prebuilt downloads (because prebuiltUrl() auto-prefixes autobuild-), but scripts/sync-webkit-source.ts line 24 checks out WEBKIT_VERSION directly without normalization. Since only the autobuild- prefixed tag exists in oven-sh/WebKit, the checkout will fail. Either store the full ref in WEBKIT_VERSION or teach sync logic to apply the same prefix normalization.

🤖 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 WEBKIT_VERSION constant is
missing the upstream tag prefix, causing the git checkout in the sync logic to
fail; update either the constant or the sync checkout to normalize the ref:
either set WEBKIT_VERSION to the full ref (include the "autobuild-" prefix) or
change the checkout path in the sync code that uses WEBKIT_VERSION (the git
checkout call in the sync-webkit-source.ts checkout logic) to prepend
"autobuild-" when the value doesn't already start with that prefix so both
prebuiltUrl() and the source sync use the same ref.

Copy link
Copy Markdown
Contributor

@claude claude bot Apr 13, 2026

Choose a reason for hiding this comment

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

🟡 The JSDoc comment at the top of webkit.ts says "WebKit commit" and --webkit-version=<hash>, but WEBKIT_VERSION is now "preview-pr-181-e38dd2fc" — a named preview tag, not a git commit SHA. The <hash> placeholder in the override hint is misleading since preview-style strings are equally valid.

Extended reasoning...

What the bug is: The JSDoc block at lines 1–4 of webkit.ts uses two commit-specific terms: (1) "WebKit commit — determines prebuilt download URL + what to checkout" and (2) --webkit-version=<hash>. Both phrases imply the value is always a 40-character hexadecimal git SHA.

The code path that triggers it: WEBKIT_VERSION = "preview-pr-181-e38dd2fc" is a human-readable preview build tag (the actual GitHub release tag is autobuild-preview-pr-181-e38dd2fc, constructed by prebuiltUrl()). It is unambiguously not a commit hash — it contains hyphens and a PR number, not hex digits.

Why existing code doesn't prevent it: The functional code in prebuiltUrl() already handles both formats correctly via the autobuild- prefix logic. Only the documentation is wrong. The comment was presumably written when only SHA values were ever used, and was not updated when the preview-tag format was introduced.

What the impact would be: A developer reading the JSDoc might wrongly conclude that --webkit-version only accepts commit SHAs and that passing a preview tag string like preview-pr-181-e38dd2fc is invalid. This could cause confusion when testing against preview builds or when reviewing future PRs of this type.

How to fix it: Update the JSDoc to say "WebKit version identifier" instead of "WebKit commit", and change <hash> to something like <version> or <hash-or-tag>. Example:

/**
 * WebKit version — determines prebuilt download URL + what to checkout
 * for local mode. Override via `--webkit-version=<version>` to test a branch.
 * From https://github.com/oven-sh/WebKit releases.
 */

Step-by-step proof:

  1. Before this PR: WEBKIT_VERSION = "42f80a684c5df57121a97e20825a3bcab7a0741b" — a 40-char hex SHA. The comment "WebKit commit" and <hash> were accurate.
  2. This PR changes it to "preview-pr-181-e38dd2fc" — contains letters like 'p', 'r', '-' which are not valid hex digits; it is clearly a named tag, not a hash.
  3. The comment at line 1 still says "WebKit commit" — inaccurate.
  4. The comment at line 3 still says --webkit-version=<hash> — the <hash> placeholder no longer describes the actual current value or the valid input format.


Check warning on line 7 in scripts/build/deps/webkit.ts

View check run for this annotation

Claude / Claude Code Review

prebuiltDestDir slice(0,16) causes cache collisions for preview-style version strings 

The `prebuiltDestDir()` function uses `cfg.webkitVersion.slice(0, 16)` to build the cache directory name, but for the new preview-style version strings like `"preview-pr-181-e38dd2fc"` this yields `"preview-pr-181-e"` — capturing only the first character of the 8-char unique commit hash. Two preview builds of the same PR whose hashes share the same leading hex digit (e.g. `preview-pr-181-e38dd2fc` and `preview-pr-181-eabcdef0`) would collide on the same destDir, preventing them from coexisting i
Copy link
Copy Markdown
Contributor

@claude claude bot Apr 13, 2026

Choose a reason for hiding this comment

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

🟡 The prebuiltDestDir() function uses cfg.webkitVersion.slice(0, 16) to build the cache directory name, but for the new preview-style version strings like "preview-pr-181-e38dd2fc" this yields "preview-pr-181-e" — capturing only the first character of the 8-char unique commit hash. Two preview builds of the same PR whose hashes share the same leading hex digit (e.g. preview-pr-181-e38dd2fc and preview-pr-181-eabcdef0) would collide on the same destDir, preventing them from coexisting in cache. Correctness is preserved by the identity field (full version string), but switching between such previews forces a re-download each time.

Extended reasoning...

What the bug is and how it manifests

prebuiltDestDir() at line 85 builds its directory name using cfg.webkitVersion.slice(0, 16). This was designed for 40-char Git SHAs (e.g. 42f80a684c5df57121a97e20825a3bcab7a0741b), where 16 hex characters provide ~64 bits of uniqueness — essentially no collision risk. The new preview version string "preview-pr-181-e38dd2fc" is only 24 characters long, and slice(0, 16) produces "preview-pr-181-e", which includes just the first character (e) of the 8-char commit hash that is the uniquely-differentiating portion.

The specific code path that triggers it

At webkit.ts:85: const version16 = cfg.webkitVersion.slice(0, 16); followed by return resolve(cfg.cacheDir, webkit-${version16}${prebuiltSuffix(cfg)});. For the version string "preview-pr-181-e38dd2fc", the resulting destDir is webkit-preview-pr-181-e (plus any suffix). Any other preview of PR 181 whose commit hash begins with e — approximately a 1-in-16 (~6%) chance for any given pair — gets the same destDir.

Why existing code doesn't prevent it

The identity field at line 179 uses the full version string (${cfg.webkitVersion}${prebuiltSuffix(cfg)}), so the stamp-based correctness check will detect a mismatch and trigger a re-download. This prevents silent ABI corruption. However, the re-download also overwrites the previously cached extraction, meaning the two preview versions can never coexist in cache simultaneously.

What the impact would be

On a shared cacheDir used across multiple machines or CI jobs, switching between two affected preview versions (same PR, hash starting with same character) always forces a fresh download — defeating the caching that destDir is meant to provide. The ~6% per-pair collision probability is low but non-trivial for active development workflows that may compare consecutive preview builds.

How to fix it

Use the full version string (or a hash of it) as the directory key instead of a fixed 16-char prefix: const version16 = cfg.webkitVersion.slice(0, 16) → e.g. const versionKey = cfg.webkitVersion.replace(/[^a-zA-Z0-9_-]/g, '_') or simply use the full string directly since it's already filesystem-safe.

Step-by-step proof

  1. Current PR sets WEBKIT_VERSION = "preview-pr-181-e38dd2fc" (24 chars).
  2. prebuiltDestDir() calls "preview-pr-181-e38dd2fc".slice(0, 16) → "preview-pr-181-e".
  3. destDir becomes $CACHE/webkit-preview-pr-181-e (plus suffix).
  4. A subsequent preview at "preview-pr-181-eabcdef0" also calls "preview-pr-181-eabcdef0".slice(0, 16) → "preview-pr-181-e".
  5. Same destDir → the identity stamp mismatch triggers re-download and overwrites the first extraction.
  6. Switching back to the first preview triggers another re-download — two previews can never coexist.

/**
* WebKit (JavaScriptCore) — the JS engine.
*
Expand Down
Loading