Skip to content

lib: cache source maps in vm sources #52153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

lib: cache source maps in vm sources #52153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/internal/modules/cjs/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1471,9 +1471,9 @@ function wrapSafe(filename, content, cjsModuleInstance, format) {
);

// Cache the source map for the module if present.
const { sourceMapURL } = script;
const { sourceMapURL, sourceURL } = script;
if (sourceMapURL) {
maybeCacheSourceMap(filename, content, cjsModuleInstance, false, undefined, sourceMapURL);
maybeCacheSourceMap(filename, content, cjsModuleInstance, false, sourceURL, sourceMapURL);
}

return {
Expand All @@ -1498,7 +1498,7 @@ function wrapSafe(filename, content, cjsModuleInstance, format) {

// Cache the source map for the module if present.
if (result.sourceMapURL) {
maybeCacheSourceMap(filename, content, cjsModuleInstance, false, undefined, result.sourceMapURL);
maybeCacheSourceMap(filename, content, cjsModuleInstance, false, result.sourceURL, result.sourceMapURL);
}

return result;
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/modules/esm/translators.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,10 @@ translators.set('module', function moduleStrategy(url, source, isMain) {
function loadCJSModule(module, source, url, filename, isMain) {
const compileResult = compileFunctionForCJSLoader(source, filename, false /* is_sea_main */, false);

const { function: compiledWrapper, sourceMapURL } = compileResult;
const { function: compiledWrapper, sourceMapURL, sourceURL } = compileResult;
// Cache the source map for the cjs module if present.
if (sourceMapURL) {
maybeCacheSourceMap(url, source, module, false, undefined, sourceMapURL);
maybeCacheSourceMap(url, source, module, false, sourceURL, sourceMapURL);
}

const cascadedLoader = require('internal/modules/esm/loader').getOrInitializeCascadedLoader();
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/modules/esm/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ function compileSourceTextModule(url, source, cascadedLoader) {
}
// Cache the source map for the module if present.
if (wrap.sourceMapURL) {
maybeCacheSourceMap(url, source, wrap, false, undefined, wrap.sourceMapURL);
maybeCacheSourceMap(url, source, wrap, false, wrap.sourceURL, wrap.sourceMapURL);
}
return wrap;
}
Expand Down
10 changes: 5 additions & 5 deletions lib/internal/source_map/source_map_cache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,10 +148,10 @@ function maybeCacheSourceMap(filename, content, moduleInstance, isGeneratedSourc
return;
}

// FIXME: callers should obtain sourceURL from v8 and pass it
// rather than leaving it undefined and extract by regex.
if (sourceURL === undefined) {
sourceURL = extractSourceURLMagicComment(content);
if (sourceURL !== undefined) {
// SourceURL magic comment content might be a file path or URL.
// Normalize the sourceURL to be a file URL if it is a file path.
sourceURL = normalizeReferrerURL(sourceURL);
}

const data = dataFromUrl(filename, sourceMapURL);
Expand Down Expand Up @@ -190,7 +190,7 @@ function maybeCacheGeneratedSourceMap(content) {
return;
}
try {
maybeCacheSourceMap(sourceURL, content, null, true, sourceURL);
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.
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/vm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const {
const {
getOptionValue,
} = require('internal/options');
const { maybeCacheSourceMap } = require('internal/source_map/source_map_cache');
const {
privateSymbols: {
contextify_context_private_symbol,
Expand Down Expand Up @@ -151,6 +152,7 @@ function internalCompileFunction(
}

registerImportModuleDynamically(result.function, importModuleDynamically);
maybeCacheSourceMap(filename, code, result.function, false, result.sourceURL, result.sourceMapURL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What’s the difference between a “source URL” and a “source map URL”? My naïve guess is that a source URL is a file: URL to the module itself, and a source map URL is either a file: URL to a .map file or a data: URL with the source map. I feel like these guesses are probably wrong, though, so these variables should at least get comments or perhaps get better names (since you reuse these names across many files).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your guess is correct. SourceURL is used to specify the URL of the eval-ed source, as described in https://tc39.es/source-map/#linking-evald-code-to-named-generated-code. However, it can be present in sources loaded by CJS/ESM loaders from filesystem as well. V8 will take this magic comment and report errors with it as the script resource name instead:

node/src/node_errors.cc

Lines 62 to 65 in f1949ac

// The ScriptResourceName of the message may be different from the one we use
// to compile the script. V8 replaces it when it detects magic comments in
// the source texts.
Local<Value> script_resource_name = message->GetScriptResourceName();
. So it has to be used as cache key along with its filename.

The name, source url, is taken from the magic comments as:

//# sourceMappingURL=<url>
//# sourceURL=foo.js


return result;
}
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/vm/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ const kPerContextModuleId = Symbol('kPerContextModuleId');
const kLink = Symbol('kLink');

const { isContext } = require('internal/vm');
const { maybeCacheSourceMap } = require('internal/source_map/source_map_cache');

function isModule(object) {
if (typeof object !== 'object' || object === null || !ObjectPrototypeHasOwnProperty(object, kWrap)) {
Expand Down Expand Up @@ -150,6 +151,7 @@ class Module {
registry.callbackReferrer = this;
const { registerModule } = require('internal/modules/esm/utils');
registerModule(this[kWrap], registry);
maybeCacheSourceMap(identifier, sourceText, this, false, this[kWrap].sourceURL, this[kWrap].sourceMapURL);
} else {
assert(syntheticEvaluationSteps);
this[kWrap] = new ModuleWrap(identifier, context,
Expand Down
2 changes: 2 additions & 0 deletions lib/vm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ const {
isContext: _isContext,
registerImportModuleDynamically,
} = require('internal/vm');
const { maybeCacheSourceMap } = require('internal/source_map/source_map_cache');
const {
vm_dynamic_import_main_context_default,
vm_context_no_contextify,
Expand Down Expand Up @@ -127,6 +128,7 @@ class Script extends ContextifyScript {
}

registerImportModuleDynamically(this, importModuleDynamically);
maybeCacheSourceMap(filename, code, this, false, this.sourceURL, this.sourceMapURL);
}

runInThisContext(options) {
Expand Down
1 change: 1 addition & 0 deletions src/env_properties.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@
V(sni_context_string, "sni_context") \
V(source_string, "source") \
V(source_map_url_string, "sourceMapURL") \
V(source_url_string, "sourceURL") \
V(specifier_string, "specifier") \
V(stack_string, "stack") \
V(standard_name_string, "standardName") \
Expand Down
7 changes: 7 additions & 0 deletions src/module_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,13 @@ void ModuleWrap::New(const FunctionCallbackInfo<Value>& args) {
return;
}

if (that->Set(context,
realm->env()->source_url_string(),
module->GetUnboundModuleScript()->GetSourceURL())
.IsNothing()) {
return;
}

if (that->Set(context,
realm->env()->source_map_url_string(),
module->GetUnboundModuleScript()->GetSourceMappingURL())
Expand Down
20 changes: 20 additions & 0 deletions src/node_contextify.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1098,6 +1098,13 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
return;
}

if (args.This()
->Set(env->context(),
env->source_url_string(),
v8_script->GetSourceURL())
.IsNothing())
return;

if (args.This()
->Set(env->context(),
env->source_map_url_string(),
Expand Down Expand Up @@ -1556,6 +1563,15 @@ Local<Object> ContextifyContext::CompileFunctionAndCacheResult(
Local<Object> result = Object::New(isolate);
if (result->Set(parsing_context, env->function_string(), fn).IsNothing())
return Object::New(env->isolate());

// ScriptOrigin::ResourceName() returns SourceURL magic comment content if
// present.
if (result
->Set(parsing_context,
env->source_url_string(),
fn->GetScriptOrigin().ResourceName())
.IsNothing())
return Object::New(env->isolate());
if (result
->Set(parsing_context,
env->source_map_url_string(),
Expand Down Expand Up @@ -1793,12 +1809,16 @@ static void CompileFunctionForCJSLoader(
std::vector<Local<Name>> names = {
env->cached_data_rejected_string(),
env->source_map_url_string(),
env->source_url_string(),
env->function_string(),
FIXED_ONE_BYTE_STRING(isolate, "canParseAsESM"),
};
std::vector<Local<Value>> values = {
Boolean::New(isolate, cache_rejected),
fn.IsEmpty() ? undefined : fn->GetScriptOrigin().SourceMapUrl(),
// ScriptOrigin::ResourceName() returns SourceURL magic comment content if
// present.
fn.IsEmpty() ? undefined : fn->GetScriptOrigin().ResourceName(),
fn.IsEmpty() ? undefined : fn.As<Value>(),
Boolean::New(isolate, can_parse_as_esm),
};
Expand Down
19 changes: 16 additions & 3 deletions src/node_errors.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,20 @@ static std::string GetSourceMapErrorSource(Isolate* isolate,
bool* added_exception_line) {
v8::TryCatch try_catch(isolate);
HandleScope handle_scope(isolate);
Environment* env = Environment::GetCurrent(context);
Realm* realm = Realm::GetCurrent(context);

Local<Function> get_source;
if (realm != nullptr) {
// If we are in a Realm, call the realm specific getSourceMapErrorSource
// callback to avoid passing the JS objects (the exception and trace) across
// the realm boundary.
get_source = realm->get_source_map_error_source();
} else {
Environment* env = Environment::GetCurrent(context);
// The context is created with ContextifyContext, call the principal
// realm's getSourceMapErrorSource callback.
get_source = env->principal_realm()->get_source_map_error_source();
}

// The ScriptResourceName of the message may be different from the one we use
// to compile the script. V8 replaces it when it detects magic comments in
Expand All @@ -70,8 +83,8 @@ static std::string GetSourceMapErrorSource(Isolate* isolate,
Local<Value> argv[] = {script_resource_name,
v8::Int32::New(isolate, linenum),
v8::Int32::New(isolate, columnum)};
MaybeLocal<Value> maybe_ret = env->get_source_map_error_source()->Call(
context, Undefined(isolate), arraysize(argv), argv);
MaybeLocal<Value> maybe_ret =
get_source->Call(context, Undefined(isolate), arraysize(argv), argv);
Local<Value> ret;
if (!maybe_ret.ToLocal(&ret)) {
// Ignore the caught exceptions.
Expand Down
30 changes: 27 additions & 3 deletions test/common/assertSnapshot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ const assert = require('node:assert/strict');
const stackFramesRegexp = /(?<=\n)(\s+)((.+?)\s+\()?(?:\(?(.+?):(\d+)(?::(\d+))?)\)?(\s+\{)?(\[\d+m)?(\n|$)/g;
const windowNewlineRegexp = /\r/g;

function replaceExperimentalWarning(str) {
return str.replace(/\(node:\d+\) ExperimentalWarning: (.*)\n/g, '')
.replace('(Use `node --trace-warnings ...` to show where the warning was created)\n', '');
}

function replaceNodeVersion(str) {
return str.replaceAll(process.version, '*');
}
Expand All @@ -16,6 +21,10 @@ function replaceStackTrace(str, replacement = '$1*$7$8\n') {
return str.replace(stackFramesRegexp, replacement);
}

function replaceInternalStackTrace(str) {
return str.replaceAll(/(\W+).*node:internal.*/g, '$1*');
}

function replaceWindowsLineEndings(str) {
return str.replace(windowNewlineRegexp, '');
}
Expand All @@ -24,8 +33,20 @@ function replaceWindowsPaths(str) {
return common.isWindows ? str.replaceAll(path.win32.sep, path.posix.sep) : str;
}

function replaceFullPaths(str) {
return str.replaceAll('\\\'', "'").replaceAll(path.resolve(__dirname, '../..'), '');
function replaceWindowsDriveLetter(str) {
if (!common.isWindows) {
return str;
}
const currentDriveLetter = path.parse(process.cwd()).root.substring(0, 1).toLowerCase();
const regex = new RegExp(`${currentDriveLetter}:`, 'gi');
return str.replaceAll('\\\'', "'").replaceAll(regex, '');
}

function transformCwd(replacement = '') {
const cwd = process.cwd();
return (str) => {
return str.replaceAll('\\\'', "'").replaceAll(cwd, replacement);
};
}

function transform(...args) {
Expand Down Expand Up @@ -94,11 +115,14 @@ async function spawnAndAssert(filename, transform = (x) => x, { tty = false, ...
module.exports = {
assertSnapshot,
getSnapshotPath,
replaceFullPaths,
replaceExperimentalWarning,
replaceNodeVersion,
replaceStackTrace,
replaceInternalStackTrace,
replaceWindowsLineEndings,
replaceWindowsPaths,
replaceWindowsDriveLetter,
spawnAndAssert,
transform,
transformCwd,
};
20 changes: 10 additions & 10 deletions test/fixtures/source-map/output/source_map_disabled_by_api.snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
Error: an error!
at functionD (*enclosing-call-site-min.js:1:156)
at functionC (*enclosing-call-site-min.js:1:97)
at functionB (*enclosing-call-site-min.js:1:60)
at functionA (*enclosing-call-site-min.js:1:26)
at Object.<anonymous> (*enclosing-call-site-min.js:1:199)
at functionD (*/test/fixtures/source-map/enclosing-call-site-min.js:1:156)
at functionC (*/test/fixtures/source-map/enclosing-call-site-min.js:1:97)
at functionB (*/test/fixtures/source-map/enclosing-call-site-min.js:1:60)
at functionA (*/test/fixtures/source-map/enclosing-call-site-min.js:1:26)
at Object.<anonymous> (*/test/fixtures/source-map/enclosing-call-site-min.js:1:199)
Error: an error!
at functionD (*enclosing-call-site.js:16:17)
at functionC (*enclosing-call-site.js:10:3)
at functionB (*enclosing-call-site.js:6:3)
at functionA (*enclosing-call-site.js:2:3)
at Object.<anonymous> (*enclosing-call-site.js:24:3)
at functionD (*/test/fixtures/source-map/enclosing-call-site.js:16:17)
at functionC (*/test/fixtures/source-map/enclosing-call-site.js:10:3)
at functionB (*/test/fixtures/source-map/enclosing-call-site.js:6:3)
at functionA (*/test/fixtures/source-map/enclosing-call-site.js:2:3)
at Object.<anonymous> (*/test/fixtures/source-map/enclosing-call-site.js:24:3)
20 changes: 10 additions & 10 deletions test/fixtures/source-map/output/source_map_enabled_by_api.snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
Error: an error!
at functionD (*enclosing-call-site.js:16:17)
at functionC (*enclosing-call-site.js:10:3)
at functionB (*enclosing-call-site.js:6:3)
at functionA (*enclosing-call-site.js:2:3)
at Object.<anonymous> (*enclosing-call-site.js:24:3)
at functionD (*/test/fixtures/source-map/enclosing-call-site.js:16:17)
at functionC (*/test/fixtures/source-map/enclosing-call-site.js:10:3)
at functionB (*/test/fixtures/source-map/enclosing-call-site.js:6:3)
at functionA (*/test/fixtures/source-map/enclosing-call-site.js:2:3)
at Object.<anonymous> (*/test/fixtures/source-map/enclosing-call-site.js:24:3)
Error: an error!
at functionD (*enclosing-call-site-min.js:1:156)
at functionC (*enclosing-call-site-min.js:1:97)
at functionB (*enclosing-call-site-min.js:1:60)
at functionA (*enclosing-call-site-min.js:1:26)
at Object.<anonymous> (*enclosing-call-site-min.js:1:199)
at functionD (*/test/fixtures/source-map/enclosing-call-site-min.js:1:156)
at functionC (*/test/fixtures/source-map/enclosing-call-site-min.js:1:97)
at functionB (*/test/fixtures/source-map/enclosing-call-site-min.js:1:60)
at functionA (*/test/fixtures/source-map/enclosing-call-site-min.js:1:26)
at Object.<anonymous> (*/test/fixtures/source-map/enclosing-call-site-min.js:1:199)
12 changes: 6 additions & 6 deletions test/fixtures/source-map/output/source_map_enclosing_function.snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
*enclosing-call-site.js:26
*/test/fixtures/source-map/enclosing-call-site.js:26
throw err
^


Error: an error!
at functionD (*enclosing-call-site.js:16:17)
at functionC (*enclosing-call-site.js:10:3)
at functionB (*enclosing-call-site.js:6:3)
at functionA (*enclosing-call-site.js:2:3)
at Object.<anonymous> (*enclosing-call-site.js:24:3)
at functionD (*/test/fixtures/source-map/enclosing-call-site.js:16:17)
at functionC (*/test/fixtures/source-map/enclosing-call-site.js:10:3)
at functionB (*/test/fixtures/source-map/enclosing-call-site.js:6:3)
at functionA (*/test/fixtures/source-map/enclosing-call-site.js:2:3)
at Object.<anonymous> (*/test/fixtures/source-map/enclosing-call-site.js:24:3)

Node.js *
1 change: 1 addition & 0 deletions test/fixtures/source-map/output/source_map_eval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ Error.stackTraceLimit = 3;
const fs = require('fs');

const content = fs.readFileSync(require.resolve('../tabs.js'), 'utf8');
// SourceURL magic comment is hardcoded in the source content.
eval(content);
8 changes: 4 additions & 4 deletions test/fixtures/source-map/output/source_map_eval.snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
*tabs.coffee:26
/synthethized/workspace/tabs.coffee:26
alert "I knew it!"
^


ReferenceError: alert is not defined
at Object.eval (*tabs.coffee:26:2)
at eval (*tabs.coffee:1:14)
at Object.<anonymous> (*output*source_map_eval.js:10:1)
at Object.eval (/synthethized/workspace/tabs.coffee:26:2)
at eval (/synthethized/workspace/tabs.coffee:1:14)
at Object.<anonymous> (*/test/fixtures/source-map/output/source_map_eval.js:11:1)

Node.js *
6 changes: 3 additions & 3 deletions test/fixtures/source-map/output/source_map_no_source_file.snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
*no-source.js:2
*/test/fixtures/source-map/no-source.js:2
throw new Error('foo');
^

Error: foo
at Throw (*file-not-exists.ts:2:9)
at Object.<anonymous> (*file-not-exists.ts:5:1)
at Throw (*/test/fixtures/source-map/file-not-exists.ts:2:9)
at Object.<anonymous> (*/test/fixtures/source-map/file-not-exists.ts:5:1)

Node.js *
16 changes: 8 additions & 8 deletions test/fixtures/source-map/output/source_map_prepare_stack_trace.snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
Error: an error!
at functionD (*enclosing-call-site.js:16:17)
at functionB (*enclosing-call-site.js:6:3)
at functionA (*enclosing-call-site.js:2:3)
at Object.<anonymous> (*enclosing-call-site.js:24:3)
at functionD (*/test/fixtures/source-map/enclosing-call-site.js:16:17)
at functionB (*/test/fixtures/source-map/enclosing-call-site.js:6:3)
at functionA (*/test/fixtures/source-map/enclosing-call-site.js:2:3)
at Object.<anonymous> (*/test/fixtures/source-map/enclosing-call-site.js:24:3)
Error: an error!
at functionD (*enclosing-call-site-min.js:1:156)
at functionB (*enclosing-call-site-min.js:1:60)
at functionA (*enclosing-call-site-min.js:1:26)
at Object.<anonymous> (*enclosing-call-site-min.js:1:199)
at functionD (*/test/fixtures/source-map/enclosing-call-site-min.js:1:156)
at functionB (*/test/fixtures/source-map/enclosing-call-site-min.js:1:60)
at functionA (*/test/fixtures/source-map/enclosing-call-site-min.js:1:26)
at Object.<anonymous> (*/test/fixtures/source-map/enclosing-call-site-min.js:1:199)
Loading
Loading