Skip to content

Commit

Permalink
lib,src: add source map support for global eval
Browse files Browse the repository at this point in the history
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 <bencoe@gmail.com>
  • Loading branch information
legendecas authored and targos committed Jul 31, 2022
1 parent 5edfccf commit abc134c
Show file tree
Hide file tree
Showing 11 changed files with 193 additions and 19 deletions.
46 changes: 46 additions & 0 deletions benchmark/es/eval.js
Original file line number Diff line number Diff line change
@@ -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}"`);
}
}
9 changes: 7 additions & 2 deletions lib/internal/source_map/prepare_stack_trace.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
71 changes: 56 additions & 15 deletions lib/internal/source_map/source_map_cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const {
ObjectKeys,
ObjectGetOwnPropertyDescriptor,
ObjectPrototypeHasOwnProperty,
RegExpPrototypeTest,
RegExpPrototypeExec,
SafeMap,
StringPrototypeMatch,
StringPrototypeSplit,
Expand All @@ -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;

Expand Down Expand Up @@ -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;
}
Expand All @@ -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),
Expand All @@ -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);
Expand Down Expand Up @@ -218,21 +254,26 @@ function appendCJSCache(obj) {
}
}

function findSourceMap(sourceURL) {
if (!RegExpPrototypeTest(/^\w+:\/\//, sourceURL)) {
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')
};
}
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions src/api/environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,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);

if ((s.flags & SHOULD_NOT_SET_PROMISE_REJECTION_CALLBACK) == 0) {
auto* promise_reject_cb = s.promise_reject_callback ?
Expand Down Expand Up @@ -645,6 +647,9 @@ 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));
return InitializePrimordials(context);
Expand Down
1 change: 1 addition & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,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) \
Expand Down
8 changes: 7 additions & 1 deletion src/node_context_data.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
43 changes: 43 additions & 0 deletions src/node_errors.cc
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,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() {
Expand Down Expand Up @@ -832,6 +864,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);
Expand Down Expand Up @@ -866,6 +905,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);
Expand All @@ -879,6 +919,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);
Expand Down
5 changes: 5 additions & 0 deletions src/node_errors.h
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,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
Expand Down
8 changes: 8 additions & 0 deletions test/message/source_map_eval.js
Original file line number Diff line number Diff line change
@@ -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);
10 changes: 10 additions & 0 deletions test/message/source_map_eval.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
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 Object.Module._extensions..js (node:internal/modules/cjs/loader:*)
at Module.load (node:internal/modules/cjs/loader:*)
at Function.Module._load (node:internal/modules/cjs/loader:*)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:*)
at node:internal/main/run_main_module:*

0 comments on commit abc134c

Please sign in to comment.