lib: stop using prepareStackTrace

PR-URL: #29777 Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
nodejs · Oct 2, 2019 · 70c2444 · 70c2444
1 parent 48a1f75
commit 70c2444
Show file tree

Hide file tree

Showing 4 changed files with 65 additions and 52 deletions.
diff --git a/lib/assert.js b/lib/assert.js
@@ -23,13 +23,16 @@
 const { Object, ObjectPrototype } = primordials;
 
 const { Buffer } = require('buffer');
-const { codes: {
-  ERR_AMBIGUOUS_ARGUMENT,
-  ERR_INVALID_ARG_TYPE,
-  ERR_INVALID_ARG_VALUE,
-  ERR_INVALID_RETURN_VALUE,
-  ERR_MISSING_ARGS
-} } = require('internal/errors');
+const {
+  codes: {
+    ERR_AMBIGUOUS_ARGUMENT,
+    ERR_INVALID_ARG_TYPE,
+    ERR_INVALID_ARG_VALUE,
+    ERR_INVALID_RETURN_VALUE,
+    ERR_MISSING_ARGS,
+  },
+  overrideStackTrace,
+} = require('internal/errors');
 const AssertionError = require('internal/assert/assertion_error');
 const { openSync, closeSync, readSync } = require('fs');
 const { inspect } = require('internal/util/inspect');
@@ -262,10 +265,8 @@ function getErrMessage(message, fn) {
   Error.captureStackTrace(err, fn);
   Error.stackTraceLimit = tmpLimit;
 
-  const tmpPrepare = Error.prepareStackTrace;
-  Error.prepareStackTrace = (_, stack) => stack;
+  overrideStackTrace.set(err, (_, stack) => stack);
   const call = err.stack[0];
-  Error.prepareStackTrace = tmpPrepare;
 
   const filename = call.getFileName();
   const line = call.getLineNumber() - 1;

diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -21,9 +21,18 @@ const { kMaxLength } = internalBinding('buffer');
 
 const MainContextError = Error;
 const ErrorToString = Error.prototype.toString;
-// Polyfill of V8's Error.prepareStackTrace API.
-// https://crbug.com/v8/7848
+const overrideStackTrace = new WeakMap();
 const prepareStackTrace = (globalThis, error, trace) => {
+  // API for node internals to override error stack formatting
+  // without interfering with userland code.
+  if (overrideStackTrace.has(error)) {
+    const f = overrideStackTrace.get(error);
+    overrideStackTrace.delete(error);
+    return f(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') {
@@ -36,6 +45,11 @@ const prepareStackTrace = (globalThis, error, trace) => {
     return MainContextError.prepareStackTrace(error, trace);
   }
 
+  // Normal error formatting:
+  //
+  // Error: Message
+  //     at function (file)
+  //     at file
   const errorString = ErrorToString.call(error);
   if (trace.length === 0) {
     return errorString;
@@ -680,6 +694,7 @@ module.exports = {
   // This is exported only to facilitate testing.
   E,
   prepareStackTrace,
+  overrideStackTrace,
   kEnhanceStackBeforeInspector,
   fatalExceptionStackEnhancers
 };

diff --git a/lib/internal/util.js b/lib/internal/util.js
@@ -7,7 +7,8 @@ const {
     ERR_NO_CRYPTO,
     ERR_UNKNOWN_SIGNAL
   },
-  uvErrmapGet
+  uvErrmapGet,
+  overrideStackTrace,
 } = require('internal/errors');
 const { signals } = internalBinding('constants').os;
 const {
@@ -338,15 +339,13 @@ function isInsideNodeModules() {
     // side-effect-free. Since this is currently only used for a deprecated API,
     // the perf implications should be okay.
     getStructuredStack = runInNewContext(`(function() {
-      Error.prepareStackTrace = function(err, trace) {
-        return trace;
-      };
       Error.stackTraceLimit = Infinity;
-
       return function structuredStack() {
-        return new Error().stack;
+        const e = new Error();
+        overrideStackTrace.set(e, (err, trace) => trace);
+        return e.stack;
       };
-    })()`, {}, { filename: 'structured-stack' });
+    })()`, { overrideStackTrace }, { filename: 'structured-stack' });
   }
 
   const stack = getStructuredStack();

diff --git a/lib/repl.js b/lib/repl.js
@@ -69,12 +69,15 @@ const CJSModule = require('internal/modules/cjs/loader').Module;
 const domain = require('domain');
 const debug = require('internal/util/debuglog').debuglog('repl');
 const {
-  ERR_CANNOT_WATCH_SIGINT,
-  ERR_INVALID_ARG_TYPE,
-  ERR_INVALID_REPL_EVAL_CONFIG,
-  ERR_INVALID_REPL_INPUT,
-  ERR_SCRIPT_EXECUTION_INTERRUPTED
-} = require('internal/errors').codes;
+  codes: {
+    ERR_CANNOT_WATCH_SIGINT,
+    ERR_INVALID_ARG_TYPE,
+    ERR_INVALID_REPL_EVAL_CONFIG,
+    ERR_INVALID_REPL_INPUT,
+    ERR_SCRIPT_EXECUTION_INTERRUPTED,
+  },
+  overrideStackTrace,
+} = require('internal/errors');
 const { sendInspectorCommand } = require('internal/util/inspector');
 const experimentalREPLAwait = require('internal/options').getOptionValue(
   '--experimental-repl-await'
@@ -473,10 +476,29 @@ function REPLServer(prompt,
     let errStack = '';
 
     if (typeof e === 'object' && e !== null) {
-      const pstrace = Error.prepareStackTrace;
-      Error.prepareStackTrace = prepareStackTrace(pstrace);
+      overrideStackTrace.set(e, (error, stackFrames) => {
+        let frames;
+        if (typeof stackFrames === 'object') {
+          // Search from the bottom of the call stack to
+          // find the first frame with a null function name
+          const idx = stackFrames
+            .reverse()
+            .findIndex((frame) => frame.getFunctionName() === null);
+          // If found, get rid of it and everything below it
+          frames = stackFrames.splice(idx + 1);
+        } else {
+          frames = stackFrames;
+        }
+        // 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);
+        }
+        frames.push(error);
+        return frames.reverse().join('\n    at ');
+      });
       decorateErrorStack(e);
-      Error.prepareStackTrace = pstrace;
 
       if (e.domainThrown) {
         delete e.domain;
@@ -590,30 +612,6 @@ function REPLServer(prompt,
     }
   }
 
-  function filterInternalStackFrames(structuredStack) {
-    // Search from the bottom of the call stack to
-    // find the first frame with a null function name
-    if (typeof structuredStack !== 'object')
-      return structuredStack;
-    const idx = structuredStack.reverse().findIndex(
-      (frame) => frame.getFunctionName() === null);
-
-    // If found, get rid of it and everything below it
-    structuredStack = structuredStack.splice(idx + 1);
-    return structuredStack;
-  }
-
-  function prepareStackTrace(fn) {
-    return (error, stackFrames) => {
-      const frames = filterInternalStackFrames(stackFrames);
-      if (fn) {
-        return fn(error, frames);
-      }
-      frames.push(error);
-      return frames.reverse().join('\n    at ');
-    };
-  }
-
   function _parseREPLKeyword(keyword, rest) {
     const cmd = this.commands[keyword];
     if (cmd) {