From 02eb10b87b2a33afdcb7b2135e6907a9102649c0 Mon Sep 17 00:00:00 2001 From: legendecas Date: Tue, 14 Jun 2022 01:34:31 +0800 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/43428 Refs: https://github.com/nodejs/node/issues/43047 Reviewed-By: Ben Coe --- benchmark/es/eval.js | 46 +++++++++++++ .../source_map/prepare_stack_trace.js | 9 ++- lib/internal/source_map/source_map_cache.js | 69 +++++++++++++++---- src/api/environment.cc | 5 ++ src/env.h | 1 + src/node_context_data.h | 8 ++- src/node_contextify.cc | 6 +- src/node_errors.cc | 43 ++++++++++++ src/node_errors.h | 5 ++ test/message/source_map_eval.js | 8 +++ test/message/source_map_eval.out | 12 ++++ 11 files changed, 194 insertions(+), 18 deletions(-) create mode 100644 benchmark/es/eval.js create mode 100644 test/message/source_map_eval.js create mode 100644 test/message/source_map_eval.out diff --git a/benchmark/es/eval.js b/benchmark/es/eval.js new file mode 100644 index 00000000000000..b4c6ee2e0a64c7 --- /dev/null +++ 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 index 11591b610ec485..55dc8b344f8be6 100644 --- 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 index 43acebae609cf2..c7770e7a6c733b 100644 --- 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=(?[^\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 index f7455fc5f1e98c..011b14c9954eeb 100644 --- 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 InitializeContextForSnapshot(Local 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 index 921b6758ac5b6c..52bfaa3db8d46c 100644 --- 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 index 912e65b42707fa..42aae872e60a13 100644 --- 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 index 2460fa9361b56b..f383b6def93bbe 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -233,7 +233,11 @@ MaybeLocal 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 index be4baa80fd1b80..052fbafcb1aab8 100644 --- 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 context, + v8::Local 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 maybe_cache_source_map = + env->maybe_cache_generated_source_map(); + Local argv[1] = {source}; + + MaybeLocal maybe_cached = maybe_cache_source_map->Call( + context, context->Global(), arraysize(argv), argv); + if (maybe_cached.IsEmpty()) { + DCHECK(try_catch.HasCaught()); + } + } + + Local 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& args) { env->set_source_maps_enabled(args[0].As()->Value()); } +static void SetMaybeCacheGeneratedSourceMap( + const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + CHECK(args[0]->IsFunction()); + env->set_maybe_cache_generated_source_map(args[0].As()); +} + static void SetEnhanceStackForFatalException( const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -870,6 +909,7 @@ static void TriggerUncaughtException(const FunctionCallbackInfo& 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 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 index d5c669b3df12b9..08dda5e08bbd4e 100644 --- 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 context, + v8::Local 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 new file mode 100644 index 00000000000000..534d16bd34c28f --- /dev/null +++ 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 new file mode 100644 index 00000000000000..ed03caa4630787 --- /dev/null +++ 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. (*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 *