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: #16852
PR-URL: #17085
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>

Author: Eugene Ostroukhov

src/inspector_agent.cc

@@ -548,10 +548,6 @@ void Agent::Connect(InspectorSessionDelegate* delegate) {
   client_->connectFrontend(delegate);
 }
 
-bool Agent::IsConnected() {
-  return io_ && io_->IsConnected();
-}
-
 void Agent::WaitForDisconnect() {
   CHECK_NE(client_, nullptr);
   client_->contextDestroyed(parent_env_->context());

src/inspector_agent.h

@@ -48,7 +48,6 @@ class Agent {
   bool IsStarted() { return !!client_; }
 
   // IO thread started, and client connected
-  bool IsConnected();
   bool IsWaitingForConnect();
 
   void WaitForDisconnect();

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, "");
 }
 

src/inspector_io.h

@@ -36,6 +36,7 @@ class InspectorIoDelegate;
 
 enum class InspectorAction {
   kStartSession,
+  kStartSessionUnconditionally,  // First attach with --inspect-brk
   kEndSession,
   kSendMessage
 };
@@ -44,7 +45,9 @@ enum class InspectorAction {
 enum class TransportAction {
   kKill,
   kSendMessage,
-  kStop
+  kStop,
+  kAcceptSession,
+  kDeclineSession
 };
 
 class InspectorIo {
@@ -61,7 +64,6 @@ class InspectorIo {
   void Stop();
 
   bool IsStarted();
-  bool IsConnected();
 
   void WaitForDisconnect();
   // Called from thread to queue an incoming message and trigger
@@ -124,6 +126,8 @@ class InspectorIo {
   void WaitForFrontendMessageWhilePaused();
   // Broadcast incoming_message_cond_
   void NotifyMessageReceived();
+  // Attach session to an inspector. Either kAcceptSession or kDeclineSession
+  TransportAction Attach(int session_id);
 
   const DebugOptions options_;