Error.prepareStackTrace: index source URLs by visible frame, not by JSC frame

src/jsc/bindings/ErrorStackTrace.cpp

@@ -94,7 +94,7 @@ bool isImplementationVisibilityPrivate(const JSC::StackFrame& frame)
     return implementationVisibility != ImplementationVisibility::Public;
 }
 
-JSCStackTrace JSCStackTrace::fromExisting(JSC::VM& vm, const WTF::Vector<JSC::StackFrame>& existingFrames)
+JSCStackTrace JSCStackTrace::fromExisting(JSC::VM& vm, const WTF::Vector<JSC::StackFrame>& existingFrames, WTF::Vector<const JSC::StackFrame*>& visibleFrames)
 {
     WTF::Vector<JSCStackFrame> newFrames;
 
@@ -104,9 +104,12 @@ JSCStackTrace JSCStackTrace::fromExisting(JSC::VM& vm, const WTF::Vector<JSC::St
     }
 
     newFrames.reserveInitialCapacity(frameCount);
+    visibleFrames.reserveInitialCapacity(frameCount);
     for (size_t i = 0; i < frameCount; i++) {
-        if (!isImplementationVisibilityPrivate(existingFrames.at(i))) {
-            newFrames.constructAndAppend(vm, existingFrames.at(i));
+        const JSC::StackFrame& frame = existingFrames.at(i);
+        if (!isImplementationVisibilityPrivate(frame)) {
+            newFrames.constructAndAppend(vm, frame);
+            visibleFrames.append(&frame);
         }
     }
 
@@ -182,27 +185,6 @@ void JSCStackTrace::getFramesForCaller(JSC::VM& vm, JSC::CallFrame* callFrame, J
         stackTrace.shrink(stackTraceLimit);
 }
 
-JSCStackTrace JSCStackTrace::getStackTraceForThrownValue(JSC::VM& vm, JSC::JSValue thrownValue)
-{
-    const WTF::Vector<JSC::StackFrame>* jscStackTrace = nullptr;
-
-    JSC::Exception* currentException = DECLARE_TOP_EXCEPTION_SCOPE(vm).exception();
-    if (currentException && currentException->value() == thrownValue) {
-        jscStackTrace = &currentException->stack();
-    } else {
-        JSC::ErrorInstance* error = dynamicDowncast<JSC::ErrorInstance>(thrownValue);
-        if (error) {
-            jscStackTrace = error->stackTrace();
-        }
-    }
-
-    if (!jscStackTrace) {
-        return JSCStackTrace();
-    }
-
-    return fromExisting(vm, *jscStackTrace);
-}
-
 static bool isVisibleBuiltinFunction(JSC::CodeBlock* codeBlock)
 {
     if (!codeBlock->ownerExecutable()) {

src/jsc/bindings/ErrorStackTrace.h

@@ -179,28 +179,14 @@ class JSCStackTrace {
 
     WTF::Vector<JSCStackFrame>&& frames() { return WTF::move(m_frames); }
 
-    static JSCStackTrace fromExisting(JSC::VM& vm, const WTF::Vector<JSC::StackFrame>& existingFrames);
+    /* Skips private-visibility frames. JSC only omits them from `existingFrames` itself while
+     * Options::showPrivateScriptsInStackTraces() is off (debug builds turn it on), so the result can be
+     * shorter than `existingFrames`. `visibleFrames` receives the JSC::StackFrame behind each returned
+     * frame at the same index, pointing into `existingFrames`. */
+    static JSCStackTrace fromExisting(JSC::VM& vm, const WTF::Vector<JSC::StackFrame>& existingFrames, WTF::Vector<const JSC::StackFrame*>& visibleFrames);
 
     static void getFramesForCaller(JSC::VM& vm, JSC::CallFrame* callFrame, JSC::JSCell* owner, JSC::JSValue caller, WTF::Vector<JSC::StackFrame>& stackTrace, size_t stackTraceLimit);
 
-    /* In JSC, JSC::Exception points to the actual value that was thrown, usually
-     * a JSC::ErrorInstance (but could be any JSValue). In v8, on the other hand,
-     * TryCatch::Exception returns the thrown value, and we follow the same rule in jscshim.
-     * This is a problem, since JSC::Exception is the one that holds the stack trace.
-     * ErrorInstances might also hold the stack trace (until the error properties are
-     * "materialized" and it is no longer needed). So, to try to get the stack trace for a thrown JSValue,
-     * we'll try two things:
-     * - If the current JSC (vm) exception points to our value, it means our value is probably the current
-     *   exception and we could take the stack trace from the vm's current JSC::Exception. The downside
-     *   of doing this is that we'll get the last stack trace of the thrown value, meaning that if the value
-     *   was thrown, stored in the api and than re-thrown, we'll get the latest stack trace and not the one
-     *   that was available when we stored it. For now it'll do.
-     * - If that failed and our thrown value is a JSC::ErrorInstance, we'll try to use it's stack trace,
-     *   if it currently has one.
-     *
-     * Return value must remain stack allocated */
-    static JSCStackTrace getStackTraceForThrownValue(JSC::VM& vm, JSC::JSValue thrownValue);
-
 private:
     JSCStackTrace(WTF::Vector<JSCStackFrame>& frames)
         : m_frames(WTF::move(frames))

src/jsc/bindings/FormatStackTraceForJS.cpp

@@ -432,7 +432,9 @@ static JSValue computeErrorInfoWithPrepareStackTrace(JSC::VM& vm, Zig::GlobalObj
 {
     auto scope = DECLARE_THROW_SCOPE(vm);
 
-    JSCStackTrace stackTrace = JSCStackTrace::fromExisting(vm, stackFrames);
+    // Everything below is indexed per CallSite; `stackFrames` may still hold private frames that get none.
+    WTF::Vector<const JSC::StackFrame*> visibleFrames;
+    JSCStackTrace stackTrace = JSCStackTrace::fromExisting(vm, stackFrames, visibleFrames);
 
     // Note: we cannot use tryCreateUninitializedRestricted here because we cannot allocate memory inside initializeIndex()
     MarkedArgumentBuffer callSites;
@@ -454,7 +456,7 @@ static JSValue computeErrorInfoWithPrepareStackTrace(JSC::VM& vm, Zig::GlobalObj
 
     for (int i = 0; i < n; i++) {
         ZigStackFrame& frame = remappedFrames[i];
-        auto& stackFrame = stackFrames.at(i);
+        const JSC::StackFrame& stackFrame = *visibleFrames[i];
         sourceURLs[i] = Zig::sourceURL(vm, stackFrame);
         didRemap[i] = false;
         frame.position.line_zero_based = -1;

test/js/node/v8/capture-stack-trace.test.js

@@ -1,7 +1,7 @@
 import { nativeFrameForTesting } from "bun:internal-for-testing";
 import { noInline } from "bun:jsc";
 import { afterEach, expect, mock, test } from "bun:test";
-import { bunEnv, bunExe } from "harness";
+import { bunEnv, bunExe, tempDir } from "harness";
 const origPrepareStackTrace = Error.prepareStackTrace;
 afterEach(() => {
   Error.prepareStackTrace = origPrepareStackTrace;
@@ -1121,3 +1121,76 @@ test("lazy error-info materialization does not store an empty stack value when t
   });
   expect(exitCode).toBe(0);
 });
+
+test("Error.prepareStackTrace call sites keep their own file when a hidden frame is on the stack", async () => {
+  // A bound function call is a frame with private implementation visibility. JSC omits it from the
+  // trace unless showPrivateScriptsInStackTraces is on (debug builds turn it on); Bun then has to
+  // drop it while building the CallSites, and the frames after it must keep their own file.
+  using dir = tempDir("prepare-stack-trace-hidden-frame", {
+    "main.cjs": [
+      `const { outer, viaAsyncLocalStorage } = require("./callers.cjs");`,
+      `function inner() {`,
+      `  const error = new Error("boom");`,
+      `  return error;`,
+      `}`,
+      `function main() {`,
+      `  const error = outer(inner.bind(null));`,
+      `  return error;`,
+      `}`,
+      `const { basename } = require("node:path");`,
+      `const wanted = new Set(["inner", "outer", "main", "run", "viaAsyncLocalStorage"]);`,
+      `Error.prepareStackTrace = (_error, callSites) =>`,
+      `  callSites`,
+      `    .filter(callSite => wanted.has(callSite.getFunctionName()))`,
+      `    .map(callSite => ({`,
+      `      name: callSite.getFunctionName(),`,
+      `      file: basename(callSite.getFileName()),`,
+      `      line: callSite.getLineNumber(),`,
+      `    }));`,
+      `console.log(JSON.stringify({ bound: main().stack, asyncLocalStorage: viaAsyncLocalStorage(inner.bind(null)).stack }));`,
+    ].join("\n"),
+    "callers.cjs": [
+      `const { AsyncLocalStorage } = require("node:async_hooks");`,
+      `exports.outer = function outer(callback) {`,
+      `  const error = callback();`,
+      `  return error;`,
+      `};`,
+      `const storage = new AsyncLocalStorage();`,
+      `exports.viaAsyncLocalStorage = function viaAsyncLocalStorage(callback) {`,
+      `  const error = storage.run("store", callback);`,
+      `  return error;`,
+      `};`,
+    ].join("\n"),
+  });
+
+  const results = await Promise.all(
+    ["0", "1"].map(async showPrivateScriptsInStackTraces => {
+      await using proc = Bun.spawn({
+        cmd: [bunExe(), "main.cjs"],
+        cwd: String(dir),
+        env: { ...bunEnv, BUN_JSC_showPrivateScriptsInStackTraces: showPrivateScriptsInStackTraces },
+        stdout: "pipe",
+        stderr: "pipe",
+      });
+      const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+      return { showPrivateScriptsInStackTraces, callSites: stdout && JSON.parse(stdout), stderr, exitCode };
+    }),
+  );
+
+  const expected = {
+    bound: [
+      { name: "inner", file: "main.cjs", line: 3 },
+      { name: "outer", file: "callers.cjs", line: 3 },
+      { name: "main", file: "main.cjs", line: 7 },
+    ],
+    asyncLocalStorage: [
+      { name: "inner", file: "main.cjs", line: 3 },
+      { name: "run", file: "node:async_hooks", line: expect.any(Number) },
+      { name: "viaAsyncLocalStorage", file: "callers.cjs", line: 8 },
+    ],
+  };
+  expect(results).toEqual([
+    { showPrivateScriptsInStackTraces: "0", callSites: expected, stderr: "", exitCode: 0 },
+    { showPrivateScriptsInStackTraces: "1", callSites: expected, stderr: "", exitCode: 0 },
+  ]);
+});