node:vm: don't remap vm code through the source map of the file its filename names - #38344
node:vm: don't remap vm code through the source map of the file its filename names#38344robobun wants to merge 1 commit into
Conversation
…e file its filename names Bun's source map table is keyed by file path. The stack formatter excluded vm code by comparing the frame's realm with the error's, which misses vm code run in the main realm (runInThisContext, Script#runInThisContext, compileFunction), and the error printer and the CPU profiler had no exclusion at all. A vm script compiled under the name of a file Bun had transpiled therefore reported positions remapped through that file's map. Decide per frame from the code itself instead: every node:vm compile path attaches a NodeVMScriptFetcher to its SourceOrigin, so Zig::isNodeVMSource() identifies such code regardless of the realm it runs in. error.stack and Error.prepareStackTrace call sites skip the remap request for these frames, populateStackFrameMetadata records the flag on the ZigStackFrame so remap_zig_exception skips them too, and the profiler's computeLineColumnWithSourcemap callback returns early for such providers.
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
Comment |
|
Status: fix pushed, waiting on CI. Reproduced on 1.4.0 and main with Fix in this PR: decide per frame from the code's |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it replaces the realm-comparison heuristic in the stack-trace remap paths (with a documented side effect on ShadowRealm / bun test --isolate frames) and threads a new field through the ZigStackFrame C++/Rust FFI struct, a human look would still be worthwhile.
What was reviewed:
isNodeVMSourcepredicate matches the existingScriptFetcher::Type::NodeVMcheck inNodeVM.cpp; null codeBlock/ownerExecutable/provider paths return false.ZigStackFrame.is_node_vmoccupies the existing padding byte (offset 63); the newassert_ffi_layout!pins size 72 / align 8, and both C++ and Rust default-initializers set it to false.computeErrorInfoWithPrepareStackTrace: the removedhasLineAndColumnInfo()guard only fed the deleted realm comparison;getSourcePositions()was already reached without it.remap_zig_exception: the top-frameis_node_vmbranch falls through tocollect_source_lines(matching the non-colliding-name path); non-top frames skip the per-frame remap loop.
Extended reasoning...
Overview
The PR fixes a bug where node:vm code compiled under a filename that Bun already transpiled gets its stack-frame positions remapped through that file's source map. It adds Zig::isNodeVMSource() (checks the SourceProvider's SourceOrigin fetcher for ScriptFetcher::Type::NodeVM), applies it in four remap sites (formatStackTrace, computeErrorInfoWithPrepareStackTrace, computeLineColumnWithSourcemap, remap_zig_exception), and threads the result through a new ZigStackFrame.is_node_vm field so the Rust error printer can skip those frames. Ten files touched across src/jsc/ (Rust and C++ bindings), plus two test files with comprehensive coverage of error.stack, prepareStackTrace CallSites, Bun.inspect, the uncaught-error printer, and --cpu-prof.
Security risks
None identified. The change only affects which frames get looked up in the source-map table when formatting stack traces; no new user input is parsed and no trust boundaries are crossed.
Level of scrutiny
Moderate-to-high. Stack-trace formatting is user-visible and runs on every error, including from finalizers. The change replaces the previous realm-comparison heuristic entirely rather than adding to it, and the PR description explicitly calls out a behavioral side effect: frames from other Zig::GlobalObjects (ShadowRealm, bun test --isolate leftovers) will now be remapped where they previously were not. That's argued to be correct (they are Bun-loaded code), but it's a deliberate behavior change beyond the headline fix that a maintainer should sign off on.
Other factors
- The FFI struct change is well-guarded: the new bool sits in an existing padding byte, both the C++ constructor and the Rust
ZEROconstant initialize it, all Rust literal constructors are updated, and a newassert_ffi_layout!macro invocation pins every field offset. - The predicate reuses an existing marker (
NodeVMScriptFetcher) already checked the same way inNodeVM.cpp:277, so the "every node:vm compile path attaches one" claim is grounded in existing code. - Test coverage is thorough (own-name vs other-name equality across five vm entry points, top-frame vs non-top-frame, separate context, plus a
--cpu-profregression) and follows harness conventions (tempDir,bunEnv, concurrent pipe drain,test.concurrent). - The removed
isDefaultGlobalObjectInAFinalizerspecial case informatStackTracewas only there to keep the realm comparison working when no lexical global was available; with the comparison gone it's dead, and the finalizer-safe path still takesFinalizerSafety::MustNotTriggerGCfor name lookup. - No CODEOWNERS cover the touched files.
Given the cross-language FFI change and the intentional side effect on non-vm cross-realm frames, this exceeds the "simple/mechanical" bar for auto-approval.
|
Updated 1:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 1e7a68f has some failures in 🧪 To try this PR locally: bunx bun-pr 38344That installs a local version of the PR into your bun-38344 --bun |
Problem
at f (/tmp/prrepro/remap_script.js:5:48)witherr.line === 5,err.originalLine === 10. Under any other name it prints:10:32; Node prints line 10 under both.error.stack,Error.prepareStackTracecall sites,Bun.inspect(err)/console.error(err)/ the uncaught exception output (wrong line, plus a code frame taken from the host file), and--cpu-prof(callFrame.lineNumberandpositionTicksof the vm function).formatStackTraceandcomputeErrorInfoWithPrepareStackTrace(src/jsc/bindings/FormatStackTraceForJS.cpp) excluded vm code by comparing the frame callee's global object with the error's. That only catches vm contexts;runInThisContext,Script#runInThisContextandcompileFunctionrun in the main realm. Once node:vm: apply SourceTextModule lineOffset/columnOffset like Node and name frames after the identifier #38235 namesSourceTextModuleframes after their identifier,identifier: import.meta.pathgets there too.remap_zig_exception(src/jsc/VirtualMachine.rs, the error printer) andcomputeLineColumnWithSourcemap(the profiler callback) had no exclusion at all, so the printer misreports vm code even in a separate context.Fix
Zig::isNodeVMSource()(ErrorStackTrace.cpp): a frame is vm code if itsSourceProvider'sSourceOrigincarries aNodeVMScriptFetcher. Every node:vm compile path attaches one (Script,runInThisContext,runInContext/runInNewContext,compileFunction,SourceTextModule),evalinside vm code inherits the origin, and nothing else in Bun creates a fetcher.Zig::GlobalObjects (ShadowRealm,bun test --isolateleftovers) are Bun-loaded code and now remap like main-realm frames instead of being skipped.populateStackFrameMetadatastores the result in a newZigStackFrame.is_node_vm(occupies the existing padding byte; the Rust side now asserts the layout) andremap_zig_exceptionskips those frames. A vm top frame falls through to the existingcollect_source_linespath, so the code frame comes from the vm source, exactly as it already does for non-colliding names.computeLineColumnWithSourcemapreturns early for such providers; the profiler uses it for both definition and sample positions.require.extensionsflow, where Bun's transpiled output is handed to an overriddenmodule._compileand compiled in a plainStringSourceProviderunder the file's name; remapping that by name is intended.Bun__CallFrame__getCallerSrcLoc,Bun__CallFrame__getLineNumberandInspectorTestReporterAgentalso remap by name, but they locatetest()/expect()/inline snapshot calls, which only mean anything for code Bun loaded from the test file itself.<parse>frame anderr.lineof a vm SyntaxError are still remapped by name. JSC'saddErrorInfomaterializes the error before Bun sees anything but the URL string, so that needs a different fix.test/js/node/vm/vm.test.ts, "vm code compiled under the filename of a file Bun transpiled": a fixture compiles the same code under its own path and under an unrelated name and requires identical results forerror.stack(Script,runInThisContext,runInNewContext,compileFunction,SourceTextModule),prepareStackTracecall sites,Bun.inspect(vm frame on top, in a separate context, and below a host frame) and the uncaught output, plus the physical line 5 and the fixture's own frame still being remapped. Before the fix the own-name variants reported line 23 of the fixture.test/cli/run/cpu-prof.test.ts, "vm code compiled under the filename of the profiled file keeps its own positions":callFrameline/column andpositionTicksmatch the unrelated-name run (before: line 23 instead of 4).test/js/node/vm/,test/js/bun/sourcemap/,test/js/node/module/sourcemap*,test/js/bun/test/stack.test.ts,test/js/node/v8/capture-stack-trace.test.js,test/regression/issue/29240.test.ts, Node'stest-vm-source-map-url,test-vm-syntax-error-*,test-vm-module-errors. The two "Error inside minified file" snapshots ininspect-error.test.jsfail identically on main with this debug build.remap_zig_exception; whichever lands second needs a one-line rebase.Background
SavedSourceMap). Formatting a stack looks each frame's(sourceURL, line, column)up in that table, so any frame whose URL equals a transpiled module's path is remapped, whatever code it came from.SourceProvideris JSC's handle to one compiled source text. Bun's modules useZig::SourceProvider; node:vm uses JSC'sStringSourceProviderwith the caller'sfilenameas its URL. Each provider has aSourceOrigin, which may carry aScriptFetcher; Bun's JSC fork gives fetchers afetcherType(), and node:vm already attaches aNodeVMScriptFetcherto everything it compiles so thatimport()inside vm code can find itsimportModuleDynamicallycallback. This change reuses that marker.ZigStackFrameis the#[repr(C)]frame record the C++ bindings fill from JSC frames and the Rust error printer remaps and prints. Stack strings take a different route: C++ builds the request frames itself and remaps them in a batch viaBun__remapStackFramePositions.Repro (release 1.4.0 vs this branch)
The column difference from Node exists under both names and is unrelated (#37396).
originalLineis only set on frames that went through a source map, so it is now absent for vm frames under either name; before it was set under both.In the padded test fixtures, the own-name uncaught output showed fixture lines 18 to 23 as the code frame above
at f (fixture.js:23:16), and--cpu-profreportedcallFrame.lineNumber23 with ticks on line 24. The unrelated-name runs, and this branch under both names, show the vm source with:5:and report line 4 (0-based) with ticks on line 5.