lib: refactor Error.captureStackTrace() usage

When using `Errors.captureStackFrames` the error's stack property
is set again. This adds a helper function that wraps this functionality
in a simple API that does not only set the stack including the `code`
property but it also improves the performance to create the error.
The helper works for thrown errors and errors returned from wrapped
functions in case they are Node.js core errors.

PR-URL: #26738
Fixes: #26669
Fixes: #20253
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Author: BridgeAR

Diff for: lib/_http_outgoing.js

@@ -34,16 +34,19 @@ const {
   symbols: { async_id_symbol }
 } = require('internal/async_hooks');
 const {
-  ERR_HTTP_HEADERS_SENT,
-  ERR_HTTP_INVALID_HEADER_VALUE,
-  ERR_HTTP_TRAILER_INVALID,
-  ERR_INVALID_HTTP_TOKEN,
-  ERR_INVALID_ARG_TYPE,
-  ERR_INVALID_CHAR,
-  ERR_METHOD_NOT_IMPLEMENTED,
-  ERR_STREAM_CANNOT_PIPE,
-  ERR_STREAM_WRITE_AFTER_END
-} = require('internal/errors').codes;
+  codes: {
+    ERR_HTTP_HEADERS_SENT,
+    ERR_HTTP_INVALID_HEADER_VALUE,
+    ERR_HTTP_TRAILER_INVALID,
+    ERR_INVALID_HTTP_TOKEN,
+    ERR_INVALID_ARG_TYPE,
+    ERR_INVALID_CHAR,
+    ERR_METHOD_NOT_IMPLEMENTED,
+    ERR_STREAM_CANNOT_PIPE,
+    ERR_STREAM_WRITE_AFTER_END
+  },
+  hideStackFrames
+} = require('internal/errors');
 const { validateString } = require('internal/validators');
 
 const { CRLF, debug } = common;
@@ -443,39 +446,21 @@ function matchHeader(self, state, field, value) {
   }
 }
 
-function validateHeaderName(name) {
+const validateHeaderName = hideStackFrames((name) => {
   if (typeof name !== 'string' || !name || !checkIsHttpToken(name)) {
-    // Reducing the limit improves the performance significantly. We do not
-    // lose the stack frames due to the `captureStackTrace()` function that is
-    // called later.
-    const tmpLimit = Error.stackTraceLimit;
-    Error.stackTraceLimit = 0;
-    const err = new ERR_INVALID_HTTP_TOKEN('Header name', name);
-    Error.stackTraceLimit = tmpLimit;
-    Error.captureStackTrace(err, validateHeaderName);
-    throw err;
+    throw new ERR_INVALID_HTTP_TOKEN('Header name', name);
   }
-}
+});
 
-function validateHeaderValue(name, value) {
-  let err;
-  // Reducing the limit improves the performance significantly. We do not loose
-  // the stack frames due to the `captureStackTrace()` function that is called
-  // later.
-  const tmpLimit = Error.stackTraceLimit;
-  Error.stackTraceLimit = 0;
+const validateHeaderValue = hideStackFrames((name, value) => {
   if (value === undefined) {
-    err = new ERR_HTTP_INVALID_HEADER_VALUE(value, name);
-  } else if (checkInvalidHeaderChar(value)) {
-    debug('Header "%s" contains invalid characters', name);
-    err = new ERR_INVALID_CHAR('header content', name);
+    throw new ERR_HTTP_INVALID_HEADER_VALUE(value, name);
   }
-  Error.stackTraceLimit = tmpLimit;
-  if (err !== undefined) {
-    Error.captureStackTrace(err, validateHeaderValue);
-    throw err;
+  if (checkInvalidHeaderChar(value)) {
+    debug('Header "%s" contains invalid characters', name);
+    throw new ERR_INVALID_CHAR('header content', name);
   }
-}
+});
 
 OutgoingMessage.prototype.setHeader = function setHeader(name, value) {
   if (this._header) {

Diff for: lib/buffer.js

@@ -62,15 +62,18 @@ const {
 } = require('internal/util/inspect');
 
 const {
-  ERR_BUFFER_OUT_OF_BOUNDS,
-  ERR_OUT_OF_RANGE,
-  ERR_INVALID_ARG_TYPE,
-  ERR_INVALID_ARG_VALUE,
-  ERR_INVALID_BUFFER_SIZE,
-  ERR_INVALID_OPT_VALUE,
-  ERR_NO_LONGER_SUPPORTED,
-  ERR_UNKNOWN_ENCODING
-} = require('internal/errors').codes;
+  codes: {
+    ERR_BUFFER_OUT_OF_BOUNDS,
+    ERR_OUT_OF_RANGE,
+    ERR_INVALID_ARG_TYPE,
+    ERR_INVALID_ARG_VALUE,
+    ERR_INVALID_BUFFER_SIZE,
+    ERR_INVALID_OPT_VALUE,
+    ERR_NO_LONGER_SUPPORTED,
+    ERR_UNKNOWN_ENCODING
+  },
+  hideStackFrames
+} = require('internal/errors');
 const { validateString } = require('internal/validators');
 
 const {
@@ -247,20 +250,14 @@ Object.setPrototypeOf(Buffer, Uint8Array);
 // The 'assertSize' method will remove itself from the callstack when an error
 // occurs. This is done simply to keep the internal details of the
 // implementation from bleeding out to users.
-function assertSize(size) {
-  let err = null;
-
+const assertSize = hideStackFrames((size) => {
   if (typeof size !== 'number') {
-    err = new ERR_INVALID_ARG_TYPE('size', 'number', size);
-  } else if (!(size >= 0 && size <= kMaxLength)) {
-    err = new ERR_INVALID_OPT_VALUE.RangeError('size', size);
+    throw new ERR_INVALID_ARG_TYPE('size', 'number', size);
   }
-
-  if (err !== null) {
-    Error.captureStackTrace(err, assertSize);
-    throw err;
+  if (!(size >= 0 && size <= kMaxLength)) {
+    throw new ERR_INVALID_OPT_VALUE.RangeError('size', size);
   }
-}
+});
 
 /**
  * Creates a new filled Buffer instance.

Diff for: lib/fs.js

@@ -43,13 +43,15 @@ const pathModule = require('path');
 const { isArrayBufferView } = require('internal/util/types');
 const binding = internalBinding('fs');
 const { Buffer, kMaxLength } = require('buffer');
-const errors = require('internal/errors');
 const {
-  ERR_FS_FILE_TOO_LARGE,
-  ERR_INVALID_ARG_VALUE,
-  ERR_INVALID_ARG_TYPE,
-  ERR_INVALID_CALLBACK
-} = errors.codes;
+  codes: {
+    ERR_FS_FILE_TOO_LARGE,
+    ERR_INVALID_ARG_VALUE,
+    ERR_INVALID_ARG_TYPE,
+    ERR_INVALID_CALLBACK
+  },
+  uvException
+} = require('internal/errors');
 
 const { FSReqCallback, statValues } = binding;
 const { toPathIfFileURL } = require('internal/url');
@@ -114,10 +116,11 @@ function showTruncateDeprecation() {
 
 function handleErrorFromBinding(ctx) {
   if (ctx.errno !== undefined) {  // libuv error numbers
-    const err = errors.uvException(ctx);
+    const err = uvException(ctx);
     Error.captureStackTrace(err, handleErrorFromBinding);
     throw err;
-  } else if (ctx.error !== undefined) {  // errors created in C++ land.
+  }
+  if (ctx.error !== undefined) {  // errors created in C++ land.
     // TODO(joyeecheung): currently, ctx.error are encoding errors
     // usually caused by memory problems. We need to figure out proper error
     // code(s) for this.
@@ -310,7 +313,7 @@ function tryStatSync(fd, isUserFd) {
   const stats = binding.fstat(fd, false, undefined, ctx);
   if (ctx.errno !== undefined && !isUserFd) {
     fs.closeSync(fd);
-    throw errors.uvException(ctx);
+    throw uvException(ctx);
   }
   return stats;
 }

Diff for: lib/internal/crypto/pbkdf2.js

@@ -65,8 +65,8 @@ function check(password, salt, iterations, keylen, digest) {
 
   password = validateArrayBufferView(password, 'password');
   salt = validateArrayBufferView(salt, 'salt');
-  iterations = validateUint32(iterations, 'iterations', 0);
-  keylen = validateUint32(keylen, 'keylen', 0);
+  validateUint32(iterations, 'iterations', 0);
+  validateUint32(keylen, 'keylen', 0);
 
   return { password, salt, iterations, keylen, digest };
 }

Diff for: lib/internal/errors.js

@@ -18,7 +18,7 @@ const codes = {};
 const { kMaxLength } = internalBinding('buffer');
 const { defineProperty } = Object;
 
-let useOriginalName = false;
+let excludedStackFn;
 
 // Lazily loaded
 let util;
@@ -49,7 +49,15 @@ function lazyBuffer() {
 // and may have .path and .dest.
 class SystemError extends Error {
   constructor(key, context) {
-    super();
+    if (excludedStackFn === undefined) {
+      super();
+    } else {
+      const limit = Error.stackTraceLimit;
+      Error.stackTraceLimit = 0;
+      super();
+      // Reset the limit and setting the name property.
+      Error.stackTraceLimit = limit;
+    }
     const prefix = getMessage(key, [], this);
     let message = `${prefix}: ${context.syscall} returned ` +
                   `${context.code} (${context.message})`;
@@ -148,7 +156,15 @@ function makeSystemErrorWithCode(key) {
 function makeNodeErrorWithCode(Base, key) {
   return class NodeError extends Base {
     constructor(...args) {
-      super();
+      if (excludedStackFn === undefined) {
+        super();
+      } else {
+        const limit = Error.stackTraceLimit;
+        Error.stackTraceLimit = 0;
+        super();
+        // Reset the limit and setting the name property.
+        Error.stackTraceLimit = limit;
+      }
       const message = getMessage(key, args, this);
       Object.defineProperty(this, 'message', {
         value: message,
@@ -178,9 +194,30 @@ function makeNodeErrorWithCode(Base, key) {
   };
 }
 
+// This function removes unnecessary frames from Node.js core errors.
+function hideStackFrames(fn) {
+  return function hidden(...args) {
+    // Make sure the most outer `hideStackFrames()` function is used.
+    let setStackFn = false;
+    if (excludedStackFn === undefined) {
+      excludedStackFn = hidden;
+      setStackFn = true;
+    }
+    try {
+      return fn(...args);
+    } finally {
+      if (setStackFn === true) {
+        excludedStackFn = undefined;
+      }
+    }
+  };
+}
+
 function addCodeToName(err, name, code) {
-  if (useOriginalName) {
-    return;
+  // Set the stack
+  if (excludedStackFn !== undefined) {
+    // eslint-disable-next-line no-restricted-syntax
+    Error.captureStackTrace(err, excludedStackFn);
   }
   // Add the error code to the name to include it in the stack trace.
   err.name = `${name} [${code}]`;
@@ -308,6 +345,7 @@ function uvException(ctx) {
     err[prop] = ctx[prop];
   }
 
+  // TODO(BridgeAR): Show the `code` property as part of the stack.
   err.code = code;
   if (path) {
     err.path = path;
@@ -316,7 +354,7 @@ function uvException(ctx) {
     err.dest = dest;
   }
 
-  Error.captureStackTrace(err, uvException);
+  Error.captureStackTrace(err, excludedStackFn || uvException);
   return err;
 }
 
@@ -358,7 +396,7 @@ function uvExceptionWithHostPort(err, syscall, address, port) {
     ex.port = port;
   }
 
-  Error.captureStackTrace(ex, uvExceptionWithHostPort);
+  Error.captureStackTrace(ex, excludedStackFn || uvExceptionWithHostPort);
   return ex;
 }
 
@@ -386,7 +424,7 @@ function errnoException(err, syscall, original) {
   ex.code = ex.errno = code;
   ex.syscall = syscall;
 
-  Error.captureStackTrace(ex, errnoException);
+  Error.captureStackTrace(ex, excludedStackFn || errnoException);
   return ex;
 }
 
@@ -434,7 +472,7 @@ function exceptionWithHostPort(err, syscall, address, port, additional) {
     ex.port = port;
   }
 
-  Error.captureStackTrace(ex, exceptionWithHostPort);
+  Error.captureStackTrace(ex, excludedStackFn || exceptionWithHostPort);
   return ex;
 }
 
@@ -473,7 +511,8 @@ function dnsException(code, syscall, hostname) {
   if (hostname) {
     ex.hostname = hostname;
   }
-  Error.captureStackTrace(ex, dnsException);
+
+  Error.captureStackTrace(ex, excludedStackFn || dnsException);
   return ex;
 }
 
@@ -523,21 +562,19 @@ function oneOf(expected, thing) {
 }
 
 module.exports = {
+  addCodeToName, // Exported for NghttpError
+  codes,
   dnsException,
   errnoException,
   exceptionWithHostPort,
+  getMessage,
+  hideStackFrames,
+  isStackOverflowError,
   uvException,
   uvExceptionWithHostPort,
-  isStackOverflowError,
-  getMessage,
   SystemError,
-  codes,
   // This is exported only to facilitate testing.
-  E,
-  // This allows us to tell the type of the errors without using
-  // instanceof, which is necessary in WPT harness.
-  get useOriginalName() { return useOriginalName; },
-  set useOriginalName(value) { useOriginalName = value; }
+  E
 };
 
 // To declare an error message, use the E(sym, val, def) function above. The sym
@@ -556,7 +593,6 @@ module.exports = {
 // Note: Please try to keep these in alphabetical order
 //
 // Note: Node.js specific errors must begin with the prefix ERR_
-
 E('ERR_AMBIGUOUS_ARGUMENT', 'The "%s" argument is ambiguous. %s', TypeError);
 E('ERR_ARG_NOT_ITERABLE', '%s must be iterable', TypeError);
 E('ERR_ASSERTION', '%s', Error);
@@ -630,7 +666,10 @@ E('ERR_ENCODING_INVALID_ENCODED_DATA', function(encoding, ret) {
 }, TypeError);
 E('ERR_ENCODING_NOT_SUPPORTED', 'The "%s" encoding is not supported',
   RangeError);
-E('ERR_FALSY_VALUE_REJECTION', 'Promise was rejected with falsy value', Error);
+E('ERR_FALSY_VALUE_REJECTION', function(reason) {
+  this.reason = reason;
+  return 'Promise was rejected with falsy value';
+}, Error);
 E('ERR_FS_FILE_TOO_LARGE', 'File size (%s) is greater than possible Buffer: ' +
     `${kMaxLength} bytes`,
   RangeError);