Commit eddde6c
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
PR-URL: #39025
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>1 parent d2b972e commit eddde6c
5 files changed
Lines changed: 28 additions & 24 deletions
File tree
- lib/internal
- modules/cjs
- source_map
- test
- fixtures/source-map
- parallel
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -74,9 +74,7 @@ module.exports = { | |||
| 74 | 74 | ||
| 75 | 75 | const { NativeModule } = require('internal/bootstrap/loaders'); | |
| 76 | 76 | const { | |
| 77 | - getSourceMapsEnabled, | ||
| 78 | 77 | maybeCacheSourceMap, | |
| 79 | - rekeySourceMap | ||
| 80 | 78 | } = require('internal/source_map/source_map_cache'); | |
| 81 | 79 | const { pathToFileURL, fileURLToPath, isURLInstance } = require('internal/url'); | |
| 82 | 80 | const { deprecate } = require('internal/util'); | |
@@ -815,19 +813,7 @@ Module._load = function(request, parent, isMain) { | |||
| 815 | 813 | ||
| 816 | 814 | let threw = true; | |
| 817 | 815 | try { | |
| 818 | - // Intercept exceptions that occur during the first tick and rekey them | ||
| 819 | - // on error instance rather than module instance (which will immediately be | ||
| 820 | - // garbage collected). | ||
| 821 | - if (getSourceMapsEnabled()) { | ||
| 822 | - try { | ||
| 823 | - module.load(filename); | ||
| 824 | - } catch (err) { | ||
| 825 | - rekeySourceMap(Module._cache[filename], err); | ||
| 826 | - throw err; /* node-do-not-add-exception-line */ | ||
| 827 | - } | ||
| 828 | - } else { | ||
| 829 | - module.load(filename); | ||
| 830 | - } | ||
| 816 | + module.load(filename); | ||
| 831 | 817 | threw = false; | |
| 832 | 818 | } finally { | |
| 833 | 819 | if (threw) { | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -172,14 +172,6 @@ function sourcesToAbsolute(baseURL, data) { | |||
| 172 | 172 | return data; | |
| 173 | 173 | } | |
| 174 | 174 | ||
| 175 | - // Move source map from garbage collected module to alternate key. | ||
| 176 | - function rekeySourceMap(cjsModuleInstance, newInstance) { | ||
| 177 | - const sourceMap = cjsSourceMapCache.get(cjsModuleInstance); | ||
| 178 | - if (sourceMap) { | ||
| 179 | - cjsSourceMapCache.set(newInstance, sourceMap); | ||
| 180 | - } | ||
| 181 | - } | ||
| 182 | - | ||
| 183 | 175 | // WARNING: The `sourceMapCacheToObject` and `appendCJSCache` run during | |
| 184 | 176 | // shutdown. In particular, they also run when Workers are terminated, making | |
| 185 | 177 | // it important that they do not call out to any user-provided code, including | |
@@ -240,6 +232,5 @@ module.exports = { | |||
| 240 | 232 | findSourceMap, | |
| 241 | 233 | getSourceMapsEnabled, | |
| 242 | 234 | maybeCacheSourceMap, | |
| 243 | - rekeySourceMap, | ||
| 244 | 235 | sourceMapCacheToObject, | |
| 245 | 236 | }; | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -0,0 +1,8 @@ | |||
| 1 | + /* | ||
| 2 | + * comments dropped by uglify. | ||
| 3 | + */ | ||
| 4 | + function Hello() { | ||
| 5 | + throw 'goodbye'; | ||
| 6 | + } | ||
| 7 | + | ||
| 8 | + Hello(); | ||
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -307,6 +307,23 @@ function nextdir() { | |||
| 307 | 307 | assert.ok(sourceMap); | |
| 308 | 308 | } | |
| 309 | 309 | ||
| 310 | + // Does not throw TypeError when primitive value is thrown. | ||
| 311 | + { | ||
| 312 | + const coverageDirectory = nextdir(); | ||
| 313 | + const output = spawnSync(process.execPath, [ | ||
| 314 | + '--enable-source-maps', | ||
| 315 | + require.resolve('../fixtures/source-map/throw-string.js'), | ||
| 316 | + ], { env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } }); | ||
| 317 | + const sourceMap = getSourceMapFromCache( | ||
| 318 | + 'throw-string.js', | ||
| 319 | + coverageDirectory | ||
| 320 | + ); | ||
| 321 | + // Original stack trace. | ||
| 322 | + assert.match(output.stderr.toString(), /goodbye/); | ||
| 323 | + // Source map should have been serialized. | ||
| 324 | + assert.ok(sourceMap); | ||
| 325 | + } | ||
| 326 | + | ||
| 310 | 327 | function getSourceMapFromCache(fixtureFile, coverageDirectory) { | |
| 311 | 328 | const jsonFiles = fs.readdirSync(coverageDirectory); | |
| 312 | 329 | for (const jsonFile of jsonFiles) { | |
0 commit comments