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

lib: fix inspector links for stack traces #35725

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 8 additions & 1 deletion lib/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ const { isPromise, isRegExp } = require('internal/util/types');
const { EOL } = require('internal/constants');
const { NativeModule } = require('internal/bootstrap/loaders');
const { isError } = require('internal/util');
const { fileURLToPath } = require('internal/url');

const errorCache = new SafeMap();
const CallTracker = require('internal/assert/calltracker');
Expand Down Expand Up @@ -291,12 +292,18 @@ function getErrMessage(message, fn) {
overrideStackTrace.set(err, (_, stack) => stack);
const call = err.stack[0];

const filename = call.getFileName();
let filename = call.getFileName();
const line = call.getLineNumber() - 1;
let column = call.getColumnNumber() - 1;
let identifier;
let code;

if (filename && StringPrototypeStartsWith(filename, 'file:')) {
try {
filename = fileURLToPath(filename);
Copy link
Contributor

Choose a reason for hiding this comment

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

With the check for file:, under what conditions do we expect this to fail?

If we leave this try/catch, we might want to put a debug in the catch.

Copy link
Author

Choose a reason for hiding this comment

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

fileURLToPath and normalizeReferrerURL call URL constructor with a single argument. There was a string (I do not remember exactly, but I can try to remove the try/catch and rebuild to find it) like [eval something] that makes URL throws an ERR_INVALID_URL error. Since that is in the unit tests, it might be used in the future. Moreover, users of vm can pass any string for filename or have source map containing any string. The combination of both is possible to make otherwise invalid URL to appear as valid module and in general invalid URLs supplied by the user should not crush for the proper reason: MODULE_NOT_FOUND for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth getting a unit test around the edge case if possible 👌

} catch {}
}

if (filename) {
identifier = `${filename}${line}${column}`;

Expand Down
17 changes: 14 additions & 3 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,12 @@ const {
maybeCacheSourceMap,
rekeySourceMap
} = require('internal/source_map/source_map_cache');
const { pathToFileURL, fileURLToPath, isURLInstance } = require('internal/url');
const {
pathToFileURL,
fileURLToPath,
isURLInstance,
URL
} = require('internal/url');
const { deprecate } = require('internal/util');
const vm = require('vm');
const assert = require('internal/assert');
Expand Down Expand Up @@ -1005,12 +1010,18 @@ Module.prototype.require = function(id) {
// (needed for setting breakpoint when called with --inspect-brk)
let resolvedArgv;
let hasPausedEntry = false;
const absolutePathToUrlConverter = new URL('file://');

function wrapSafe(filename, content, cjsModuleInstance) {
let filenameUrl = filename;
if (path.isAbsolute(filename)) {
absolutePathToUrlConverter.pathname = filename;
filenameUrl = absolutePathToUrlConverter.href;
}
if (patched) {
const wrapper = Module.wrap(content);
return vm.runInThisContext(wrapper, {
filename,
filename: filenameUrl,
lineOffset: 0,
displayErrors: true,
importModuleDynamically: async (specifier) => {
Expand All @@ -1023,7 +1034,7 @@ function wrapSafe(filename, content, cjsModuleInstance) {
try {
compiled = compileFunction(
content,
filename,
filenameUrl,
0,
0,
undefined,
Expand Down
5 changes: 1 addition & 4 deletions lib/internal/source_map/prepare_stack_trace.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,7 @@ const prepareStackTrace = (globalThis, error, trace) => {
const prefix = (name && name !== t.getFunctionName()) ?
`\n -> at ${name}` :
'\n ->';
const originalSourceNoScheme =
StringPrototypeStartsWith(originalSource, 'file://') ?
fileURLToPath(originalSource) : originalSource;
str += `${prefix} (${originalSourceNoScheme}:${originalLine + 1}:` +
str += `${prefix} (${originalSource}:${originalLine + 1}:` +
`${originalColumn + 1})`;
}
}
Expand Down
12 changes: 9 additions & 3 deletions test/parallel/test-common-must-not-call.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const common = require('../common');
const assert = require('assert');
const path = require('path');
const util = require('util');
const { fileURLToPath } = require('url');

const message = 'message';
const testFunction1 = common.mustNotCall(message);
Expand All @@ -13,10 +14,15 @@ const testFunction2 = common.mustNotCall(message);
const createValidate = (line, args = []) => common.mustCall((e) => {
const prefix = `${message} at `;
assert.ok(e.message.startsWith(prefix));
e.message = e.message.substring(prefix.length);
if (e.message.startsWith('file:')) {
const url = /.*/.exec(e.message)[0];
e.message = fileURLToPath(url) + e.message.substring(url.length);
}
if (process.platform === 'win32') {
e.message = e.message.substring(2); // remove 'C:'
}
const msg = e.message.substring(prefix.length);
const msg = e.message;
const firstColon = msg.indexOf(':');
const fileName = msg.substring(0, firstColon);
const rest = msg.substring(firstColon + 1);
Expand All @@ -26,14 +32,14 @@ const createValidate = (line, args = []) => common.mustCall((e) => {
assert.strictEqual(rest, line + argsInfo);
});

const validate1 = createValidate('9');
const validate1 = createValidate('10');
try {
testFunction1();
} catch (e) {
validate1(e);
}

const validate2 = createValidate('11', ['hello', 42]);
const validate2 = createValidate('12', ['hello', 42]);
try {
testFunction2('hello', 42);
} catch (e) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const { pathToFileURL } = require('url');

(function foobar() {
require('domain');
Expand All @@ -20,7 +21,7 @@ assert.throws(
assert(err.stack.includes('-'.repeat(40)),
`expected ${err.stack} to contain dashes`);

const location = `at foobar (${__filename}:`;
const location = `at foobar (${pathToFileURL(__filename)}:`;
assert(err.stack.includes(location),
`expected ${err.stack} to contain ${location}`);
return true;
Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-http-timeout-client-warning.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@
const common = require('../common');
const http = require('http');
const assert = require('assert');
const { pathToFileURL } = require('url');

// Checks that the setTimeout duration overflow warning is emitted
// synchronously and therefore contains a meaningful stacktrace.

process.on('warning', common.mustCall((warning) => {
assert(warning.stack.includes(__filename));
assert(warning.stack.includes(pathToFileURL(__filename)));
}));

const server = http.createServer((req, resp) => resp.end());
Expand Down
4 changes: 3 additions & 1 deletion test/sequential/test-cli-syntax-bad.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
const common = require('../common');
const assert = require('assert');
const { exec } = require('child_process');
const { pathToFileURL } = require('url');
const fixtures = require('../common/fixtures');

const node = process.execPath;
Expand Down Expand Up @@ -42,7 +43,8 @@ const syntaxErrorRE = /^SyntaxError: \b/m;
assert(syntaxErrorRE.test(stderr), `${syntaxErrorRE} === ${stderr}`);

// stderr should include the filename
assert(stderr.startsWith(file), `${stderr} starts with ${file}`);
assert(stderr.startsWith(pathToFileURL(file)),
`${stderr} starts with ${pathToFileURL(file)}`);
}));
});
});
6 changes: 4 additions & 2 deletions test/sequential/test-cli-syntax-require.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const common = require('../common');
const assert = require('assert');
const { exec } = require('child_process');
const fixtures = require('../common/fixtures');
const { pathToFileURL } = require('url');

const node = process.execPath;

Expand All @@ -29,8 +30,9 @@ const syntaxErrorRE = /^SyntaxError: \b/m;
// stderr should have a syntax error message
assert(syntaxErrorRE.test(stderr), `${syntaxErrorRE} === ${stderr}`);

// stderr should include the filename
assert(stderr.startsWith(file), `${stderr} starts with ${file}`);
// stderr should include the file URL
const fileUrl = pathToFileURL(file);
assert(stderr.startsWith(fileUrl), `${stderr} starts with ${file}`);
}));
});
});