Repository navigation
Conversation
StackLine::from_address drops any captured PC that cannot be mapped to a
loaded image (GetModuleHandleExW / dl_iterate_phdr / dyld): the encoder
writes a single '_' and the raw address is lost, which bun.report renders
as pkg:"?" addr:0x0000. JSC's fixed executable-memory pool (Baseline/
DFG/FTL/Wasm/Yarr and host-call thunks) is a bare VirtualAlloc/mmap, not
an image, so every JIT frame hit that path. ~16% of Windows crash events
in the last 7 days carried at least one such frame.
Mirror g_jscConfig.{start,end}ExecutableMemory into the crash handler at
JSCInitialize time and range-check each captured PC against it before the
image lookup. A match is encoded with object "JIT" and address =
offset-into-pool, reusing the existing foreign-module trace-string slot so
bun.report needs no decoder change.
WalkthroughChangesJSC registers its fixed JIT memory pool with the crash handler. Stack traces encode in-range addresses as stable offsets labeled JIT pool crash-frame tagging
Suggested reviewers: 🚥 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: 4
🤖 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 `@src/crash_handler/lib.rs`:
- Around line 2565-2576: Update the JIT-frame handling in
cli_state::jit_pool_offset to avoid allocating when constructing StackLine;
replace Box::<[u8]>::from(&b"JIT"[..]) with the existing static, borrowed, or
non-allocating representation supported by StackLine while preserving the "JIT"
label and pool offset.
In `@src/runtime/api/crash_handler_jsc.rs`:
- Around line 35-36: Update the internal testing binding declaration in
internal-for-testing.ts to include the new crash_handler members: jitPoolRange
returning a [bigint, bigint] tuple and stackLineObject accepting a bigint
address and returning string | null | undefined. Preserve the existing
declarations.
In `@test/cli/run/run-crash-handler.test.ts`:
- Around line 16-21: Remove the historical “~16% of Windows crash events”
statistic from the comment above the crash-report trace-string encoder. Preserve
the durable explanation that JSC JIT memory is not a loaded image and that
unmapped JIT frames were previously encoded as `_` and rendered incorrectly.
- Around line 29-35: Strengthen the assertions in the pool-address test around
stackLineObject so they validate the encoded offset, not just the "JIT" label:
assert start encodes as 0 and end - 1 encodes as end - start - 1. Expose the
encoded address or assert the resulting trace representation, while preserving
the existing exclusive-end and zero-PC checks.
🪄 Autofix (Beta)
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: ee0ced41-ad5c-4c56-9e92-017a57a43e67
📒 Files selected for processing (4)
src/crash_handler/lib.rssrc/jsc/bindings/ZigGlobalObject.cppsrc/runtime/api/crash_handler_jsc.rstest/cli/run/run-crash-handler.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 3:31 PM PT - Jul 24th, 2026
✅ @robobun, your commit f09345b6eb2acc09d9be17593bf9d391d24c972b passed in 🧪 To try this PR locally: bunx bun-pr 35440That installs a local version of the PR into your bun-35440 --bun |
…_symbolizer, return numbers from jitPoolRange
- StackLine.object is now Option<Cow<'static, [u8]>> so the JIT branch
stays alloc-free on the POSIX signal-handler path (only the Windows DLL
branch owns).
- spawn_symbolizer skips frames with object.is_some() so a JIT pool offset
(or a Windows DLL offset, pre-existing) is not fed to llvm-symbolizer /
pdb-addr2line against bun's own image.
- jitPoolRange returns doubles (user-space addrs are <2^48) instead of
BigInt; avoids the unchecked JSBigInt::tryCreateFrom throw scope the
ASAN lane's validateExceptionChecks tripped on.
- stackLine hook now returns {address, object}; test asserts the pool
offset as well as the label.
- internal-for-testing.ts type annotations updated; dropped the incident
stat from the test comment.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/crash_handler/lib.rs (1)
565-586: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPublish the JIT range with release/acquire ordering.
src/crash_handler/lib.rs:565-586The twoRelaxedstores/loads can expose a mixed(start, end)snapshot during startup, so a valid JIT PC may be classified as unknown. StoreendwithReleaseand load it withAcquirebefore readingstartin both readers.🤖 Prompt for 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. In `@src/crash_handler/lib.rs` around lines 565 - 586, Update set_jit_pool_range, jit_pool_range, and jit_pool_offset to publish JIT_POOL_END with Release ordering and load it with Acquire ordering before reading JIT_POOL_START. Preserve the existing range and offset behavior while ensuring readers observe a consistent initialized JIT range.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/crash_handler/lib.rs`:
- Around line 565-586: Update set_jit_pool_range, jit_pool_range, and
jit_pool_offset to publish JIT_POOL_END with Release ordering and load it with
Acquire ordering before reading JIT_POOL_START. Preserve the existing range and
offset behavior while ensuring readers observe a consistent initialized JIT
range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f5bb8ef-5982-4908-9ce8-2ad0ef973f88
📒 Files selected for processing (4)
src/crash_handler/lib.rssrc/js/internal-for-testing.tssrc/runtime/api/crash_handler_jsc.rstest/cli/run/run-crash-handler.test.ts
|
Re the release/acquire suggestion for
A reader racing the two stores can only observe a combination where |
There was a problem hiding this comment.
No issues found on the follow-up commit, but this touches the async-signal-safe crash-report encoding path and adds a C++→Rust FFI symbol called from JSCInitialize, so it's worth a human look before merging.
What was reviewed:
- The
Cow::Borrowed(b"JIT")change keeps the POSIXfrom_addresspath alloc-free;write_encodedderefsCow<[u8]>identically to the oldBox<[u8]>. spawn_symbolizernow skips foreign-object frames, restoring the pre-PR Linux behavior and fixing the pre-existing Windows DLL-offset case.- Torn read of
JIT_POOL_START/ENDduring a mid-set_jit_pool_rangecrash is benign:start != 0, end == 0fails the range check and falls through to the platform lookup. #if ENABLE(JIT)+start == 0sentinel handles JIT-disabled builds and runtime--jitless.
Extended reasoning...
Overview
The PR mirrors JSC's fixed executable-memory pool bounds (g_jscConfig.{start,end}ExecutableMemory) into two AtomicUsizes in crash_handler::cli_state, written once from JSCInitialize after JSC::initialize(). StackLine::from_address now range-checks each captured PC against these bounds before the per-platform module lookup and, on match, encodes the frame as object: "JIT" with address = pc - pool_start. StackLine.object was widened from Option<Box<[u8]>> to Option<Cow<'static, [u8]>> so the JIT label is a static borrow. Two bun:internal-for-testing hooks and a boundary-focused test cover the classification.
Security risks
None identified. The change is read-only diagnostic labeling of already-captured PCs; it does not alter fault handling, signal disposition, or the trace-string wire format (it reuses the existing foreign-module VLQ(1) + name + addr slot). The FFI symbol takes two usize values and stores them in atomics — no pointer dereference.
Level of scrutiny
Higher than average. from_address runs inside the POSIX SIGSEGV/SIGABRT/SIGTRAP handler on the release path, where the surrounding code is deliberately alloc-free and lock-free. The first-round review caught that the original Box::from(b"JIT") allocation violated that invariant, and that spawn_symbolizer would mis-symbolize JIT offsets against bun's own binary; both were fixed in f09345b. The Cow change is correct (write_encoded uses writer.write_all(object) which derefs Cow<[u8]> to &[u8] identically), and spawn_symbolizer now continues on object.is_some(). The atomics use Relaxed ordering, which is correct for this write-once/read-many pattern with a zero sentinel; a partially-observed update degrades to "not classified" rather than misbehaving.
Other factors
#include <JavaScriptCore/ExecutableAllocator.h>was moved out of the Windows-only guard;startOfFixedExecutableMemoryPoolis already used unconditionally-per-ENABLE(JIT)elsewhere in JSC and the PR description reportsrust:check-all(10/10 targets) clean.- The test asserts exact offsets at
start,start+0x100, andend-1, plus the exclusive-end and zero-PC boundaries, and uses?.objectonstackLine(end)so it tolerates whatever the platform lookup returns for the byte after the pool. - All prior CodeRabbit and claude[bot] inline comments are marked resolved with corresponding fixes in f09345b.
Deferring rather than approving because crash-handler signal-path changes plus a new cross-language FFI symbol warrant a maintainer glance, even though the mechanism is simple and the failure mode (mis-tagged frame) is diagnostic-only.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-24 and it conflicts with main. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
2 participants
Problem
StackLine::from_address(src/crash_handler/lib.rs) discards any captured PC it cannot map to a loaded image: on WindowsGetModuleHandleExW(FROM_ADDRESS), on Linuxdl_iterate_phdr, on macOS the_dyldimage walk. The encoder writes a single_for those frames and the raw address is thrown away; bun.report renders them aspkg:\"?\" addr:0x0000.JSC's fixed executable-memory pool (Baseline/DFG/FTL/Wasm/Yarr, plus host-call thunks) is a bare
VirtualAlloc/mmapreservation, not a loaded image, so every JIT frame hits that path. In Sentry, 9,122 of 58,414 Windows crash events over the last 7 days (~16%) carried at least one such blank frame; e.g. BUN-2PYE events on1.4.0-canary+892b1dabcshow 19/20 frames aspkg:\"?\" addr:0x0000for a user running JIT'd code withprocess_dlopen.Fix
Mirror
g_jscConfig.{start,end}ExecutableMemory(JSC'sisJITPCbounds) into the crash handler via a newBun__setJITPoolRangecalled once fromJSCInitializeafterJSC::initialize().StackLine::from_addressrange-checks each PC against it before the per-platform image lookup (two pointer compares, no locks, no allocation). A match is encoded withobject: \"JIT\"andaddress= offset into the pool, which reuses the existing foreign-module trace-string slot (VLQ(1) + VLQ(len) + name + VLQ(addr)), so bun.report needs no decoder change and will renderpkg:\"JIT\" addr:0x<offset>.Pool bounds live in
cli_statealongsideCMD_CHAR/MAIN_THREAD_ID(same write-once-from-a-higher-tier-crate pattern as #31688).Verification
New cross-platform test in
test/cli/run/run-crash-handler.test.tsvia twobun:internal-for-testinghooks (jitPoolRange(),stackLineObject(addr)):Full
run-crash-handler.test.ts: 15 pass / 8 skip / 0 fail.crash-report-command-char.test.ts: 3 pass.bun run rust:check-all: 10/10 targets clean.Deriving JIT tier / JS function name from the PC (via
VMInspector::codeBlockForMachinePCor avm.topCallFramewalk) is a larger follow-up.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/run/run-crash-handler.test.ts