lib,src: add source map support for global eval

Dynamic sources with FunctionConstructor is not supported yet as V8 prepends lines to the sources which makes the stack line number incorrect. PR-URL: #43428 Refs: #43047 Reviewed-By: Ben Coe <[email protected]>
nodejs · Jul 11, 2022 · 02eb10b · 02eb10b
1 parent c6ec630
commit 02eb10b
Show file tree

Hide file tree

Showing 11 changed files with 194 additions and 18 deletions.
diff --git a/benchmark/es/eval.js b/benchmark/es/eval.js
@@ -0,0 +1,46 @@
+'use strict';
+
+const common = require('../common.js');
+
+const bench = common.createBenchmark(main, {
+  method: ['without-sourcemap', 'sourcemap'],
+  n: [1e6],
+});
+
+const sourceWithoutSourceMap = `
+'use strict';
+(function() {
+  let a = 1;
+  for (let i = 0; i < 1000; i++) {
+    a++;
+  }
+  return a;
+})();
+`;
+const sourceWithSourceMap = `
+${sourceWithoutSourceMap}
+//# sourceMappingURL=https://ci.nodejs.org/405
+`;
+
+function evalN(n, source) {
+  bench.start();
+  for (let i = 0; i < n; i++) {
+    eval(source);
+  }
+  bench.end(n);
+}
+
+function main({ n, method }) {
+  switch (method) {
+    case 'without-sourcemap':
+      process.setSourceMapsEnabled(false);
+      evalN(n, sourceWithoutSourceMap);
+      break;
+    case 'sourcemap':
+      process.setSourceMapsEnabled(true);
+      evalN(n, sourceWithSourceMap);
+      break;
+    default:
+      throw new Error(`Unexpected method "${method}"`);
+  }
+}
diff --git a/lib/internal/source_map/prepare_stack_trace.js b/lib/internal/source_map/prepare_stack_trace.js
@@ -61,10 +61,15 @@ const prepareStackTrace = (globalThis, error, trace) => {
     try {
       // A stack trace will often have several call sites in a row within the
       // same file, cache the source map and file content accordingly:
-      const fileName = t.getFileName();
+      let fileName = t.getFileName();
+      let generated = false;
+      if (fileName === undefined) {
+        fileName = t.getEvalOrigin();
+        generated = true;
+      }
       const sm = fileName === lastFileName ?
         lastSourceMap :
-        findSourceMap(fileName);
+        findSourceMap(fileName, generated);
       lastSourceMap = sm;
       lastFileName = fileName;
       if (sm) {

diff --git a/lib/internal/source_map/source_map_cache.js b/lib/internal/source_map/source_map_cache.js
@@ -30,12 +30,18 @@ const {
   normalizeReferrerURL,
 } = require('internal/modules/cjs/helpers');
 const { validateBoolean } = require('internal/validators');
+const { setMaybeCacheGeneratedSourceMap } = internalBinding('errors');
+
 // Since the CJS module cache is mutable, which leads to memory leaks when
 // modules are deleted, we use a WeakMap so that the source map cache will
 // be purged automatically:
 const cjsSourceMapCache = new IterableWeakMap();
 // The esm cache is not mutable, so we can use a Map without memory concerns:
 const esmSourceMapCache = new SafeMap();
+// The generated sources is not mutable, so we can use a Map without memory concerns:
+const generatedSourceMapCache = new SafeMap();
+const kLeadingProtocol = /^\w+:\/\//;
+
 const { fileURLToPath, pathToFileURL, URL } = require('internal/url');
 let SourceMap;
 
@@ -71,14 +77,13 @@ function setSourceMapsEnabled(val) {
   sourceMapsEnabled = val;
 }
 
-function maybeCacheSourceMap(filename, content, cjsModuleInstance) {
+function maybeCacheSourceMap(filename, content, cjsModuleInstance, isGeneratedSource) {
   const sourceMapsEnabled = getSourceMapsEnabled();
   if (!(process.env.NODE_V8_COVERAGE || sourceMapsEnabled)) return;
   try {
     filename = normalizeReferrerURL(filename);
   } catch (err) {
-    // This is most likely an [eval]-wrapper, which is currently not
-    // supported.
+    // This is most likely an invalid filename in sourceURL of [eval]-wrapper.
     debug(err);
     return;
   }
@@ -96,8 +101,14 @@ function maybeCacheSourceMap(filename, content, cjsModuleInstance) {
         data,
         url
       });
+    } else if (isGeneratedSource) {
+      generatedSourceMapCache.set(filename, {
+        lineLengths: lineLengths(content),
+        data,
+        url
+      });
     } else {
-      // If there is no cjsModuleInstance assume we are in a
+      // If there is no cjsModuleInstance and is not generated source assume we are in a
       // "modules/esm" context.
       esmSourceMapCache.set(filename, {
         lineLengths: lineLengths(content),
@@ -108,6 +119,31 @@ function maybeCacheSourceMap(filename, content, cjsModuleInstance) {
   }
 }
 
+function maybeCacheGeneratedSourceMap(content) {
+  const sourceMapsEnabled = getSourceMapsEnabled();
+  if (!(process.env.NODE_V8_COVERAGE || sourceMapsEnabled)) return;
+
+  const matchSourceURL = RegExpPrototypeExec(
+    /\/[*/]#\s+sourceURL=(?<sourceURL>[^\s]+)/,
+    content
+  );
+  if (matchSourceURL == null) {
+    return;
+  }
+  let sourceURL = matchSourceURL.groups.sourceURL;
+  if (RegExpPrototypeExec(kLeadingProtocol, sourceURL) === null) {
+    sourceURL = pathToFileURL(sourceURL).href;
+  }
+  try {
+    maybeCacheSourceMap(sourceURL, content, null, true);
+  } catch (err) {
+    // This can happen if the filename is not a valid URL.
+    // If we fail to cache the source map, we should not fail the whole process.
+    debug(err);
+  }
+}
+setMaybeCacheGeneratedSourceMap(maybeCacheGeneratedSourceMap);
+
 function dataFromUrl(sourceURL, sourceMappingURL) {
   try {
     const url = new URL(sourceMappingURL);
@@ -218,21 +254,26 @@ function appendCJSCache(obj) {
   }
 }
 
-function findSourceMap(sourceURL) {
-  if (RegExpPrototypeExec(/^\w+:\/\//, sourceURL) === null) {
+function findSourceMap(sourceURL, isGenerated) {
+  if (RegExpPrototypeExec(kLeadingProtocol, sourceURL) === null) {
     sourceURL = pathToFileURL(sourceURL).href;
   }
   if (!SourceMap) {
     SourceMap = require('internal/source_map/source_map').SourceMap;
   }
-  let sourceMap = esmSourceMapCache.get(sourceURL);
-  if (sourceMap === undefined) {
-    for (const value of cjsSourceMapCache) {
-      const filename = ObjectGetValueSafe(value, 'filename');
-      if (sourceURL === filename) {
-        sourceMap = {
-          data: ObjectGetValueSafe(value, 'data')
-        };
+  let sourceMap;
+  if (isGenerated) {
+    sourceMap = generatedSourceMapCache.get(sourceURL);
+  } else {
+    sourceMap = esmSourceMapCache.get(sourceURL);
+    if (sourceMap === undefined) {
+      for (const value of cjsSourceMapCache) {
+        const filename = ObjectGetValueSafe(value, 'filename');
+        if (sourceURL === filename) {
+          sourceMap = {
+            data: ObjectGetValueSafe(value, 'data')
+          };
+        }
       }
     }
   }

diff --git a/src/api/environment.cc b/src/api/environment.cc
@@ -254,6 +254,8 @@ void SetIsolateMiscHandlers(v8::Isolate* isolate, const IsolateSettings& s) {
   auto* allow_wasm_codegen_cb = s.allow_wasm_code_generation_callback ?
     s.allow_wasm_code_generation_callback : AllowWasmCodeGenerationCallback;
   isolate->SetAllowWasmCodeGenerationCallback(allow_wasm_codegen_cb);
+  isolate->SetModifyCodeGenerationFromStringsCallback(
+      ModifyCodeGenerationFromStrings);
 
   Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
   if (per_process::cli_options->get_per_isolate_options()
@@ -665,6 +667,9 @@ Maybe<bool> InitializeContextForSnapshot(Local<Context> context) {
   Isolate* isolate = context->GetIsolate();
   HandleScope handle_scope(isolate);
 
+  context->AllowCodeGenerationFromStrings(false);
+  context->SetEmbedderData(
+      ContextEmbedderIndex::kAllowCodeGenerationFromStrings, True(isolate));
   context->SetEmbedderData(ContextEmbedderIndex::kAllowWasmCodeGeneration,
                            True(isolate));
 

diff --git a/src/env.h b/src/env.h
@@ -543,6 +543,7 @@ class NoArrayBufferZeroFillScope {
   V(inspector_console_extension_installer, v8::Function)                       \
   V(inspector_disable_async_hooks, v8::Function)                               \
   V(inspector_enable_async_hooks, v8::Function)                                \
+  V(maybe_cache_generated_source_map, v8::Function)                            \
   V(messaging_deserialize_create_object, v8::Function)                         \
   V(message_port, v8::Object)                                                  \
   V(native_module_require, v8::Function)                                       \

diff --git a/src/node_context_data.h b/src/node_context_data.h
@@ -29,12 +29,18 @@ namespace node {
 #define NODE_BINDING_LIST_INDEX 36
 #endif
 
+#ifndef NODE_CONTEXT_ALLOW_CODE_GENERATION_FROM_STRINGS_INDEX
+#define NODE_CONTEXT_ALLOW_CODE_GENERATION_FROM_STRINGS_INDEX 37
+#endif
+
 enum ContextEmbedderIndex {
   kEnvironment = NODE_CONTEXT_EMBEDDER_DATA_INDEX,
   kSandboxObject = NODE_CONTEXT_SANDBOX_OBJECT_INDEX,
   kAllowWasmCodeGeneration = NODE_CONTEXT_ALLOW_WASM_CODE_GENERATION_INDEX,
   kContextTag = NODE_CONTEXT_TAG,
-  kBindingListIndex = NODE_BINDING_LIST_INDEX
+  kBindingListIndex = NODE_BINDING_LIST_INDEX,
+  kAllowCodeGenerationFromStrings =
+      NODE_CONTEXT_ALLOW_CODE_GENERATION_FROM_STRINGS_INDEX
 };
 
 }  // namespace node

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -233,7 +233,11 @@ MaybeLocal<Context> ContextifyContext::CreateV8Context(
                           ctx->Global());
 
   Utf8Value name_val(env->isolate(), options.name);
-  ctx->AllowCodeGenerationFromStrings(options.allow_code_gen_strings->IsTrue());
+  // Delegate the code generation validation to
+  // node::ModifyCodeGenerationFromStrings.
+  ctx->AllowCodeGenerationFromStrings(false);
+  ctx->SetEmbedderData(ContextEmbedderIndex::kAllowCodeGenerationFromStrings,
+                       options.allow_code_gen_strings);
   ctx->SetEmbedderData(ContextEmbedderIndex::kAllowWasmCodeGeneration,
                        options.allow_code_gen_wasm);
 

diff --git a/src/node_errors.cc b/src/node_errors.cc
@@ -454,6 +454,38 @@ void OnFatalError(const char* location, const char* message) {
   ABORT();
 }
 
+v8::ModifyCodeGenerationFromStringsResult ModifyCodeGenerationFromStrings(
+    v8::Local<v8::Context> context,
+    v8::Local<v8::Value> source,
+    bool is_code_like) {
+  HandleScope scope(context->GetIsolate());
+
+  Environment* env = Environment::GetCurrent(context);
+  if (env->source_maps_enabled()) {
+    // We do not expect the maybe_cache_generated_source_map to throw any more
+    // exceptions. If it does, just ignore it.
+    errors::TryCatchScope try_catch(env);
+    Local<Function> maybe_cache_source_map =
+        env->maybe_cache_generated_source_map();
+    Local<Value> argv[1] = {source};
+
+    MaybeLocal<Value> maybe_cached = maybe_cache_source_map->Call(
+        context, context->Global(), arraysize(argv), argv);
+    if (maybe_cached.IsEmpty()) {
+      DCHECK(try_catch.HasCaught());
+    }
+  }
+
+  Local<Value> allow_code_gen = context->GetEmbedderData(
+      ContextEmbedderIndex::kAllowCodeGenerationFromStrings);
+  bool codegen_allowed =
+      allow_code_gen->IsUndefined() || allow_code_gen->IsTrue();
+  return {
+      codegen_allowed,
+      {},
+  };
+}
+
 namespace errors {
 
 TryCatchScope::~TryCatchScope() {
@@ -836,6 +868,13 @@ static void SetSourceMapsEnabled(const FunctionCallbackInfo<Value>& args) {
   env->set_source_maps_enabled(args[0].As<Boolean>()->Value());
 }
 
+static void SetMaybeCacheGeneratedSourceMap(
+    const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+  CHECK(args[0]->IsFunction());
+  env->set_maybe_cache_generated_source_map(args[0].As<Function>());
+}
+
 static void SetEnhanceStackForFatalException(
     const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
@@ -870,6 +909,7 @@ static void TriggerUncaughtException(const FunctionCallbackInfo<Value>& args) {
 void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
   registry->Register(SetPrepareStackTraceCallback);
   registry->Register(SetSourceMapsEnabled);
+  registry->Register(SetMaybeCacheGeneratedSourceMap);
   registry->Register(SetEnhanceStackForFatalException);
   registry->Register(NoSideEffectsToString);
   registry->Register(TriggerUncaughtException);
@@ -883,6 +923,9 @@ void Initialize(Local<Object> target,
   env->SetMethod(
       target, "setPrepareStackTraceCallback", SetPrepareStackTraceCallback);
   env->SetMethod(target, "setSourceMapsEnabled", SetSourceMapsEnabled);
+  env->SetMethod(target,
+                 "setMaybeCacheGeneratedSourceMap",
+                 SetMaybeCacheGeneratedSourceMap);
   env->SetMethod(target,
                  "setEnhanceStackForFatalException",
                  SetEnhanceStackForFatalException);

diff --git a/src/node_errors.h b/src/node_errors.h
@@ -271,6 +271,11 @@ void DecorateErrorStack(Environment* env,
                         const errors::TryCatchScope& try_catch);
 }  // namespace errors
 
+v8::ModifyCodeGenerationFromStringsResult ModifyCodeGenerationFromStrings(
+    v8::Local<v8::Context> context,
+    v8::Local<v8::Value> source,
+    bool is_code_like);
+
 }  // namespace node
 
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

diff --git a/test/message/source_map_eval.js b/test/message/source_map_eval.js
@@ -0,0 +1,8 @@
+// Flags:  --enable-source-maps
+
+'use strict';
+require('../common');
+const fs = require('fs');
+
+const content = fs.readFileSync(require.resolve('../fixtures/source-map/tabs.js'), 'utf8');
+eval(content);
diff --git a/test/message/source_map_eval.out b/test/message/source_map_eval.out
@@ -0,0 +1,12 @@
+ReferenceError: alert is not defined
+    at Object.eval (*tabs.coffee:26:2)
+    at eval (*tabs.coffee:1:14)
+    at Object.<anonymous> (*source_map_eval.js:8:1)
+    at Module._compile (node:internal/modules/cjs/loader:*)
+    at Module._extensions..js (node:internal/modules/cjs/loader:*)
+    at Module.load (node:internal/modules/cjs/loader:*)
+    at Module._load (node:internal/modules/cjs/loader:*)
+    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:*)
+    at node:internal/main/run_main_module:*
+
+Node.js *