From 1e7a68f7e2d6fb43d8e0d082e264c3a8f5e45dd1 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 03:00:19 +0000 Subject: [PATCH] node:vm: never remap frames from vm code through the source map of the 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. --- src/jsc/VirtualMachine.rs | 6 +- src/jsc/ZigStackFrame.rs | 14 ++ src/jsc/bindings/ErrorStackTrace.cpp | 20 +++ src/jsc/bindings/ErrorStackTrace.h | 8 + src/jsc/bindings/FormatStackTraceForJS.cpp | 63 ++----- src/jsc/bindings/ZigException.cpp | 1 + src/jsc/bindings/headers-handwritten.h | 3 + .../bake/dev_server/error_report_request.rs | 1 + test/cli/run/cpu-prof.test.ts | 52 ++++++ test/js/node/vm/vm.test.ts | 155 +++++++++++++++++- 10 files changed, 274 insertions(+), 49 deletions(-) diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index ac42b0903872..320ac7fb8210 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -5626,7 +5626,9 @@ impl VirtualMachine { let top_source_url = frames[top].source_url.to_utf8(); let already_remapped = frames[top].remapped; - let maybe_lookup: Option = if already_remapped { + let maybe_lookup: Option = if frames[top].is_node_vm { + None + } else if already_remapped { Some(bun_sourcemap::mapping::Lookup { mapping: bun_sourcemap::mapping::Mapping { generated: bun_sourcemap::LineColumnOffset::default(), @@ -5775,7 +5777,7 @@ impl VirtualMachine { if frames.len() > 1 { for i in 0..frames.len() { - if i == top || frames[i].position.is_invalid() { + if i == top || frames[i].position.is_invalid() || frames[i].is_node_vm { continue; } let source_url = frames[i].source_url.to_utf8(); diff --git a/src/jsc/ZigStackFrame.rs b/src/jsc/ZigStackFrame.rs index 4909e063006a..660d13ce68fd 100644 --- a/src/jsc/ZigStackFrame.rs +++ b/src/jsc/ZigStackFrame.rs @@ -23,10 +23,23 @@ pub struct ZigStackFrame { /// This informs formatters whether to display as a blob URL or not pub remapped: bool, + /// Set by C++ (`Zig::isNodeVMSource`) for code compiled by `node:vm`. Its + /// `source_url` is whatever filename the caller passed, so it must never be + /// looked up in the source map table even when it names a file Bun transpiled. + pub is_node_vm: bool, + /// -1 means not set. pub jsc_stack_frame_index: i32, } +// Mirrors `struct ZigStackFrame` in `src/jsc/bindings/headers-handwritten.h`, +// which C++ populates in place. +bun_core::assert_ffi_layout!( + ZigStackFrame, 72, 8; + function_name @ 0, source_url @ 24, position @ 48, code_type @ 60, is_async @ 61, + remapped @ 62, is_node_vm @ 63, jsc_stack_frame_index @ 64, +); + impl ZigStackFrame { /// Explicit deref of owned strings. /// @@ -72,6 +85,7 @@ impl ZigStackFrame { position: ZigStackFramePosition::INVALID, is_async: false, remapped: false, + is_node_vm: false, jsc_stack_frame_index: -1, }; diff --git a/src/jsc/bindings/ErrorStackTrace.cpp b/src/jsc/bindings/ErrorStackTrace.cpp index 7afd14d4b236..3c0c31092e98 100644 --- a/src/jsc/bindings/ErrorStackTrace.cpp +++ b/src/jsc/bindings/ErrorStackTrace.cpp @@ -524,6 +524,26 @@ String sourceURL(JSC::VM& vm, JSC::JSFunction* function) return Zig::sourceURL(function->jsExecutable()->source()); } +bool isNodeVMSource(JSC::SourceProvider* sourceProvider) +{ + if (!sourceProvider) [[unlikely]] { + return false; + } + + auto* fetcher = sourceProvider->sourceOrigin().fetcher(); + return fetcher && fetcher->fetcherType() == JSC::ScriptFetcher::Type::NodeVM; +} + +bool isNodeVMSource(const JSC::StackFrame& frame) +{ + auto* codeBlock = frame.codeBlock(); + if (!codeBlock || !codeBlock->ownerExecutable()) { + return false; + } + + return isNodeVMSource(codeBlock->source().provider()); +} + String functionName(JSC::VM& vm, JSC::CodeBlock* codeBlock) { auto codeType = codeBlock->codeType(); diff --git a/src/jsc/bindings/ErrorStackTrace.h b/src/jsc/bindings/ErrorStackTrace.h index 17c9dc6822ea..00094a41c663 100644 --- a/src/jsc/bindings/ErrorStackTrace.h +++ b/src/jsc/bindings/ErrorStackTrace.h @@ -220,6 +220,14 @@ String sourceURL(JSC::VM& vm, const JSC::StackFrame& frame); String sourceURL(JSC::StackVisitor& visitor); String sourceURL(JSC::VM& vm, JSC::JSFunction* function); +// True for code compiled by node:vm (every node:vm compile path attaches a +// NodeVMScriptFetcher to its SourceOrigin). node:vm compiles the string it was +// given as-is under a caller-chosen filename, while Bun's source maps are keyed +// by filename, so positions in such code must never be remapped, even when the +// filename is a file Bun transpiled. +bool isNodeVMSource(JSC::SourceProvider* sourceProvider); +bool isNodeVMSource(const JSC::StackFrame& frame); + enum class FinalizerSafety { NotInFinalizer, MustNotTriggerGC, diff --git a/src/jsc/bindings/FormatStackTraceForJS.cpp b/src/jsc/bindings/FormatStackTraceForJS.cpp index d622679f31aa..96a0c6499b00 100644 --- a/src/jsc/bindings/FormatStackTraceForJS.cpp +++ b/src/jsc/bindings/FormatStackTraceForJS.cpp @@ -254,7 +254,7 @@ WTF::String formatStackTrace( StackFrame& frame = stackTrace.at(i); ZigStackFrame& remappedFrame = remappedFrames[i]; // Match `ZigStackFramePosition::INVALID` exactly so the Rust batch loop's - // `position.isInvalid()` skips frames we never populate (vm-context + // `position.isInvalid()` skips frames we never populate (node:vm // frames, frames without line/col info). memset alone leaves // `line_start_byte = 0` which fails that byte-compare. remappedFrame.position.line_zero_based = -1; @@ -265,26 +265,14 @@ WTF::String formatStackTrace( if (!frame.hasLineAndColumnInfo()) continue; originalLineColumns[i] = frame.computeLineAndColumn(); - - JSC::JSGlobalObject* globalObjectForFrame = lexicalGlobalObject; - if (auto* callee = frame.callee()) { - if (auto* object = callee->getObject()) { - globalObjectForFrame = object->globalObject(); - } - } - sourceURLs[i] = Zig::sourceURL(vm, frame); - bool isDefinitelyNotRunninginNodeVMGlobalObject = globalObject == globalObjectForFrame; - bool isDefaultGlobalObjectInAFinalizer = (globalObject && !lexicalGlobalObject && !errorInstance); - if (isDefinitelyNotRunninginNodeVMGlobalObject || isDefaultGlobalObjectInAFinalizer) { - // https://github.com/oven-sh/bun/issues/3595 - if (!sourceURLs[i].isEmpty()) { - remappedFrame.position.line_zero_based = OrdinalNumber::fromOneBasedInt(originalLineColumns[i].line).zeroBasedInt(); - remappedFrame.position.column_zero_based = OrdinalNumber::fromOneBasedInt(originalLineColumns[i].column).zeroBasedInt(); - remappedFrame.source_url = Bun::toStringRef(sourceURLs[i]); - anyRemap = true; - } + // https://github.com/oven-sh/bun/issues/3595 + if (!sourceURLs[i].isEmpty() && !Zig::isNodeVMSource(frame)) { + remappedFrame.position.line_zero_based = OrdinalNumber::fromOneBasedInt(originalLineColumns[i].line).zeroBasedInt(); + remappedFrame.position.column_zero_based = OrdinalNumber::fromOneBasedInt(originalLineColumns[i].column).zeroBasedInt(); + remappedFrame.source_url = Bun::toStringRef(sourceURLs[i]); + anyRemap = true; } } @@ -440,8 +428,6 @@ static JSValue computeErrorInfoWithPrepareStackTrace(JSC::VM& vm, Zig::GlobalObj // Create the call sites (one per frame) Zig::createCallSitesFromFrames(globalObject, lexicalGlobalObject, stackTrace, callSites); - // We need to sourcemap it if it's a GlobalObject. - const int n = stackTrace.size(); WTF::Vector remappedFrames; WTF::Vector sourceURLs; @@ -461,33 +447,18 @@ static JSValue computeErrorInfoWithPrepareStackTrace(JSC::VM& vm, Zig::GlobalObj frame.position.column_zero_based = -1; frame.position.byte_position = -1; - // When you use node:vm, the global object can be different on a - // per-frame basis. We should sourcemap the frames which are in Bun's - // global object, and not sourcemap the frames which are in a different - // global object. - JSGlobalObject* globalObjectForFrame = lexicalGlobalObject; + if (Zig::isNodeVMSource(stackFrame)) + continue; - if (stackFrame.hasLineAndColumnInfo()) { - auto* callee = stackFrame.callee(); - // https://github.com/oven-sh/bun/issues/17698 - if (callee) { - if (auto* object = callee->getObject()) { - globalObjectForFrame = object->globalObject(); - } - } + if (JSCStackFrame::SourcePositions* sourcePositions = stackTrace.at(i).getSourcePositions()) { + frame.position.line_zero_based = sourcePositions->line.zeroBasedInt(); + frame.position.column_zero_based = sourcePositions->column.zeroBasedInt(); } - if (globalObjectForFrame == globalObject) { - if (JSCStackFrame::SourcePositions* sourcePositions = stackTrace.at(i).getSourcePositions()) { - frame.position.line_zero_based = sourcePositions->line.zeroBasedInt(); - frame.position.column_zero_based = sourcePositions->column.zeroBasedInt(); - } - - if (!sourceURLs[i].isEmpty()) { - frame.source_url = Bun::toStringRef(sourceURLs[i]); - didRemap[i] = true; - anyRemap = true; - } + if (!sourceURLs[i].isEmpty()) { + frame.source_url = Bun::toStringRef(sourceURLs[i]); + didRemap[i] = true; + anyRemap = true; } } @@ -616,7 +587,7 @@ WTF::String computeErrorInfoWrapperToString(JSC::VM& vm, Vector& sta void computeLineColumnWithSourcemap(JSC::VM& vm, JSC::SourceProvider* _Nonnull sourceProvider, JSC::LineColumn& lineColumn, WTF::String& remappedSourceURL) { auto sourceURL = sourceProvider->sourceURL(); - if (sourceURL.isEmpty()) { + if (sourceURL.isEmpty() || Zig::isNodeVMSource(sourceProvider)) { return; } diff --git a/src/jsc/bindings/ZigException.cpp b/src/jsc/bindings/ZigException.cpp index ea84d276eda9..c5fc7d44c41e 100644 --- a/src/jsc/bindings/ZigException.cpp +++ b/src/jsc/bindings/ZigException.cpp @@ -86,6 +86,7 @@ static void populateStackFrameMetadata(JSC::VM& vm, JSC::JSGlobalObject* globalO auto sourceURL = Zig::sourceURL(vm, stackFrame); frame.source_url = Bun::toStringRef(sourceURL); + frame.is_node_vm = Zig::isNodeVMSource(stackFrame); auto m_codeBlock = stackFrame.codeBlock(); if (m_codeBlock) { switch (m_codeBlock->codeType()) { diff --git a/src/jsc/bindings/headers-handwritten.h b/src/jsc/bindings/headers-handwritten.h index 67e94e8c3deb..e0f80097a4d8 100644 --- a/src/jsc/bindings/headers-handwritten.h +++ b/src/jsc/bindings/headers-handwritten.h @@ -199,6 +199,8 @@ typedef struct ZigStackFrame { ZigStackFrameCode code_type; bool is_async; bool remapped; + // See Zig::isNodeVMSource(). The error printer skips source maps for these. + bool is_node_vm; int32_t jsc_stack_frame_index; ZigStackFrame() @@ -208,6 +210,7 @@ typedef struct ZigStackFrame { , code_type {} , is_async(false) , remapped(false) + , is_node_vm(false) , jsc_stack_frame_index(-1) { } diff --git a/src/runtime/bake/dev_server/error_report_request.rs b/src/runtime/bake/dev_server/error_report_request.rs index 48bd7434f12d..fcc7bd124b73 100644 --- a/src/runtime/bake/dev_server/error_report_request.rs +++ b/src/runtime/bake/dev_server/error_report_request.rs @@ -152,6 +152,7 @@ impl ErrorReportRequest { code_type: ZigStackFrameCode::NONE, is_async: false, remapped: false, + is_node_vm: false, jsc_stack_frame_index: -1, }); } diff --git a/test/cli/run/cpu-prof.test.ts b/test/cli/run/cpu-prof.test.ts index 4d5fe51aea7b..7bf923c1b506 100644 --- a/test/cli/run/cpu-prof.test.ts +++ b/test/cli/run/cpu-prof.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from "bun:test"; import { readdirSync, readFileSync } from "fs"; import { bunEnv, bunExe, isWindows, tempDir } from "harness"; import { join } from "path"; +import { pathToFileURL } from "url"; // Every workload below is time-bounded for 100ms. On Windows JSC's // SamplingProfiler effectively ticks at the ~15.6ms default timer quantum, and @@ -444,4 +445,55 @@ describe.concurrent("--cpu-prof", () => { const mdContent = readFileSync(join(String(dir), mdFiles[0]), "utf-8"); expect(mdContent).toContain("# CPU Profile"); }); + + test("vm code compiled under the filename of the profiled file keeps its own positions", async () => { + // Source maps are keyed by filename. `f` is compiled by node:vm on line 5 of + // its source, under the fixture's own filename ("own") or under a name Bun + // never loaded ("other"); both must report the same, physical position. The + // PADDING-line comment is stripped by the transpiler, so any line the + // fixture's source map could produce is > PADDING and never mistaken for 5. + const PADDING = 20; + const padding = ["/*", ...Array.from({ length: PADDING - 2 }, () => " *"), " */"].join("\n") + "\n"; + const body = String.raw` +const vm = require("node:vm"); +const name = process.argv[2] === "own" ? __filename : "not-a-loaded-file.js"; +const source = "\n\n\n\n(function f() { for (const end = performance.now() + 100; performance.now() < end; ) {} })"; +const f = vm.runInThisContext(source, { filename: name }); +f(); +console.log(name); +`; + using dir = tempDir("cpu-prof-vm-filename", { "fixture.js": padding + body }); + + const profileFor = async (kind: string) => { + await using proc = Bun.spawn({ + cmd: [bunExe(), "--cpu-prof", `--cpu-prof-name=${kind}.cpuprofile`, "fixture.js", kind], + cwd: String(dir), + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + const profile = JSON.parse(readFileSync(join(String(dir), `${kind}.cpuprofile`), "utf8")); + const fNodes = profile.nodes.filter((n: any) => n.callFrame.functionName === "f"); + expect(fNodes.length).toBeGreaterThan(0); + return { + name: stdout.trim(), + callFrames: [ + ...new Set( + fNodes.map((n: any) => JSON.stringify([n.callFrame.url, n.callFrame.lineNumber, n.callFrame.columnNumber])), + ), + ].map(s => JSON.parse(s as string)), + tickLines: new Set(fNodes.flatMap((n: any) => (n.positionTicks ?? []).map((t: any) => t.line))), + }; + }; + const [own, other] = await Promise.all([profileFor("own"), profileFor("other")]); + + // callFrame.lineNumber is 0-based; positionTicks.line is 1-based. + expect(other.callFrames).toEqual([["not-a-loaded-file.js", 4, expect.any(Number)]]); + const [[, line, column]] = other.callFrames; + expect(own.callFrames).toEqual([[pathToFileURL(own.name).href, line, column]]); + expect(other.tickLines).toEqual(new Set([5])); + expect(own.tickLines).toEqual(new Set([5])); + }); }); diff --git a/test/js/node/vm/vm.test.ts b/test/js/node/vm/vm.test.ts index d3d04239d179..bad9cf0336b6 100644 --- a/test/js/node/vm/vm.test.ts +++ b/test/js/node/vm/vm.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { bunEnv, bunExe, normalizeBunSnapshot } from "harness"; +import { bunEnv, bunExe, normalizeBunSnapshot, tempDir } from "harness"; import { compileFunction, constants, @@ -1077,6 +1077,159 @@ describe("context options with throwing getters", () => { }); }); +describe("vm code compiled under the filename of a file Bun transpiled", () => { + // Bun's source maps are keyed by filename. The fixture compiles vm code under + // its own path, and that code must keep its physical positions instead of + // being remapped through the fixture's source map. The fixture opens with a + // comment spanning PADDING lines that the transpiler strips, so every line + // its source map can produce is > PADDING, while every vm function below + // sits on line 5 of its source: a remapped position can never be mistaken + // for the physical one. "other" compiles the same code under a name Bun never + // loaded and is what "own" has to match. + const PADDING = 20; + const padding = ["/*", ...Array.from({ length: PADDING - 2 }, () => " *"), " */"].join("\n") + "\n"; + const body = String.raw` +const vm = require("node:vm"); +const source = "\n\n\n\n(function f() { throw new Error('boom'); })"; +const moduleSource = "\n\n\n\nexport function f() { throw new Error('boom'); }"; +const callingBackSource = "\n\n\n\n(function f(callback) { callback(); })"; +const names = { own: __filename, other: "not-a-loaded-file.js" }; +const compile = { + script: name => new vm.Script(source, { filename: name }).runInThisContext(), + runInThisContext: name => vm.runInThisContext(source, { filename: name }), + runInNewContext: name => vm.runInNewContext(source, {}, { filename: name }), + compileFunction: name => vm.compileFunction("\n\n\n\nthrow new Error('boom')", [], { filename: name }), +}; + +function thrown(fn) { + try { + fn(); + } catch (e) { + return e; + } + throw new Error("did not throw"); +} + +function position(err) { + const [, line, column] = /:(\d+):(\d+)\)?$/.exec(err.stack.split("\n")[1]); + return { line: +line, column: +column, errLine: err.line, originalLine: err.originalLine ?? null }; +} + +function perName(collect) { + return Object.fromEntries(Object.entries(names).map(([kind, name]) => [kind, collect(name)])); +} + +async function collect() { + const results = {}; + for (const [api, make] of Object.entries(compile)) { + results[api] = perName(name => position(thrown(make(name)))); + } + + results.sourceTextModule = {}; + for (const [kind, name] of Object.entries(names)) { + const module = new vm.SourceTextModule(moduleSource, { identifier: name }); + await module.link(() => {}); + await module.evaluate(); + results.sourceTextModule[kind] = position(thrown(module.namespace.f)); + } + + // Error.prepareStackTrace receives CallSites, which are remapped separately + // from the stack string. + Error.prepareStackTrace = (_, callSites) => callSites; + results.callSite = perName(name => { + const site = thrown(compile.script(name)).stack[0]; + return { fileName: site.getFileName(), line: site.getLineNumber(), column: site.getColumnNumber() }; + }); + Error.prepareStackTrace = undefined; + + // Bun.inspect (like the uncaught-error printer) remaps the JSC frames itself, + // handling the top frame separately from the rest. In callingBack the vm + // frame is the second one: the error is thrown by a callback from this file. + results.inspect = { + script: perName(name => Bun.inspect(thrown(compile.script(name)))), + runInNewContext: perName(name => Bun.inspect(thrown(compile.runInNewContext(name)))), + callingBack: perName(name => { + const f = vm.runInThisContext(callingBackSource, { filename: name }); + return Bun.inspect(thrown(() => f(() => { throw new Error("boom"); }))); + }), + }; + return results; +} + +const uncaught = process.argv[2]; +if (uncaught) { + console.log(__filename); + compile.script(names[uncaught])(); +} else { + collect().then(results => console.log(JSON.stringify({ filename: __filename, results }))); +} +`; + // Line (in the fixture file) of the `fn();` call inside thrown(): the frame + // below the vm frame. It is only reported at this line if the fixture's own + // source map still applies. + const thrownCallLine = PADDING + body.split("\n").indexOf(" fn();") + 1; + const otherFrame = "at f (not-a-loaded-file.js:5:"; + const ownToOther = (output: string, filename: string) => + output.replace(`at f (${filename}:`, "at f (not-a-loaded-file.js:"); + + test.concurrent("error.stack, CallSites and Bun.inspect report the physical position", async () => { + using dir = tempDir("vm-own-filename", { "fixture.js": padding + body }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "fixture.js"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + const { filename, results } = JSON.parse(stdout); + + for (const api of ["script", "runInThisContext", "runInNewContext", "compileFunction", "sourceTextModule"]) { + expect(results[api].own).toEqual(results[api].other); + } + for (const api of ["script", "runInThisContext", "runInNewContext", "sourceTextModule"]) { + expect(results[api].own).toMatchObject({ line: 5, errLine: 5, originalLine: null }); + } + // compileFunction's body line is reported relative to its wrapper, so only + // the own/other equality above pins it; like every other vm frame it must + // not have been remapped at all. + expect(results.compileFunction.own.originalLine).toBeNull(); + + expect(results.callSite.other).toMatchObject({ fileName: "not-a-loaded-file.js", line: 5 }); + expect(results.callSite.own).toEqual({ ...results.callSite.other, fileName: filename }); + + for (const api of ["script", "runInNewContext"]) { + expect(results.inspect[api].other).toContain("5 | (function f() { throw new Error('boom'); })"); + } + for (const { own, other } of Object.values(results.inspect)) { + expect(other).toContain(otherFrame); + expect(other).toContain(`at thrown (${filename}:${thrownCallLine}:`); + expect(ownToOther(own, filename)).toBe(other); + } + }); + + test.concurrent("the uncaught error printer reports the physical position", async () => { + using dir = tempDir("vm-own-filename-uncaught", { "fixture.js": padding + body }); + const run = async (kind: string) => { + await using proc = Bun.spawn({ + cmd: [bunExe(), "fixture.js", kind], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(exitCode).toBe(1); + return { filename: stdout.trim(), stderr }; + }; + const [own, other] = await Promise.all([run("own"), run("other")]); + + expect(other.stderr).toContain("5 | (function f() { throw new Error('boom'); })"); + expect(other.stderr).toContain(otherFrame); + expect(ownToOther(own.stderr, own.filename)).toBe(other.stderr); + }); +}); + describe("DONT_CONTEXTIFY", () => { test("globalThis prototype chain stays inside the sandbox realm", () => { const ctx = createContext(constants.DONT_CONTEXTIFY);