diff --git a/lib/internal/debugger/inspect_helpers.js b/lib/internal/debugger/inspect_helpers.js index f83876e96bc0..68d7901b38c5 100644 --- a/lib/internal/debugger/inspect_helpers.js +++ b/lib/internal/debugger/inspect_helpers.js @@ -1,11 +1,20 @@ 'use strict'; const { + ArrayPrototypePop, + ArrayPrototypePush, ArrayPrototypePushApply, + MapPrototypeGet, Number, Promise, + PromiseWithResolvers, RegExpPrototypeExec, + RegExpPrototypeSymbolReplace, + SafePromiseRace, StringPrototypeEndsWith, + StringPrototypeIndexOf, + StringPrototypeSlice, + StringPrototypeStartsWith, } = primordials; const { spawn } = require('child_process'); @@ -18,12 +27,24 @@ const { AbortController, } = require('internal/abort_controller'); -const { ERR_DEBUGGER_STARTUP_ERROR } = require('internal/errors').codes; +const { + ERR_DEBUGGER_ERROR, + ERR_DEBUGGER_STARTUP_ERROR, +} = require('internal/errors').codes; const { exitCodes: { kInvalidCommandLineArgument, }, } = internalBinding('errors'); +const { + types: { + kBoolean, + kNoOp, + kV8Option, + }, +} = internalBinding('options'); + +const { getCLIOptionsInfo } = require('internal/options'); const debugRegex = /Debugger listening on ws:\/\/\[?(.+?)\]?:(\d+)\//; @@ -61,6 +82,176 @@ function ensureTrailingNewline(text) { return StringPrototypeEndsWith(text, '\n') ? text : `${text}\n`; } +// Mirror OptionsParser::Parse() far enough to find the child script. Options +// before it must not undo the inspector setup added by launchChildProcess(). +function validateChildArgs(childArgs) { + const { options, aliases } = getCLIOptionsInfo(); + const syntheticArgs = []; + let breakFirstLine = true; + let childArgIndex = 0; + let inspectorEnabled = true; + + function peekArg() { + return syntheticArgs.length > 0 ? + syntheticArgs[syntheticArgs.length - 1] : + childArgs[childArgIndex]; + } + + function shiftArg() { + return syntheticArgs.length > 0 ? + ArrayPrototypePop(syntheticArgs) : + childArgs[childArgIndex++]; + } + + while (true) { + const nextArg = peekArg(); + if (nextArg === undefined || nextArg.length <= 1 || nextArg[0] !== '-') { + break; + } + + const isSynthetic = syntheticArgs.length > 0; + const arg = shiftArg(); + if (arg === '--') { break; } + if (!isSynthetic && + (arg === '--experimental-config-file' || + arg === '--experimental-default-config-file')) { + // ConfigReader rewrites these to an inline default path before parsing. + continue; + } + if (!isSynthetic && + StringPrototypeStartsWith( + arg, '--experimental-default-config-file=')) { + // ConfigReader rejects this form before parsing the remaining options. + return; + } + + const equalsIndex = arg[1] === '-' ? StringPrototypeIndexOf(arg, '=') : -1; + let name = equalsIndex === -1 ? arg : StringPrototypeSlice(arg, 0, equalsIndex); + if (name.length > 2) { + name = `${StringPrototypeSlice(name, 0, 2)}${ + RegExpPrototypeSymbolReplace(/_/g, StringPrototypeSlice(name, 2), '-')}`; + } + + let isNegation = false; + if (StringPrototypeStartsWith(name, '--no-')) { + name = `--${StringPrototypeSlice(name, 5)}`; + isNegation = true; + } + + while (true) { + let expansion = MapPrototypeGet(aliases, name); + if (expansion === undefined && equalsIndex !== -1) { + expansion = MapPrototypeGet(aliases, `${name}=`); + } + const aliasArg = peekArg(); + if (expansion === undefined && + aliasArg !== undefined && + aliasArg.length > 0 && + aliasArg[0] !== '-') { + expansion = MapPrototypeGet(aliases, `${name} `); + } + if (expansion === undefined) { break; } + + const previousName = name; + // process.allowedNodeEnvironmentFlags may remove a self-recursive + // first entry from the cached alias metadata. Preserve the native + // parser's synthetic option terminator in that case. + if (expansion[0] === '--') { + for (let i = expansion.length - 1; i >= 0; i--) { + ArrayPrototypePush(syntheticArgs, expansion[i]); + } + break; + } + name = expansion[0]; + for (let i = expansion.length - 1; i > 0; i--) { + ArrayPrototypePush(syntheticArgs, expansion[i]); + } + if (name === previousName) { break; } + } + + const info = MapPrototypeGet(options, name); + if (info === undefined) { continue; } + if (isNegation && info.type !== kBoolean && info.type !== kV8Option) { + return; + } + if (info.type === kBoolean || info.type === kNoOp || info.type === kV8Option) { + if (name === '--inspect') { + inspectorEnabled = !isNegation; + } else if (name === '--inspect-brk') { + breakFirstLine = !isNegation; + if (!isNegation) { inspectorEnabled = true; } + } else if (!isNegation && + (name === '--inspect-wait' || + name === '--inspect-brk-node')) { + inspectorEnabled = true; + } + continue; + } + + if (equalsIndex !== -1) { + if (equalsIndex === arg.length - 1) { return; } + continue; + } + + const value = peekArg(); + if (value === undefined || (value.length > 0 && value[0] === '-')) { + return; + } + shiftArg(); + } + + if (!inspectorEnabled) { + throw new ERR_DEBUGGER_STARTUP_ERROR( + '--no-inspect is incompatible with node inspect before the child script'); + } + if (!breakFirstLine) { + throw new ERR_DEBUGGER_STARTUP_ERROR( + '--no-inspect-brk is incompatible with node inspect before the child script'); + } +} + +async function waitForDebugger( + client, + callMethod = (method) => client.callMethod(method), +) { + const { + promise: waitingPromise, + resolve: resolveWaiting, + } = PromiseWithResolvers(); + const { + promise: closedPromise, + reject: rejectClosed, + } = PromiseWithResolvers(); + const onWaiting = () => resolveWaiting(); + const onClose = () => { + rejectClosed(new ERR_DEBUGGER_ERROR( + 'Debugger session ended while waiting for target startup')); + }; + + // The inspector can accept a connection before the target reaches its + // startup wait. Enabling NodeRuntime makes that state observable whether + // the target was already waiting or starts waiting later. + client.once('NodeRuntime.waitingForDebugger', onWaiting); + client.once('close', onClose); + try { + await SafePromiseRace([ + callMethod('NodeRuntime.enable'), + closedPromise, + ]); + await SafePromiseRace([ + waitingPromise, + closedPromise, + ]); + await SafePromiseRace([ + callMethod('NodeRuntime.disable'), + closedPromise, + ]); + } finally { + client.removeListener('NodeRuntime.waitingForDebugger', onWaiting); + client.removeListener('close', onClose); + } +} + function writeInspectUsageAndExit(invokedAs, message, exitCode) { const code = exitCode ?? (message ? kInvalidCommandLineArgument : 0); const out = code === 0 ? process.stdout : process.stderr; @@ -141,6 +332,8 @@ probe output schema. async function launchChildProcess(childArgs, inspectHost, inspectPort, childOutput, options = { __proto__: null }) { + validateChildArgs(childArgs); + if (!options.skipPortPreflight) { await portIsFree(inspectHost, inspectPort); } @@ -189,5 +382,6 @@ async function launchChildProcess(childArgs, inspectHost, inspectPort, module.exports = { ensureTrailingNewline, launchChildProcess, + waitForDebugger, writeInspectUsageAndExit, }; diff --git a/lib/internal/debugger/inspect_probe.js b/lib/internal/debugger/inspect_probe.js index b6cac6dc5779..3b77a46f1ba1 100644 --- a/lib/internal/debugger/inspect_probe.js +++ b/lib/internal/debugger/inspect_probe.js @@ -33,6 +33,7 @@ const InspectClient = require('internal/debugger/inspect_client'); const { ensureTrailingNewline, launchChildProcess, + waitForDebugger, } = require('internal/debugger/inspect_helpers'); const { ERR_DEBUGGER_STARTUP_ERROR } = require('internal/errors').codes; @@ -1044,6 +1045,17 @@ class ProbeInspectorSession { this.connected = true; try { + try { + await waitForDebugger( + this.client, + (method) => this.callCdp(method), + ); + } catch (err) { + // A close event may have completed the structured report while the + // readiness helper was rejecting its disconnect race. + if (this.finished) { throw kInspectorFailedSentinel; } + throw err; + } await this.callCdp('Runtime.enable'); await this.callCdp('Debugger.enable'); await this.bindBreakpoints(); diff --git a/lib/internal/debugger/inspect_repl.js b/lib/internal/debugger/inspect_repl.js index 548df089fb14..69ca174dd241 100644 --- a/lib/internal/debugger/inspect_repl.js +++ b/lib/internal/debugger/inspect_repl.js @@ -60,6 +60,7 @@ const { fileURLToPath } = require('internal/url'); const { customInspectSymbol, SideEffectFreeRegExpPrototypeSymbolReplace } = require('internal/util'); const { inspect: utilInspect } = require('internal/util/inspect'); const { isObjectLiteral } = require('internal/repl/utils'); +const { waitForDebugger } = require('internal/debugger/inspect_helpers'); const debuglog = require('internal/util/debuglog').debuglog('inspect'); const SHORTCUTS = { @@ -1204,9 +1205,13 @@ function createRepl(inspector) { } async function initAfterStart() { + const waitForDebuggerOnStart = !!inspector.options?.script; waitForInitialBreakRender = - !!inspector.options?.script && + waitForDebuggerOnStart && process.env.NODE_INSPECT_RESUME_ON_START !== '1'; + if (waitForDebuggerOnStart) { + await waitForDebugger(inspector.client); + } await Runtime.enable(); await Profiler.enable(); await Profiler.setSamplingInterval({ interval: 100 }); diff --git a/test/parallel/test-debugger-no-inspect-brk.js b/test/parallel/test-debugger-no-inspect-brk.js new file mode 100644 index 000000000000..611d28fd9f3e --- /dev/null +++ b/test/parallel/test-debugger-no-inspect-brk.js @@ -0,0 +1,158 @@ +// Flags: --expose-internals + +// This tests that child --no-inspect and --no-inspect-brk options cannot leave +// the inspector setup disabled, while remaining valid as application args. +'use strict'; + +const common = require('../common'); +common.skipIfInspectorDisabled(); + +const assert = require('assert'); +const fixtures = require('../common/fixtures'); +const { + spawnSyncAndAssert, + spawnSyncAndExit, +} = require('../common/child_process'); +const { assertProbeJson } = require('../common/debugger-probe'); +const { launchChildProcess } = require('internal/debugger/inspect_helpers'); + +const cwd = fixtures.path('debugger'); +const probeUrl = fixtures.fileURL('debugger', 'probe.js').href; +const probeArgs = [ + '--probe', 'probe.js:12', + '--expr', 'finalValue', +]; +const incompatibleInspectBrk = + /--no-inspect-brk is incompatible with node inspect before the child script/; +const incompatibleInspect = + /--no-inspect is incompatible with node inspect before the child script/; + +function assertSuccessfulProbe(childArgs) { + spawnSyncAndAssert(process.execPath, [ + 'inspect', + '--json', + ...probeArgs, + '--', + ...childArgs, + ], { cwd }, { + stdout(output) { + assertProbeJson(output, { + v: 2, + probes: [{ + expr: 'finalValue', + target: { suffix: 'probe.js', line: 12 }, + }], + results: [{ + probe: 0, + event: 'hit', + hit: 1, + location: { url: probeUrl, line: 12, column: 1 }, + result: { type: 'number', value: 81, description: '81' }, + }, { + event: 'completed', + }], + }); + }, + trim: true, + }); +} + +for (const childOptions of [ + ['--require', 'assert', '--no-inspect-brk'], + ['--require=assert', '--no-inspect-brk'], + ['-r', 'assert', '--no_inspect_brk'], +]) { + spawnSyncAndExit(process.execPath, [ + 'inspect', + ...probeArgs, + '--', + ...childOptions, + 'probe.js', + ], { cwd }, { + signal: null, + status: 1, + stderr: incompatibleInspectBrk, + trim: true, + }); +} + +spawnSyncAndExit(process.execPath, [ + 'inspect', + ...probeArgs, + '--', + '--require', 'assert', + '--no-inspect', + 'probe.js', +], { cwd }, { + signal: null, + status: 1, + stderr: incompatibleInspect, + trim: true, +}); + +for (const { option, error } of [ + { option: '--no-inspect-brk', error: incompatibleInspectBrk }, + { option: '--no-inspect', error: incompatibleInspect }, +]) { + spawnSyncAndExit(process.execPath, [ + 'inspect', + option, + 'probe.js', + ], { cwd }, { + signal: null, + status: 1, + stderr: error, + trim: true, + }); + + assertSuccessfulProbe(['probe.js', option]); +} + +// Node options are last-write-wins. A later --inspect-brk restores both +// startup requirements. +assertSuccessfulProbe([ + '--no-inspect', + '--no-inspect-brk', + '--inspect-brk', + 'probe.js', +]); + +// ConfigReader rewrites these bare options to use the default path without +// consuming the following argument. +Promise.all([ + assert.rejects( + launchChildProcess([ + '--experimental-config-file', + '--no-inspect-brk', + 'probe.js', + ], '127.0.0.1', 0, () => {}), + incompatibleInspectBrk, + ), + assert.rejects( + launchChildProcess([ + '--experimental-default-config-file', + '--no-inspect-brk', + 'probe.js', + ], '127.0.0.1', 0, () => {}), + incompatibleInspectBrk, + ), + // These options imply --inspect, but do not restore --inspect-brk. + assert.rejects( + launchChildProcess([ + '--no-inspect', + '--inspect-wait', + '--no-inspect-brk', + 'probe.js', + ], '127.0.0.1', 0, () => {}), + incompatibleInspectBrk, + ), + assert.rejects( + launchChildProcess([ + '--no-inspect', + '--inspect-brk-node', + '--no-inspect-brk', + 'probe.js', + ], '127.0.0.1', 0, () => {}), + incompatibleInspectBrk, + ), +]).then(common.mustCall()); diff --git a/test/parallel/test-debugger-probe-startup-disconnect.js b/test/parallel/test-debugger-probe-startup-disconnect.js new file mode 100644 index 000000000000..68c8432dc552 --- /dev/null +++ b/test/parallel/test-debugger-probe-startup-disconnect.js @@ -0,0 +1,56 @@ +// Flags: --expose-internals +// This tests that a disconnect while probe mode is waiting for target startup +// is reported as a structured probe failure instead of an internal error. +'use strict'; + +const common = require('../common'); +common.skipIfInspectorDisabled(); + +const assert = require('assert'); +const { EventEmitter } = require('events'); +const { assertProbeJson } = require('../common/debugger-probe'); +const { ProbeInspectorSession } = require('internal/debugger/inspect_probe'); + +const probe = { + expr: 'value', + target: { suffix: 'probe-target.js', line: 1 }, +}; +const client = new EventEmitter(); +client.connect = common.mustCall(); +client.callMethod = common.mustCall((method) => { + assert.strictEqual(method, 'NodeRuntime.enable'); + setImmediate(() => client.emit('close')); + return new Promise(() => {}); +}); +client.reset = common.mustCall(); + +const session = new ProbeInspectorSession({ + childArgv: ['-e', ''], + host: '127.0.0.1', + port: 0, + probes: [probe], + skipPortPreflight: true, +}); +session.client = client; + +session.run().then(common.mustCall(({ code, report }) => { + assert.strictEqual(code, 1); + assertProbeJson(report, { + v: 2, + probes: [probe], + results: [{ + event: 'error', + pending: [0], + error: { + code: 'probe_failure', + message: + 'Inspector connection lost before probes started before probes: ' + + 'probe-target.js:1. The target startup may have torn down the ' + + 'inspector. If startup does not touch the inspector, this is likely ' + + 'a Node.js bug. Please file an issue.', + stderr: '', + details: { lastCdpMethod: 'NodeRuntime.enable' }, + }, + }], + }); +})); diff --git a/test/parallel/test-debugger-run-restart-init.js b/test/parallel/test-debugger-run-restart-init.js index 78f237353baf..b57939135f80 100644 --- a/test/parallel/test-debugger-run-restart-init.js +++ b/test/parallel/test-debugger-run-restart-init.js @@ -79,9 +79,29 @@ async function assertCommandWaitsForInit(repl, command, gate, calls) { const runGate = createGate(); const restartGate = createGate(); const gates = [null, runGate, restartGate]; + const client = new EventEmitter(); + let nodeRuntimeEnableCount = 0; + client.callMethod = common.mustCall(async (method) => { + calls.push(method); + if (method === 'NodeRuntime.enable') { + const emitWaiting = () => { + calls.push('NodeRuntime.waitingForDebugger'); + client.emit('NodeRuntime.waitingForDebugger'); + }; + // Cover notifications arriving both before and after the enable reply. + if (nodeRuntimeEnableCount++ % 2 === 0) { + emitWaiting(); + } else { + setImmediate(emitWaiting); + } + } else { + assert.strictEqual(method, 'NodeRuntime.disable'); + } + }, 6); const inspector = { - client: new EventEmitter(), + client, domainNames: ['Debugger', 'HeapProfiler', 'Profiler', 'Runtime'], + options: { script: 'debugger-target.js' }, stdin: new PassThrough(), stdout: new PassThrough(), run: common.mustCall(async () => { @@ -101,6 +121,29 @@ async function assertCommandWaitsForInit(repl, command, gate, calls) { await assertCommandWaitsForInit(repl, 'run', runGate, calls); await assertCommandWaitsForInit(repl, 'restart', restartGate, calls); + assert.deepStrictEqual( + calls.filter((call) => ( + call === 'NodeRuntime.enable' || + call === 'NodeRuntime.waitingForDebugger' || + call === 'NodeRuntime.disable' || + call === 'Runtime.runIfWaitingForDebugger' + )), + [ + 'NodeRuntime.enable', + 'NodeRuntime.waitingForDebugger', + 'NodeRuntime.disable', + 'Runtime.runIfWaitingForDebugger', + 'NodeRuntime.enable', + 'NodeRuntime.waitingForDebugger', + 'NodeRuntime.disable', + 'Runtime.runIfWaitingForDebugger', + 'NodeRuntime.enable', + 'NodeRuntime.waitingForDebugger', + 'NodeRuntime.disable', + 'Runtime.runIfWaitingForDebugger', + ], + ); + assert.deepStrictEqual( calls.filter((call) => ( call === 'inspector.run' || @@ -116,4 +159,25 @@ async function assertCommandWaitsForInit(repl, command, gate, calls) { ); repl.close(); + + const attachCalls = []; + const attachClient = new EventEmitter(); + attachClient.callMethod = common.mustNotCall(); + const attachInspector = { + client: attachClient, + domainNames: ['Debugger', 'HeapProfiler', 'Profiler', 'Runtime'], + options: {}, + stdin: new PassThrough(), + stdout: new PassThrough(), + suspendReplWhile(fn) { + return fn(); + }, + }; + + for (const domain of attachInspector.domainNames) { + attachInspector[domain] = createAgent(domain, attachCalls, []); + } + + const attachRepl = await createRepl(attachInspector)(); + attachRepl.close(); })().then(common.mustCall()); diff --git a/test/parallel/test-debugger-wait-for-debugger.js b/test/parallel/test-debugger-wait-for-debugger.js new file mode 100644 index 000000000000..b438147ea832 --- /dev/null +++ b/test/parallel/test-debugger-wait-for-debugger.js @@ -0,0 +1,137 @@ +// Flags: --expose-internals +'use strict'; + +const common = require('../common'); + +common.skipIfInspectorDisabled(); + +const assert = require('assert'); +const { EventEmitter } = require('events'); +const { + waitForDebugger, +} = require('internal/debugger/inspect_helpers'); + +function assertListenersRemoved(client) { + assert.strictEqual( + client.listenerCount('NodeRuntime.waitingForDebugger'), + 0, + ); + assert.strictEqual(client.listenerCount('close'), 0); +} + +async function testWaitingNotification(beforeEnableReply) { + const client = new EventEmitter(); + const calls = []; + client.callMethod = common.mustCall(async (method) => { + calls.push(method); + const emitWaiting = () => { + client.emit('NodeRuntime.waitingForDebugger'); + }; + if (method === 'NodeRuntime.enable') { + if (beforeEnableReply) { + emitWaiting(); + } else { + setImmediate(emitWaiting); + } + } else { + assert.strictEqual(method, 'NodeRuntime.disable'); + } + }, 2); + + await waitForDebugger(client); + assert.deepStrictEqual(calls, [ + 'NodeRuntime.enable', + 'NodeRuntime.disable', + ]); + assertListenersRemoved(client); +} + +async function testCloseWhileWaiting(beforeEnableReply) { + const client = new EventEmitter(); + client.callMethod = common.mustCall((method) => { + assert.strictEqual(method, 'NodeRuntime.enable'); + setImmediate(() => client.emit('close')); + return beforeEnableReply ? new Promise(() => {}) : Promise.resolve(); + }); + + await assert.rejects( + waitForDebugger(client), + { + code: 'ERR_DEBUGGER_ERROR', + message: 'Debugger session ended while waiting for target startup', + }, + ); + assertListenersRemoved(client); +} + +async function testCloseWhileDisabling() { + const client = new EventEmitter(); + client.callMethod = common.mustCall((method) => { + if (method === 'NodeRuntime.enable') { + client.emit('NodeRuntime.waitingForDebugger'); + return Promise.resolve(); + } + assert.strictEqual(method, 'NodeRuntime.disable'); + setImmediate(() => client.emit('close')); + return new Promise(() => {}); + }, 2); + + await assert.rejects( + waitForDebugger(client), + { + code: 'ERR_DEBUGGER_ERROR', + message: 'Debugger session ended while waiting for target startup', + }, + ); + assertListenersRemoved(client); +} + +async function testEnableFailure() { + const client = new EventEmitter(); + const expected = new Error('NodeRuntime.enable failed'); + client.callMethod = common.mustCall(async (method) => { + assert.strictEqual(method, 'NodeRuntime.enable'); + throw expected; + }); + + await assert.rejects( + waitForDebugger(client), + (error) => { + assert.strictEqual(error, expected); + return true; + }, + ); + assertListenersRemoved(client); +} + +async function testDisableFailure() { + const client = new EventEmitter(); + const expected = new Error('NodeRuntime.disable failed'); + client.callMethod = common.mustCall(async (method) => { + if (method === 'NodeRuntime.enable') { + client.emit('NodeRuntime.waitingForDebugger'); + return; + } + assert.strictEqual(method, 'NodeRuntime.disable'); + throw expected; + }, 2); + + await assert.rejects( + waitForDebugger(client), + (error) => { + assert.strictEqual(error, expected); + return true; + }, + ); + assertListenersRemoved(client); +} + +(async () => { + await testWaitingNotification(true); + await testWaitingNotification(false); + await testCloseWhileWaiting(true); + await testCloseWhileWaiting(false); + await testCloseWhileDisabling(); + await testEnableFailure(); + await testDisableFailure(); +})().then(common.mustCall());