diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index ac42b0903872..99d6614b37be 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -5724,7 +5724,7 @@ impl VirtualMachine { }; if enable_source_code_preview.get() && code.slice().is_empty() { - exception.collect_source_lines(error_instance, global); + exception.collect_source_lines(error_instance, global, top as u8); } // Direct copy; both sides are `bun_core::Ordinal`. @@ -5768,7 +5768,9 @@ impl VirtualMachine { *source_code_slice = Some(code); } } else if enable_source_code_preview.get() { - exception.collect_source_lines(error_instance, global); + // Nothing to remap through (node:vm script, eval, new Function): + // excerpt the frame picked above straight from its JSC source. + exception.collect_source_lines(error_instance, global, top as u8); } drop(top_source_url); diff --git a/src/jsc/ZigException.rs b/src/jsc/ZigException.rs index 7852d6f5d457..09c25252d9e2 100644 --- a/src/jsc/ZigException.rs +++ b/src/jsc/ZigException.rs @@ -12,12 +12,15 @@ use crate::{JSErrorCode, JSGlobalObject, JSRuntimeType, JSValue, ZigStackFrame, // SAFETY (safe fn): `JSValue` is a by-value scalar; `JSGlobalObject` is an // opaque `UnsafeCell`-backed handle (`&` is ABI-identical to non-null `*mut`); -// `ZigException` is a `#[repr(C)]` out-param the C++ side fills in-place. +// `ZigException` is a `#[repr(C)]` out-param the C++ side fills in-place; +// `frame_index` is a by-value scalar that C++ bounds-checks against +// `stack.frames_len`. unsafe extern "C" { pub(crate) safe fn ZigException__collectSourceLines( js_value: JSValue, global: &JSGlobalObject, exception: &mut ZigException, + frame_index: u8, ); } @@ -50,8 +53,18 @@ pub struct ZigException { } impl ZigException { - pub(crate) fn collect_source_lines(&mut self, value: JSValue, global: &JSGlobalObject) { - ZigException__collectSourceLines(value, global, self); + /// Fills `stack.source_lines_*` with the source around + /// `stack.frames()[frame_index]`, read from that frame's JSC source + /// provider. For sources bun transpiled, `remap_zig_exception` reads the + /// original file instead; this is the path for everything else (`node:vm` + /// scripts, `eval`, `new Function`). + pub(crate) fn collect_source_lines( + &mut self, + value: JSValue, + global: &JSGlobalObject, + frame_index: u8, + ) { + ZigException__collectSourceLines(value, global, self, frame_index); } // Kept as explicit `deinit` (not `Drop`) — this is a #[repr(C)] FFI diff --git a/src/jsc/bindings/ZigException.cpp b/src/jsc/bindings/ZigException.cpp index ea84d276eda9..026dce6c8fc7 100644 --- a/src/jsc/bindings/ZigException.cpp +++ b/src/jsc/bindings/ZigException.cpp @@ -225,7 +225,7 @@ static void populateStackFramePosition(const JSC::StackFrame& stackFrame, BunStr } static void populateStackFrame(JSC::VM& vm, ZigStackTrace& trace, const JSC::StackFrame& stackFrame, - ZigStackFrame& frame, bool is_top, JSC::SourceProvider** referenced_source_provider, JSC::JSGlobalObject* globalObject, PopulateStackTraceFlags flags, FinalizerSafety finalizerSafety) + ZigStackFrame& frame, JSC::SourceProvider** referenced_source_provider, JSC::JSGlobalObject* globalObject, PopulateStackTraceFlags flags, FinalizerSafety finalizerSafety) { if (flags == PopulateStackTraceFlags::OnlyPosition) { populateStackFrameMetadata(vm, globalObject, stackFrame, frame, finalizerSafety); @@ -233,9 +233,9 @@ static void populateStackFrame(JSC::VM& vm, ZigStackTrace& trace, const JSC::Sta nullptr, 0, frame.position, referenced_source_provider, flags); } else if (flags == PopulateStackTraceFlags::OnlySourceLines) { - populateStackFramePosition(stackFrame, is_top ? trace.source_lines_ptr : nullptr, - is_top ? trace.source_lines_numbers : nullptr, - is_top ? trace.source_lines_to_collect : 0, frame.position, referenced_source_provider, flags); + populateStackFramePosition(stackFrame, trace.source_lines_ptr, + trace.source_lines_numbers, + trace.source_lines_to_collect, frame.position, referenced_source_provider, flags); } } @@ -421,7 +421,11 @@ class V8StackTraceIterator { } }; -static void populateStackTrace(JSC::VM& vm, const WTF::Vector& frames, ZigStackTrace& trace, JSC::JSGlobalObject* globalObject, PopulateStackTraceFlags flags, FinalizerSafety finalizerSafety = FinalizerSafety::NotInFinalizer) +// OnlyPosition fills `trace.frames_ptr` from `frames`. OnlySourceLines runs +// later, after Rust has filtered the frames and picked the one whose source +// to excerpt (`remap_zig_exception`), and collects the source lines of +// `trace.frames_ptr[source_lines_frame_index]` only. +static void populateStackTrace(JSC::VM& vm, const WTF::Vector& frames, ZigStackTrace& trace, JSC::JSGlobalObject* globalObject, PopulateStackTraceFlags flags, FinalizerSafety finalizerSafety = FinalizerSafety::NotInFinalizer, uint8_t source_lines_frame_index = 0) { if (flags == PopulateStackTraceFlags::OnlyPosition) { uint8_t frame_i = 0; @@ -439,18 +443,19 @@ static void populateStackTrace(JSC::VM& vm, const WTF::Vector& ZigStackFrame& frame = trace.frames_ptr[frame_i]; frame.jsc_stack_frame_index = static_cast(stack_frame_i); - populateStackFrame(vm, trace, frames[stack_frame_i], frame, frame_i == 0, &trace.referenced_source_provider, globalObject, flags, finalizerSafety); + populateStackFrame(vm, trace, frames[stack_frame_i], frame, &trace.referenced_source_provider, globalObject, flags, finalizerSafety); stack_frame_i++; frame_i++; } trace.frames_len = frame_i; } else if (flags == PopulateStackTraceFlags::OnlySourceLines) { - for (uint8_t i = 0; i < trace.frames_len; i++) { - ZigStackFrame& frame = trace.frames_ptr[i]; - if (frame.jsc_stack_frame_index < 0 || static_cast(frame.jsc_stack_frame_index) >= frames.size()) - continue; - populateStackFrame(vm, trace, frames[frame.jsc_stack_frame_index], frame, i == 0, &trace.referenced_source_provider, globalObject, flags, finalizerSafety); - } + if (source_lines_frame_index >= trace.frames_len) + return; + ZigStackFrame& frame = trace.frames_ptr[source_lines_frame_index]; + // -1 when the frames were parsed out of an `error.stack` string. + if (frame.jsc_stack_frame_index < 0 || static_cast(frame.jsc_stack_frame_index) >= frames.size()) + return; + populateStackFrame(vm, trace, frames[frame.jsc_stack_frame_index], frame, &trace.referenced_source_provider, globalObject, flags, finalizerSafety); } } @@ -866,7 +871,8 @@ extern "C" [[ZIG_EXPORT(check_slow)]] void JSC__JSValue__toZigException(JSC::Enc exceptionFromString(*exception, value, global); } -extern "C" void ZigException__collectSourceLines(JSC::EncodedJSValue jsException, JSC::JSGlobalObject* global, ZigException* exception) +// `frame_index` indexes `exception->stack.frames_ptr` (after Rust's frame filtering). +extern "C" void ZigException__collectSourceLines(JSC::EncodedJSValue jsException, JSC::JSGlobalObject* global, ZigException* exception, uint8_t frame_index) { JSC::JSValue value = JSC::JSValue::decode(jsException); if (value == JSC::JSValue {}) { @@ -878,7 +884,7 @@ extern "C" void ZigException__collectSourceLines(JSC::EncodedJSValue jsException JSValue unwrapped = jscException->value(); if (jscException->stack().size() > 0) { - populateStackTrace(global->vm(), jscException->stack(), exception->stack, global, PopulateStackTraceFlags::OnlySourceLines); + populateStackTrace(global->vm(), jscException->stack(), exception->stack, global, PopulateStackTraceFlags::OnlySourceLines, FinalizerSafety::NotInFinalizer, frame_index); } exceptionFromString(*exception, unwrapped, global); @@ -887,7 +893,7 @@ extern "C" void ZigException__collectSourceLines(JSC::EncodedJSValue jsException if (JSC::ErrorInstance* error = dynamicDowncast(value)) { if (error->stackTrace() != nullptr && error->stackTrace()->size() > 0) { - populateStackTrace(global->vm(), *error->stackTrace(), exception->stack, global, PopulateStackTraceFlags::OnlySourceLines, FinalizerSafety::MustNotTriggerGC); + populateStackTrace(global->vm(), *error->stackTrace(), exception->stack, global, PopulateStackTraceFlags::OnlySourceLines, FinalizerSafety::MustNotTriggerGC, frame_index); } return; } diff --git a/test/js/node/vm/vm.test.ts b/test/js/node/vm/vm.test.ts index d3d04239d179..13452fa0e3bd 100644 --- a/test/js/node/vm/vm.test.ts +++ b/test/js/node/vm/vm.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "bun:test"; import { bunEnv, bunExe, normalizeBunSnapshot } from "harness"; +import { EventEmitter } from "node:events"; import { compileFunction, constants, @@ -463,6 +464,118 @@ describe("Script", () => { }); }); +// The code frame bun prints above an error is the line of the first frame that +// is in one of the user's sources. A vm script (like eval and new Function) has +// no source map, so that line is cut out of the source JSC compiled. When the +// error is thrown inside a JS builtin or one of bun's own modules, the user's +// frame is not frame 0; the excerpt used to be cut out of frame 0's source +// regardless, i.e. it showed the builtin's own text and line number. +describe("code frame of an error thrown inside a builtin called from a source without a source map", () => { + type Expected = { + // Names the user's source in the stack; the excerpt has to be its line. + filename: string; + // The `name: message` line printed under the code frame. + header: string; + // The text of the line that calls into the builtin. + excerpt: string; + }; + + function expectCodeFrame(printed: string, { filename, header, excerpt }: Expected) { + const lines = printed.split("\n"); + const headerIndex = lines.indexOf(header); + expect(headerIndex).toBeGreaterThanOrEqual(2); + const userFrame = printed.match(new RegExp(`${RegExp.escape(filename)}:(\\d+):\\d+`)); + expect(userFrame).not.toBeNull(); + // Directly above the header are the excerpt and the caret. The excerpt is + // labelled with the line the user's frame reports (line 2 of the source + // below; for new Function, 2 plus its `function anonymous(\n) {` wrapper). + expect(lines.slice(headerIndex - 2, headerIndex)).toEqual([ + `${userFrame![1]} | ${excerpt}`, + expect.stringMatching(/^ +\^$/), + ]); + } + + // The builtin is called from line 2 so that the excerpt's line number has to + // come from the user's frame as well as its text. + const source = '"line 1";\n[].reduce((a, b) => a);'; + const reduce = { + excerpt: "[].reduce((a, b) => a);", + header: "TypeError: reduce of empty array with no initial value", + }; + // displayErrors: false keeps node:vm from rewriting err.stack; the code frame + // under test is the one bun's error printer builds from the error's frames. + const displayErrors = false; + + const cases: (Expected & { name: string; run(filename: string): unknown })[] = [ + { + name: "vm.Script", + filename: "/virtual/script.js", + ...reduce, + run: filename => new Script(source, { filename }).runInThisContext({ displayErrors }), + }, + { + name: "runInNewContext", + filename: "/virtual/new-context.js", + ...reduce, + run: filename => runInNewContext(source, {}, { filename, displayErrors }), + }, + { + name: "eval", + filename: "/virtual/eval.js", + ...reduce, + run: filename => (0, eval)(`${source}\n//# sourceURL=${filename}`), + }, + { + name: "new Function", + filename: "/virtual/function.js", + ...reduce, + run: filename => new Function(`${source}\n//# sourceURL=${filename}`)(), + }, + { + // emit() with no "error" listener throws from inside bun's node:events, + // so the frame on top is a node:events frame rather than a JS builtin's. + name: "node: module on top of the stack", + filename: "/virtual/events.js", + header: "error: Unhandled error. (undefined)", + excerpt: 'emitter.emit("error");', + run: filename => + runInNewContext( + '"line 1";\nemitter.emit("error");', + { emitter: new EventEmitter() }, + { filename, displayErrors }, + ), + }, + ]; + + test.each(cases)("Bun.inspect: $name", testCase => { + let thrown: unknown; + try { + testCase.run(testCase.filename); + } catch (e) { + thrown = e; + } + expectCodeFrame(Bun.inspect(thrown), testCase); + }); + + test("uncaught error", async () => { + const filename = "/virtual/uncaught.js"; + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + `require("node:vm").runInThisContext(${JSON.stringify(source)}, ${JSON.stringify({ filename, displayErrors })})`, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout).toBe(""); + expectCodeFrame(stderr, { filename, ...reduce }); + expect(exitCode).toBe(1); + }); +}); + type TestRunInContextArg = | { fn: typeof runInContext; isIsolated: true; isNew?: boolean } | { fn: typeof runInThisContext; isIsolated?: false; isNew?: boolean };