Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
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 = "549170099226f816a4b204ea1d8fa102fb79eefa";
export const WEBKIT_VERSION = "autobuild-preview-pr-353-4b51ec68";

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

View check run for this annotation

Claude / Claude Code Review

WEBKIT_VERSION points to unmerged preview build

`WEBKIT_VERSION` is pointing at `autobuild-preview-pr-353-4b51ec68`, an ephemeral preview-build release for the still-unmerged oven-sh/WebKit#353. Preview releases are deleted once the upstream PR merges/closes, at which point every fresh prebuilt-mode build (CI and local) will 404 on the WebKit download. Per the description this is a known TODO — flagging so it doesn't slip: bump to the merged commit sha before landing.

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.

🔴 WEBKIT_VERSION is pointing at autobuild-preview-pr-353-4b51ec68, an ephemeral preview-build release for the still-unmerged oven-sh/WebKit#353. Preview releases are deleted once the upstream PR merges/closes, at which point every fresh prebuilt-mode build (CI and local) will 404 on the WebKit download. Per the description this is a known TODO — flagging so it doesn't slip: bump to the merged commit sha before landing.

Extended reasoning...

What the bug is

scripts/build/deps/webkit.ts:6 sets:

export const WEBKIT_VERSION = "autobuild-preview-pr-353-4b51ec68";

replacing the previous 40-hex commit sha. The autobuild-preview-pr-* tag is the CI preview release that oven-sh/WebKit publishes for open PRs so downstream Bun PRs can test against them before the WebKit change lands. These preview releases are ephemeral — they are deleted (or become stale/unreferenced) once the WebKit PR is merged or closed and the real autobuild-<sha> release is published for the merged commit.

Code path that triggers it

prebuiltUrl() in the same file builds the download URL directly from this constant:

const version = cfg.webkitVersion;
const tag = version.startsWith("autobuild-") ? version : `autobuild-${version}`;
return `https://github.com/oven-sh/WebKit/releases/download/${tag}/${name}.tar.gz`;

so every cfg.webkit === "prebuilt" build — which is the default for CI and for anyone not running a local WebKit checkout — fetches https://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-353-4b51ec68/bun-webkit-<os>-<arch><suffix>.tar.gz. prebuiltDestDir() also keys the cache dir on this string, so there is no fallback to a previously-cached sha.

Why nothing else prevents it

There is no guard in the build system that rejects preview tags or falls back to a pinned release; prebuiltUrl() explicitly accommodates the autobuild- prefix so preview tags work during development. The only thing preventing this from breaking builds today is that oven-sh/WebKit#353 is still open and its preview artifacts still exist.

Impact

If this PR merges as-is and oven-sh/WebKit#353 subsequently merges (which it must, since this PR depends on it), the preview release will be cleaned up. From that point every fresh clone / cache-cold CI runner hits a 404 downloading WebKit and the build fails outright — a regression in something that currently works on main. Even before deletion, pinning main to an unmerged, force-pushable PR branch is fragile.

Step-by-step proof

  1. Merge this PR with WEBKIT_VERSION = "autobuild-preview-pr-353-4b51ec68".
  2. JSBigInt::parseInt: O(n) fast path for power-of-two radix WebKit#353 merges; its preview release autobuild-preview-pr-353-4b51ec68 is deleted per the preview-release lifecycle.
  3. A contributor (or CI) with a cold cache runs bun bd. resolveDep for WebKit computes prebuiltUrl(cfg).../releases/download/autobuild-preview-pr-353-4b51ec68/bun-webkit-linux-amd64.tar.gz.
  4. GitHub returns 404; the fetch step fails; the build aborts before compiling anything.
  5. Every subsequent PR's CI is red until someone lands a follow-up bumping WEBKIT_VERSION.

Fix

Before merging, land oven-sh/WebKit#353 first, then update this line to the merged commit's 40-hex sha (matching the autobuild-<sha> release), e.g.:

export const WEBKIT_VERSION = "<merged-commit-sha>";

The PR description already states this intent ("will be updated to the merged sha once oven-sh/WebKit#353 lands"); this comment is the merge-blocking reminder so it can't be forgotten.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, intentional and tracked: PR is in draft until oven-sh/WebKit#353 lands, then this line gets swapped to the merged 40-hex sha. Leaving this thread open as the merge-blocker.


/**
* WebKit (JavaScriptCore) — the JS engine.
Expand Down
93 changes: 93 additions & 0 deletions test/js/bun/jsc/bigint-parse.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
import { test, expect, describe } from "bun:test";

// JSBigInt::parseInt historically routed every radix through a loop that
// calls multiplyAdd over the full-length digit vector for each small group
// of characters, which is O(n^2) in the number of characters. For
// power-of-two radixes (0b/0o/0x prefixes) the parse is a straight bit-pack
// and should be O(n), matching toStringBasePowerOfTwo in the other direction.

const rep = (c: string, n: number) => Buffer.alloc(n, c).toString();

describe("BigInt string parse, power-of-two radix", () => {
const roundtrip = (prefix: string, radix: number, body: string) => {
const v = BigInt(prefix + body);
expect(v.toString(radix)).toBe(body.toLowerCase().replace(/^0+(?=.)/, ""));
return v;
};

test("hex correctness", () => {
roundtrip("0x", 16, "1");
roundtrip("0x", 16, "F");
roundtrip("0x", 16, "DeadBeef");
roundtrip("0x", 16, rep("ff", 8));
roundtrip("0x", 16, rep("ff", 9));
roundtrip("0x", 16, "1" + rep("0", 100));
roundtrip("0x", 16, rep("f", 1000));
roundtrip("0x", 16, rep("123456789abcdef0", 200));
Comment thread
robobun marked this conversation as resolved.
Outdated
// Non-16-aligned lengths to hit partial high digits.
for (let n = 1; n <= 40; n++) roundtrip("0x", 16, rep("a", n));
});

test("binary correctness", () => {
roundtrip("0b", 2, "1");
roundtrip("0b", 2, rep("1", 63));
roundtrip("0b", 2, rep("1", 64));
roundtrip("0b", 2, rep("1", 65));
roundtrip("0b", 2, rep("1", 1000));
roundtrip("0b", 2, rep("10", 500));
for (let n = 1; n <= 130; n++) roundtrip("0b", 2, rep("1", n));
});

test("octal correctness (3 bits/char, spans digit boundaries)", () => {
roundtrip("0o", 8, "7");
roundtrip("0o", 8, rep("7", 21));
roundtrip("0o", 8, rep("7", 22));
roundtrip("0o", 8, rep("7", 23));
roundtrip("0o", 8, rep("1234567", 200));
roundtrip("0o", 8, rep("7", 1000));
// The msb of the leading char can land in the low bits of the next
// 64-bit word, leaving that word zero. Exercise every alignment.
for (let n = 1; n <= 70; n++) roundtrip("0o", 8, rep("1", n));
for (let n = 1; n <= 70; n++) roundtrip("0o", 8, rep("7", n));
});

test("cross-radix agreement on large values", () => {
const hex = rep("f", 4096);
const v16 = BigInt("0x" + hex);
const v2 = BigInt("0b" + rep("1", 4096 * 4));
expect(v16 === v2).toBe(true);
expect(v16.toString(16)).toBe(hex);
expect(v16).toBe((1n << BigInt(4096 * 4)) - 1n);
});

test("leading zeros and whitespace still handled", () => {
expect(BigInt("0x" + rep("0", 1000) + "ff")).toBe(255n);
expect(BigInt(" 0x" + rep("f", 100) + " ")).toBe(BigInt("0x" + rep("f", 100)));
expect(BigInt("0x" + rep("0", 1000))).toBe(0n);
});

test("invalid characters still throw SyntaxError", () => {
expect(() => BigInt("0x" + rep("f", 1000) + "g")).toThrow(SyntaxError);
expect(() => BigInt("0b" + rep("1", 1000) + "2")).toThrow(SyntaxError);
expect(() => BigInt("0o" + rep("7", 1000) + "8")).toThrow(SyntaxError);
expect(() => BigInt("0xg" + rep("f", 1000))).toThrow(SyntaxError);
});

test("parse is linear, not quadratic", () => {
// JSC caps BigInt at 2^20 bits (262144 hex chars). Use 250000, large
// enough that the O(n^2) path is unmistakably slow on any build while the
// O(n) path stays well under the threshold even under debug+ASAN.
// Quadratic: ~570ms release, seconds under debug. Linear: a few ms
// release, tens of ms debug.
const n = 250_000;
const s = "0x" + rep("f", n);
const t0 = performance.now();
const v = BigInt(s);
const parseMs = performance.now() - t0;

// Correctness check on the same value.
expect(v.toString(16)).toBe(rep("f", n));

expect(parseMs).toBeLessThan(250);
Comment thread
robobun marked this conversation as resolved.
Outdated
});
});
Loading