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 @@ -8,7 +8,7 @@
// importing), every x64 at the nehalem floor (no separate -baseline variant),
// typed-array constructor ClassInfo kept address-unique under LTO, and the
// Windows ICU data table filtered + per-item zstd compressed.
export const WEBKIT_VERSION = "c9296e353e365ecf0de82f273bb0a88a3df465be";
export const WEBKIT_VERSION = "autobuild-preview-pr-317-d9b06a28";

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

View check run for this annotation

Claude / Claude Code Review

WEBKIT_VERSION pinned to ephemeral preview tag

`WEBKIT_VERSION` is pinned to `autobuild-preview-pr-317-d9b06a28`, a PR-preview release tag rather than a merged `oven-sh/WebKit` main sha (contradicting the comment directly above). Preview-PR release artifacts can be garbage-collected once WebKit#317 merges/closes, at which point `prebuiltUrl()` will 404 and fresh clones of Bun stop building. The PR description already notes this needs updating — flagging so it isn't merged before the pin moves to the merged main sha.

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 pinned to autobuild-preview-pr-317-d9b06a28, a PR-preview release tag rather than a merged oven-sh/WebKit main sha (contradicting the comment directly above). Preview-PR release artifacts can be garbage-collected once WebKit#317 merges/closes, at which point prebuiltUrl() will 404 and fresh clones of Bun stop building. The PR description already notes this needs updating — flagging so it isn't merged before the pin moves to the merged main sha.

Extended reasoning...

What the bug is

WEBKIT_VERSION in scripts/build/deps/webkit.ts:11 is set to "autobuild-preview-pr-317-d9b06a28", a CI preview tag produced for oven-sh/WebKit#317, rather than a commit sha on oven-sh/WebKit main. The comment block directly above the constant documents the invariant this violates: // oven-sh/WebKit main: macOS + Windows artifacts cross-compiled on Linux, .... The PR description acknowledges this is temporary ("Depends on oven-sh/WebKit#317. WEBKIT_VERSION points at its preview tag; once that PR is merged the pin should move to the merged main sha"), so the intent is clear — this comment is a merge-gate reminder, not a disagreement.

Code path that triggers it

prebuiltUrl(cfg) constructs the download URL directly from cfg.webkitVersion:

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

and prebuiltDestDir(cfg) keys the extraction cache on the same value. The build system does branch on the autobuild- prefix, so the preview tag works today — the tarball resolves and extracts correctly. The problem is durability, not correctness-right-now.

Why nothing prevents it

Nothing in scripts/build/deps/webkit.ts validates that WEBKIT_VERSION points at a stable release. The autobuild- handling in prebuiltUrl/prebuiltDestDir was added precisely so preview tags can be tested locally via --webkit-version=<tag>, but committing one as the default pin means every consumer of cfg.webkit === "prebuilt" (the default mode — local mode requires a manual 10+ min clone) depends on that release existing.

Impact

Preview-PR release artifacts on oven-sh/WebKit are ephemeral: once PR #317 merges or closes, its autobuild-preview-pr-317-* release can be pruned by the release workflow. After that point:

  1. A developer runs bun bd on a fresh clone (or after rm -rf build/ clears the cache dir).
  2. resolveDep for WebKit hits the prebuilt branch and calls prebuiltUrl(cfg)https://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-317-d9b06a28/bun-webkit-linux-amd64-debug.tar.gz.
  3. GitHub returns 404; the build fails before compiling a single source file.

Anyone with the tarball already cached (identity autobuild-preview-pr-317-d9b06a28-debug) is unaffected, which is exactly why this class of pin tends to slip through — it works on the author's and reviewers' machines and only breaks for new contributors / fresh CI runners later.

Step-by-step proof

  1. WEBKIT_VERSION = "autobuild-preview-pr-317-d9b06a28" (line 11).
  2. Config.webkitVersion defaults to WEBKIT_VERSION unless overridden by --webkit-version.
  3. Default cfg.webkit is "prebuilt", so webkit.source(cfg) returns { kind: "prebuilt", url: prebuiltUrl(cfg), identity: "autobuild-preview-pr-317-d9b06a28-debug", destDir: ... }.
  4. prebuiltUrl sees version.startsWith("autobuild-") → uses the tag verbatim → URL points at the autobuild-preview-pr-317-d9b06a28 GitHub release.
  5. That release exists only while the preview workflow keeps it. Once LiteralParser: throw RangeError on OOM when copying a JSON string value WebKit#317 merges and preview releases are cleaned up, step 4's URL 404s and bun bd fails at dependency fetch.

How to fix

Before merging this PR, land oven-sh/WebKit#317 and update line 11 to the resulting 40-hex main sha (matching the format of the previous pin c9296e353e365ecf0de82f273bb0a88a3df465be and the // oven-sh/WebKit main comment above it). No other change needed — prebuiltUrl/prebuiltDestDir already handle both forms.

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.

Agreed, and the PR body already calls this out as the merge-gate step. Leaving this thread open as the reminder: once oven-sh/WebKit#317 lands on main this line moves to the merged sha before this PR is merged.


/**
* WebKit (JavaScriptCore) — the JS engine.
Expand Down
49 changes: 49 additions & 0 deletions test/js/bun/util/json-parse-oom-fixture.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

69 changes: 69 additions & 0 deletions test/js/bun/util/json-parse-oom.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe, isASAN, isLinux } from "harness";
import { join } from "node:path";

// JSON.parse copies every string value longer than 16 chars into a fresh
// StringImpl. That copy used the crash-or-succeed StringImplMalloc::malloc
// path, so a near-OOM process died with SIGILL inside fastCompactMalloc
// instead of throwing. The fix routes through StringImpl::tryCreateUninitialized
// and surfaces a RangeError, same as "x".repeat(tooLarge).
//
// RLIMIT_AS is the only portable way to make tryMalloc actually fail, and it
// cannot be combined with AddressSanitizer's 16 TB shadow reservation, so this
// test runs on non-ASAN Linux only.
test.skipIf(!isLinux || isASAN)(
"JSON.parse throws RangeError instead of crashing when a string value cannot be allocated",
async () => {
const fixture = join(import.meta.dir, "json-parse-oom-fixture.js");
const limitKiB = 5 * 1024 * 1024;
// A single size/limit can miss the window on a given machine's address-space
// layout. Sweep a few sizes; every run that reaches INPUT-OK must either
// succeed or throw RangeError, never crash.
const sizes = [200, 300, 400, 500].map(mb => mb * 1024 * 1024);
let sawCaught = false;
let sawInputOK = false;

for (const size of sizes) {
await using proc = Bun.spawn({
cmd: [
"/bin/sh",
"-c",
`ulimit -v ${limitKiB} && ulimit -c 0 && exec "$0" "$1" "$2"`,
bunExe(),
fixture,
String(size),
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

const reachedParse = stdout.includes("INPUT-OK");
if (!reachedParse) continue;
sawInputOK = true;

// Once the input is built, JSON.parse must not kill the process.
expect({ size, stdout: stdout.trim(), stderr: stderr.trim(), exitCode, signal: proc.signalCode }).toMatchObject({
size,
signal: null,
});
expect([0, 1]).toContain(exitCode);

if (stdout.includes("CAUGHT:")) {
expect(stdout).toContain("CAUGHT:RangeError:Out of memory");
sawCaught = true;
} else {
expect(stdout).toContain("PARSED");
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}

// The sweep has to actually reach JSON.parse at least once; otherwise the
// address-space cap was too tight and nothing was exercised.
expect(sawInputOK).toBe(true);
// And at least one of those runs must have taken the out-of-memory branch,
// otherwise the sweep never exercised the path this test is for.
expect(sawCaught).toBe(true);
},
30_000,

Check warning on line 68 in test/js/bun/util/json-parse-oom.test.ts

View check run for this annotation

Claude / Claude Code Review

Explicit 30s per-test timeout violates test/CLAUDE.md

nit: `test/CLAUDE.md:120` says "**CRITICAL**: Do not set a timeout on tests. Bun already has timeouts", and REVIEW.md says to shrink the workload rather than raise per-test timeouts. Consider dropping the explicit `30_000` and reducing the sweep — e.g. two sizes instead of four, and/or a lower `limitKiB` (2–3 GiB) so each fixture has less address space to fill before the halving loop bottoms out.
Comment thread
robobun marked this conversation as resolved.
Outdated
);
Loading