From a33ec1248bc2df74811da8b0cd811f16663205db Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Tue, 17 Dec 2024 23:12:00 +0000 Subject: [PATCH 1/2] lib: suppress source map lookup exceptions When the source map data are invalid json strings, skip construct `SourceMap` on it. Additionally, suppress exceptions on source map lookups and fix test runners crash on invalid source maps. PR-URL: https://github.com/nodejs/node/pull/56299 Refs: https://github.com/nodejs/node/issues/56296 Reviewed-By: Matteo Collina Reviewed-By: Xuguang Mei Reviewed-By: Yagiz Nizipli Reviewed-By: Colin Ihrig Reviewed-By: Chemi Atlow Reviewed-By: Pietro Marchini --- lib/internal/source_map/source_map_cache.js | 39 ++++++++++++++----- .../test-runner-source-maps-invalid-json.js | 12 ++++++ 2 files changed, 41 insertions(+), 10 deletions(-) create mode 100644 test/parallel/test-runner-source-maps-invalid-json.js diff --git a/lib/internal/source_map/source_map_cache.js b/lib/internal/source_map/source_map_cache.js index dfb42f83f6f1b1..ea87a579636671 100644 --- a/lib/internal/source_map/source_map_cache.js +++ b/lib/internal/source_map/source_map_cache.js @@ -155,6 +155,9 @@ function maybeCacheSourceMap(filename, content, moduleInstance, isGeneratedSourc } const data = dataFromUrl(filename, sourceMapURL); + // `data` could be null if the source map is invalid. + // In this case, create a cache entry with null data with source url for test coverage. + const entry = { __proto__: null, lineLengths: lineLengths(content), @@ -277,6 +280,8 @@ function sourceMapFromDataUrl(sourceURL, url) { const parsedData = JSONParse(decodedData); return sourcesToAbsolute(sourceURL, parsedData); } catch (err) { + // TODO(legendecas): warn about invalid source map JSON string. + // But it could be verbose. debug(err); return null; } @@ -331,24 +336,38 @@ function sourceMapCacheToObject() { /** * Find a source map for a given actual source URL or path. + * + * This function may be invoked from user code or test runner, this must not throw + * any exceptions. * @param {string} sourceURL - actual source URL or path * @returns {import('internal/source_map/source_map').SourceMap | undefined} a source map or undefined if not found */ function findSourceMap(sourceURL) { - if (RegExpPrototypeExec(kLeadingProtocol, sourceURL) === null) { - sourceURL = pathToFileURL(sourceURL).href; + if (typeof sourceURL !== 'string') { + return undefined; } + SourceMap ??= require('internal/source_map/source_map').SourceMap; - const entry = getModuleSourceMapCache().get(sourceURL) ?? generatedSourceMapCache.get(sourceURL); - if (entry === undefined) { + try { + if (RegExpPrototypeExec(kLeadingProtocol, sourceURL) === null) { + // If the sourceURL is an invalid path, this will throw an error. + sourceURL = pathToFileURL(sourceURL).href; + } + const entry = getModuleSourceMapCache().get(sourceURL) ?? generatedSourceMapCache.get(sourceURL); + if (entry?.data == null) { + return undefined; + } + + let sourceMap = entry.sourceMap; + if (sourceMap === undefined) { + sourceMap = new SourceMap(entry.data, { lineLengths: entry.lineLengths }); + entry.sourceMap = sourceMap; + } + return sourceMap; + } catch (err) { + debug(err); return undefined; } - let sourceMap = entry.sourceMap; - if (sourceMap === undefined) { - sourceMap = new SourceMap(entry.data, { lineLengths: entry.lineLengths }); - entry.sourceMap = sourceMap; - } - return sourceMap; } module.exports = { diff --git a/test/parallel/test-runner-source-maps-invalid-json.js b/test/parallel/test-runner-source-maps-invalid-json.js new file mode 100644 index 00000000000000..508e2d432117f7 --- /dev/null +++ b/test/parallel/test-runner-source-maps-invalid-json.js @@ -0,0 +1,12 @@ +// Flags: --enable-source-maps +'use strict'; + +require('../common'); +const test = require('node:test'); + +// Verify that test runner can handle invalid source maps. + +test('ok', () => {}); + +// eslint-disable-next-line @stylistic/js/spaced-comment +//# sourceMappingURL=data:application/json;base64,-1 From 28557ef5f1b71f9ffcb7f7ec1290195d65ff4d92 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Tue, 17 Dec 2024 23:38:54 +0000 Subject: [PATCH 2/2] lib: optimize `prepareStackTrace` on builtin frames Only invalidates source map lookup cache when a new source map is found. This improves when user codes interleave with builtin functions, like `array.map`. PR-URL: https://github.com/nodejs/node/pull/56299 Refs: https://github.com/nodejs/node/issues/56296 Reviewed-By: Matteo Collina Reviewed-By: Xuguang Mei Reviewed-By: Yagiz Nizipli Reviewed-By: Colin Ihrig Reviewed-By: Chemi Atlow Reviewed-By: Pietro Marchini --- benchmark/fixtures/simple-error-stack.js | 21 ++++++++++--------- benchmark/fixtures/simple-error-stack.ts | 12 ++++++----- .../source_map/prepare_stack_trace.js | 10 +++++++-- lib/internal/source_map/source_map_cache.js | 5 +++++ 4 files changed, 31 insertions(+), 17 deletions(-) diff --git a/benchmark/fixtures/simple-error-stack.js b/benchmark/fixtures/simple-error-stack.js index 0057807795b072..74aae191800778 100644 --- a/benchmark/fixtures/simple-error-stack.js +++ b/benchmark/fixtures/simple-error-stack.js @@ -1,15 +1,16 @@ 'use strict'; -exports.__esModule = true; -exports.simpleErrorStack = void 0; +Object.defineProperty(exports, "__esModule", { value: true }); +exports.simpleErrorStack = simpleErrorStack; // Compile with `tsc --inlineSourceMap benchmark/fixtures/simple-error-stack.ts`. var lorem = 'Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.'; function simpleErrorStack() { - try { - lorem.BANG(); - } - catch (e) { - return e.stack; - } + [1].map(function () { + try { + lorem.BANG(); + } + catch (e) { + return e.stack; + } + }); } -exports.simpleErrorStack = simpleErrorStack; -//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoic2ltcGxlLWVycm9yLXN0YWNrLmpzIiwic291cmNlUm9vdCI6IiIsInNvdXJjZXMiOlsic2ltcGxlLWVycm9yLXN0YWNrLnRzIl0sIm5hbWVzIjpbXSwibWFwcGluZ3MiOiJBQUFBLFlBQVksQ0FBQzs7O0FBRWIsaUZBQWlGO0FBRWpGLElBQU0sS0FBSyxHQUFHLCtiQUErYixDQUFDO0FBRTljLFNBQVMsZ0JBQWdCO0lBQ3ZCLElBQUk7UUFDRCxLQUFhLENBQUMsSUFBSSxFQUFFLENBQUM7S0FDdkI7SUFBQyxPQUFPLENBQUMsRUFBRTtRQUNWLE9BQU8sQ0FBQyxDQUFDLEtBQUssQ0FBQztLQUNoQjtBQUNILENBQUM7QUFHQyw0Q0FBZ0IifQ== +//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoic2ltcGxlLWVycm9yLXN0YWNrLmpzIiwic291cmNlUm9vdCI6IiIsInNvdXJjZXMiOlsic2ltcGxlLWVycm9yLXN0YWNrLnRzIl0sIm5hbWVzIjpbXSwibWFwcGluZ3MiOiJBQUFBLFlBQVksQ0FBQzs7QUFpQlgsNENBQWdCO0FBZmxCLGlGQUFpRjtBQUVqRixJQUFNLEtBQUssR0FBRywrYkFBK2IsQ0FBQztBQUU5YyxTQUFTLGdCQUFnQjtJQUN2QixDQUFDLENBQUMsQ0FBQyxDQUFDLEdBQUcsQ0FBQztRQUNOLElBQUksQ0FBQztZQUNGLEtBQWEsQ0FBQyxJQUFJLEVBQUUsQ0FBQztRQUN4QixDQUFDO1FBQUMsT0FBTyxDQUFDLEVBQUUsQ0FBQztZQUNYLE9BQU8sQ0FBQyxDQUFDLEtBQUssQ0FBQztRQUNqQixDQUFDO0lBQ0gsQ0FBQyxDQUFDLENBQUE7QUFDSixDQUFDIn0= \ No newline at end of file diff --git a/benchmark/fixtures/simple-error-stack.ts b/benchmark/fixtures/simple-error-stack.ts index 58034e92f24b98..1335df3478b99b 100644 --- a/benchmark/fixtures/simple-error-stack.ts +++ b/benchmark/fixtures/simple-error-stack.ts @@ -5,11 +5,13 @@ const lorem = 'Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.'; function simpleErrorStack() { - try { - (lorem as any).BANG(); - } catch (e) { - return e.stack; - } + [1].map(() => { + try { + (lorem as any).BANG(); + } catch (e) { + return e.stack; + } + }) } export { diff --git a/lib/internal/source_map/prepare_stack_trace.js b/lib/internal/source_map/prepare_stack_trace.js index 60c9d1ed3316ff..3e4b0825e7b3a5 100644 --- a/lib/internal/source_map/prepare_stack_trace.js +++ b/lib/internal/source_map/prepare_stack_trace.js @@ -53,9 +53,15 @@ function prepareStackTraceWithSourceMaps(error, trace) { const sm = fileName === lastFileName ? lastSourceMap : findSourceMap(fileName); - lastSourceMap = sm; - lastFileName = fileName; + // Only when a source map is found, cache it for the next iteration. + // This is a performance optimization to avoid interleaving with JS builtin function + // invalidating the cache. + // - at myFunc (file:///path/to/file.js:1:2) + // - at Array.map () + // - at myFunc (file:///path/to/file.js:3:4) if (sm) { + lastSourceMap = sm; + lastFileName = fileName; return `${kStackLineAt}${serializeJSStackFrame(sm, callSite, trace[i + 1])}`; } } catch (err) { diff --git a/lib/internal/source_map/source_map_cache.js b/lib/internal/source_map/source_map_cache.js index ea87a579636671..aaca27136e66a0 100644 --- a/lib/internal/source_map/source_map_cache.js +++ b/lib/internal/source_map/source_map_cache.js @@ -347,6 +347,11 @@ function findSourceMap(sourceURL) { return undefined; } + // No source maps for builtin modules. + if (sourceURL.startsWith('node:')) { + return undefined; + } + SourceMap ??= require('internal/source_map/source_map').SourceMap; try { if (RegExpPrototypeExec(kLeadingProtocol, sourceURL) === null) {