From c5cc3d4a8b22f4677311bccd609c7792d788db2b Mon Sep 17 00:00:00 2001
From: bcoe <bencoe@google.com>
Date: Sun, 13 Jun 2021 09:21:40 -0700
Subject: [PATCH] errors: don't rekey on primitive type
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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: https://github.com/nodejs/node/pull/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>
---
 lib/internal/modules/cjs/loader.js              | 16 +---------------
 lib/internal/source_map/source_map_cache.js     |  9 ---------
 .../source-map/throw-string-original.js         |  8 ++++++++
 test/fixtures/source-map/throw-string.js        |  2 ++
 test/parallel/test-source-map-enable.js         | 17 +++++++++++++++++
 5 files changed, 28 insertions(+), 24 deletions(-)
 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/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
index 4f2b594755262f..d969a6449cf545 100644
--- 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
index f0d911f78fad8b..07cdf8a337af7e 100644
--- 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
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..e0c222d1a333bf 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 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) {