lib: use debugger statement to break scripts in -e --inspect-brk

[joyeecheung]

When wrapping scripts being evaluated with -e and --inspect-brk,
instead of wrapping it with an internal binding:

process.binding('inspector').callAndPauseOnStart(function() {

${source};

}, {})

simply insert a debugger statement:

debugger;
${source}

so that the wrapped script do not rely on any internal magic
since we should treat it as a normal user land script. In addition,
if the source starts with declarations, it now breaks before the
declarations instead of skipping through the declarations and jumping
to the first statement.

This patch also introduces helpers for the inspector tests to calculate
debug break line numbers programmatically, instead of hard-coding the
numbers, since these numbers can change when the way we wrap different
kinds of scripts change.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[nodejs-github-bot]

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1887/pipeline

[devsnek]

just to be sure, there isn't some API we can use to do this without modifying the source?

[joyeecheung]

just to be sure, there isn't some API we can use to do this without modifying the source?

Maybe, but I suspect it would be harder to implement because -e goes through all these

node/lib/internal/bootstrap/node.js

Lines 666 to 687 in d695a01

	function evalScript(name, body) {
	const CJSModule = NativeModule.require('internal/modules/cjs/loader');
	const path = NativeModule.require('path');
	const { tryGetCwd } = NativeModule.require('internal/util');
	const cwd = tryGetCwd(path);
	const module = new CJSModule(name);
	module.filename = path.join(cwd, name);
	module.paths = CJSModule._nodeModulePaths(cwd);
	const script = `global.__filename = ${JSON.stringify(name)};\n` +
	'global.exports = exports;\n' +
	'global.module = module;\n' +
	'global.__dirname = __dirname;\n' +
	'global.require = require;\n' +
	'return require("vm").runInThisContext(' +
	`${JSON.stringify(body)}, { filename: ` +
	`${JSON.stringify(name)}, displayErrors: true });\n`;
	const result = module._compile(script, `${name}-wrapper`);
	if (process._print_eval) console.log(result);
	// Handle any nextTicks added in the first tick of the program.
	process._tickCallback();
	}

And module._compile is even another beast that is exposed to the user land. So to specialize the compilation and execution of -e without breaking anything would be trickier than just changing the way the scripts are wrapped.

[joyeecheung]

I think another alternative is to pass the result of a vm.compileFunction call to process.binding('inspector').callAndPauseOnStart (this time this binding is called directly and it won't show up in the wrapped source), the tricky part would be to make sure we don't break the module and exports being passed into the binding without going semver-major.

[eugeneo]

This had been discussed back when this was originally implemented - and I was informed that the wrapper is considered a part of Node.JS API and that some tools may be relying on a fixed length of the wrapper prefix.

I do prefer solution with the debugger statement, but my understanding is that this may need to be cleared with the TSC or someone like that.

Do ES6 modules get wrapped as well?

[joyeecheung]

This had been discussed back when this was originally implemented - and I was informed that the wrapper is considered a part of Node.JS API and that some tools may be relying on a fixed length of the wrapper prefix.

@eugeneo I think that would be more critical for the CJS module evaluation, specifically, this part in the code base:

node/lib/internal/modules/cjs/loader.js

Lines 696 to 719 in 9fb4fa8

	if (process._breakFirstLine && process._eval == null) {
	if (!resolvedArgv) {
	// we enter the repl if we're not given a filename argument.
	if (process.argv[1]) {
	resolvedArgv = Module._resolveFilename(process.argv[1], null, false);
	} else {
	resolvedArgv = 'repl';
	}
	}
	// Set breakpoint on module start
	if (filename === resolvedArgv) {
	delete process._breakFirstLine;
	inspectorWrapper = process.binding('inspector').callAndPauseOnStart;
	}
	}
	var dirname = path.dirname(filename);
	var require = makeRequireFunction(this);
	var depth = requireDepth;
	if (depth === 0) stat.cache = new Map();
	var result;
	if (inspectorWrapper) {
	result = inspectorWrapper(compiledWrapper, this.exports, this.exports,
	require, this, filename, dirname);

(BTW, #21573 is trying to change that part)

This PR, however, is changing the wrapping of node -e <source_text> instead of the normal execution of node <source_file>. The script being evaluated by -e is run differently, see

node/lib/internal/bootstrap/node.js

Lines 666 to 687 in d695a01

	function evalScript(name, body) {
	const CJSModule = NativeModule.require('internal/modules/cjs/loader');
	const path = NativeModule.require('path');
	const { tryGetCwd } = NativeModule.require('internal/util');
	const cwd = tryGetCwd(path);
	const module = new CJSModule(name);
	module.filename = path.join(cwd, name);
	module.paths = CJSModule._nodeModulePaths(cwd);
	const script = `global.__filename = ${JSON.stringify(name)};\n` +
	'global.exports = exports;\n' +
	'global.module = module;\n' +
	'global.__dirname = __dirname;\n' +
	'global.require = require;\n' +
	'return require("vm").runInThisContext(' +
	`${JSON.stringify(body)}, { filename: ` +
	`${JSON.stringify(name)}, displayErrors: true });\n`;
	const result = module._compile(script, `${name}-wrapper`);
	if (process._print_eval) console.log(result);
	// Handle any nextTicks added in the first tick of the program.
	process._tickCallback();
	}

(to summarize: module, exports ., etc are passed through global exposed to the vm through vm.runInThisContext instead of being passed through a wrapper)

[devsnek]

@eugeneo for esm:

node/lib/internal/modules/esm/module_job.js

Lines 74 to 80 in 9fb4fa8

	if (this.isMain && process._breakFirstLine) {
	delete process._breakFirstLine;
	const initWrapper = process.binding('inspector').callAndPauseOnStart;
	initWrapper(this.module.instantiate, this.module);
	} else {
	this.module.instantiate();
	}

[eugeneo]

I assumed this was for all the codepaths.

I understand the reasons explained is the first comment, but I believe all code paths should use same call so it is easier to maintain.

[joyeecheung]

I understand the reasons explained is the first comment, but I believe all code paths should use same call so it is easier to maintain.

It is already quite different, though, because this is what it conceptually looks like when evaluating a CJS module with --inspect-brk

const source = fs.readFileSync(filename);  // and strip the BOM, etc.
const result = vm.runInThisContext(
`(function (exports, require, module, __filename, __dirname) {${source}})`, {
...
});
process.binding('inspector').callAndPauseOnStart(result, exports, require, module, filename, dirname);

And this is what happens when evaluating a script with -e --inspect-brk:

const source = ... // read from CLI
// modifies module, filename, ...
const wrappedSource = `
global.__filename = ${JSON.stringify(name)};
global.exports = exports;
global.module = module;
global.__dirname = __dirname;
global.require = require;
return require("vm").runInThisContext('
${JSON.stringify(`
process.binding('inspector').callAndPauseOnStart(function() {

${source};

}, {})`)}, { filename: ${JSON.stringify(name)}, displayErrors: true });
`;

const result = vm.runInThisContext(
`(function (exports, require, module, __filename, __dirname) {${wrappedSource}})`, {
...
});
result.call(exports, require, module, '[eval]-wrapper', dirname);

In particular we are not calling callAndPauseOnStart the same way for -e as the way we call it for normal CJS modules - the -e call does not get any arguments at all because those are passed through globals. Also, the process.binding('inspector').callAndPauseOnStart(function() {...}) would show up in the inspector UI when we double-wrap the source this way.

And as #24931 demonstrates, calling callAndPauseOnStart in a wrapper would prevent us from deprecating process.binding('inspector'), because the wrapped script has the same level of access as a user land script - unless we make internalBinding available to the wrapper, but then we will also leak internalBinding to the user script

[joyeecheung]

Superseded by #25832