From 147abb99d1478b3d88be38f0c9c0ad9ed6a7af73 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Mon, 20 Nov 2023 15:15:38 +0800 Subject: [PATCH] lib: expose default prepareStackTrace Expose the default prepareStackTrace implementation as `Error.prepareStackTrace` so that userland can chain up formatting of stack traces with built-in source maps support. PR-URL: https://github.com/nodejs/node/pull/50827 Fixes: https://github.com/nodejs/node/issues/50733 Reviewed-By: Ethan Arrowood Reviewed-By: James M Snell Reviewed-By: Joyee Cheung Reviewed-By: Geoffrey Booth --- doc/api/cli.md | 15 +++- lib/.eslintrc.yaml | 2 +- lib/internal/bootstrap/realm.js | 13 +++- lib/internal/errors.js | 71 +++++++++++++------ .../source_map/prepare_stack_trace.js | 26 ++----- lib/internal/source_map/source_map_cache.js | 12 ++-- lib/repl.js | 4 +- .../output/source_map_prepare_stack_trace.js | 34 +++++++++ .../source_map_prepare_stack_trace.snapshot | 10 +++ .../test-error-prepare-stack-trace.js | 16 ++++- test/parallel/test-node-output-sourcemaps.mjs | 1 + 11 files changed, 148 insertions(+), 56 deletions(-) create mode 100644 test/fixtures/source-map/output/source_map_prepare_stack_trace.js create mode 100644 test/fixtures/source-map/output/source_map_prepare_stack_trace.snapshot diff --git a/doc/api/cli.md b/doc/api/cli.md index 1b2195934075ce..04e7f95c0ca573 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -598,8 +598,19 @@ application reference the transpiled code, not the original source position. `--enable-source-maps` enables caching of Source Maps and makes a best effort to report stack traces relative to the original source file. -Overriding `Error.prepareStackTrace` prevents `--enable-source-maps` from -modifying the stack trace. +Overriding `Error.prepareStackTrace` may prevent `--enable-source-maps` from +modifying the stack trace. Call and return the results of the original +`Error.prepareStackTrace` in the overriding function to modify the stack trace +with source maps. + +```js +const originalPrepareStackTrace = Error.prepareStackTrace; +Error.prepareStackTrace = (error, trace) => { + // Modify error and trace and format stack trace with + // original Error.prepareStackTrace. + return originalPrepareStackTrace(error, trace); +}; +``` Note, enabling source maps can introduce latency to your application when `Error.stack` is accessed. If you access `Error.stack` frequently diff --git a/lib/.eslintrc.yaml b/lib/.eslintrc.yaml index 3eb3b2089b0596..93ad0a986786b7 100644 --- a/lib/.eslintrc.yaml +++ b/lib/.eslintrc.yaml @@ -23,7 +23,7 @@ rules: message: Use an error exported by the internal/errors module. - selector: CallExpression[callee.object.name='Error'][callee.property.name='captureStackTrace'] message: Please use `require('internal/errors').hideStackFrames()` instead. - - selector: AssignmentExpression:matches([left.name='prepareStackTrace'], [left.property.name='prepareStackTrace']) + - selector: AssignmentExpression:matches([left.object.name='Error']):matches([left.name='prepareStackTrace'], [left.property.name='prepareStackTrace']) message: Use 'overrideStackTrace' from 'lib/internal/errors.js' instead of 'Error.prepareStackTrace'. - selector: ThrowStatement > NewExpression[callee.name=/^ERR_[A-Z_]+$/] > ObjectExpression:first-child:not(:has([key.name='message']):has([key.name='code']):has([key.name='syscall'])) message: The context passed into SystemError constructor must have .code, .syscall and .message. diff --git a/lib/internal/bootstrap/realm.js b/lib/internal/bootstrap/realm.js index 93a939cc7e4645..57ab47178d033d 100644 --- a/lib/internal/bootstrap/realm.js +++ b/lib/internal/bootstrap/realm.js @@ -445,7 +445,8 @@ function setupPrepareStackTrace() { setPrepareStackTraceCallback, } = internalBinding('errors'); const { - prepareStackTrace, + prepareStackTraceCallback, + ErrorPrepareStackTrace, fatalExceptionStackEnhancers: { beforeInspector, afterInspector, @@ -453,9 +454,17 @@ function setupPrepareStackTrace() { } = requireBuiltin('internal/errors'); // Tell our PrepareStackTraceCallback passed to the V8 API // to call prepareStackTrace(). - setPrepareStackTraceCallback(prepareStackTrace); + setPrepareStackTraceCallback(prepareStackTraceCallback); // Set the function used to enhance the error stack for printing setEnhanceStackForFatalException(beforeInspector, afterInspector); + // Setup the default Error.prepareStackTrace. + ObjectDefineProperty(Error, 'prepareStackTrace', { + __proto__: null, + writable: true, + enumerable: false, + configurable: true, + value: ErrorPrepareStackTrace, + }); } // Store the internal loaders in C++. diff --git a/lib/internal/errors.js b/lib/internal/errors.js index e460c2f6875001..d2250f5c7f053c 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -85,21 +85,14 @@ const kTypes = [ const MainContextError = Error; const overrideStackTrace = new SafeWeakMap(); -const kNoOverride = Symbol('kNoOverride'); - -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); - } - - const globalOverride = - maybeOverridePrepareStackTrace(globalThis, error, trace); - if (globalOverride !== kNoOverride) return globalOverride; +let internalPrepareStackTrace = defaultPrepareStackTrace; +/** + * The default implementation of `Error.prepareStackTrace` with simple + * concatenation of stack frames. + * Read more about `Error.prepareStackTrace` at https://v8.dev/docs/stack-trace-api#customizing-stack-traces. + */ +function defaultPrepareStackTrace(error, trace) { // Normal error formatting: // // Error: Message @@ -115,9 +108,35 @@ const prepareStackTrace = (globalThis, error, trace) => { return errorString; } return `${errorString}\n at ${ArrayPrototypeJoin(trace, '\n at ')}`; -}; +} + +function setInternalPrepareStackTrace(callback) { + internalPrepareStackTrace = callback; +} + +/** + * Every realm has its own prepareStackTraceCallback. When `error.stack` is + * accessed, if the error is created in a shadow realm, the shadow realm's + * prepareStackTraceCallback is invoked. Otherwise, the principal realm's + * prepareStackTraceCallback is invoked. Note that accessing `error.stack` + * of error objects created in a VM Context will always invoke the + * prepareStackTraceCallback of the principal realm. + * @param {object} globalThis The global object of the realm that the error was + * created in. When the error object is created in a VM Context, this is the + * global object of that VM Context. + * @param {object} error The error object. + * @param {CallSite[]} trace An array of CallSite objects, read more at https://v8.dev/docs/stack-trace-api#customizing-stack-traces. + * @returns {string} + */ +function prepareStackTraceCallback(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); + } -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 @@ -132,8 +151,17 @@ const maybeOverridePrepareStackTrace = (globalThis, error, trace) => { return MainContextError.prepareStackTrace(error, trace); } - return kNoOverride; -}; + // If the Error.prepareStackTrace was not a function, fallback to the + // internal implementation. + return internalPrepareStackTrace(error, trace); +} + +/** + * The default Error.prepareStackTrace implementation. + */ +function ErrorPrepareStackTrace(error, trace) { + return internalPrepareStackTrace(error, trace); +} const aggregateTwoErrors = (innerError, outerError) => { if (innerError && outerError && innerError !== outerError) { @@ -1055,10 +1083,11 @@ module.exports = { isStackOverflowError, kEnhanceStackBeforeInspector, kIsNodeError, - kNoOverride, - maybeOverridePrepareStackTrace, + defaultPrepareStackTrace, + setInternalPrepareStackTrace, overrideStackTrace, - prepareStackTrace, + prepareStackTraceCallback, + ErrorPrepareStackTrace, setArrowMessage, SystemError, uvErrmapGet, diff --git a/lib/internal/source_map/prepare_stack_trace.js b/lib/internal/source_map/prepare_stack_trace.js index 860564f4a34ab5..568e86f7faaf74 100644 --- a/lib/internal/source_map/prepare_stack_trace.js +++ b/lib/internal/source_map/prepare_stack_trace.js @@ -19,9 +19,6 @@ const { getStringWidth } = require('internal/util/inspect'); const { readFileSync } = require('fs'); const { findSourceMap } = require('internal/source_map/source_map_cache'); const { - kNoOverride, - overrideStackTrace, - maybeOverridePrepareStackTrace, kIsNodeError, } = require('internal/errors'); const { fileURLToPath } = require('internal/url'); @@ -29,20 +26,7 @@ const { setGetSourceMapErrorSource } = internalBinding('errors'); // 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. - if (overrideStackTrace.has(error)) { - const f = overrideStackTrace.get(error); - overrideStackTrace.delete(error); - return f(error, trace); - } - - const globalOverride = - maybeOverridePrepareStackTrace(globalThis, error, trace); - if (globalOverride !== kNoOverride) return globalOverride; - +function prepareStackTraceWithSourceMaps(error, trace) { let errorString; if (kIsNodeError in error) { errorString = `${error.name} [${error.code}]: ${error.message}`; @@ -57,7 +41,7 @@ const prepareStackTrace = (globalThis, error, trace) => { let lastSourceMap; let lastFileName; const preparedTrace = ArrayPrototypeJoin(ArrayPrototypeMap(trace, (t, i) => { - const str = i !== 0 ? '\n at ' : ''; + const str = '\n at '; try { // A stack trace will often have several call sites in a row within the // same file, cache the source map and file content accordingly: @@ -106,8 +90,8 @@ const prepareStackTrace = (globalThis, error, trace) => { } return `${str}${t}`; }), ''); - return `${errorString}\n at ${preparedTrace}`; -}; + return `${errorString}${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: @@ -210,5 +194,5 @@ function getSourceMapErrorSource(fileName, lineNumber, columnNumber) { setGetSourceMapErrorSource(getSourceMapErrorSource); module.exports = { - prepareStackTrace, + prepareStackTraceWithSourceMaps, }; diff --git a/lib/internal/source_map/source_map_cache.js b/lib/internal/source_map/source_map_cache.js index 23a8ceb4da4d1e..17da1ca476448f 100644 --- a/lib/internal/source_map/source_map_cache.js +++ b/lib/internal/source_map/source_map_cache.js @@ -19,8 +19,10 @@ let debug = require('internal/util/debuglog').debuglog('source_map', (fn) => { const { validateBoolean } = require('internal/validators'); const { setSourceMapsEnabled: setSourceMapsNative, - setPrepareStackTraceCallback, } = internalBinding('errors'); +const { + setInternalPrepareStackTrace, +} = require('internal/errors'); const { getLazy } = require('internal/util'); // Since the CJS module cache is mutable, which leads to memory leaks when @@ -56,15 +58,15 @@ function setSourceMapsEnabled(val) { setSourceMapsNative(val); if (val) { const { - prepareStackTrace, + prepareStackTraceWithSourceMaps, } = require('internal/source_map/prepare_stack_trace'); - setPrepareStackTraceCallback(prepareStackTrace); + setInternalPrepareStackTrace(prepareStackTraceWithSourceMaps); } else if (sourceMapsEnabled !== undefined) { // Reset prepare stack trace callback only when disabling source maps. const { - prepareStackTrace, + defaultPrepareStackTrace, } = require('internal/errors'); - setPrepareStackTraceCallback(prepareStackTrace); + setInternalPrepareStackTrace(defaultPrepareStackTrace); } sourceMapsEnabled = val; diff --git a/lib/repl.js b/lib/repl.js index 8b96a1f55652af..3effdc7bb82d41 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -151,6 +151,7 @@ const { }, isErrorStackTraceLimitWritable, overrideStackTrace, + ErrorPrepareStackTrace, } = require('internal/errors'); const { sendInspectorCommand } = require('internal/util/inspector'); const { getOptionValue } = require('internal/options'); @@ -692,8 +693,7 @@ function REPLServer(prompt, if (typeof MainContextError.prepareStackTrace === 'function') { return MainContextError.prepareStackTrace(error, frames); } - ArrayPrototypeUnshift(frames, error); - return ArrayPrototypeJoin(frames, '\n at '); + return ErrorPrepareStackTrace(error, frames); }); decorateErrorStack(e); diff --git a/test/fixtures/source-map/output/source_map_prepare_stack_trace.js b/test/fixtures/source-map/output/source_map_prepare_stack_trace.js new file mode 100644 index 00000000000000..d49bad116b479f --- /dev/null +++ b/test/fixtures/source-map/output/source_map_prepare_stack_trace.js @@ -0,0 +1,34 @@ +// Flags: --enable-source-maps + +'use strict'; +require('../../../common'); +const assert = require('assert'); +Error.stackTraceLimit = 5; + +assert.strictEqual(typeof Error.prepareStackTrace, 'function'); +const defaultPrepareStackTrace = Error.prepareStackTrace; +Error.prepareStackTrace = (error, trace) => { + trace = trace.filter(it => { + return it.getFunctionName() !== 'functionC'; + }); + return defaultPrepareStackTrace(error, trace); +}; + +try { + require('../enclosing-call-site-min.js'); +} catch (e) { + console.log(e); +} + +delete require.cache[require + .resolve('../enclosing-call-site-min.js')]; + +// Disable +process.setSourceMapsEnabled(false); +assert.strictEqual(process.sourceMapsEnabled, false); + +try { + require('../enclosing-call-site-min.js'); +} catch (e) { + console.log(e); +} diff --git a/test/fixtures/source-map/output/source_map_prepare_stack_trace.snapshot b/test/fixtures/source-map/output/source_map_prepare_stack_trace.snapshot new file mode 100644 index 00000000000000..9e9740a26fc34e --- /dev/null +++ b/test/fixtures/source-map/output/source_map_prepare_stack_trace.snapshot @@ -0,0 +1,10 @@ +Error: an error! + at functionD (*enclosing-call-site.js:16:17) + at functionB (*enclosing-call-site.js:6:3) + at functionA (*enclosing-call-site.js:2:3) + at Object. (*enclosing-call-site.js:24:3) +Error: an error! + at functionD (*enclosing-call-site-min.js:1:156) + at functionB (*enclosing-call-site-min.js:1:60) + at functionA (*enclosing-call-site-min.js:1:26) + at Object. (*enclosing-call-site-min.js:1:199) diff --git a/test/parallel/test-error-prepare-stack-trace.js b/test/parallel/test-error-prepare-stack-trace.js index 28ecdd25f50135..0d375a6db38675 100644 --- a/test/parallel/test-error-prepare-stack-trace.js +++ b/test/parallel/test-error-prepare-stack-trace.js @@ -1,10 +1,13 @@ -// Flags: --enable-source-maps 'use strict'; require('../common'); +const { spawnSyncAndExitWithoutError } = require('../common/child_process'); const assert = require('assert'); -// Error.prepareStackTrace() can be overridden with source maps enabled. +// Verify that the default Error.prepareStackTrace is present. +assert.strictEqual(typeof Error.prepareStackTrace, 'function'); + +// Error.prepareStackTrace() can be overridden. { let prepareCalled = false; Error.prepareStackTrace = (_error, trace) => { @@ -17,3 +20,12 @@ const assert = require('assert'); } assert(prepareCalled); } + +if (process.argv[2] !== 'child') { + // Verify that the above test still passes when source-maps support is + // enabled. + spawnSyncAndExitWithoutError( + process.execPath, + ['--enable-source-maps', __filename, 'child'], + {}); +} diff --git a/test/parallel/test-node-output-sourcemaps.mjs b/test/parallel/test-node-output-sourcemaps.mjs index b01f30765c7de8..21060c3b342cf1 100644 --- a/test/parallel/test-node-output-sourcemaps.mjs +++ b/test/parallel/test-node-output-sourcemaps.mjs @@ -31,6 +31,7 @@ describe('sourcemaps output', { concurrency: true }, () => { { name: 'source-map/output/source_map_enclosing_function.js' }, { name: 'source-map/output/source_map_eval.js' }, { name: 'source-map/output/source_map_no_source_file.js' }, + { name: 'source-map/output/source_map_prepare_stack_trace.js' }, { name: 'source-map/output/source_map_reference_error_tabs.js' }, { name: 'source-map/output/source_map_sourcemapping_url_string.js' }, { name: 'source-map/output/source_map_throw_catch.js' },