Skip to content
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.

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

src,lib: print source map error source on demand #43875

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 26 additions & 18 deletions lib/internal/source_map/prepare_stack_trace.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const {
kIsNodeError,
} = require('internal/errors');
const { fileURLToPath } = require('internal/url');
const { setGetSourceMapErrorSource } = internalBinding('errors');

// Create a prettified stacktrace, inserting context from source maps
// if possible.
Expand Down Expand Up @@ -53,7 +54,6 @@ const prepareStackTrace = (globalThis, error, trace) => {
return errorString;
}

let errorSource = '';
let lastSourceMap;
let lastFileName;
const preparedTrace = ArrayPrototypeJoin(ArrayPrototypeMap(trace, (t, i) => {
Expand All @@ -62,14 +62,12 @@ const prepareStackTrace = (globalThis, error, trace) => {
// A stack trace will often have several call sites in a row within the
// same file, cache the source map and file content accordingly:
let fileName = t.getFileName();
let generated = false;
if (fileName === undefined) {
fileName = t.getEvalOrigin();
generated = true;
}
const sm = fileName === lastFileName ?
lastSourceMap :
findSourceMap(fileName, generated);
findSourceMap(fileName);
lastSourceMap = sm;
lastFileName = fileName;
if (sm) {
Expand All @@ -83,14 +81,6 @@ const prepareStackTrace = (globalThis, error, trace) => {
if (originalSource && originalLine !== undefined &&
originalColumn !== undefined) {
const name = getOriginalSymbolName(sm, trace, i);
if (i === 0) {
errorSource = getErrorSource(
sm,
originalSource,
originalLine,
originalColumn
);
}
// Construct call site name based on: v8.dev/docs/stack-trace-api:
const fnName = t.getFunctionName() ?? t.getMethodName();
const typeName = t.getTypeName();
Expand All @@ -116,7 +106,7 @@ const prepareStackTrace = (globalThis, error, trace) => {
}
return `${str}${t}`;
}), '');
return `${errorSource}${errorString}\n at ${preparedTrace}`;
return `${errorString}\n at ${preparedTrace}`;
};

// Transpilers may have removed the original symbol name used in the stack
Expand Down Expand Up @@ -155,7 +145,7 @@ function getErrorSource(
fileURLToPath(originalSourcePath) : originalSourcePath;
const source = getOriginalSource(
sourceMap.payload,
originalSourcePathNoScheme
originalSourcePath
);
const lines = RegExpPrototypeSymbolSplit(/\r?\n/, source, originalLine + 1);
const line = lines[originalLine];
Expand All @@ -178,28 +168,46 @@ function getErrorSource(

function getOriginalSource(payload, originalSourcePath) {
let source;
const originalSourcePathNoScheme =
StringPrototypeStartsWith(originalSourcePath, 'file://') ?
fileURLToPath(originalSourcePath) : originalSourcePath;
// payload.sources has been normalized to be an array of absolute urls.
const sourceContentIndex =
ArrayPrototypeIndexOf(payload.sources, originalSourcePath);
if (payload.sourcesContent?.[sourceContentIndex]) {
// First we check if the original source content was provided in the
// source map itself:
source = payload.sourcesContent[sourceContentIndex];
} else {
} else if (StringPrototypeStartsWith(originalSourcePath, 'file://')) {
// If no sourcesContent was found, attempt to load the original source
// from disk:
debug(`read source of ${originalSourcePath} from filesystem`);
const originalSourcePathNoScheme = fileURLToPath(originalSourcePath);
try {
source = readFileSync(originalSourcePathNoScheme, 'utf8');
} catch (err) {
debug(err);
source = '';
}
} else {
source = '';
}
return source;
}

function getSourceMapErrorSource(fileName, lineNumber, columnNumber) {
const sm = findSourceMap(fileName);
if (sm === null) {
return;
}
const {
originalLine,
originalColumn,
originalSource,
} = sm.findEntry(lineNumber - 1, columnNumber);
const errorSource = getErrorSource(sm, originalSource, originalLine, originalColumn);
return errorSource;
}

setGetSourceMapErrorSource(getSourceMapErrorSource);

module.exports = {
prepareStackTrace,
};
91 changes: 54 additions & 37 deletions lib/internal/source_map/source_map_cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ 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 kSourceMappingURLMagicComment = /\/[*/]#\s+sourceMappingURL=(?<sourceMappingURL>[^\s]+)/;
const kSourceURLMagicComment = /\/[*/]#\s+sourceURL=(?<sourceURL>[^\s]+)/;

const { fileURLToPath, pathToFileURL, URL } = require('internal/url');
let SourceMap;
Expand Down Expand Up @@ -77,7 +79,22 @@ function setSourceMapsEnabled(val) {
sourceMapsEnabled = val;
}

function maybeCacheSourceMap(filename, content, cjsModuleInstance, isGeneratedSource) {
function extractSourceURLMagicComment(content) {
bcoe marked this conversation as resolved.
Show resolved Hide resolved
const matchSourceURL = RegExpPrototypeExec(
kSourceURLMagicComment,
content
);
if (matchSourceURL === null) {
return null;
}
let sourceURL = matchSourceURL.groups.sourceURL;
if (sourceURL != null && RegExpPrototypeExec(kLeadingProtocol, sourceURL) === null) {
sourceURL = pathToFileURL(sourceURL).href;
}
return sourceURL;
}

function maybeCacheSourceMap(filename, content, cjsModuleInstance, isGeneratedSource, sourceURL) {
const sourceMapsEnabled = getSourceMapsEnabled();
if (!(process.env.NODE_V8_COVERAGE || sourceMapsEnabled)) return;
try {
Expand All @@ -87,10 +104,10 @@ function maybeCacheSourceMap(filename, content, cjsModuleInstance, isGeneratedSo
debug(err);
return;
}
const match = RegExpPrototypeExec(
/\/[*/]#\s+sourceMappingURL=(?<sourceMappingURL>[^\s]+)/,
content,
);
const match = RegExpPrototypeExec(kSourceMappingURLMagicComment, content);
if (sourceURL === undefined) {
sourceURL = extractSourceURLMagicComment(content);
}
if (match) {
const data = dataFromUrl(filename, match.groups.sourceMappingURL);
const url = data ? null : match.groups.sourceMappingURL;
Expand All @@ -99,22 +116,33 @@ function maybeCacheSourceMap(filename, content, cjsModuleInstance, isGeneratedSo
filename,
lineLengths: lineLengths(content),
data,
url
url,
sourceURL,
});
} else if (isGeneratedSource) {
generatedSourceMapCache.set(filename, {
const entry = {
lineLengths: lineLengths(content),
data,
url
});
url,
sourceURL
};
generatedSourceMapCache.set(filename, entry);
if (sourceURL) {
generatedSourceMapCache.set(sourceURL, entry);
}
} else {
// If there is no cjsModuleInstance and is not generated source assume we are in a
// "modules/esm" context.
esmSourceMapCache.set(filename, {
const entry = {
lineLengths: lineLengths(content),
data,
url
});
url,
sourceURL,
};
esmSourceMapCache.set(filename, entry);
if (sourceURL) {
esmSourceMapCache.set(sourceURL, entry);
}
}
}
}
Expand All @@ -123,19 +151,12 @@ 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) {
const sourceURL = extractSourceURLMagicComment(content);
if (sourceURL === null) {
return;
}
let sourceURL = matchSourceURL.groups.sourceURL;
if (RegExpPrototypeExec(kLeadingProtocol, sourceURL) === null) {
sourceURL = pathToFileURL(sourceURL).href;
}
try {
maybeCacheSourceMap(sourceURL, content, null, true);
maybeCacheSourceMap(sourceURL, content, null, true, sourceURL);
} 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 Expand Up @@ -254,33 +275,29 @@ function appendCJSCache(obj) {
}
}

function findSourceMap(sourceURL, isGenerated) {
function findSourceMap(sourceURL) {
if (RegExpPrototypeExec(kLeadingProtocol, sourceURL) === null) {
sourceURL = pathToFileURL(sourceURL).href;
}
if (!SourceMap) {
SourceMap = require('internal/source_map/source_map').SourceMap;
}
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')
};
}
let sourceMap = esmSourceMapCache.get(sourceURL) ?? generatedSourceMapCache.get(sourceURL);
if (sourceMap === undefined) {
for (const value of cjsSourceMapCache) {
const filename = ObjectGetValueSafe(value, 'filename');
const cachedSourceURL = ObjectGetValueSafe(value, 'sourceURL');
if (sourceURL === filename || sourceURL === cachedSourceURL) {
sourceMap = {
data: ObjectGetValueSafe(value, 'data')
};
}
}
}
if (sourceMap && sourceMap.data) {
return new SourceMap(sourceMap.data);
}
return undefined;
return null;
Copy link

Choose a reason for hiding this comment

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

Isn't this a breaking change? AVA for example relies on this that it returns undefined here.

Copy link
Member

Choose a reason for hiding this comment

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

Can you open a separate issue? This looks like something unintended that we can fix.

Copy link

Choose a reason for hiding this comment

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

}

module.exports = {
Expand Down
1 change: 1 addition & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,7 @@ class NoArrayBufferZeroFillScope {
V(enhance_fatal_stack_after_inspector, v8::Function) \
V(enhance_fatal_stack_before_inspector, v8::Function) \
V(fs_use_promises_symbol, v8::Symbol) \
V(get_source_map_error_source, v8::Function) \
V(host_import_module_dynamically_callback, v8::Function) \
V(host_initialize_import_meta_object_callback, v8::Function) \
V(http2session_on_altsvc_function, v8::Function) \
Expand Down
56 changes: 51 additions & 5 deletions src/node_errors.cc
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,40 @@ namespace per_process {
static Mutex tty_mutex;
} // namespace per_process

static std::string GetSourceMapErrorSource(Isolate* isolate,
Local<Context> context,
Local<Message> message,
bool* added_exception_line) {
v8::TryCatch try_catch(isolate);
HandleScope handle_scope(isolate);
Environment* env = Environment::GetCurrent(context);

// 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();
int linenum = message->GetLineNumber(context).FromJust();
int columnum = message->GetStartColumn(context).FromJust();

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(
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this approach a lot 👍 good idea moving this behaviour into C++ land.

context, Undefined(isolate), arraysize(argv), argv);
Local<Value> ret;
if (!maybe_ret.ToLocal(&ret)) {
// Ignore the caught exceptions.
DCHECK(try_catch.HasCaught());
return std::string();
}
if (!ret->IsString()) {
return std::string();
}
*added_exception_line = true;
node::Utf8Value error_source_utf8(isolate, ret.As<String>());
return *error_source_utf8;
}

static std::string GetErrorSource(Isolate* isolate,
Local<Context> context,
Local<Message> message,
Expand All @@ -58,17 +92,19 @@ static std::string GetErrorSource(Isolate* isolate,
std::string sourceline(*encoded_source, encoded_source.length());
*added_exception_line = false;

if (sourceline.find("node-do-not-add-exception-line") != std::string::npos) {
return sourceline;
}

// If source maps have been enabled, the exception line will instead be
// added in the JavaScript context:
Environment* env = Environment::GetCurrent(isolate);
const bool has_source_map_url =
!message->GetScriptOrigin().SourceMapUrl().IsEmpty();
if (has_source_map_url && env != nullptr && env->source_maps_enabled()) {
return sourceline;
}

if (sourceline.find("node-do-not-add-exception-line") != std::string::npos) {
return sourceline;
std::string source = GetSourceMapErrorSource(
isolate, context, message, added_exception_line);
return added_exception_line ? source : sourceline;
}

// Because of how node modules work, all scripts are wrapped with a
Expand Down Expand Up @@ -868,6 +904,13 @@ static void SetSourceMapsEnabled(const FunctionCallbackInfo<Value>& args) {
env->set_source_maps_enabled(args[0].As<Boolean>()->Value());
}

static void SetGetSourceMapErrorSource(
const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
CHECK(args[0]->IsFunction());
env->set_get_source_map_error_source(args[0].As<Function>());
}

static void SetMaybeCacheGeneratedSourceMap(
const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Expand Down Expand Up @@ -908,6 +951,7 @@ static void TriggerUncaughtException(const FunctionCallbackInfo<Value>& args) {

void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
registry->Register(SetPrepareStackTraceCallback);
registry->Register(SetGetSourceMapErrorSource);
registry->Register(SetSourceMapsEnabled);
registry->Register(SetMaybeCacheGeneratedSourceMap);
registry->Register(SetEnhanceStackForFatalException);
Expand All @@ -922,6 +966,8 @@ void Initialize(Local<Object> target,
Environment* env = Environment::GetCurrent(context);
env->SetMethod(
target, "setPrepareStackTraceCallback", SetPrepareStackTraceCallback);
env->SetMethod(
target, "setGetSourceMapErrorSource", SetGetSourceMapErrorSource);
env->SetMethod(target, "setSourceMapsEnabled", SetSourceMapsEnabled);
env->SetMethod(target,
"setMaybeCacheGeneratedSourceMap",
Expand Down
6 changes: 5 additions & 1 deletion test/message/source_map_disabled_by_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,8 @@ delete require.cache[require
// Re-enable.
process.setSourceMapsEnabled(true);

require('../fixtures/source-map/enclosing-call-site-min.js');
try {
require('../fixtures/source-map/enclosing-call-site-min.js');
} catch (e) {
console.log(e);
}