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 <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
nodejs · Jun 22, 2021 · eddde6c · eddde6c
1 parent d2b972e
commit eddde6c
Show file tree

Hide file tree

Showing 5 changed files with 28 additions and 24 deletions.
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -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');
@@ -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) {

diff --git a/lib/internal/source_map/source_map_cache.js b/lib/internal/source_map/source_map_cache.js
@@ -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
@@ -240,6 +232,5 @@ module.exports = {
   findSourceMap,
   getSourceMapsEnabled,
   maybeCacheSourceMap,
-  rekeySourceMap,
   sourceMapCacheToObject,
 };
diff --git a/test/fixtures/source-map/throw-string-original.js 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
diff --git 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 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) {