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

src/jsc/bindings/CallSite.cpp

@@ -20,7 +20,7 @@ namespace Zig {
 
 const JSC::ClassInfo CallSite::s_info = { "CallSite"_s, &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(CallSite) };
 
-void CallSite::finishCreation(VM& vm, JSC::JSGlobalObject* globalObject, JSCStackFrame& stackFrame, bool encounteredStrictFrame)
+void CallSite::finishCreation(VM& vm, JSCStackFrame& stackFrame, bool encounteredStrictFrame)
 {
     Base::finishCreation(vm);
 
@@ -39,20 +39,12 @@ void CallSite::finishCreation(VM& vm, JSC::JSGlobalObject* globalObject, JSCStac
         }
     }
 
-    // Initialize "this" and "function" (and set the "IsStrict" flag if needed)
-    JSC::CallFrame* callFrame = stackFrame.callFrame();
+    // JSC::StackFrame has no receiver, so getThis() is always undefined.
+    m_thisValue.set(vm, this, JSC::jsUndefined());
     if (isStrictFrame) {
-        m_thisValue.set(vm, this, JSC::jsUndefined());
         m_function.set(vm, this, JSC::jsUndefined());
         m_flags |= static_cast<unsigned int>(Flags::IsStrict);
     } else {
-        if (callFrame && callFrame->thisValue()) {
-            // We know that we're not in strict mode
-            m_thisValue.set(vm, this, callFrame->thisValue().toThis(globalObject, JSC::ECMAMode::sloppy()));
-        } else {
-            m_thisValue.set(vm, this, JSC::jsUndefined());
-        }
-
         m_function.set(vm, this, stackFrame.callee());
     }
 

src/jsc/bindings/CallSite.h

@@ -48,7 +48,7 @@ class CallSite final : public JSC::JSNonFinalObject {
     {
         auto& vm = JSC::getVM(globalObject);
         CallSite* callSite = new (NotNull, JSC::allocateCell<CallSite>(vm)) CallSite(vm, structure);
-        callSite->finishCreation(vm, globalObject, stackFrame, encounteredStrictFrame);
+        callSite->finishCreation(vm, stackFrame, encounteredStrictFrame);
         return callSite;
     }
 
@@ -101,7 +101,7 @@ class CallSite final : public JSC::JSNonFinalObject {
     {
     }
 
-    void finishCreation(VM& vm, JSC::JSGlobalObject* globalObject, JSCStackFrame& stackFrame, bool encounteredStrictFrame);
+    void finishCreation(VM& vm, JSCStackFrame& stackFrame, bool encounteredStrictFrame);
 
     DECLARE_VISIT_CHILDREN;
 };

src/jsc/bindings/ErrorStackTrace.cpp

@@ -182,27 +182,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()) {
@@ -213,57 +192,9 @@ static bool isVisibleBuiltinFunction(JSC::CodeBlock* codeBlock)
     return !Zig::sourceURL(source).isEmpty();
 }
 
-JSCStackFrame::JSCStackFrame(JSC::VM& vm, JSC::StackVisitor& visitor)
-    : m_vm(vm)
-    , m_codeBlock(nullptr)
-    , m_bytecodeIndex(JSC::BytecodeIndex())
-    , m_sourceURL()
-    , m_functionName()
-    , m_isWasmFrame(false)
-    , m_isAsync(false)
-    , m_sourcePositionsState(SourcePositionsState::NotCalculated)
-{
-    m_callee = visitor->callee().asCell();
-    m_callFrame = visitor->callFrame();
-
-    if (auto* codeBlock = visitor->codeBlock()) {
-        auto codeType = codeBlock->codeType();
-        if (codeType == JSC::FunctionCode || codeType == JSC::EvalCode) {
-            m_isFunctionOrEval = true;
-        }
-    }
-
-    // Based on JSC's GetStackTraceFunctor (Interpreter.cpp)
-    if (visitor->isNativeCalleeFrame()) {
-        auto* nativeCallee = visitor->callee().asNativeCallee();
-        switch (nativeCallee->category()) {
-        case NativeCallee::Category::Wasm: {
-            m_wasmFunctionIndexOrName = visitor->wasmFunctionIndexOrName();
-            m_isWasmFrame = true;
-            break;
-        }
-        case NativeCallee::Category::InlineCache: {
-            break;
-        }
-        }
-    } else if (auto* codeBlock = visitor->codeBlock()) {
-        auto* unlinkedCodeBlock = codeBlock->unlinkedCodeBlock();
-        if (!unlinkedCodeBlock->isBuiltinFunction() || isVisibleBuiltinFunction(codeBlock)) {
-            m_codeBlock = codeBlock;
-            m_bytecodeIndex = visitor->bytecodeIndex();
-        }
-    }
-
-    if (!m_bytecodeIndex && visitor->hasLineAndColumnInfo()) {
-        auto lineColumn = visitor->computeLineAndColumn();
-        m_sourcePositions = { OrdinalNumber::fromOneBasedInt(lineColumn.line), OrdinalNumber::fromOneBasedInt(lineColumn.column) };
-        m_sourcePositionsState = SourcePositionsState::Calculated;
-    }
-}
-
 JSCStackFrame::JSCStackFrame(JSC::VM& vm, const JSC::StackFrame& frame)
     : m_vm(vm)
-    , m_callFrame(nullptr)
+    , m_stackFrame(&frame)
     , m_codeBlock(nullptr)
     , m_bytecodeIndex(JSC::BytecodeIndex())
     , m_sourceURL()

src/jsc/bindings/ErrorStackTrace.h

@@ -16,8 +16,7 @@ using namespace WebCore;
 
 namespace Zig {
 
-/* JSCStackFrame is an alternative to JSC::StackFrame, which provides the following advantages\changes:
- * - Also hold the call frame (ExecState). This is mainly used by CallSite to get "this value".
+/* JSCStackFrame is a view over a JSC::StackFrame, which provides the following advantages\changes:
  * - More detailed and v8 compatible "source offsets" calculations: JSC::StackFrame only provides the
  *   line number and column numbers. It's column calculation seems to be different than v8's column.
  *   According to v8's unit tests, it seems that their column number points to the beginning of
@@ -46,11 +45,10 @@ class JSCStackFrame {
 
 private:
     JSC::VM& m_vm;
+    // Points into the vector this frame was built from (JSCStackTrace::fromExisting).
+    const JSC::StackFrame* m_stackFrame;
     JSC::JSCell* m_callee { nullptr };
 
-    // May be null
-    JSC::CallFrame* m_callFrame;
-
     // May be null
     JSC::CodeBlock* m_codeBlock { nullptr };
     JSC::BytecodeIndex m_bytecodeIndex;
@@ -77,11 +75,10 @@ class JSCStackFrame {
     SourcePositionsState m_sourcePositionsState;
 
 public:
-    JSCStackFrame(JSC::VM& vm, JSC::StackVisitor& visitor);
     JSCStackFrame(JSC::VM& vm, const JSC::StackFrame& frame);
 
+    const JSC::StackFrame& stackFrame() const { return *m_stackFrame; }
     JSC::JSCell* callee() const { return m_callee; }
-    JSC::CallFrame* callFrame() const { return m_callFrame; }
     JSC::CodeBlock* codeBlock() const { return m_codeBlock; }
 
     intptr_t sourceID() const;
@@ -179,28 +176,11 @@ class JSCStackTrace {
 
     WTF::Vector<JSCStackFrame>&& frames() { return WTF::move(m_frames); }
 
+    // Drops private-visibility frames (present when Options::showPrivateScriptsInStackTraces() is on), so the result does not index like existingFrames.
     static JSCStackTrace fromExisting(JSC::VM& vm, const WTF::Vector<JSC::StackFrame>& existingFrames);
 
     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

@@ -454,7 +454,8 @@ static JSValue computeErrorInfoWithPrepareStackTrace(JSC::VM& vm, Zig::GlobalObj
 
     for (int i = 0; i < n; i++) {
         ZigStackFrame& frame = remappedFrames[i];
-        auto& stackFrame = stackFrames.at(i);
+        JSCStackFrame& visibleFrame = stackTrace.at(i);
+        const JSC::StackFrame& stackFrame = visibleFrame.stackFrame();
         sourceURLs[i] = Zig::sourceURL(vm, stackFrame);
         didRemap[i] = false;
         frame.position.line_zero_based = -1;
@@ -478,7 +479,7 @@ static JSValue computeErrorInfoWithPrepareStackTrace(JSC::VM& vm, Zig::GlobalObj
         }
 
         if (globalObjectForFrame == globalObject) {
-            if (JSCStackFrame::SourcePositions* sourcePositions = stackTrace.at(i).getSourcePositions()) {
+            if (JSCStackFrame::SourcePositions* sourcePositions = visibleFrame.getSourcePositions()) {
                 frame.position.line_zero_based = sourcePositions->line.zeroBasedInt();
                 frame.position.column_zero_based = sourcePositions->column.zeroBasedInt();
             }

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 },
+  ]);
+});