Skip to content

Commit

Permalink
errors: don't rekey on primitive type
Browse files Browse the repository at this point in the history
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
  • Loading branch information
bcoe authored and danielleadams committed Jun 22, 2021
1 parent d2b972e commit eddde6c
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 24 deletions.
16 changes: 1 addition & 15 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,7 @@ module.exports = {

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');
Expand Down Expand Up @@ -815,19 +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) {
rekeySourceMap(Module._cache[filename], err);
throw err; /* node-do-not-add-exception-line */
}
} else {
module.load(filename);
}
module.load(filename);
threw = false;
} finally {
if (threw) {
Expand Down
9 changes: 0 additions & 9 deletions lib/internal/source_map/source_map_cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,14 +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 (sourceMap) {
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
Expand Down Expand Up @@ -240,6 +232,5 @@ module.exports = {
findSourceMap,
getSourceMapsEnabled,
maybeCacheSourceMap,
rekeySourceMap,
sourceMapCacheToObject,
};
8 changes: 8 additions & 0 deletions test/fixtures/source-map/throw-string-original.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/*
* comments dropped by uglify.
*/
function Hello() {
throw 'goodbye';
}

Hello();
2 changes: 2 additions & 0 deletions test/fixtures/source-map/throw-string.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 17 additions & 0 deletions test/parallel/test-source-map-enable.js
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,23 @@ function nextdir() {
assert.ok(sourceMap);
}

// Does not throw TypeError when primitive value is thrown.
{
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) {
Expand Down

0 comments on commit eddde6c

Please sign in to comment.