diff --git a/lib/internal/main/mksnapshot.js b/lib/internal/main/mksnapshot.js index 0d00acd6a168ba..63df8c50087aad 100644 --- a/lib/internal/main/mksnapshot.js +++ b/lib/internal/main/mksnapshot.js @@ -23,10 +23,10 @@ const { emitWarningSync } = require('internal/process/warning'); const { initializeCallbacks, namespace: { - addSerializeCallback, addDeserializeCallback, isBuildingSnapshot, }, + addAfterUserSerializeCallback, } = require('internal/v8/startup_snapshot'); const { @@ -34,6 +34,7 @@ const { } = 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 Error.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 Error.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; + // Error.stackTraceLimit needs to be removed during serialization, because when V8 deserializes + // the snapshot, it expects Error.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); } }); diff --git a/lib/internal/v8/startup_snapshot.js b/lib/internal/v8/startup_snapshot.js index 7c789577aec969..01204b96239406 100644 --- a/lib/internal/v8/startup_snapshot.js +++ b/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, }; diff --git a/src/env-inl.h b/src/env-inl.h index 8fdcf293805429..4f5fd036e15aed 100644 --- a/src/env-inl.h +++ b/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 Environment::options() { return options_; } diff --git a/src/env.cc b/src/env.cc index 3b3bf696555a45..4455f31560eb67 100644 --- a/src/env.cc +++ b/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(stack_trace_limit()), + StackTrace::kDetailed)); } MaybeLocal 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(exit_code)); - PrintStackTrace(isolate(), - StackTrace::CurrentStackTrace( - isolate(), stack_trace_limit(), StackTrace::kDetailed)); + PrintStackTrace( + isolate(), + StackTrace::CurrentStackTrace(isolate(), + static_cast(stack_trace_limit()), + StackTrace::kDetailed)); } process_exit_handler_(this, exit_code); } diff --git a/src/env.h b/src/env.h index 118ccf16cdeed1..02747ccf26e839 100644 --- a/src/env.h +++ b/src/env.h @@ -977,7 +977,7 @@ class Environment final : public MemoryRetainer { inline std::shared_ptr options(); inline std::shared_ptr> 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( diff --git a/src/node_options-inl.h b/src/node_options-inl.h index fabf3be149be93..24954e0b583834 100644 --- a/src/node_options-inl.h +++ b/src/node_options-inl.h @@ -447,9 +447,14 @@ void OptionsParser::Parse( case kBoolean: *Lookup(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(info.field, options) = std::atoll(value.c_str()); break; + } case kUInteger: *Lookup(info.field, options) = std::strtoull(value.c_str(), nullptr, 10); diff --git a/src/node_options.cc b/src/node_options.cc index 77e68f5104c0b1..c92cadc4c71038 100644 --- a/src/node_options.cc +++ b/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& 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( + *_ppop_instance.Lookup(field, opts))); } else { value = undefined_value; } diff --git a/src/node_options.h b/src/node_options.h index b521ca76185512..df2e36d4b17cdf 100644 --- a/src/node_options.h +++ b/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; diff --git a/test/fixtures/deep-exit.js b/test/fixtures/deep-exit.js new file mode 100644 index 00000000000000..357137a279c556 --- /dev/null +++ b/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(); diff --git a/test/fixtures/snapshot/error-stack.js b/test/fixtures/snapshot/error-stack.js new file mode 100644 index 00000000000000..96afaec22521ee --- /dev/null +++ b/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(); +} diff --git a/test/fixtures/snapshot/mutate-error-stack-trace-limit.js b/test/fixtures/snapshot/mutate-error-stack-trace-limit.js new file mode 100644 index 00000000000000..e9d704ceb32f23 --- /dev/null +++ b/test/fixtures/snapshot/mutate-error-stack-trace-limit.js @@ -0,0 +1,44 @@ + +const { + addSerializeCallback, + setDeserializeMainFunction, +} = require('v8').startupSnapshot; +const assert = require('assert'); + +if (process.env.TEST_IN_SERIALIZER) { + addSerializeCallback(checkMutate); +} else { + checkMutate(); +} + +function checkMutate() { + // 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(); +} diff --git a/test/parallel/test-snapshot-stack-trace-limit-mutation.js b/test/parallel/test-snapshot-stack-trace-limit-mutation.js new file mode 100644 index 00000000000000..fe6c9fbd45ca0b --- /dev/null +++ b/test/parallel/test-snapshot-stack-trace-limit-mutation.js @@ -0,0 +1,46 @@ +'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'); + +function test(additionalArguments = [], additionalEnv = {}) { + tmpdir.refresh(); + // Check the mutation works without --stack-trace-limit. + spawnSyncAndAssert(process.execPath, [ + ...additionalArguments, + '--snapshot-blob', + blobPath, + '--build-snapshot', + fixtures.path('snapshot', 'mutate-error-stack-trace-limit.js'), + ], { + cwd: tmpdir.path, + env: { + ...process.env, + ...additionalEnv, + } + }, { + 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 + }); +} + +test(); +test([], { TEST_IN_SERIALIZER: 1 }); +test(['--stack-trace-limit=50']); +test(['--stack-trace-limit=50'], { TEST_IN_SERIALIZER: 1 }); diff --git a/test/parallel/test-snapshot-stack-trace-limit.js b/test/parallel/test-snapshot-stack-trace-limit.js new file mode 100644 index 00000000000000..9ba760a9f2373e --- /dev/null +++ b/test/parallel/test-snapshot-stack-trace-limit.js @@ -0,0 +1,46 @@ +'use strict'; + +// This tests Error.stackTraceLimit is fixed up for snapshot-building contexts, +// and can be restored after deserialization. + +require('../common'); +const assert = require('assert'); +const tmpdir = require('../common/tmpdir'); +const fixtures = require('../common/fixtures'); +const { spawnSyncAndAssert } = require('../common/child_process'); + +tmpdir.refresh(); +const blobPath = tmpdir.resolve('snapshot.blob'); +{ + spawnSyncAndAssert(process.execPath, [ + '--stack-trace-limit=50', + '--snapshot-blob', + blobPath, + '--build-snapshot', + fixtures.path('snapshot', 'error-stack.js'), + ], { + cwd: tmpdir.path + }, { + stdout(output) { + assert.match(output, /During snapshot building, Error\.stackTraceLimit = 50/); + const matches = [...output.matchAll(/at recurse/g)]; + assert.strictEqual(matches.length, 30); + } + }); +} + +{ + spawnSyncAndAssert(process.execPath, [ + '--stack-trace-limit=20', + '--snapshot-blob', + blobPath, + ], { + cwd: tmpdir.path + }, { + stdout(output) { + assert.match(output, /After snapshot deserialization, Error\.stackTraceLimit = 20/); + const matches = [...output.matchAll(/at recurse/g)]; + assert.strictEqual(matches.length, 20); + } + }); +} diff --git a/test/parallel/test-trace-exit-stack-limit.js b/test/parallel/test-trace-exit-stack-limit.js new file mode 100644 index 00000000000000..c937ad828fc032 --- /dev/null +++ b/test/parallel/test-trace-exit-stack-limit.js @@ -0,0 +1,42 @@ +'use strict'; + +// This tests that --stack-trace-limit can be used to tweak the stack trace size of --trace-exit. +require('../common'); +const fixture = require('../common/fixtures'); +const { spawnSyncAndAssert } = require('../common/child_process'); +const assert = require('assert'); + +// When the stack trace limit is bigger than the stack trace size, it should output them all. +spawnSyncAndAssert( + process.execPath, + ['--trace-exit', '--stack-trace-limit=50', fixture.path('deep-exit.js')], + { + env: { + ...process.env, + STACK_DEPTH: 30 + } + }, + { + stderr(output) { + const matches = [...output.matchAll(/at recurse/g)]; + assert.strictEqual(matches.length, 30); + } + }); + +// When the stack trace limit is smaller than the stack trace size, it should truncate the stack size. +spawnSyncAndAssert( + process.execPath, + ['--trace-exit', '--stack-trace-limit=30', fixture.path('deep-exit.js')], + { + env: { + ...process.env, + STACK_DEPTH: 30 + } + }, + { + stderr(output) { + const matches = [...output.matchAll(/at recurse/g)]; + // The top frame is process.exit(), so one frame from recurse() is truncated. + assert.strictEqual(matches.length, 29); + } + });