inspector: Allows reentry when paused

This change allows reentering the message dispatch loop when the Node is
paused. This is necessary when the pause happened as a result of the
message sent by a debug frontend, such as evaluating a function with a
breakpoint inside.

Fixes: https://github.com/nodejs/node/issues/13320
PR-URL: https://github.com/nodejs/node/pull/13350
Reviewed-By: James M Snell <jasnell@gmail.com>

src/inspector_agent.cc

@@ -203,7 +203,7 @@ class JsBindingsSessionDelegate : public InspectorSessionDelegate {
     callback_.Reset();
   }
 
-  bool WaitForFrontendMessage() override {
+  bool WaitForFrontendMessageWhilePaused() override {
     return false;
   }
 
@@ -393,7 +393,7 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel {
   }
 
   bool waitForFrontendMessage() {
-    return delegate_->WaitForFrontendMessage();
+    return delegate_->WaitForFrontendMessageWhilePaused();
   }
 
   void schedulePauseOnNextStatement(const std::string& reason) {

src/inspector_agent.h

@@ -38,7 +38,7 @@ namespace inspector {
 class InspectorSessionDelegate {
  public:
   virtual ~InspectorSessionDelegate() = default;
-  virtual bool WaitForFrontendMessage() = 0;
+  virtual bool WaitForFrontendMessageWhilePaused() = 0;
   virtual void SendMessageToFrontend(const v8_inspector::StringView& message)
                                      = 0;
 };

src/inspector_io.cc

@@ -134,7 +134,7 @@ std::unique_ptr<StringBuffer> Utf8ToStringView(const std::string& message) {
 class IoSessionDelegate : public InspectorSessionDelegate {
  public:
   explicit IoSessionDelegate(InspectorIo* io) : io_(io) { }
-  bool WaitForFrontendMessage() override;
+  bool WaitForFrontendMessageWhilePaused() override;
   void SendMessageToFrontend(const v8_inspector::StringView& message) override;
  private:
   InspectorIo* io_;
@@ -354,7 +354,8 @@ void InspectorIo::PostIncomingMessage(InspectorAction action, int session_id,
   NotifyMessageReceived();
 }
 
-void InspectorIo::WaitForIncomingMessage() {
+void InspectorIo::WaitForFrontendMessageWhilePaused() {
+  dispatching_messages_ = false;
   Mutex::ScopedLock scoped_lock(state_lock_);
   if (incoming_message_queue_.empty())
     incoming_message_cond_.Wait(scoped_lock);
@@ -373,11 +374,15 @@ void InspectorIo::DispatchMessages() {
   if (dispatching_messages_)
     return;
   dispatching_messages_ = true;
-  MessageQueue<InspectorAction> tasks;
+  bool had_messages = false;
   do {
-    tasks.clear();
+    if (dispatching_message_queue_.empty())
-    SwapBehindLock(&incoming_message_queue_, &tasks);
+      SwapBehindLock(&incoming_message_queue_, &dispatching_message_queue_);
-    for (const auto& task : tasks) {
+    had_messages = !dispatching_message_queue_.empty();
+    while (!dispatching_message_queue_.empty()) {
+      MessageQueue<InspectorAction>::value_type task;
+      std::swap(dispatching_message_queue_.front(), task);
+      dispatching_message_queue_.pop_front();
       StringView message = std::get<2>(task)->string();
       switch (std::get<0>(task)) {
       case InspectorAction::kStartSession:
@@ -404,7 +409,7 @@ void InspectorIo::DispatchMessages() {
         break;
       }
     }
-  } while (!tasks.empty());
+  } while (had_messages);
   dispatching_messages_ = false;
 }
 
@@ -485,8 +490,8 @@ std::string InspectorIoDelegate::GetTargetUrl(const std::string& id) {
   return "file://" + script_path_;
 }
 
-bool IoSessionDelegate::WaitForFrontendMessage() {
+bool IoSessionDelegate::WaitForFrontendMessageWhilePaused() {
-  io_->WaitForIncomingMessage();
+  io_->WaitForFrontendMessageWhilePaused();
   return true;
 }
 

src/inspector_io.h

@@ -6,9 +6,9 @@
 #include "node_mutex.h"
 #include "uv.h"
 
+#include <deque>
 #include <memory>
 #include <stddef.h>
-#include <vector>
 
 #if !HAVE_INSPECTOR
 #error("This header can only be used when inspector is enabled")
@@ -76,7 +76,7 @@ class InspectorIo {
  private:
   template <typename Action>
   using MessageQueue =
-      std::vector<std::tuple<Action, int,
+      std::deque<std::tuple<Action, int,
                   std::unique_ptr<v8_inspector::StringBuffer>>>;
   enum class State {
     kNew,
@@ -115,7 +115,7 @@ class InspectorIo {
   void SwapBehindLock(MessageQueue<ActionType>* vector1,
                       MessageQueue<ActionType>* vector2);
   // Wait on incoming_message_cond_
-  void WaitForIncomingMessage();
+  void WaitForFrontendMessageWhilePaused();
   // Broadcast incoming_message_cond_
   void NotifyMessageReceived();
 
@@ -145,6 +145,7 @@ class InspectorIo {
   Mutex state_lock_;  // Locked before mutating either queue.
   MessageQueue<InspectorAction> incoming_message_queue_;
   MessageQueue<TransportAction> outgoing_message_queue_;
+  MessageQueue<InspectorAction> dispatching_message_queue_;
 
   bool dispatching_messages_;
   int session_id_;

test/inspector/global-function.js

@@ -0,0 +1,13 @@
+'use strict'; // eslint-disable-line required-modules
+let invocations = 0;
+const interval = setInterval(() => {}, 1000);
+
+global.sum = function() {
+  const a = 1;
+  const b = 2;
+  const c = a + b;
+  clearInterval(interval);
+  console.log(invocations++, c);
+};
+
+console.log('Ready!');

test/inspector/inspector-helper.js

@@ -10,6 +10,7 @@ const url = require('url');
 const DEBUG = false;
 const TIMEOUT = 15 * 1000;
 const EXPECT_ALIVE_SYMBOL = Symbol('isAlive');
+const DONT_EXPECT_RESPONSE_SYMBOL = Symbol('dontExpectResponse');
 const mainScript = path.join(common.fixturesDir, 'loop.js');
 
 function send(socket, message, id, callback) {
@@ -183,7 +184,6 @@ TestSession.prototype.processMessage_ = function(message) {
   this.messagefilter_ && this.messagefilter_(message);
   const id = message['id'];
   if (id) {
-    assert.strictEqual(id, this.expectedId_);
     this.expectedId_++;
     if (this.responseCheckers_[id]) {
       const messageJSON = JSON.stringify(message);
@@ -207,16 +207,21 @@ TestSession.prototype.sendAll_ = function(commands, callback) {
   if (!commands.length) {
     callback();
   } else {
-    this.lastId_++;
+    let id = ++this.lastId_;
     let command = commands[0];
     if (command instanceof Array) {
-      this.responseCheckers_[this.lastId_] = command[1];
+      this.responseCheckers_[id] = command[1];
       command = command[0];
     }
     if (command instanceof Function)
       command = command();
-    this.messages_[this.lastId_] = command;
+    if (!command[DONT_EXPECT_RESPONSE_SYMBOL]) {
-    send(this.socket_, command, this.lastId_,
+      this.messages_[id] = command;
+    } else {
+      id += 100000;
+      this.lastId_--;
+    }
+    send(this.socket_, command, id,
          () => this.sendAll_(commands.slice(1), callback));
   }
 };
@@ -497,12 +502,13 @@ Harness.prototype.kill = function() {
 
 exports.startNodeForInspectorTest = function(callback,
                                              inspectorFlags = ['--inspect-brk'],
-                                             opt_script_contents) {
+                                             scriptContents = '',
+                                             scriptFile = mainScript) {
   const args = [].concat(inspectorFlags);
-  if (opt_script_contents) {
+  if (scriptContents) {
-    args.push('-e', opt_script_contents);
+    args.push('-e', scriptContents);
   } else {
-    args.push(mainScript);
+    args.push(scriptFile);
   }
 
   const child = spawn(process.execPath, args);
@@ -534,3 +540,7 @@ exports.startNodeForInspectorTest = function(callback,
 exports.mainScriptSource = function() {
   return fs.readFileSync(mainScript, 'utf8');
 };
+
+exports.markMessageNoResponse = function(message) {
+  message[DONT_EXPECT_RESPONSE_SYMBOL] = true;
+};

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

@@ -0,0 +1,128 @@
+'use strict';
+const common = require('../common');
+common.skipIfInspectorDisabled();
+const assert = require('assert');
+const helper = require('./inspector-helper.js');
+const path = require('path');
+
+const script = path.join(path.dirname(module.filename), 'global-function.js');
+
+
+function setupExpectBreakOnLine(line, url, session) {
+  return function(message) {
+    if ('Debugger.paused' === message['method']) {
+      const callFrame = message['params']['callFrames'][0];
+      const location = callFrame['location'];
+      assert.strictEqual(url, session.scriptUrlForId(location['scriptId']));
+      assert.strictEqual(line, location['lineNumber']);
+      return true;
+    }
+  };
+}
+
+function setupExpectConsoleOutputAndBreak(type, values) {
+  if (!(values instanceof Array))
+    values = [ values ];
+  let consoleLog = false;
+  function matchConsoleLog(message) {
+    if ('Runtime.consoleAPICalled' === message['method']) {
+      const params = message['params'];
+      if (params['type'] === type) {
+        let i = 0;
+        for (const value of params['args']) {
+          if (value['value'] !== values[i++])
+            return false;
+        }
+        return i === values.length;
+      }
+    }
+  }
+
+  return function(message) {
+    if (consoleLog)
+      return message['method'] === 'Debugger.paused';
+    consoleLog = matchConsoleLog(message);
+    return false;
+  };
+}
+
+function setupExpectContextDestroyed(id) {
+  return function(message) {
+    if ('Runtime.executionContextDestroyed' === message['method'])
+      return message['params']['executionContextId'] === id;
+  };
+}
+
+function setupDebugger(session) {
+  console.log('[test]', 'Setting up a debugger');
+  const commands = [
+    { 'method': 'Runtime.enable' },
+    { 'method': 'Debugger.enable' },
+    { 'method': 'Debugger.setAsyncCallStackDepth',
+      'params': {'maxDepth': 0} },
+    { 'method': 'Runtime.runIfWaitingForDebugger' },
+  ];
+
+  session
+    .sendInspectorCommands(commands)
+    .expectMessages((message) => 'Runtime.consoleAPICalled' === message.method);
+}
+
+function breakOnLine(session) {
+  console.log('[test]', 'Breaking in the code');
+  const commands = [
+    { 'method': 'Debugger.setBreakpointByUrl',
+      'params': { 'lineNumber': 9,
+                  'url': script,
+                  'columnNumber': 0,
+                  'condition': ''
+      }
+    },
+    { 'method': 'Runtime.evaluate',
+      'params': { 'expression': 'sum()',
+                  'objectGroup': 'console',
+                  'includeCommandLineAPI': true,
+                  'silent': false,
+                  'contextId': 1,
+                  'returnByValue': false,
+                  'generatePreview': true,
+                  'userGesture': true,
+                  'awaitPromise': false
+      }
+    }
+  ];
+  helper.markMessageNoResponse(commands[1]);
+  session
+    .sendInspectorCommands(commands)
+    .expectMessages(setupExpectBreakOnLine(9, script, session));
+}
+
+function stepOverConsoleStatement(session) {
+  console.log('[test]', 'Step over console statement and test output');
+  session
+    .sendInspectorCommands({ 'method': 'Debugger.stepOver' })
+    .expectMessages(setupExpectConsoleOutputAndBreak('log', [0, 3]));
+}
+
+function testWaitsForFrontendDisconnect(session, harness) {
+  console.log('[test]', 'Verify node waits for the frontend to disconnect');
+  session.sendInspectorCommands({ 'method': 'Debugger.resume'})
+    .expectMessages(setupExpectContextDestroyed(1))
+    .expectStderrOutput('Waiting for the debugger to disconnect...')
+    .disconnect(true);
+}
+
+function runTests(harness) {
+  harness
+    .runFrontendSession([
+      setupDebugger,
+      breakOnLine,
+      stepOverConsoleStatement,
+      testWaitsForFrontendDisconnect
+    ]).expectShutDown(0);
+}
+
+helper.startNodeForInspectorTest(runTests,
+                                 ['--inspect'],
+                                 undefined,
+                                 script);