inspector: Fix crash for WS connection

Attaching WS session will now include a roundtrip onto the main thread
to make sure there is no other session (e.g. JS bindings)

This change also required refactoring WS socket implementation to better
support scenarios like this.

Fixes: https://github.com/nodejs/node/issues/16852
PR-URL: https://github.com/nodejs/node/pull/17085
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>

1 files changed, 47 insertions, 26 deletions

diff --git a/src/inspector_io.cc b/src/inspector_io.cc
index 538cbab3c9..9af4458c6b 100644
--- a/src/inspector_io.cc
+++ b/src/inspector_io.cc
@@ -136,7 +136,7 @@ class InspectorIoDelegate: public node::inspector::SocketServerDelegate {
                       const std::string& script_name, bool wait);
   // Calls PostIncomingMessage() with appropriate InspectorAction:
   //   kStartSession
-  bool StartSession(int session_id, const std::string& target_id) override;
+  void StartSession(int session_id, const std::string& target_id) override;
   //   kSendMessage
   void MessageReceived(int session_id, const std::string& message) override;
   //   kEndSession
@@ -145,19 +145,22 @@ class InspectorIoDelegate: public node::inspector::SocketServerDelegate {
   std::vector<std::string> GetTargetIds() override;
   std::string GetTargetTitle(const std::string& id) override;
   std::string GetTargetUrl(const std::string& id) override;
-  bool IsConnected() { return connected_; }
   void ServerDone() override {
     io_->ServerDone();
   }
 
+  void AssignTransport(InspectorSocketServer* server) {
+    server_ = server;
+  }
+
  private:
   InspectorIo* io_;
-  bool connected_;
   int session_id_;
   const std::string script_name_;
   const std::string script_path_;
   const std::string target_id_;
   bool waiting_;
+  InspectorSocketServer* server_;
 };
 
 void InterruptCallback(v8::Isolate*, void* agent) {
@@ -226,10 +229,6 @@ void InspectorIo::Stop() {
   DispatchMessages();
 }
 
-bool InspectorIo::IsConnected() {
-  return delegate_ != nullptr && delegate_->IsConnected();
-}
-
 bool InspectorIo::IsStarted() {
   return platform_ != nullptr;
 }
@@ -264,6 +263,7 @@ void InspectorIo::IoThreadAsyncCb(uv_async_t* async) {
   MessageQueue<TransportAction> outgoing_message_queue;
   io->SwapBehindLock(&io->outgoing_message_queue_, &outgoing_message_queue);
   for (const auto& outgoing : outgoing_message_queue) {
+    int session_id = std::get<1>(outgoing);
     switch (std::get<0>(outgoing)) {
     case TransportAction::kKill:
       transport->TerminateConnections();
@@ -272,8 +272,14 @@ void InspectorIo::IoThreadAsyncCb(uv_async_t* async) {
       transport->Stop(nullptr);
       break;
     case TransportAction::kSendMessage:
-      std::string message = StringViewToUtf8(std::get<2>(outgoing)->string());
-      transport->Send(std::get<1>(outgoing), message);
+      transport->Send(session_id,
+                      StringViewToUtf8(std::get<2>(outgoing)->string()));
+      break;
+    case TransportAction::kAcceptSession:
+      transport->AcceptSession(session_id);
+      break;
+    case TransportAction::kDeclineSession:
+      transport->DeclineSession(session_id);
       break;
     }
   }
@@ -293,6 +299,7 @@ void InspectorIo::ThreadMain() {
                                wait_for_connect_);
   delegate_ = &delegate;
   Transport server(&delegate, &loop, options_.host_name(), options_.port());
+  delegate.AssignTransport(&server);
   TransportAndIo<Transport> queue_transport(&server, this);
   thread_req_.data = &queue_transport;
   if (!server.Start()) {
@@ -308,6 +315,7 @@ void InspectorIo::ThreadMain() {
   uv_run(&loop, UV_RUN_DEFAULT);
   thread_req_.data = nullptr;
   CHECK_EQ(uv_loop_close(&loop), 0);
+  delegate.AssignTransport(nullptr);
   delegate_ = nullptr;
 }
 
@@ -358,6 +366,21 @@ void InspectorIo::NotifyMessageReceived() {
   incoming_message_cond_.Broadcast(scoped_lock);
 }
 
+TransportAction InspectorIo::Attach(int session_id) {
+  Agent* agent = parent_env_->inspector_agent();
+  if (agent->delegate() != nullptr)
+    return TransportAction::kDeclineSession;
+
+  CHECK_EQ(session_delegate_, nullptr);
+  session_id_ = session_id;
+  state_ = State::kConnected;
+  fprintf(stderr, "Debugger attached.\n");
+  session_delegate_ = std::unique_ptr<InspectorSessionDelegate>(
+      new IoSessionDelegate(this));
+  agent->Connect(session_delegate_.get());
+  return TransportAction::kAcceptSession;
+}
+
 void InspectorIo::DispatchMessages() {
   // This function can be reentered if there was an incoming message while
   // V8 was processing another inspector request (e.g. if the user is
@@ -375,16 +398,14 @@ void InspectorIo::DispatchMessages() {
       MessageQueue<InspectorAction>::value_type task;
       std::swap(dispatching_message_queue_.front(), task);
       dispatching_message_queue_.pop_front();
+      int id = std::get<1>(task);
       StringView message = std::get<2>(task)->string();
       switch (std::get<0>(task)) {
       case InspectorAction::kStartSession:
-        CHECK_EQ(session_delegate_, nullptr);
-        session_id_ = std::get<1>(task);
-        state_ = State::kConnected;
-        fprintf(stderr, "Debugger attached.\n");
-        session_delegate_ = std::unique_ptr<InspectorSessionDelegate>(
-            new IoSessionDelegate(this));
-        parent_env_->inspector_agent()->Connect(session_delegate_.get());
+        Write(Attach(id), id, StringView());
+        break;
+      case InspectorAction::kStartSessionUnconditionally:
+        Attach(id);
         break;
       case InspectorAction::kEndSession:
         CHECK_NE(session_delegate_, nullptr);
@@ -428,22 +449,23 @@ InspectorIoDelegate::InspectorIoDelegate(InspectorIo* io,
                                          const std::string& script_name,
                                          bool wait)
                                          : io_(io),
-                                           connected_(false),
                                            session_id_(0),
                                            script_name_(script_name),
                                            script_path_(script_path),
                                            target_id_(GenerateID()),
-                                           waiting_(wait) { }
+                                           waiting_(wait),
+                                           server_(nullptr) { }
 
 
-bool InspectorIoDelegate::StartSession(int session_id,
+void InspectorIoDelegate::StartSession(int session_id,
                                        const std::string& target_id) {
-  if (connected_)
-    return false;
-  connected_ = true;
-  session_id_++;
-  io_->PostIncomingMessage(InspectorAction::kStartSession, session_id, "");
-  return true;
+  session_id_ = session_id;
+  InspectorAction action = InspectorAction::kStartSession;
+  if (waiting_) {
+    action = InspectorAction::kStartSessionUnconditionally;
+    server_->AcceptSession(session_id);
+  }
+  io_->PostIncomingMessage(action, session_id, "");
 }
 
 void InspectorIoDelegate::MessageReceived(int session_id,
@@ -464,7 +486,6 @@ void InspectorIoDelegate::MessageReceived(int session_id,
 }
 
 void InspectorIoDelegate::EndSession(int session_id) {
-  connected_ = false;
   io_->PostIncomingMessage(InspectorAction::kEndSession, session_id, "");
 }