From 773ea75254a1d792a7adc59fb7945b380cb95d0f Mon Sep 17 00:00:00 2001 From: bcoe Date: Sun, 13 Jun 2021 09:21:40 -0700 Subject: [PATCH 1/5] errors: don't rekey on primitive type If an error is thrown before a module is loaded, we attempt to cache source map against error object, rather than module object. We can't do this if the error is a primitive type Fixes #38945 --- lib/internal/source_map/source_map_cache.js | 6 +++++- .../source-map/throw-string-original.js | 8 ++++++++ test/fixtures/source-map/throw-string.js | 2 ++ test/parallel/test-source-map-enable.js | 17 +++++++++++++++++ 4 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 test/fixtures/source-map/throw-string-original.js create mode 100644 test/fixtures/source-map/throw-string.js diff --git a/lib/internal/source_map/source_map_cache.js b/lib/internal/source_map/source_map_cache.js index f0d911f78fad8b..0d33a52b38d435 100644 --- a/lib/internal/source_map/source_map_cache.js +++ b/lib/internal/source_map/source_map_cache.js @@ -175,7 +175,11 @@ function sourcesToAbsolute(baseURL, data) { // Move source map from garbage collected module to alternate key. function rekeySourceMap(cjsModuleInstance, newInstance) { const sourceMap = cjsSourceMapCache.get(cjsModuleInstance); - if (sourceMap) { + // If an exception occurs before a module finishes loading it will removed + // from the module cache and our WeakMap. To allow a source map to still be + // applied to the stack trace when this happens, we rekey on the thrown error. + // This is only possible if the error is an object and not a primitive type. + if (sourceMap && typeof newInstance === 'object') { cjsSourceMapCache.set(newInstance, sourceMap); } } diff --git a/test/fixtures/source-map/throw-string-original.js b/test/fixtures/source-map/throw-string-original.js new file mode 100644 index 00000000000000..fe177b669fdf7e --- /dev/null +++ b/test/fixtures/source-map/throw-string-original.js @@ -0,0 +1,8 @@ +/* + * comments dropped by uglify. + */ +function Hello() { + throw 'goodbye'; +} + +Hello(); diff --git a/test/fixtures/source-map/throw-string.js b/test/fixtures/source-map/throw-string.js new file mode 100644 index 00000000000000..7a810e12f5f5a9 --- /dev/null +++ b/test/fixtures/source-map/throw-string.js @@ -0,0 +1,2 @@ +function Hello(){throw"goodbye"}Hello(); +//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbInRocm93LXN0cmluZy1vcmlnaW5hbC5qcyJdLCJuYW1lcyI6WyJIZWxsbyJdLCJtYXBwaW5ncyI6IkFBR0EsU0FBU0EsUUFDUCxLQUFNLFVBR1JBIn0= diff --git a/test/parallel/test-source-map-enable.js b/test/parallel/test-source-map-enable.js index 766c1e2955465f..1226b4534abc28 100644 --- a/test/parallel/test-source-map-enable.js +++ b/test/parallel/test-source-map-enable.js @@ -307,6 +307,23 @@ function nextdir() { assert.ok(sourceMap); } +// Does not attempt to rekey source map on primitive type. +{ + const coverageDirectory = nextdir(); + const output = spawnSync(process.execPath, [ + '--enable-source-maps', + require.resolve('../fixtures/source-map/throw-string.js'), + ], { env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } }); + const sourceMap = getSourceMapFromCache( + 'throw-string.js', + coverageDirectory + ); + // Original stack trace. + assert.match(output.stderr.toString(), /goodbye/); + // Source map should have been serialized. + assert.ok(sourceMap); +} + function getSourceMapFromCache(fixtureFile, coverageDirectory) { const jsonFiles = fs.readdirSync(coverageDirectory); for (const jsonFile of jsonFiles) { From 0b67b1e8310c6b62166d9835bbeff3b446c3a8fb Mon Sep 17 00:00:00 2001 From: "Benjamin E. Coe" Date: Sun, 13 Jun 2021 19:14:19 -0400 Subject: [PATCH 2/5] Update lib/internal/source_map/source_map_cache.js Co-authored-by: Rich Trott --- lib/internal/source_map/source_map_cache.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/source_map/source_map_cache.js b/lib/internal/source_map/source_map_cache.js index 0d33a52b38d435..6191bae904412f 100644 --- a/lib/internal/source_map/source_map_cache.js +++ b/lib/internal/source_map/source_map_cache.js @@ -175,7 +175,7 @@ function sourcesToAbsolute(baseURL, data) { // Move source map from garbage collected module to alternate key. function rekeySourceMap(cjsModuleInstance, newInstance) { const sourceMap = cjsSourceMapCache.get(cjsModuleInstance); - // If an exception occurs before a module finishes loading it will removed + // If an exception occurs before a module finishes loading, it is removed // from the module cache and our WeakMap. To allow a source map to still be // applied to the stack trace when this happens, we rekey on the thrown error. // This is only possible if the error is an object and not a primitive type. From 9a7fad64b36870d5f30da5540cb50a7e016349bb Mon Sep 17 00:00:00 2001 From: bcoe Date: Sun, 13 Jun 2021 16:33:18 -0700 Subject: [PATCH 3/5] stop rekeying cjs cache --- lib/internal/modules/cjs/loader.js | 2 -- lib/internal/source_map/source_map_cache.js | 13 ------------- 2 files changed, 15 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 4f2b594755262f..813a8d32f544f1 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -76,7 +76,6 @@ const { NativeModule } = require('internal/bootstrap/loaders'); const { getSourceMapsEnabled, maybeCacheSourceMap, - rekeySourceMap } = require('internal/source_map/source_map_cache'); const { pathToFileURL, fileURLToPath, isURLInstance } = require('internal/url'); const { deprecate } = require('internal/util'); @@ -822,7 +821,6 @@ Module._load = function(request, parent, isMain) { try { module.load(filename); } catch (err) { - rekeySourceMap(Module._cache[filename], err); throw err; /* node-do-not-add-exception-line */ } } else { diff --git a/lib/internal/source_map/source_map_cache.js b/lib/internal/source_map/source_map_cache.js index 6191bae904412f..07cdf8a337af7e 100644 --- a/lib/internal/source_map/source_map_cache.js +++ b/lib/internal/source_map/source_map_cache.js @@ -172,18 +172,6 @@ function sourcesToAbsolute(baseURL, data) { return data; } -// Move source map from garbage collected module to alternate key. -function rekeySourceMap(cjsModuleInstance, newInstance) { - const sourceMap = cjsSourceMapCache.get(cjsModuleInstance); - // If an exception occurs before a module finishes loading, it is removed - // from the module cache and our WeakMap. To allow a source map to still be - // applied to the stack trace when this happens, we rekey on the thrown error. - // This is only possible if the error is an object and not a primitive type. - if (sourceMap && typeof newInstance === 'object') { - cjsSourceMapCache.set(newInstance, sourceMap); - } -} - // WARNING: The `sourceMapCacheToObject` and `appendCJSCache` run during // shutdown. In particular, they also run when Workers are terminated, making // it important that they do not call out to any user-provided code, including @@ -244,6 +232,5 @@ module.exports = { findSourceMap, getSourceMapsEnabled, maybeCacheSourceMap, - rekeySourceMap, sourceMapCacheToObject, }; From 4af02fd96d8b899d59df906d67950f1a757b19d1 Mon Sep 17 00:00:00 2001 From: bcoe Date: Sun, 13 Jun 2021 16:39:42 -0700 Subject: [PATCH 4/5] delete more code --- lib/internal/modules/cjs/loader.js | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 813a8d32f544f1..d969a6449cf545 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -74,7 +74,6 @@ module.exports = { const { NativeModule } = require('internal/bootstrap/loaders'); const { - getSourceMapsEnabled, maybeCacheSourceMap, } = require('internal/source_map/source_map_cache'); const { pathToFileURL, fileURLToPath, isURLInstance } = require('internal/url'); @@ -814,18 +813,7 @@ Module._load = function(request, parent, isMain) { let threw = true; try { - // Intercept exceptions that occur during the first tick and rekey them - // on error instance rather than module instance (which will immediately be - // garbage collected). - if (getSourceMapsEnabled()) { - try { - module.load(filename); - } catch (err) { - throw err; /* node-do-not-add-exception-line */ - } - } else { - module.load(filename); - } + module.load(filename); threw = false; } finally { if (threw) { From b56a91459ab1efb34bd612f30a3f263f53688093 Mon Sep 17 00:00:00 2001 From: bcoe Date: Mon, 14 Jun 2021 17:50:42 -0700 Subject: [PATCH 5/5] test: update test to reflect refactor --- test/parallel/test-source-map-enable.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-source-map-enable.js b/test/parallel/test-source-map-enable.js index 1226b4534abc28..e0c222d1a333bf 100644 --- a/test/parallel/test-source-map-enable.js +++ b/test/parallel/test-source-map-enable.js @@ -307,7 +307,7 @@ function nextdir() { assert.ok(sourceMap); } -// Does not attempt to rekey source map on primitive type. +// Does not throw TypeError when primitive value is thrown. { const coverageDirectory = nextdir(); const output = spawnSync(process.execPath, [