Skip to content

Bump WebKit (oven-sh/WebKit#425 preview): coverage of if bodies that only break or continue - #38321

Open
robobun wants to merge 1 commit into
mainfrom
farm/079ec7fc/coverage-folded-break-continue
Open

Bump WebKit (oven-sh/WebKit#425 preview): coverage of if bodies that only break or continue#38321
robobun wants to merge 1 commit into
mainfrom
farm/079ec7fc/coverage-folded-break-continue

Conversation

@robobun

@robobun robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Fixes #20141

Problem

  • bun test --coverage reports an if body that consists only of break; or continue; with the opposite of its real status: a break;/continue; line that ran on every iteration is listed as uncovered, and one that never ran is listed as covered. The while (true) { if (2 === 2) { break; } } function from [With test] Test coverage incorrect #20141 reports its break; line as uncovered.

  • The engine records the wrong block. IfElseNode::emitBytecode (Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp, tryFoldBreakAndContinue) folds such a body into the condition's jump: the condition jumps straight to the break/continue target when true and falls through when false, and the body gets no bytecode of its own. The op_profile_control_flow for the body's text range is still emitted right after the condition, which makes it the first instruction on the fall-through path. With the profiler on, for (...) { if (xs[i]) { continue; } n++; } compiled to:

    jtrue                condition, -> loop increment
    profile_control_flow <range of `{ continue; }`>     <- only reached when the condition is false
    profile_control_flow <range of `n++`>
    
  • src/sourcemap_jsc/CodeCoverage.rs reports what the profiler recorded, so there is nothing to change on the bun side. (test(coverage): attribute basic blocks to the lines of their code, not the lines around them #38282 is about which lines a block is charged to, a separate problem; it does not change this one.)

Fix

  • Do not fold if-break/if-continue when emitting control flow profiler hooks WebKit#425: tryFoldBreakAndContinue returns false while profiler hooks are being emitted, so the body is emitted in full (profile op, jump, and the dead profile op after the jump). That is the shape { f(); continue; } already produces, so nothing after bytecode generation sees anything new, and bytecode generated without the profiler is unchanged (dumped from both shells and compared). trivialTarget() already skips the same fold for the debugger, for the same reason: the statement needs bytecode of its own for the hook to sit in.
  • This PR points WEBKIT_VERSION at that PR's preview build (autobuild-preview-pr-425-01bb96e5) so CI runs against it. Before landing, Do not fold if-break/if-continue when emitting control flow profiler hooks WebKit#425 has to merge and WEBKIT_VERSION has to be repointed at the resulting main sha, which will be the current pin caad865e plus that one commit (the build prints this instruction itself once the preview release disappears). I will push the repin when Bun to an organization project #425 merges.
  • Test: test/cli/test/coverage.test.ts, "coverage of if bodies that only break or continue reflects whether they ran". One fixture function per shape: for-of continue taken and skipped, while break taken, for break skipped, braceless continue, break out of a switch case, and the constant-condition shape from [With test] Test coverage incorrect #20141. For each it asserts the lcov status of the break;/continue; line and of the statement the jump skips. The WebKit PR adds JSTests/controlFlowProfiler/if-break-continue.js, which checks the same shapes plus else branches, labeled targets and do-while at the basic-block level ($vm is not available under bun, so that half stays engine-side).
  • Verification:
    • bun bd --webkit-version=caad865eb1a6e5ca4427f5ea1f066140b11953e7 test test/cli/test/coverage.test.ts (current pin): the new test fails with exactly the seven break;/continue; lines inverted; the seven lines after the jumps are already right.
    • bun bd test test/cli/test/coverage.test.ts (preview pin): all 13 tests pass, existing snapshots unchanged.
    • Both runs repeated with test(coverage): attribute basic blocks to the lines of their code, not the lines around them #38282's src/ changes applied give the same two results, so the test does not care which of the two PRs lands first: it only asserts lines that hold a statement, and the fixture keeps a statement between each asserted line and the closing braces whose attribution test(coverage): attribute basic blocks to the lines of their code, not the lines around them #38282 changes.
    • The JSTest fails on the unpatched prebuilt jsc shell (continueTaken: 'continue;' executed 0 times, expected 3.) and passes, along with the rest of JSTests/controlFlowProfiler, on a local build of the patched engine, with and without the JIT.

Background

  • Bun links a prebuilt JavaScriptCore from oven-sh/WebKit; scripts/build/deps/webkit.ts pins which build. An engine fix lands as a WebKit PR plus a pin bump here, and the autobuild-preview-pr-* tags are how a bump PR runs bun's suite against a WebKit PR before it merges.
  • Control flow profiler: --coverage turns on JSC's control flow profiler. The bytecode generator then emits an op_profile_control_flow at the start of every basic block it knows about, and executing one marks that block as executed.
  • Block text ranges: JSC derives each block's source range as "from this op's text offset up to the next emitted op's text offset" (CodeBlock::insertBasicBlockBoundariesForControlFlowProfiler). The ranges were right here; the op for the body was just placed on the wrong path.
  • Line coverage: CodeCoverage.rs asks JSC for these ranges and marks every line touched by an executed range as hit, so a misplaced op becomes a wrong line status directly.

… path that ran

JSC folds `if (c) { break; }` / `if (c) { continue; }` into the condition's
jump and left the body's op_profile_control_flow on the fall-through path, so
the body's block was recorded as executed exactly when it was skipped. Bun
reports those blocks as line coverage: a break/continue line that ran on
every iteration showed up as uncovered and one that never ran as covered.

The engine fix is oven-sh/WebKit#425 (the fold is skipped while profiler
hooks are being emitted). Pin WEBKIT_VERSION to it and add a coverage test
covering both directions for for-of, while, for, braceless, switch and the
constant-condition shape from #20141.
@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 3 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 105d415b-2274-47bf-a726-9ac3b49adf23

📥 Commits

Reviewing files that changed from the base of the PR and between e697804 and 6225017.

📒 Files selected for processing (2)
  • scripts/build/deps/webkit.ts
  • test/cli/test/coverage.test.ts

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

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 10:05 PM PT - Aug 13th, 2026

@robobun, your commit 6225017 has 2 failures in Build #95288 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 38321

That installs a local version of the PR into your bun-38321 executable, so you can run:

bun-38321 --bun

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Status

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

I reviewed this and didn't find any bugs — the new coverage test is well-constructed and the fixture line numbers check out. Since this is a WebKit engine bump and WEBKIT_VERSION is still pointing at the preview tag (pending oven-sh/WebKit#425 merging and the repin you mentioned), a human should sign off before this lands.

What was reviewed:

  • Verified the 14 asserted line numbers in lib.js map to the intended break;/continue; and skipped-statement lines.
  • lineCoverage helper: drains stdout/stderr/exited concurrently, await using for the subprocess, lcov record lookup by bare filename (no path-separator concerns).
  • prebuiltUrl/prebuiltDestDir already handle autobuild--prefixed tags, so the preview pin resolves correctly.
Extended reasoning...

Overview

This PR bumps WEBKIT_VERSION in scripts/build/deps/webkit.ts from the stable sha caad865e… to the preview tag autobuild-preview-pr-425-01bb96e5, and adds a test in test/cli/test/coverage.test.ts covering lcov line-coverage status for if bodies that contain only break or continue. The actual behavioral fix lives in oven-sh/WebKit#425 (disabling tryFoldBreakAndContinue when the control-flow profiler is on); this PR is the bun-side pin bump plus regression test.

Security risks

None. The change is a dependency version pin plus a hermetic test that spawns bun test --coverage in a temp dir. No new attack surface, no untrusted input handling.

Level of scrutiny

High — any WebKit bump changes the JS engine that all of bun runs on, so even a one-commit bump warrants a maintainer's sign-off and a full CI pass. Additionally, the PR description is explicit that this is not yet in its final landable state: the pin targets an autobuild-preview-pr-* release that will disappear once the upstream PR merges, and the author plans to push a repin to the resulting main sha before landing. Approving now would risk merging a build pinned to an ephemeral artifact.

Other factors

  • The test itself is solid: I hand-counted the fixture and confirmed lines 5/7, 15/17, 25/27, 35/37, 45/46, 55/57, 66/68 are the correct break/continue and skipped-statement pairs. The helper follows harness conventions (tempDir, bunEnv, bunExe, concurrent pipe drain, await using, test.concurrent).
  • scripts/build/deps/webkit.ts already special-cases autobuild--prefixed versions in both prebuiltUrl (uses the tag as-is) and prebuiltDestDir (strips the prefix for the cache key), so the preview pin is handled correctly by existing machinery.
  • The lcov record lookup uses SF:lib.js\n — a bare filename with no directory separator, so no Windows path-normalization concern; consistent with the existing lcov snapshot in the same file.
  • No prior reviews from me on this PR.

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.

[With test] Test coverage incorrect

1 participant