src: fixup Error.stackTraceLimit during snapshot building

lib/internal/main/mksnapshot.js

@@ -23,17 +23,18 @@ const { emitWarningSync } = require('internal/process/warning');
 const {
   initializeCallbacks,
   namespace: {
-    addSerializeCallback,
     addDeserializeCallback,
     isBuildingSnapshot,
   },
+  addAfterUserSerializeCallback,
 } = require('internal/v8/startup_snapshot');
 
 const {
   prepareMainThreadExecution,
 } = require('internal/process/pre_execution');
 
 const path = require('path');
+const { getOptionValue } = require('internal/options');
 
 const supportedModules = new SafeSet(new SafeArrayIterator([
   // '_http_agent',
@@ -140,22 +141,56 @@ function main() {
   prepareMainThreadExecution(false, false);
   initializeCallbacks();
 
-  let stackTraceLimitDesc;
-  addDeserializeCallback(() => {
-    if (stackTraceLimitDesc !== undefined) {
-      ObjectDefineProperty(Error, 'stackTraceLimit', stackTraceLimitDesc);
+  // In a context created for building snapshots, V8 does not install Erorr.stackTraceLimit and as
+  // a result, if an error is created during the snapshot building process, error.stack would be
+  // undefined. To prevent users from tripping over this, install Erorr.stackTraceLimit based on
+  // --stack-trace-limit by ourselves (which defaults to 10).
+  // See https://chromium-review.googlesource.com/c/v8/v8/+/3319481
+  const initialStackTraceLimitDesc = {
+    value: getOptionValue('--stack-trace-limit'),
+    configurable: true,
+    writable: true,
+    enumerable: true,
+    __proto__: null,
+  };
+  ObjectDefineProperty(Error, 'stackTraceLimit', initialStackTraceLimitDesc);
+
+  let stackTraceLimitDescToRestore;
+  // Erorr.stackTraceLimit needs to be removed during serialization, because when V8 deserializes
+  // the snapshot, it expects Erorr.stackTraceLimit to be unset so that it can install it as a new property
+  // using the value of --stack-trace-limit.
+  addAfterUserSerializeCallback(() => {
+    const desc = ObjectGetOwnPropertyDescriptor(Error, 'stackTraceLimit');
+
+    // If it's modified by users, emit a warning.
+    if (desc && (
+      desc.value !== initialStackTraceLimitDesc.value ||
+        desc.configurable !== initialStackTraceLimitDesc.configurable ||
+        desc.writable !== initialStackTraceLimitDesc.writable ||
+        desc.enumerable !== initialStackTraceLimitDesc.enumerable
+    )) {
+      process._rawDebug('Error.stackTraceLimit has been modified by the snapshot builder script.');
+      // We want to use null-prototype objects to not rely on globally mutable
+      // %Object.prototype%.
+      if (desc.configurable) {
+        stackTraceLimitDescToRestore = desc;
+        ObjectSetPrototypeOf(stackTraceLimitDescToRestore, null);
+        process._rawDebug('It will be preserved after snapshot deserialization and override ' +
+                          '--stack-trace-limit passed into the deserialized application.\n' +
+                          'To allow --stack-trace-limit override in the deserialized application, ' +
+                          'delete Error.stackTraceLimit.');
+      } else {
+        process._rawDebug('It is not configurable and will crash the application upon deserialization.\n' +
+                          'To fix the error, make Error.stackTraceLimit configurable.');
+      }
     }
+
+    delete Error.stackTraceLimit;
   });
-  addSerializeCallback(() => {
-    stackTraceLimitDesc = ObjectGetOwnPropertyDescriptor(Error, 'stackTraceLimit');
 
-    if (stackTraceLimitDesc !== undefined) {
-      // We want to use null-prototype objects to not rely on globally mutable
-      // %Object.prototype%.
-      ObjectSetPrototypeOf(stackTraceLimitDesc, null);
-      process._rawDebug('Deleting Error.stackTraceLimit from the snapshot. ' +
-                        'It will be re-installed after deserialization');
-      delete Error.stackTraceLimit;
+  addDeserializeCallback(() => {
+    if (stackTraceLimitDescToRestore) {
+      ObjectDefineProperty(Error, 'stackTraceLimit', stackTraceLimitDescToRestore);
     }
   });
 

lib/internal/v8/startup_snapshot.js

@@ -58,11 +58,16 @@ function addDeserializeCallback(callback, data) {
 }
 
 const serializeCallbacks = [];
+const afterUserSerializeCallbacks = [];  // Callbacks to run after user callbacks
 function runSerializeCallbacks() {
   while (serializeCallbacks.length > 0) {
     const { 0: callback, 1: data } = serializeCallbacks.shift();
     callback(data);
   }
+  while (afterUserSerializeCallbacks.length > 0) {
+    const { 0: callback, 1: data } = afterUserSerializeCallbacks.shift();
+    callback(data);
+  }
 }
 
 function addSerializeCallback(callback, data) {
@@ -71,6 +76,10 @@ function addSerializeCallback(callback, data) {
   serializeCallbacks.push([callback, data]);
 }
 
+function addAfterUserSerializeCallback(callback, data) {
+  afterUserSerializeCallbacks.push([callback, data]);
+}
+
 function initializeCallbacks() {
   // Only run the serialize callbacks in snapshot building mode, otherwise
   // they throw.
@@ -117,4 +126,5 @@ module.exports = {
     setDeserializeMainFunction,
     isBuildingSnapshot,
   },
+  addAfterUserSerializeCallback,
 };

src/env-inl.h

@@ -433,6 +433,10 @@ inline double Environment::get_default_trigger_async_id() {
   return default_trigger_async_id;
 }
 
+inline int64_t Environment::stack_trace_limit() const {
+  return isolate_data_->options()->stack_trace_limit;
+}
+
 inline std::shared_ptr<EnvironmentOptions> Environment::options() {
   return options_;
 }

src/env.cc

@@ -1241,9 +1241,11 @@ void Environment::PrintSyncTrace() const {
 
   fprintf(
       stderr, "(node:%d) WARNING: Detected use of sync API\n", uv_os_getpid());
-  PrintStackTrace(isolate(),
-                  StackTrace::CurrentStackTrace(
-                      isolate(), stack_trace_limit(), StackTrace::kDetailed));
+  PrintStackTrace(
+      isolate(),
+      StackTrace::CurrentStackTrace(isolate(),
+                                    static_cast<int>(stack_trace_limit()),
+                                    StackTrace::kDetailed));
 }
 
 MaybeLocal<Value> Environment::RunSnapshotSerializeCallback() const {
@@ -1845,9 +1847,11 @@ void Environment::Exit(ExitCode exit_code) {
     fprintf(stderr,
             "WARNING: Exited the environment with code %d\n",
             static_cast<int>(exit_code));
-    PrintStackTrace(isolate(),
-                    StackTrace::CurrentStackTrace(
-                        isolate(), stack_trace_limit(), StackTrace::kDetailed));
+    PrintStackTrace(
+        isolate(),
+        StackTrace::CurrentStackTrace(isolate(),
+                                      static_cast<int>(stack_trace_limit()),
+                                      StackTrace::kDetailed));
   }
   process_exit_handler_(this, exit_code);
 }

src/env.h

@@ -977,7 +977,7 @@ class Environment final : public MemoryRetainer {
   inline std::shared_ptr<EnvironmentOptions> options();
   inline std::shared_ptr<ExclusiveAccess<HostPort>> inspector_host_port();
 
-  inline int32_t stack_trace_limit() const { return 10; }
+  inline int64_t stack_trace_limit() const;
 
 #if HAVE_INSPECTOR
   void set_coverage_connection(

src/node_options-inl.h

@@ -447,9 +447,14 @@ void OptionsParser<Options>::Parse(
       case kBoolean:
         *Lookup<bool>(info.field, options) = !is_negation;
         break;
-      case kInteger:
+      case kInteger: {
+        // Special case to pass --stack-trace-limit down to V8.
+        if (name == "--stack-trace-limit") {
+          v8_args->push_back(arg);
+        }
         *Lookup<int64_t>(info.field, options) = std::atoll(value.c_str());
         break;
+      }
       case kUInteger:
         *Lookup<uint64_t>(info.field, options) =
             std::strtoull(value.c_str(), nullptr, 10);

src/node_options.cc

@@ -899,7 +899,10 @@ PerIsolateOptionsParser::PerIsolateOptionsParser(
       "--perf-basic-prof-only-functions", "", V8Option{}, kAllowedInEnvvar);
   AddOption("--perf-prof", "", V8Option{}, kAllowedInEnvvar);
   AddOption("--perf-prof-unwinding-info", "", V8Option{}, kAllowedInEnvvar);
-  AddOption("--stack-trace-limit", "", V8Option{}, kAllowedInEnvvar);
+  AddOption("--stack-trace-limit",
+            "",
+            &PerIsolateOptions::stack_trace_limit,
+            kAllowedInEnvvar);
   AddOption("--disallow-code-generation-from-strings",
             "disallow eval and friends",
             V8Option{},
@@ -1286,6 +1289,11 @@ void GetCLIOptionsValues(const FunctionCallbackInfo<Value>& args) {
         if (item.first == "--abort-on-uncaught-exception") {
           value = Boolean::New(isolate,
                                s.original_per_env->abort_on_uncaught_exception);
+        } else if (item.first == "--stack-trace-limit") {
+          value =
+              Number::New(isolate,
+                          static_cast<double>(
+                              *_ppop_instance.Lookup<int64_t>(field, opts)));
         } else {
           value = undefined_value;
         }

src/node_options.h

@@ -270,6 +270,7 @@ class PerIsolateOptions : public Options {
   bool report_uncaught_exception = false;
   bool report_on_signal = false;
   bool experimental_shadow_realm = false;
+  int64_t stack_trace_limit = 10;
   std::string report_signal = "SIGUSR2";
   bool build_snapshot = false;
   std::string build_snapshot_config;

test/fixtures/deep-exit.js

@@ -0,0 +1,15 @@
+'use strict';
+
+// This is meant to be run with --trace-exit.
+
+const depth = parseInt(process.env.STACK_DEPTH) || 30;
+let counter = 1;
+function recurse() {
+  if (counter++ < depth) {
+    recurse();
+  } else {
+    process.exit(0);
+  }
+}
+
+recurse();

test/fixtures/snapshot/error-stack.js

@@ -0,0 +1,24 @@
+
+const {
+  setDeserializeMainFunction,
+} = require('v8').startupSnapshot;
+
+console.log(`During snapshot building, Error.stackTraceLimit =`, Error.stackTraceLimit);
+console.log(getError('During snapshot building', 30));
+
+setDeserializeMainFunction(() => {
+  console.log(`After snapshot deserialization, Error.stackTraceLimit =`, Error.stackTraceLimit);
+  console.log(getError('After snapshot deserialization', 30));
+});
+
+function getError(message, depth) {
+  let counter = 1;
+  function recurse() {
+    if (counter++ < depth) {
+      return recurse();
+    }
+    const error = new Error(message);
+    return error.stack;
+  }
+  return recurse();
+}

test/fixtures/snapshot/mutate-error-stack-trace-limit.js

@@ -0,0 +1,35 @@
+
+const {
+  setDeserializeMainFunction,
+} = require('v8').startupSnapshot;
+const assert = require('assert');
+
+// Check that mutation to Error.stackTraceLimit is effective in the snapshot
+// builder script.
+assert.strictEqual(typeof Error.stackTraceLimit, 'number');
+Error.stackTraceLimit = 0;
+assert.strictEqual(getError('', 30), 'Error');
+
+setDeserializeMainFunction(() => {
+  // Check that the mutation is preserved in the deserialized main function.
+  assert.strictEqual(Error.stackTraceLimit, 0);
+  assert.strictEqual(getError('', 30), 'Error');
+
+  // Check that it can still be mutated.
+  Error.stackTraceLimit = 10;
+  const error = getError('', 30);
+  const matches = [...error.matchAll(/at recurse/g)];
+  assert.strictEqual(matches.length, 10);
+});
+
+function getError(message, depth) {
+  let counter = 1;
+  function recurse() {
+    if (counter++ < depth) {
+      return recurse();
+    }
+    const error = new Error(message);
+    return error.stack;
+  }
+  return recurse();
+}

test/parallel/test-snapshot-stack-trace-limit-mutation.js

@@ -0,0 +1,64 @@
+'use strict';
+
+// This tests mutation to Error.stackTraceLimit in both the snapshot builder script
+// and the snapshot main script work.
+
+require('../common');
+const assert = require('assert');
+const tmpdir = require('../common/tmpdir');
+const fixtures = require('../common/fixtures');
+const { spawnSyncAndAssert, spawnSyncAndExitWithoutError } = require('../common/child_process');
+
+const blobPath = tmpdir.resolve('snapshot.blob');
+
+{
+  tmpdir.refresh();
+  // Check the mutation works without --stack-trace-limit.
+  spawnSyncAndAssert(process.execPath, [
+    '--snapshot-blob',
+    blobPath,
+    '--build-snapshot',
+    fixtures.path('snapshot', 'mutate-error-stack-trace-limit.js'),
+  ], {
+    cwd: tmpdir.path
+  }, {
+    stderr(output) {
+      assert.match(output, /Error\.stackTraceLimit has been modified by the snapshot builder script/);
+      assert.match(output, /It will be preserved after snapshot deserialization/);
+    }
+  });
+  spawnSyncAndExitWithoutError(process.execPath, [
+    '--snapshot-blob',
+    blobPath,
+  ], {
+    cwd: tmpdir.path
+  });
+}
+
+{
+  tmpdir.refresh();
+  // Check the mutation works with --stack-trace-limit.
+  spawnSyncAndAssert(process.execPath, [
+    '--stack-trace-limit=50',
+    '--snapshot-blob',
+    blobPath,
+    '--build-snapshot',
+    fixtures.path('snapshot', 'mutate-error-stack-trace-limit.js'),
+  ], {
+    cwd: tmpdir.path
+  }, {
+    stderr(output) {
+      assert.match(output, /Error\.stackTraceLimit has been modified by the snapshot builder script/);
+      assert.match(output, /It will be preserved after snapshot deserialization/);
+    }
+  });
+  spawnSyncAndExitWithoutError(process.execPath, [
+    // This checks that --stack-trace-limit=50 is ignored if the buidler script already mutated
+    // Error.stackTraceLimit.
+    '--stack-trace-limit=50',
+    '--snapshot-blob',
+    blobPath,
+  ], {
+    cwd: tmpdir.path
+  });
+}