inspector: add NodeRuntime.waitingForDebugger event

`NodeRuntime.waitingForDebugger` is a new Inspector Protocol event that
will fire when the process being inspected is waiting for the debugger
(for example, when `inspector.waitForDebugger()` is called). This allows
inspecting processes to know when the inspected process is waiting for a
`Runtime.runIfWaitingForDebugger` message to resume execution. It allows
tooling to resume execution of the inspected process as soon as it deems
necessary, without having to guess if the inspected process is waiting
or not, making the workflow more deterministic. With a more
deterministic workflow, it is possible to update Node.js core tests to
avoid race conditions that can cause flakiness. Therefore, tests were
also changed as following:

  * Remove no-op Runtime.runIfWaitingForDebugger from tests that don't
    need it
  * Use NodeRuntime.waitingForDebugger in all tests that need
    Runtime.runIfWaitingForDebugger, to ensure order of operations is
    predictable and correct
  * Simplify test-inspector-multisession-ws

There might be value in adding `NodeWorker.waitingForDebugger` in a
future patch, but as of right now, no Node.js core inspector tests using
worker threads are not failing due to race conditions.

Fixes: https://github.com/nodejs/node/issues/34730
PR-URL: https://github.com/nodejs/node/pull/51560
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

src/inspector/node_protocol.pdl

@@ -100,6 +100,12 @@ experimental domain NodeWorker
 
 # Support for inspecting node process state.
 experimental domain NodeRuntime
+  # Enable the NodeRuntime events except by `NodeRuntime.waitingForDisconnect`.
+  command enable
+
+  # Disable NodeRuntime events
+  command disable
+
   # Enable the `NodeRuntime.waitingForDisconnect`.
   command notifyWhenWaitingForDisconnect
     parameters
@@ -110,3 +116,7 @@ experimental domain NodeRuntime
   # It is fired when the Node process finished all code execution and is
   # waiting for all frontends to disconnect.
   event waitingForDisconnect
+
+  # This event is fired when the runtime is waiting for the debugger. For
+  # example, when inspector.waitingForDebugger is called
+  event waitingForDebugger

src/inspector/runtime_agent.cc

@@ -8,7 +8,9 @@ namespace inspector {
 namespace protocol {
 
 RuntimeAgent::RuntimeAgent()
-  : notify_when_waiting_for_disconnect_(false) {}
+    : notify_when_waiting_for_disconnect_(false),
+      enabled_(false),
+      is_waiting_for_debugger_(false) {}
 
 void RuntimeAgent::Wire(UberDispatcher* dispatcher) {
   frontend_ = std::make_unique<NodeRuntime::Frontend>(dispatcher->channel());
@@ -20,6 +22,30 @@ DispatchResponse RuntimeAgent::notifyWhenWaitingForDisconnect(bool enabled) {
   return DispatchResponse::OK();
 }
 
+DispatchResponse RuntimeAgent::enable() {
+  enabled_ = true;
+  if (is_waiting_for_debugger_) {
+    frontend_->waitingForDebugger();
+  }
+  return DispatchResponse::OK();
+}
+
+DispatchResponse RuntimeAgent::disable() {
+  enabled_ = false;
+  return DispatchResponse::OK();
+}
+
+void RuntimeAgent::setWaitingForDebugger() {
+  is_waiting_for_debugger_ = true;
+  if (enabled_) {
+    frontend_->waitingForDebugger();
+  }
+}
+
+void RuntimeAgent::unsetWaitingForDebugger() {
+  is_waiting_for_debugger_ = false;
+}
+
 bool RuntimeAgent::notifyWaitingForDisconnect() {
   if (notify_when_waiting_for_disconnect_) {
     frontend_->waitingForDisconnect();

src/inspector/runtime_agent.h

@@ -18,11 +18,21 @@ class RuntimeAgent : public NodeRuntime::Backend {
 
   DispatchResponse notifyWhenWaitingForDisconnect(bool enabled) override;
 
+  DispatchResponse enable() override;
+
+  DispatchResponse disable() override;
+
   bool notifyWaitingForDisconnect();
 
+  void setWaitingForDebugger();
+
+  void unsetWaitingForDebugger();
+
  private:
   std::shared_ptr<NodeRuntime::Frontend> frontend_;
   bool notify_when_waiting_for_disconnect_;
+  bool enabled_;
+  bool is_waiting_for_debugger_;
 };
 }  // namespace protocol
 }  // namespace inspector

src/inspector_agent.cc

@@ -278,6 +278,10 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel,
     return retaining_context_;
   }
 
+  void setWaitingForDebugger() { runtime_agent_->setWaitingForDebugger(); }
+
+  void unsetWaitingForDebugger() { runtime_agent_->unsetWaitingForDebugger(); }
+
   bool retainingContext() {
     return retaining_context_;
   }
@@ -418,6 +422,9 @@ class NodeInspectorClient : public V8InspectorClient {
 
   void waitForFrontend() {
     waiting_for_frontend_ = true;
+    for (const auto& id_channel : channels_) {
+      id_channel.second->setWaitingForDebugger();
+    }
     runMessageLoop();
   }
 
@@ -465,6 +472,9 @@ class NodeInspectorClient : public V8InspectorClient {
 
   void runIfWaitingForDebugger(int context_group_id) override {
     waiting_for_frontend_ = false;
+    for (const auto& id_channel : channels_) {
+      id_channel.second->unsetWaitingForDebugger();
+    }
   }
 
   int connectFrontend(std::unique_ptr<InspectorSessionDelegate> delegate,
@@ -476,6 +486,9 @@ class NodeInspectorClient : public V8InspectorClient {
                                                           std::move(delegate),
                                                           getThreadHandle(),
                                                           prevent_shutdown);
+    if (waiting_for_frontend_) {
+      channels_[session_id]->setWaitingForDebugger();
+    }
     return session_id;
   }
 

test/common/inspector-helper.js

@@ -303,6 +303,9 @@ class InspectorSession {
     console.log('[test]', 'Verify node waits for the frontend to disconnect');
     await this.send({ 'method': 'Debugger.resume' });
     await this.waitForNotification((notification) => {
+      if (notification.method === 'Debugger.paused') {
+        this.send({ 'method': 'Debugger.resume' });
+      }
       return notification.method === 'Runtime.executionContextDestroyed' &&
         notification.params.executionContextId === 1;
     });

test/parallel/test-inspect-async-hook-setup-at-inspect.js

@@ -54,7 +54,6 @@ async function runTests() {
       'params': { 'maxDepth': 10 } },
     { 'method': 'Debugger.setBlackboxPatterns',
       'params': { 'patterns': [] } },
-    { 'method': 'Runtime.runIfWaitingForDebugger' },
   ]);
 
   await waitForInitialSetup(session);

test/parallel/test-inspector-async-hook-setup-at-inspect-brk.js

@@ -30,6 +30,8 @@ async function checkAsyncStackTrace(session) {
 async function runTests() {
   const instance = new NodeInstance(undefined, script);
   const session = await instance.connectInspectorSession();
+  await session.send({ method: 'NodeRuntime.enable' });
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
   await session.send([
     { 'method': 'Runtime.enable' },
     { 'method': 'Debugger.enable' },
@@ -39,6 +41,7 @@ async function runTests() {
       'params': { 'patterns': [] } },
     { 'method': 'Runtime.runIfWaitingForDebugger' },
   ]);
+  await session.send({ method: 'NodeRuntime.disable' });
   await skipBreakpointAtStart(session);
   await checkAsyncStackTrace(session);
 

test/parallel/test-inspector-async-hook-setup-at-signal.js

@@ -69,7 +69,6 @@ async function runTests() {
       'params': { 'maxDepth': 10 } },
     { 'method': 'Debugger.setBlackboxPatterns',
       'params': { 'patterns': [] } },
-    { 'method': 'Runtime.runIfWaitingForDebugger' },
   ]);
 
   await waitForInitialSetup(session);

test/parallel/test-inspector-async-stack-traces-promise-then.js

@@ -20,6 +20,8 @@ function runTest() {
 async function runTests() {
   const instance = new NodeInstance(undefined, script);
   const session = await instance.connectInspectorSession();
+  await session.send({ method: 'NodeRuntime.enable' });
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
   await session.send([
     { 'method': 'Runtime.enable' },
     { 'method': 'Debugger.enable' },
@@ -29,6 +31,7 @@ async function runTests() {
       'params': { 'patterns': [] } },
     { 'method': 'Runtime.runIfWaitingForDebugger' },
   ]);
+  session.send({ method: 'NodeRuntime.disable' });
 
   await session.waitForBreakOnLine(0, '[eval]');
   await session.send({ 'method': 'Debugger.resume' });

test/parallel/test-inspector-async-stack-traces-set-interval.js

@@ -26,6 +26,8 @@ async function checkAsyncStackTrace(session) {
 async function runTests() {
   const instance = new NodeInstance(undefined, script);
   const session = await instance.connectInspectorSession();
+  await session.send({ method: 'NodeRuntime.enable' });
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
   await session.send([
     { 'method': 'Runtime.enable' },
     { 'method': 'Debugger.enable' },
@@ -35,6 +37,7 @@ async function runTests() {
       'params': { 'patterns': [] } },
     { 'method': 'Runtime.runIfWaitingForDebugger' },
   ]);
+  await session.send({ method: 'NodeRuntime.disable' });
 
   await skipFirstBreakpoint(session);
   await checkAsyncStackTrace(session);

test/parallel/test-inspector-break-e.js

@@ -8,11 +8,14 @@ const { NodeInstance } = require('../common/inspector-helper.js');
 async function runTests() {
   const instance = new NodeInstance(undefined, 'console.log(10)');
   const session = await instance.connectInspectorSession();
+  await session.send({ method: 'NodeRuntime.enable' });
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
   await session.send([
     { 'method': 'Runtime.enable' },
     { 'method': 'Debugger.enable' },
     { 'method': 'Runtime.runIfWaitingForDebugger' },
   ]);
+  await session.send({ method: 'NodeRuntime.disable' });
   await session.waitForBreakOnLine(0, '[eval]');
   await session.runToCompletion();
   assert.strictEqual((await instance.expectShutdown()).exitCode, 0);

test/parallel/test-inspector-break-when-eval.js

@@ -20,7 +20,10 @@ async function setupDebugger(session) {
       'params': { 'maxDepth': 0 } },
     { 'method': 'Runtime.runIfWaitingForDebugger' },
   ];
-  session.send(commands);
+  await session.send({ method: 'NodeRuntime.enable' });
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
+  await session.send(commands);
+  await session.send({ method: 'NodeRuntime.disable' });
 
   await session.waitForNotification('Debugger.paused', 'Initial pause');
 

test/parallel/test-inspector-console.js

@@ -20,7 +20,10 @@ async function runTest() {
     { 'method': 'Runtime.runIfWaitingForDebugger' },
   ];
 
-  session.send(commands);
+  await session.send({ method: 'NodeRuntime.enable' });
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
+  await session.send(commands);
+  await session.send({ method: 'NodeRuntime.disable' });
 
   const msg = await session.waitForNotification('Runtime.consoleAPICalled');
 

test/parallel/test-inspector-debug-brk-flag.js

@@ -22,7 +22,10 @@ async function testBreakpointOnStart(session) {
     { 'method': 'Runtime.runIfWaitingForDebugger' },
   ];
 
-  session.send(commands);
+  await session.send({ method: 'NodeRuntime.enable' });
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
+  await session.send(commands);
+  await session.send({ method: 'NodeRuntime.disable' });
   await session.waitForBreakOnLine(0, session.scriptURL());
 }
 

test/parallel/test-inspector-debug-end.js

@@ -30,6 +30,8 @@ async function testSessionNoCrash() {
 
   const instance = new NodeInstance('--inspect-brk=0', script);
   const session = await instance.connectInspectorSession();
+  await session.send({ method: 'NodeRuntime.enable' });
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
   await session.send({ 'method': 'Runtime.runIfWaitingForDebugger' });
   await session.waitForServerDisconnect();
   strictEqual((await instance.expectShutdown()).exitCode, 42);

test/parallel/test-inspector-esm.js

@@ -36,7 +36,10 @@ async function testBreakpointOnStart(session) {
     { 'method': 'Runtime.runIfWaitingForDebugger' },
   ];
 
+  await session.send({ method: 'NodeRuntime.enable' });
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
   await session.send(commands);
+  await session.send({ method: 'NodeRuntime.disable' });
   await session.waitForBreakOnLine(
     0, UrlResolve(session.scriptURL().toString(), 'message.mjs'));
 }

test/parallel/test-inspector-exception.js

@@ -28,7 +28,10 @@ async function testBreakpointOnStart(session) {
     { 'method': 'Runtime.runIfWaitingForDebugger' },
   ];
 
+  await session.send({ method: 'NodeRuntime.enable' });
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
   await session.send(commands);
+  await session.send({ method: 'NodeRuntime.disable' });
   await session.waitForBreakOnLine(21, pathToFileURL(script).toString());
 }
 

test/parallel/test-inspector-inspect-brk-node.js

@@ -10,9 +10,12 @@ const { NodeInstance } = require('../common/inspector-helper.js');
 async function runTest() {
   const child = new NodeInstance(['--inspect-brk-node=0', '-p', '42']);
   const session = await child.connectInspectorSession();
+  await session.send({ method: 'NodeRuntime.enable' });
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
   await session.send({ method: 'Runtime.enable' });
   await session.send({ method: 'Debugger.enable' });
   await session.send({ method: 'Runtime.runIfWaitingForDebugger' });
+  await session.send({ method: 'NodeRuntime.disable' });
   await session.waitForNotification((notification) => {
     // The main assertion here is that we do hit the loader script first.
     return notification.method === 'Debugger.scriptParsed' &&

test/parallel/test-inspector-multisession-ws.js

@@ -20,11 +20,12 @@ session.on('Debugger.paused', () => {
 session.connect();
 session.post('Debugger.enable');
 console.log('Ready');
-console.log('Ready');
 `;
 
 async function setupSession(node) {
   const session = await node.connectInspectorSession();
+  await session.send({ method: 'NodeRuntime.enable' });
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
   await session.send([
     { 'method': 'Runtime.enable' },
     { 'method': 'Debugger.enable' },
@@ -37,20 +38,19 @@ async function setupSession(node) {
       'params': { 'interval': 100 } },
     { 'method': 'Debugger.setBlackboxPatterns',
       'params': { 'patterns': [] } },
-    { 'method': 'Runtime.runIfWaitingForDebugger' },
   ]);
+
   return session;
 }
 
 async function testSuspend(sessionA, sessionB) {
   console.log('[test]', 'Breaking in code and verifying events are fired');
-  await sessionA.waitForNotification('Debugger.paused', 'Initial pause');
+  await Promise.all([
+    sessionA.waitForNotification('Debugger.paused', 'Initial sessionA paused'),
+    sessionB.waitForNotification('Debugger.paused', 'Initial sessionB paused'),
+  ]);
   sessionA.send({ 'method': 'Debugger.resume' });
 
-  await sessionA.waitForNotification('Runtime.consoleAPICalled',
-                                     'Console output');
-  // NOTE(mmarchini): Remove second console.log when
-  // https://bugs.chromium.org/p/v8/issues/detail?id=10287 is fixed.
   await sessionA.waitForNotification('Runtime.consoleAPICalled',
                                      'Console output');
   sessionA.send({ 'method': 'Debugger.pause' });
@@ -65,6 +65,14 @@ async function runTest() {
 
   const [session1, session2] =
       await Promise.all([setupSession(child), setupSession(child)]);
+  await Promise.all([
+    session1.send({ method: 'Runtime.runIfWaitingForDebugger' }),
+    session2.send({ method: 'Runtime.runIfWaitingForDebugger' }),
+  ]);
+  await Promise.all([
+    session1.send({ method: 'NodeRuntime.disable' }),
+    session2.send({ method: 'NodeRuntime.disable' }),
+  ]);
   await testSuspend(session2, session1);
   console.log('[test]', 'Should shut down after both sessions disconnect');
 

test/parallel/test-inspector-scriptparsed-context.js

@@ -46,10 +46,13 @@ async function runTests() {
   const instance = new NodeInstance(['--inspect-brk=0', '--expose-internals'],
                                     script);
   const session = await instance.connectInspectorSession();
-  await session.send([
-    { 'method': 'Debugger.enable' },
-    { 'method': 'Runtime.runIfWaitingForDebugger' },
-  ]);
+
+  await session.send({ method: 'NodeRuntime.enable' });
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
+  await session.send({ 'method': 'Debugger.enable' });
+  await session.send({ method: 'Runtime.runIfWaitingForDebugger' });
+  await session.send({ method: 'NodeRuntime.disable' });
+
   await session.waitForBreakOnLine(2, '[eval]');
 
   await session.send({ 'method': 'Runtime.enable' });

test/parallel/test-inspector-stop-profile-after-done.js

@@ -14,12 +14,15 @@ async function runTests() {
                                  }, ${common.platformTimeout(30)});`);
   const session = await child.connectInspectorSession();
 
-  session.send([
+  await session.send({ method: 'NodeRuntime.enable' });
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
+  await session.send([
     { method: 'Profiler.setSamplingInterval',
       params: { interval: common.platformTimeout(300) } },
-    { method: 'Profiler.enable' },
-    { method: 'Runtime.runIfWaitingForDebugger' },
-    { method: 'Profiler.start' }]);
+    { method: 'Profiler.enable' }]);
+  await session.send({ method: 'Runtime.runIfWaitingForDebugger' });
+  await session.send({ method: 'NodeRuntime.disable' });
+  await session.send({ method: 'Profiler.start' });
   while (await child.nextStderrString() !==
          'Waiting for the debugger to disconnect...');
   await session.send({ method: 'Profiler.stop' });

test/parallel/test-inspector-wait-for-connection.js

@@ -24,7 +24,11 @@ async function runTests() {
     }
   });
   assert.ok(value.startsWith('ws://'));
-  session.send({ method: 'Runtime.runIfWaitingForDebugger' });
+  await session.send({ method: 'NodeRuntime.enable' });
+  child.write('first');
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
+  await session.send({ method: 'Runtime.runIfWaitingForDebugger' });
+  await session.send({ method: 'NodeRuntime.disable' });
   // Check that messages after first and before second waitForDebugger are
   // received
   await session.waitForConsoleOutput('log', 'after wait for debugger');
@@ -33,7 +37,11 @@ async function runTests() {
                     .some((n) => n.method === 'Runtime.consoleAPICalled'));
   const secondSession = await child.connectInspectorSession();
   // Check that inspector.waitForDebugger can be resumed from another session
-  secondSession.send({ method: 'Runtime.runIfWaitingForDebugger' });
+  await session.send({ method: 'NodeRuntime.enable' });
+  child.write('second');
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
+  await session.send({ method: 'Runtime.runIfWaitingForDebugger' });
+  await session.send({ method: 'NodeRuntime.disable' });
   await session.waitForConsoleOutput('log', 'after second wait for debugger');
   assert.ok(!session.unprocessedNotifications()
                     .some((n) => n.method === 'Runtime.consoleAPICalled'));
@@ -45,11 +53,20 @@ async function runTests() {
     inspector.open(0, undefined, false);
     process._ws = inspector.url();
     console.log('before wait for debugger');
-    inspector.waitForDebugger();
-    console.log('after wait for debugger');
-    console.log('before second wait for debugger');
-    inspector.waitForDebugger();
-    console.log('after second wait for debugger');
+    process.stdin.once('data', (data) => {
+      if (data.toString() === 'first') {
+        inspector.waitForDebugger();
+        console.log('after wait for debugger');
+        console.log('before second wait for debugger');
+        process.stdin.once('data', (data) => {
+          if (data.toString() === 'second') {
+            inspector.waitForDebugger();
+            console.log('after second wait for debugger');
+            process.exit();
+          }
+        });
+      }
+    });
   }
 
   // Check that inspector.waitForDebugger throws if there is no active

test/parallel/test-inspector-waiting-for-disconnect.js

@@ -17,11 +17,14 @@ async function runTest() {
   const oldStyleSession = await child.connectInspectorSession();
   await oldStyleSession.send([
     { method: 'Runtime.enable' }]);
+  await session.send({ method: 'NodeRuntime.enable' });
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
   await session.send([
     { method: 'Runtime.enable' },
     { method: 'NodeRuntime.notifyWhenWaitingForDisconnect',
       params: { enabled: true } },
     { method: 'Runtime.runIfWaitingForDebugger' }]);
+  await session.send({ method: 'NodeRuntime.disable' });
   await session.waitForNotification((notification) => {
     return notification.method === 'NodeRuntime.waitingForDisconnect';
   });

test/parallel/test-inspector.js

@@ -75,7 +75,10 @@ async function testBreakpointOnStart(session) {
     { 'method': 'Runtime.runIfWaitingForDebugger' },
   ];
 
+  await session.send({ method: 'NodeRuntime.enable' });
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
   await session.send(commands);
+  await session.send({ method: 'NodeRuntime.disable' });
   await session.waitForBreakOnLine(0, session.scriptURL());
 }
 

test/parallel/test-runner-inspect.mjs

@@ -24,9 +24,12 @@ tmpdir.refresh();
 
   const session = await child.connectInspectorSession();
 
+  await session.send({ method: 'NodeRuntime.enable' });
+  await session.waitForNotification('NodeRuntime.waitingForDebugger');
   await session.send([
     { method: 'Runtime.enable' },
     { method: 'Runtime.runIfWaitingForDebugger' }]);
+  await session.send({ method: 'NodeRuntime.disable' });
 
   session.disconnect();
   assert.match(stderr,