diff --git a/src/jsc/bindings/ZigException.cpp b/src/jsc/bindings/ZigException.cpp index ea84d276eda9..8e247c839ef6 100644 --- a/src/jsc/bindings/ZigException.cpp +++ b/src/jsc/bindings/ZigException.cpp @@ -161,22 +161,34 @@ static void populateStackFramePosition(const JSC::StackFrame& stackFrame, BunStr return; if (source_lines_count > 1 && source_lines != nullptr && sourceString.is8Bit()) { - // Search for the beginning of the line - unsigned int lineStart = location.byte_position; - while (lineStart > 0 && sourceString[lineStart] != '\n') { - lineStart--; - } + const std::span bytes = sourceString.span8(); + const unsigned length = sourceString.length(); - // Search for the end of the line - unsigned int lineEnd = location.byte_position; - unsigned int maxSearch = sourceString.length(); - while (lineEnd < maxSearch && sourceString[lineEnd] != '\n') { - lineEnd++; - } + // JSC may position an expression one past its end: on the '\n' ending its line, or at `length`. + const unsigned divot = std::min(static_cast(std::max(location.byte_position, 0)), length); - const unsigned char* bytes = sourceString.span8().data(); + // `end` is the offset of a line's terminating '\n', or `length` for the last line. + auto startOfLineEndingAt = [&](unsigned end) -> unsigned { + unsigned start = end; + while (start > 0 && bytes[start - 1] != '\n') { + start--; + } + return start; + }; - // Most of the time, when you look at a stack trace, you want a couple lines above. + // The line's text without its terminator ("\r\n" included). + auto lineText = [&](unsigned start, unsigned end) -> BunString { + if (end > start && bytes[end - 1] == '\r') { + end--; + } + return Bun::toStringView(StringView_slice(sourceString, start, end)); + }; + + unsigned lineEnd = divot; + while (lineEnd < length && bytes[lineEnd] != '\n') { + lineEnd++; + } + unsigned lineStart = startOfLineEndingAt(lineEnd); // It is key to not clone this data because source code strings are large. // Usage of toStringView (non-owning) is safe as we ref the provider. @@ -185,41 +197,16 @@ static void populateStackFramePosition(const JSC::StackFrame& stackFrame, BunStr (*referenced_source_provider)->deref(); } *referenced_source_provider = provider; - source_lines[0] = Bun::toStringView(sourceString.substring(lineStart, lineEnd - lineStart)); + source_lines[0] = lineText(lineStart, lineEnd); source_line_numbers[0] = location.line(); - if (lineStart > 0) { - auto byte_offset_in_source_string = lineStart - 1; - uint8_t source_line_i = 1; - auto remaining_lines_to_grab = source_lines_count - 1; - - { - // This should probably be code points instead of newlines - while (byte_offset_in_source_string > 0 && bytes[byte_offset_in_source_string] != '\n') { - byte_offset_in_source_string--; - } - - byte_offset_in_source_string -= byte_offset_in_source_string > 0; - } - - while (byte_offset_in_source_string > 0 && remaining_lines_to_grab > 0) { - unsigned int end_of_line_offset = byte_offset_in_source_string; - - // This should probably be code points instead of newlines - while (byte_offset_in_source_string > 0 && bytes[byte_offset_in_source_string] != '\n') { - byte_offset_in_source_string--; - } - - // We are at the beginning of the line - source_lines[source_line_i] = Bun::toStringView(sourceString.substring(byte_offset_in_source_string, end_of_line_offset - byte_offset_in_source_string + 1)); - - source_line_numbers[source_line_i] = location.line().fromZeroBasedInt(location.line().zeroBasedInt() - source_line_i); - source_line_i++; - - remaining_lines_to_grab--; - - byte_offset_in_source_string -= byte_offset_in_source_string > 0; - } + // Lines above: `bytes[lineStart - 1]` is the '\n' ending the line above the one just collected. + for (uint8_t i = 1; i < source_lines_count && lineStart > 0; i++) { + const unsigned aboveEnd = lineStart - 1; + const unsigned aboveStart = startOfLineEndingAt(aboveEnd); + source_lines[i] = lineText(aboveStart, aboveEnd); + source_line_numbers[i] = OrdinalNumber::fromZeroBasedInt(location.line_zero_based - i); + lineStart = aboveStart; } } } diff --git a/test/js/node/vm/__snapshots__/vm-sourceUrl.test.ts.snap b/test/js/node/vm/__snapshots__/vm-sourceUrl.test.ts.snap index 650537102d0a..a9dbd738e57c 100644 --- a/test/js/node/vm/__snapshots__/vm-sourceUrl.test.ts.snap +++ b/test/js/node/vm/__snapshots__/vm-sourceUrl.test.ts.snap @@ -12,7 +12,10 @@ Error: hello `; exports[`can get sourceURL inside node:vm 1`] = ` -"4 | return Bun.inspect(new Error("hello")); +"1 | +2 | +3 | function hello() { +4 | return Bun.inspect(new Error("hello")); ^ error: hello at hello (hellohello.js:4:24) @@ -22,7 +25,10 @@ error: hello `; exports[`eval sourceURL is correct 1`] = ` -"4 | return Bun.inspect(new Error("hello")); +"1 | +2 | +3 | function hello() { +4 | return Bun.inspect(new Error("hello")); ^ error: hello at hello (hellohello.js:4:24) diff --git a/test/js/node/vm/vm.test.ts b/test/js/node/vm/vm.test.ts index d3d04239d179..dd49ce8ee9ed 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, isWindows, normalizeBunSnapshot } from "harness"; import { compileFunction, constants, @@ -1484,3 +1484,101 @@ test("node:vm Object.defineProperty on the context global when the sandbox is an expect(stdout.trim()).toBe(JSON.stringify({ result: 1, sandboxArray: 1 })); expect(exitCode).toBe(0); }); + +// A vm script has no source map, so the code frame bun prints above an error +// thrown from it is cut out of the script text itself (the same path serves +// eval and new Function). Bun.inspect renders the same frame as the +// uncaught-error printer. +describe("code frame of an error thrown from a vm script", () => { + function codeFrame(source: string, options: { lineOffset?: number } = {}): string[] { + let error: unknown; + try { + // With displayErrors, node:vm replaces err.stack with node's own header. + runInThisContext(source, { filename: "frame.js", displayErrors: false, ...options }); + } catch (e) { + error = e; + } + return Bun.inspect(error).split("\n"); + } + + const throwLine = "throw new Error('x');"; + const numbered = (count: number) => Array.from({ length: count }, (_, i) => `'L${i + 1}';`); + // JSC positions a ReferenceError one past the end of the identifier. + const fooFrame = ["1 | 'L1';", "2 | foo", " ^", "ReferenceError: foo is not defined"]; + const fooAtEndOfSource = "'L1';\nfoo"; + + test.each([ + [ + "every line above the error, numbered", + [...numbered(4), throwLine].join("\n"), + ["1 | 'L1';", "2 | 'L2';", "3 | 'L3';", "4 | 'L4';", `5 | ${throwLine}`, " ^", "error: x"], + ], + ["error on line 1", [throwLine, "'L2';"].join("\n"), [`1 | ${throwLine}`, " ^", "error: x"]], + ["error on line 2", ["'L1';", throwLine].join("\n"), ["1 | 'L1';", `2 | ${throwLine}`, " ^", "error: x"]], + [ + "at most five lines above the error", + [...numbered(9), throwLine].join("\n"), + [ + " 5 | 'L5';", + " 6 | 'L6';", + " 7 | 'L7';", + " 8 | 'L8';", + " 9 | 'L9';", + `10 | ${throwLine}`, + " ^", + "error: x", + ], + ], + [ + "blank lines, including a blank first line", + ["", "'L2';", "", throwLine].join("\n"), + ["1 | ", "2 | 'L2';", "3 | ", `4 | ${throwLine}`, " ^", "error: x"], + ], + [ + "CRLF line endings, including a blank line", + ["'L1';", "'L2';", "", throwLine, "'L5';", ""].join("\r\n"), + ["1 | 'L1';", "2 | 'L2';", "3 | ", `4 | ${throwLine}`, " ^", "error: x"], + ], + ["error positioned on the newline ending its line", "'L1';\nfoo\n'L3';", fooFrame], + ["error positioned at the end of the source", fooAtEndOfSource, fooFrame], + ])("%s", (_, source, expected) => { + expect(codeFrame(source).slice(0, expected.length)).toEqual(expected); + }); + + test("line numbers include lineOffset", () => { + const expected = ["11 | 'L1';", "12 | 'L2';", `13 | ${throwLine}`, " ^", "error: x"]; + expect(codeFrame([...numbered(2), throwLine].join("\n"), { lineOffset: 10 }).slice(0, expected.length)).toEqual( + expected, + ); + }); + + test("uncaught error", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + `require("node:vm").runInThisContext(${JSON.stringify(fooAtEndOfSource)}, { filename: "frame.js", displayErrors: false })`, + ], + env: { + ...bunEnv, + // Malloc=1 routes JSC's allocations through the system allocator so ASAN + // sees a read past the end of the source string (bmalloc's system heap + // is unimplemented on Windows, which has no ASAN lane anyway); + // detect_leaks=0 because it also exposes JSC's never-freed startup + // allocations to LSAN. + ...(isWindows ? {} : { Malloc: "1" }), + ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=0"].filter(Boolean).join(":"), + }, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect({ stdout, frame: stderr.split("\n").slice(0, fooFrame.length), exitCode }).toEqual({ + stdout: "", + frame: fooFrame, + exitCode: 1, + }); + }); +});