Fix JSC sampling profiler segfault walking the stack with a null entry frame (WebKit pin bump) - #37176
Fix JSC sampling profiler segfault walking the stack with a null entry frame (WebKit pin bump)#37176robobun wants to merge 4 commits into
Conversation
…frame The JSC sampling profiler could crash at 0xFFFFFFFFFFFFFFC8 when a sample landed inside a VM entry/exit transition: vm.topEntryFrame is null there while vm.entryScope is already set, and a walked frame whose caller slot read null made the stack walker dereference vmEntryRecord(nullptr). Seen in test-cpu-prof-dir-worker.js on the Windows 2019 CI runners. Picks up oven-sh/WebKit#395 (currently via its preview build; to be repinned to the merge commit) and adds a --cpu-prof test that drives the raced window: callbacks with huge declared parameter counts called from native spend most of their runtime in doVMEntry's argument pad loop, which runs before topEntryFrame is stored.
WalkthroughChangesThe PR updates the WebKit build identifier and adds CPU profiler regression coverage for callbacks with 2,000 parameters across multiple asynchronous scheduling mechanisms. WebKit build pin
CPU profiler regression coverage
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build/deps/webkit.ts`:
- Line 6: Update the WEBKIT_VERSION constant to an existing WebKit tag with a
valid release and downloadable bun-webkit-* archives, replacing the unresolved
autobuild-preview-pr-395-8ec8fb6a value.
In `@test/cli/run/cpu-prof.test.ts`:
- Around line 491-494: Strengthen the CPU profiler test around the existing
process result assertions by reading the generated .cpuprofile, parsing its
profile data, and asserting profile.samples.length > 0 before asserting exitCode
is 0. Keep the existing stdout assertion and ensure the sample-count assertion
fails when --cpu-prof produces no samples.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 27aaaf00-a7e7-42f9-a90a-109cba599ae8
📒 Files selected for processing (2)
scripts/build/deps/webkit.tstest/cli/run/cpu-prof.test.ts
|
Addressed the review: the test now parses the generated .cpuprofile and requires profile.samples.length > 0 (eef9903). The webkit.ts pin finding is the documented preview-build timing: the oven-sh/WebKit#395 preview build is still running, and the pin will be updated to the merged main sha once that PR lands; that thread stays open as the reminder. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Deferring to a maintainer because WEBKIT_VERSION bumps pull in JS-engine changes (this one also carries oven-sh/WebKit#391 per the description), and the pin is still on the autobuild-preview-pr-395-* tag that the author notes will be repinned to the merged sha before landing.
Checked: the new test follows the file's existing harness conventions (tempDir, bunEnv, concurrent describe, exit-code asserted last), the 150ms deadline is a bounded workload rather than a sleep-then-check, and prebuiltUrl/prebuiltDestDir already handle autobuild--prefixed version strings so the build script accepts the new pin shape.
Extended reasoning...
Overview
Two files: a one-line change to WEBKIT_VERSION in scripts/build/deps/webkit.ts (from a 40-hex sha to autobuild-preview-pr-395-8ec8fb6a), and a new ~55-line test in test/cli/run/cpu-prof.test.ts that stress-tests the JSC sampling profiler against VM entry/exit churn. The actual fix lives in oven-sh/WebKit#395, not in this repo — this PR is the pin bump plus regression coverage.
Security risks
None identified. The test spawns a local subprocess with bunEnv in a temp dir and reads back a file it created; no network, no untrusted input parsing, no auth/crypto surface. The build-script change only alters which prebuilt tarball URL is computed.
Level of scrutiny
High, despite the small diff. WEBKIT_VERSION controls which JavaScriptCore build every Bun binary links against, and the pinned preview branch sits on WebKit main and picks up at least one other fix (oven-sh/WebKit#391). That is a dependency bump whose real payload is outside this diff, and the author explicitly flags it as temporary — the pin is meant to move to the merged main sha once the upstream PR lands. Merging as-is would ship a preview-tag pin.
Other factors
The new test is well-constructed: it matches neighboring tests' patterns, uses a time-bounded workload (not a sleep), asserts stdout before exit code, and after the follow-up commit asserts profile.samples.length > 0 so it can't vacuously pass. The PR description is candid that there is no fail-before repro locally — the test pins the widened-window workload for CI runners where the crash did fire. prebuiltUrl() and prebuiltDestDir() already special-case autobuild- prefixes, so the non-sha version string is handled. All prior review threads (CodeRabbit) are resolved. Given the pending repin and the engine-level scope of any WebKit bump, a maintainer should sign off rather than an automated approval.
process.nextTick kept the tick queue non-empty, which drains the microtask jobs nested inside the tick-drain VM entry instead of as outermost entries, demoting two of the churn legs away from the raced state. And on Windows the sampler ticks at the ~15.6ms timer quantum while only in-entry-scope samples are recorded, so a 150ms window expects about one recorded sample; give Windows 1.5s so the samples assertion cannot flake to zero.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/run/cpu-prof.test.ts`:
- Around line 448-466: Shorten the comment above the raced-path smoke test to
document only durable constraints: the workload widens the VM-entry sampling
window, and the timing/deadline values account for Windows sampler granularity
and recorded-sample frequency. Remove the historical crash address, CI filename,
detailed VM frame-walk sequence, and other implementation-history details; rely
on the existing test code to explain the callback fanout.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a900c722-45f7-42f1-9532-3bb2ca3beecd
📒 Files selected for processing (1)
test/cli/run/cpu-prof.test.ts
|
Updated 9:11 PM PT - Aug 7th, 2026
✅ @robobun, your commit 4d7472b64e5b953e044355d22f362a27c25a9768 passed in 🧪 To try this PR locally: bunx bun-pr 37176That installs a local version of the PR into your bun-37176 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs in the test addition. Deferring to a human because WEBKIT_VERSION is still pinned to the preview-build tag autobuild-preview-pr-395-8ec8fb6a (the description says it will be repinned to the merge commit once oven-sh/WebKit#395 lands), and a WebKit bump pulls in more than the one-line diff suggests — including oven-sh/WebKit#391 per the description, which also needs coordinating with #34655.
What was reviewed
- New
sampler survives VM entry churntest: follows harness conventions (tempDir,bunEnv,describe.concurrent), drains stdout before awaiting exit, asserts stdout → profile samples → exitCode in order. - Checked the fixture terminates on its own (deadline-bounded loop, ports closed) and that
stderr: "inherit"avoids the pipe-buffer deadlock concern. - Confirmed the
samples.length > 0assertion and Windows 1.5s window were added/adjusted per prior review feedback.
Extended reasoning...
Overview
Two files: scripts/build/deps/webkit.ts (one-line WEBKIT_VERSION change from a 40-hex sha to autobuild-preview-pr-395-8ec8fb6a) and test/cli/run/cpu-prof.test.ts (a new ~60-line smoke test that runs --cpu-prof over a workload designed to widen the VM-entry race window). The actual segfault fix lives in oven-sh/WebKit#395; this PR picks it up via the pin and adds bun-side coverage.
Security risks
None identified. The test spawns a local subprocess with bunEnv in a tempDir, no network, no untrusted input parsing. The pin change downloads a prebuilt tarball from oven-sh/WebKit releases — same trust boundary as before.
Level of scrutiny
High, despite the small diff. A WEBKIT_VERSION bump replaces the entire JavaScriptCore binary across all platforms and, per the PR description, this preview branch sits on current WebKit main and also carries oven-sh/WebKit#391. That is an engine-level change whose blast radius is not visible from the one-line diff, and it interacts with another open PR (#34655) that pins the same constant. The repo's "Dependencies & vendoring" guidance calls this out as a situational area needing maintainer attention.
Other factors
- The pin is explicitly transient: the author states it will be repinned to the merged sha, and an earlier thread said "leaving this thread open until the repin." That repin has not happened yet in the diff, so a human should confirm oven-sh/WebKit#395 has merged and update the constant before this lands.
- The new test itself looks solid: it was iterated on across three review rounds (added
samples.length > 0assertion, dropped thenextTickleg, widened the Windows deadline to 1.5s, trimmed the comment). It usesreaddirSync/readFileSync/joinwhich are already imported at the top of the file, and matches the patterns of neighboring tests in the samedescribe.concurrentblock. - No prior claude[bot] review on this PR.
2 participants
What does this PR do?
Bumps
WEBKIT_VERSIONto pick up oven-sh/WebKit#395, which fixes a segfault in the JSC sampling profiler's stack walker, and adds a--cpu-profstress test for the raced path.On the Windows 2019 x64 CI runners,
test/js/node/test/sequential/test-cpu-prof-dir-worker.jsintermittently dies with:in the
jsc.sampling-profiler.thread(first seen on Buildkite build 90390 of #34424; symbolized with that build's PDB):#34424's worker cpu-prof inheritance multiplied the sampled VMs, but the raced state exists for plain main-thread
--cpu-profon main today.Cause
SamplingProfiler::takeSampleonly checksvm.entryScopebefore walking, butvm.topEntryFramecan be null whileentryScopeis set: the scope is created and destroyed in C++ aroundvmEntryToJavaScript, whiletopCallFrame/topEntryFrameare stored and restored insidedoVMEntry, after its O(paddedArgCount) argument copy loops. A sample landing in such a window walks a half-built entry frame (the PC is inside the LLInt range, so the walker starts from the machine frame pointer) or a staletopCallFrame, with a null entry-frame snapshot. The first walked frame whose caller slot reads null compares equal to the null snapshot, and the walker dereferencesvmEntryRecord(nullptr); the fault address is that record'sm_prevTopEntryFramefield at-0x38.Fix
oven-sh/WebKit#395:
CallFrame::unsafeCallerFramereturns null when the current entry frame is null instead of consulting aVMEntryRecordthat does not exist. The walker treats that as the top of the stack, which is also how a valid walk ends at the outermost entry record. The function's only caller is the sampling profiler, so the change is confined to the sampler.Testing
The crash needs CI-runner timing: it fired roughly once per fifteen builds of #34424 on the Windows 2019 runners, and did not reproduce in local probes against unfixed builds:
--cpu-prof-interval 50 --cpu-prof-dir prof --cpu-prof fibonacci-worker.js) under the exact crashing binary (1.4.0-canary.1+c6045a8d5) on Windows Server 2019: cleanThe deterministic regression test therefore lives with the fix: oven-sh/WebKit#395 adds a TestWebKitAPI case that constructs the raced state directly (a zeroed frame walked with a null entry frame cursor). Without the guard it segfaults in
VMEntryRecord::unsafePrevTopEntryFrameat the exact crash location; with the guard it passes.The new bun-side test (
sampler survives VM entry churn from callbacks with huge parameter countsintest/cli/run/cpu-prof.test.ts) is a smoke test: it pins the widened-window workload under--cpu-profso the raced path stays exercised on the runners where the crash did fire. The fixture avoidsprocess.nextTick(a non-empty tick queue drains the microtask jobs nested inside the tick-drain VM entry, and only outermost entries hit the raced state) and gives Windows a 1.5s churn window (the Windows sampler ticks at the ~15.6ms timer quantum and only in-entry-scope samples are recorded, so 150ms expects about one sample; verified 10/10 clean runs on Windows Server 2019).Verified locally against a debug build linked with the patched WebKit: the full
test/cli/run/cpu-prof.test.tssuite passes, including the new test.Note:
WEBKIT_VERSIONcurrently points at the preview build of oven-sh/WebKit#395 so CI runs with the fix; it will be repinned to the merge commit once that PR lands. The pinned branch sits on current WebKit main, so it also contains oven-sh/WebKit#391 (the JSModuleLoader termination fix that #34655 pins); whichever bun PR lands second only needs its pin line updated.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.