Skip to content

Commit 1ef283d

Browse files
legendecaslegendecas
committed
src,lib: print source map error source on demand
The source context is not prepended to the value of the `stack` property when the source map is not enabled. Rather than prepending the error source context to the value of the `stack` property unconditionally, this patch aligns the behavior and only prints the source context when the error is not handled by userland (e.g. fatal errors). Also, this patch fixes that when source-map support is enabled, the error source context is not pointing to where the error was thrown.
1 parent 71ca6d7 commit 1ef283d

14 files changed

+152
-87
lines changed

‎lib/internal/source_map/prepare_stack_trace.js

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ const {
2525
kIsNodeError,
2626
} = require('internal/errors');
2727
const { fileURLToPath } = require('internal/url');
28+
const { setGetSourceMapErrorSource } = internalBinding('errors');
2829

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

56-
let errorSource = '';
5757
let lastSourceMap;
5858
let lastFileName;
5959
const preparedTrace = ArrayPrototypeJoin(ArrayPrototypeMap(trace, (t, i) => {
@@ -62,14 +62,12 @@ const prepareStackTrace = (globalThis, error, trace) => {
6262
// A stack trace will often have several call sites in a row within the
6363
// same file, cache the source map and file content accordingly:
6464
let fileName = t.getFileName();
65-
let generated = false;
6665
if (fileName === undefined) {
6766
fileName = t.getEvalOrigin();
68-
generated = true;
6967
}
7068
const sm = fileName === lastFileName ?
7169
lastSourceMap :
72-
findSourceMap(fileName, generated);
70+
findSourceMap(fileName);
7371
lastSourceMap = sm;
7472
lastFileName = fileName;
7573
if (sm) {
@@ -83,14 +81,6 @@ const prepareStackTrace = (globalThis, error, trace) => {
8381
if (originalSource && originalLine !== undefined &&
8482
originalColumn !== undefined) {
8583
const name = getOriginalSymbolName(sm, trace, i);
86-
if (i === 0) {
87-
errorSource = getErrorSource(
88-
sm,
89-
originalSource,
90-
originalLine,
91-
originalColumn
92-
);
93-
}
9484
// Construct call site name based on: v8.dev/docs/stack-trace-api:
9585
const fnName = t.getFunctionName() ?? t.getMethodName();
9686
const typeName = t.getTypeName();
@@ -116,7 +106,7 @@ const prepareStackTrace = (globalThis, error, trace) => {
116106
}
117107
return `${str}${t}`;
118108
}), '');
119-
return `${errorSource}${errorString}\n at ${preparedTrace}`;
109+
return `${errorString}\n at ${preparedTrace}`;
120110
};
121111

122112
// Transpilers may have removed the original symbol name used in the stack
@@ -155,7 +145,7 @@ function getErrorSource(
155145
fileURLToPath(originalSourcePath) : originalSourcePath;
156146
const source = getOriginalSource(
157147
sourceMap.payload,
158-
originalSourcePathNoScheme
148+
originalSourcePath
159149
);
160150
const lines = RegExpPrototypeSymbolSplit(/\r?\n/, source, originalLine + 1);
161151
const line = lines[originalLine];
@@ -178,28 +168,46 @@ function getErrorSource(
178168

179169
function getOriginalSource(payload, originalSourcePath) {
180170
let source;
181-
const originalSourcePathNoScheme =
182-
StringPrototypeStartsWith(originalSourcePath, 'file://') ?
183-
fileURLToPath(originalSourcePath) : originalSourcePath;
171+
// payload.sources has been normalized to be an array of absolute urls.
184172
const sourceContentIndex =
185173
ArrayPrototypeIndexOf(payload.sources, originalSourcePath);
186174
if (payload.sourcesContent?.[sourceContentIndex]) {
187175
// First we check if the original source content was provided in the
188176
// source map itself:
189177
source = payload.sourcesContent[sourceContentIndex];
190-
} else {
178+
} else if (StringPrototypeStartsWith(originalSourcePath, 'file://')) {
191179
// If no sourcesContent was found, attempt to load the original source
192180
// from disk:
181+
debug(`read source of ${originalSourcePath} from filesystem`);
182+
const originalSourcePathNoScheme = fileURLToPath(originalSourcePath);
193183
try {
194184
source = readFileSync(originalSourcePathNoScheme, 'utf8');
195185
} catch (err) {
196186
debug(err);
197187
source = '';
198188
}
189+
} else {
190+
source = '';
199191
}
200192
return source;
201193
}
202194

195+
function getSourceMapErrorSource(fileName, lineNumber, columnNumber) {
196+
const sm = findSourceMap(fileName);
197+
if (sm === null) {
198+
return;
199+
}
200+
const {
201+
originalLine,
202+
originalColumn,
203+
originalSource,
204+
} = sm.findEntry(lineNumber - 1, columnNumber);
205+
const errorSource = getErrorSource(sm, originalSource, originalLine, originalColumn);
206+
return errorSource;
207+
}
208+
209+
setGetSourceMapErrorSource(getSourceMapErrorSource);
210+
203211
module.exports = {
204212
prepareStackTrace,
205213
};

‎lib/internal/source_map/source_map_cache.js

Lines changed: 54 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@ const esmSourceMapCache = new SafeMap();
4141
// The generated sources is not mutable, so we can use a Map without memory concerns:
4242
const generatedSourceMapCache = new SafeMap();
4343
const kLeadingProtocol = /^\w+:\/\//;
44+
const kSourceMappingURLMagicComment = /\/[*/]#\s+sourceMappingURL=(?<sourceMappingURL>[^\s]+)/;
45+
const kSourceURLMagicComment = /\/[*/]#\s+sourceURL=(?<sourceURL>[^\s]+)/;
4446

4547
const { fileURLToPath, pathToFileURL, URL } = require('internal/url');
4648
let SourceMap;
@@ -77,7 +79,22 @@ function setSourceMapsEnabled(val) {
7779
sourceMapsEnabled = val;
7880
}
7981

80-
function maybeCacheSourceMap(filename, content, cjsModuleInstance, isGeneratedSource) {
82+
function extractSourceURLMagicComment(content) {
83+
const matchSourceURL = RegExpPrototypeExec(
84+
kSourceURLMagicComment,
85+
content
86+
);
87+
if (matchSourceURL === null) {
88+
return null;
89+
}
90+
let sourceURL = matchSourceURL.groups.sourceURL;
91+
if (sourceURL != null && RegExpPrototypeExec(kLeadingProtocol, sourceURL) === null) {
92+
sourceURL = pathToFileURL(sourceURL).href;
93+
}
94+
return sourceURL;
95+
}
96+
97+
function maybeCacheSourceMap(filename, content, cjsModuleInstance, isGeneratedSource, sourceURL) {
8198
const sourceMapsEnabled = getSourceMapsEnabled();
8299
if (!(process.env.NODE_V8_COVERAGE || sourceMapsEnabled)) return;
83100
try {
@@ -87,10 +104,10 @@ function maybeCacheSourceMap(filename, content, cjsModuleInstance, isGeneratedSo
87104
debug(err);
88105
return;
89106
}
90-
const match = RegExpPrototypeExec(
91-
/\/[*/]#\s+sourceMappingURL=(?<sourceMappingURL>[^\s]+)/,
92-
content,
93-
);
107+
const match = RegExpPrototypeExec(kSourceMappingURLMagicComment, content);
108+
if (sourceURL === undefined) {
109+
sourceURL = extractSourceURLMagicComment(content);
110+
}
94111
if (match) {
95112
const data = dataFromUrl(filename, match.groups.sourceMappingURL);
96113
const url = data ? null : match.groups.sourceMappingURL;
@@ -99,22 +116,33 @@ function maybeCacheSourceMap(filename, content, cjsModuleInstance, isGeneratedSo
99116
filename,
100117
lineLengths: lineLengths(content),
101118
data,
102-
url
119+
url,
120+
sourceURL,
103121
});
104122
} else if (isGeneratedSource) {
105-
generatedSourceMapCache.set(filename, {
123+
const entry = {
106124
lineLengths: lineLengths(content),
107125
data,
108-
url
109-
});
126+
url,
127+
sourceURL
128+
};
129+
generatedSourceMapCache.set(filename, entry);
130+
if (sourceURL) {
131+
generatedSourceMapCache.set(sourceURL, entry);
132+
}
110133
} else {
111134
// If there is no cjsModuleInstance and is not generated source assume we are in a
112135
// "modules/esm" context.
113-
esmSourceMapCache.set(filename, {
136+
const entry = {
114137
lineLengths: lineLengths(content),
115138
data,
116-
url
117-
});
139+
url,
140+
sourceURL,
141+
};
142+
esmSourceMapCache.set(filename, entry);
143+
if (sourceURL) {
144+
esmSourceMapCache.set(sourceURL, entry);
145+
}
118146
}
119147
}
120148
}
@@ -123,19 +151,12 @@ function maybeCacheGeneratedSourceMap(content) {
123151
const sourceMapsEnabled = getSourceMapsEnabled();
124152
if (!(process.env.NODE_V8_COVERAGE || sourceMapsEnabled)) return;
125153

126-
const matchSourceURL = RegExpPrototypeExec(
127-
/\/[*/]#\s+sourceURL=(?<sourceURL>[^\s]+)/,
128-
content
129-
);
130-
if (matchSourceURL == null) {
154+
const sourceURL = extractSourceURLMagicComment(content);
155+
if (sourceURL === null) {
131156
return;
132157
}
133-
let sourceURL = matchSourceURL.groups.sourceURL;
134-
if (RegExpPrototypeExec(kLeadingProtocol, sourceURL) === null) {
135-
sourceURL = pathToFileURL(sourceURL).href;
136-
}
137158
try {
138-
maybeCacheSourceMap(sourceURL, content, null, true);
159+
maybeCacheSourceMap(sourceURL, content, null, true, sourceURL);
139160
} catch (err) {
140161
// This can happen if the filename is not a valid URL.
141162
// If we fail to cache the source map, we should not fail the whole process.
@@ -254,33 +275,29 @@ function appendCJSCache(obj) {
254275
}
255276
}
256277

257-
function findSourceMap(sourceURL, isGenerated) {
278+
function findSourceMap(sourceURL) {
258279
if (RegExpPrototypeExec(kLeadingProtocol, sourceURL) === null) {
259280
sourceURL = pathToFileURL(sourceURL).href;
260281
}
261282
if (!SourceMap) {
262283
SourceMap = require('internal/source_map/source_map').SourceMap;
263284
}
264-
let sourceMap;
265-
if (isGenerated) {
266-
sourceMap = generatedSourceMapCache.get(sourceURL);
267-
} else {
268-
sourceMap = esmSourceMapCache.get(sourceURL);
269-
if (sourceMap === undefined) {
270-
for (const value of cjsSourceMapCache) {
271-
const filename = ObjectGetValueSafe(value, 'filename');
272-
if (sourceURL === filename) {
273-
sourceMap = {
274-
data: ObjectGetValueSafe(value, 'data')
275-
};
276-
}
285+
let sourceMap = esmSourceMapCache.get(sourceURL) ?? generatedSourceMapCache.get(sourceURL);
286+
if (sourceMap === undefined) {
287+
for (const value of cjsSourceMapCache) {
288+
const filename = ObjectGetValueSafe(value, 'filename');
289+
const cachedSourceURL = ObjectGetValueSafe(value, 'sourceURL');
290+
if (sourceURL === filename || sourceURL === cachedSourceURL) {
291+
sourceMap = {
292+
data: ObjectGetValueSafe(value, 'data')
293+
};
277294
}
278295
}
279296
}
280297
if (sourceMap && sourceMap.data) {
281298
return new SourceMap(sourceMap.data);
282299
}
283-
return undefined;
300+
return null;
284301
}
285302

286303
module.exports = {

‎src/env.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -525,6 +525,7 @@ class NoArrayBufferZeroFillScope {
525525
V(enhance_fatal_stack_after_inspector, v8::Function) \
526526
V(enhance_fatal_stack_before_inspector, v8::Function) \
527527
V(fs_use_promises_symbol, v8::Symbol) \
528+
V(get_source_map_error_source, v8::Function) \
528529
V(host_import_module_dynamically_callback, v8::Function) \
529530
V(host_initialize_import_meta_object_callback, v8::Function) \
530531
V(http2session_on_altsvc_function, v8::Function) \

‎src/node_errors.cc

Lines changed: 51 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,40 @@ namespace per_process {
4949
static Mutex tty_mutex;
5050
} // namespace per_process
5151

52+
static std::string GetSourceMapErrorSource(Isolate* isolate,
53+
Local<Context> context,
54+
Local<Message> message,
55+
bool* added_exception_line) {
56+
v8::TryCatch try_catch(isolate);
57+
HandleScope handle_scope(isolate);
58+
Environment* env = Environment::GetCurrent(context);
59+
60+
// The ScriptResourceName of the message may be different from the one we use
61+
// to compile the script. V8 replaces it when it detects magic comments in
62+
// the source texts.
63+
Local<Value> script_resource_name = message->GetScriptResourceName();
64+
int linenum = message->GetLineNumber(context).FromJust();
65+
int columnum = message->GetStartColumn(context).FromJust();
66+
67+
Local<Value> argv[] = {script_resource_name,
68+
v8::Int32::New(isolate, linenum),
69+
v8::Int32::New(isolate, columnum)};
70+
MaybeLocal<Value> maybe_ret = env->get_source_map_error_source()->Call(
71+
context, Undefined(isolate), arraysize(argv), argv);
72+
Local<Value> ret;
73+
if (!maybe_ret.ToLocal(&ret)) {
74+
// Ignore the caught exceptions.
75+
DCHECK(try_catch.HasCaught());
76+
return std::string();
77+
}
78+
if (!ret->IsString()) {
79+
return std::string();
80+
}
81+
*added_exception_line = true;
82+
node::Utf8Value error_source_utf8(isolate, ret.As<String>());
83+
return *error_source_utf8;
84+
}
85+
5286
static std::string GetErrorSource(Isolate* isolate,
5387
Local<Context> context,
5488
Local<Message> message,
@@ -58,17 +92,19 @@ static std::string GetErrorSource(Isolate* isolate,
5892
std::string sourceline(*encoded_source, encoded_source.length());
5993
*added_exception_line = false;
6094

95+
if (sourceline.find("node-do-not-add-exception-line") != std::string::npos) {
96+
return sourceline;
97+
}
98+
6199
// If source maps have been enabled, the exception line will instead be
62100
// added in the JavaScript context:
63101
Environment* env = Environment::GetCurrent(isolate);
64102
const bool has_source_map_url =
65103
!message->GetScriptOrigin().SourceMapUrl().IsEmpty();
66104
if (has_source_map_url && env != nullptr && env->source_maps_enabled()) {
67-
return sourceline;
68-
}
69-
70-
if (sourceline.find("node-do-not-add-exception-line") != std::string::npos) {
71-
return sourceline;
105+
std::string source = GetSourceMapErrorSource(
106+
isolate, context, message, added_exception_line);
107+
return added_exception_line ? source : sourceline;
72108
}
73109

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

907+
static void SetGetSourceMapErrorSource(
908+
const FunctionCallbackInfo<Value>& args) {
909+
Environment* env = Environment::GetCurrent(args);
910+
CHECK(args[0]->IsFunction());
911+
env->set_get_source_map_error_source(args[0].As<Function>());
912+
}
913+
871914
static void SetMaybeCacheGeneratedSourceMap(
872915
const FunctionCallbackInfo<Value>& args) {
873916
Environment* env = Environment::GetCurrent(args);
@@ -908,6 +951,7 @@ static void TriggerUncaughtException(const FunctionCallbackInfo<Value>& args) {
908951

909952
void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
910953
registry->Register(SetPrepareStackTraceCallback);
954+
registry->Register(SetGetSourceMapErrorSource);
911955
registry->Register(SetSourceMapsEnabled);
912956
registry->Register(SetMaybeCacheGeneratedSourceMap);
913957
registry->Register(SetEnhanceStackForFatalException);
@@ -922,6 +966,8 @@ void Initialize(Local<Object> target,
922966
Environment* env = Environment::GetCurrent(context);
923967
env->SetMethod(
924968
target, "setPrepareStackTraceCallback", SetPrepareStackTraceCallback);
969+
env->SetMethod(
970+
target, "setGetSourceMapErrorSource", SetGetSourceMapErrorSource);
925971
env->SetMethod(target, "setSourceMapsEnabled", SetSourceMapsEnabled);
926972
env->SetMethod(target,
927973
"setMaybeCacheGeneratedSourceMap",

‎test/message/source_map_disabled_by_api.js

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,4 +17,8 @@ delete require.cache[require
1717
// Re-enable.
1818
process.setSourceMapsEnabled(true);
1919

20-
require('../fixtures/source-map/enclosing-call-site-min.js');
20+
try {
21+
require('../fixtures/source-map/enclosing-call-site-min.js');
22+
} catch (e) {
23+
console.log(e);
24+
}

0 commit comments

Comments
 (0)