errors,repl: align prepareStackTrace behavior

This fixes the issue that multiple prepareStackTrace functions
existed that had to duplicate code in multiple spots. Instead, the
diverging part is now replaced during runtime. This reduces the
code overhead and makes sure there is no duplication.

It also fixes the issue that `overrideStackTrace` usage would not
adhere to the `hideStackFrame` calls. Frames are now removed before
passing them to the override method.

A second fix is for the repl: the stack frames passed to a user
defined Error.prepareStackTrace() method are now in the correct
order.
As drive-by it also improves the performance for repl errors
marginally.

Signed-off-by: Ruben Bridgewater <[email protected]>

Author: BridgeAR

lib/internal/errors.js

@@ -23,6 +23,7 @@ const {
   ArrayPrototypeSplice,
   Error,
   ErrorPrototypeToString,
+  FunctionPrototypeCall,
   JSONStringify,
   MapPrototypeGet,
   MathMax,
@@ -73,18 +74,19 @@ const kTypes = [
 
 const MainContextError = Error;
 const overrideStackTrace = new SafeWeakMap();
-const kNoOverride = Symbol('kNoOverride');
 const nodeInternalPrefix = '__node_internal_';
 
-const prepareStackTrace = (globalThis, error, trace) => {
-  // API for node internals to override error stack formatting
-  // without interfering with userland code.
-  const fn = overrideStackTrace.get(error);
-  if (fn !== undefined) {
-    overrideStackTrace.delete(error);
-    return fn(error, trace);
-  }
+const defaultFormatStackTrace = (errorString, trace) => {
+  return `${errorString}\n    at ${ArrayPrototypeJoin(trace, '\n    at ')}`;
+};
+let formatStackTrace = defaultFormatStackTrace;
 
+function setFormatStackTrace(fn = defaultFormatStackTrace) {
+  formatStackTrace = fn;
+}
+
+const prepareStackTrace = (globalThis, error, trace) => {
+  // Remove stack frames that should be hidden.
   const firstFrame = trace[0]?.getFunctionName();
   if (firstFrame && StringPrototypeStartsWith(firstFrame, nodeInternalPrefix)) {
     let i = trace.length - 1;
@@ -100,9 +102,27 @@ const prepareStackTrace = (globalThis, error, trace) => {
     }
   }
 
-  const globalOverride =
-    maybeOverridePrepareStackTrace(globalThis, error, trace);
-  if (globalOverride !== kNoOverride) return globalOverride;
+  // API for node internals to override error stack formatting
+  // without interfering with userland code.
+  const fn = overrideStackTrace.get(error);
+  if (fn !== undefined) {
+    overrideStackTrace.delete(error);
+    return fn(error, trace);
+  }
+
+  // Polyfill of V8's Error.prepareStackTrace API.
+  // https://crbug.com/v8/7848
+  // `globalThis` is the global that contains the constructor which
+  // created `error`.
+  if (typeof globalThis.Error?.prepareStackTrace === 'function') {
+    return globalThis.Error.prepareStackTrace(error, trace);
+  }
+  // We still have legacy usage that depends on the main context's `Error`
+  // being used, even when the error is from a different context.
+  // TODO(devsnek): evaluate if this can be eventually deprecated/removed.
+  if (typeof MainContextError.prepareStackTrace === 'function') {
+    return MainContextError.prepareStackTrace(error, trace);
+  }
 
   // Normal error formatting:
   //
@@ -123,25 +143,7 @@ const prepareStackTrace = (globalThis, error, trace) => {
   if (trace.length === 0) {
     return errorString;
   }
-  return `${errorString}\n    at ${ArrayPrototypeJoin(trace, '\n    at ')}`;
-};
-
-const maybeOverridePrepareStackTrace = (globalThis, error, trace) => {
-  // Polyfill of V8's Error.prepareStackTrace API.
-  // https://crbug.com/v8/7848
-  // `globalThis` is the global that contains the constructor which
-  // created `error`.
-  if (typeof globalThis.Error?.prepareStackTrace === 'function') {
-    return globalThis.Error.prepareStackTrace(error, trace);
-  }
-  // We still have legacy usage that depends on the main context's `Error`
-  // being used, even when the error is from a different context.
-  // TODO(devsnek): evaluate if this can be eventually deprecated/removed.
-  if (typeof MainContextError.prepareStackTrace === 'function') {
-    return MainContextError.prepareStackTrace(error, trace);
-  }
-
-  return kNoOverride;
+  return formatStackTrace(errorString, trace);
 };
 
 const aggregateTwoErrors = hideStackFrames((innerError, outerError) => {
@@ -776,8 +778,7 @@ function hideInternalStackFrames(error) {
         result += `\n    at ${frame}`;
       }
     }
-    result = error + result;
-    return result;
+    return error + result;
   });
 }
 
@@ -869,8 +870,7 @@ module.exports = {
   setStackTraceLimit,
   isStackOverflowError,
   kEnhanceStackBeforeInspector,
-  kNoOverride,
-  maybeOverridePrepareStackTrace,
+  setFormatStackTrace,
   overrideStackTrace,
   prepareStackTrace,
   setArrowMessage,

lib/internal/source_map/prepare_stack_trace.js

@@ -3,8 +3,6 @@
 const {
   ArrayPrototypeForEach,
   ArrayPrototypeIndexOf,
-  ErrorPrototypeToString,
-  ObjectPrototype,
   RegExpPrototypeSymbolSplit,
   StringPrototypeRepeat,
   StringPrototypeSlice,
@@ -18,48 +16,12 @@ let debug = require('internal/util/debuglog').debuglog('source_map', (fn) => {
 const { getStringWidth } = require('internal/util/inspect');
 const { readFileSync } = require('fs');
 const { findSourceMap } = require('internal/source_map/source_map_cache');
-const {
-  kNoOverride,
-  overrideStackTrace,
-  maybeOverridePrepareStackTrace,
-} = require('internal/errors');
 const { fileURLToPath } = require('internal/url');
 const { setGetSourceMapErrorSource } = internalBinding('errors');
 
-const MainContextObjectToString = ObjectPrototype.toString;
-
 // Create a prettified stacktrace, inserting context from source maps
 // if possible.
-const prepareStackTrace = (globalThis, error, trace) => {
-  // API for node internals to override error stack formatting
-  // without interfering with userland code.
-  // TODO(bcoe): add support for source-maps to repl.
-  const fn = overrideStackTrace.get(error);
-  if (fn !== undefined) {
-    overrideStackTrace.delete(error);
-    return fn(error, trace);
-  }
-
-  const globalOverride =
-    maybeOverridePrepareStackTrace(globalThis, error, trace);
-  if (globalOverride !== kNoOverride) return globalOverride;
-
-  let errorString;
-  // Do not use primordials here: we intercept user code here.
-  if (typeof error.toString === 'function' &&
-      error.toString !== MainContextObjectToString) {
-    errorString = error.toString();
-    if (errorString === '[Object object]') {
-      errorString = ErrorPrototypeToString(error);
-    }
-  } else {
-    errorString = ErrorPrototypeToString(error);
-  }
-
-  if (trace.length === 0) {
-    return errorString;
-  }
-
+function formatStackTrace(errorString, trace) {
   let lastSourceMap;
   let lastFileName;
   let preparedTrace = '';
@@ -114,7 +76,7 @@ const prepareStackTrace = (globalThis, error, trace) => {
     preparedTrace += `${str}${t}`;
   });
   return `${errorString}\n    at ${preparedTrace}`;
-};
+}
 
 // Transpilers may have removed the original symbol name used in the stack
 // trace, if possible restore it from the names field of the source map:
@@ -217,5 +179,5 @@ function getSourceMapErrorSource(fileName, lineNumber, columnNumber) {
 setGetSourceMapErrorSource(getSourceMapErrorSource);
 
 module.exports = {
-  prepareStackTrace,
+  formatStackTrace,
 };

lib/internal/source_map/source_map_cache.js

@@ -56,18 +56,23 @@ function setSourceMapsEnabled(val) {
     setSourceMapsEnabled,
     setPrepareStackTraceCallback
   } = internalBinding('errors');
+
   setSourceMapsEnabled(val);
+
+  const {
+    setFormatStackTrace,
+    prepareStackTrace,
+  } = require('internal/errors');
+
+  setPrepareStackTraceCallback(prepareStackTrace);
   if (val) {
     const {
-      prepareStackTrace
+      formatStackTrace
     } = require('internal/source_map/prepare_stack_trace');
-    setPrepareStackTraceCallback(prepareStackTrace);
+    setFormatStackTrace(formatStackTrace);
   } else if (sourceMapsEnabled !== undefined) {
     // Reset prepare stack trace callback only when disabling source maps.
-    const {
-      prepareStackTrace,
-    } = require('internal/errors');
-    setPrepareStackTraceCallback(prepareStackTrace);
+    setFormatStackTrace();
   }
 
   sourceMapsEnabled = val;

lib/repl.js

@@ -44,20 +44,17 @@
 
 const {
   ArrayPrototypeFilter,
-  ArrayPrototypeFindIndex,
   ArrayPrototypeForEach,
   ArrayPrototypeIncludes,
   ArrayPrototypeJoin,
   ArrayPrototypeMap,
   ArrayPrototypePop,
   ArrayPrototypePush,
   ArrayPrototypePushApply,
-  ArrayPrototypeReverse,
   ArrayPrototypeShift,
   ArrayPrototypeSlice,
   ArrayPrototypeSome,
   ArrayPrototypeSort,
-  ArrayPrototypeSplice,
   ArrayPrototypeUnshift,
   Boolean,
   Error,
@@ -646,27 +643,30 @@ function REPLServer(prompt,
 
     if (typeof e === 'object' && e !== null) {
       overrideStackTrace.set(e, (error, stackFrames) => {
-        let frames;
+        let frames = stackFrames;
         if (typeof stackFrames === 'object') {
           // Search from the bottom of the call stack to
           // find the first frame with a null function name
-          const idx = ArrayPrototypeFindIndex(
-            ArrayPrototypeReverse(stackFrames),
-            (frame) => frame.getFunctionName() === null,
+          const idx = stackFrames.findLastIndex(
+            (frame, i) => frame.getFunctionName() === null,
           );
-          // If found, get rid of it and everything below it
-          frames = ArrayPrototypeSplice(stackFrames, idx + 1);
-        } else {
-          frames = stackFrames;
+          if (idx !== -1) {
+            // If found, get rid of it and everything above it
+            frames = ArrayPrototypeSlice(stackFrames, 0, idx);
+          }
         }
         // FIXME(devsnek): this is inconsistent with the checks
         // that the real prepareStackTrace dispatch uses in
         // lib/internal/errors.js.
         if (typeof Error.prepareStackTrace === 'function') {
           return Error.prepareStackTrace(error, frames);
         }
-        ArrayPrototypePush(frames, error);
-        return ArrayPrototypeJoin(ArrayPrototypeReverse(frames), '\n    at ');
+
+        if (frames.length === 0) {
+          return `${error}`;
+        }
+
+        return `${error}\n    at ${ArrayPrototypeJoin(frames, '\n    at ')}`;
       });
       decorateErrorStack(e);
 

test/parallel/test-repl-pretty-custom-stack.js

@@ -42,8 +42,8 @@ const origPrepareStackTrace = Error.prepareStackTrace;
 Error.prepareStackTrace = (err, stack) => {
   if (err instanceof SyntaxError)
     return err.toString();
-  stack.push(err);
-  return stack.reverse().join('--->\n');
+  stack.unshift(err);
+  return stack.join('--->\n');
 };
 
 process.on('uncaughtException', (e) => {