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);
 }
 
-boolAgent::IsConnected(){
-return io_ && io_->IsConnected();
-}
-
 voidAgent::WaitForDisconnect(){
 CHECK_NE(client_, nullptr);
  client_->contextDestroyed(parent_env_->context());

src/inspector_agent.h

@@ -48,7 +48,6 @@ class Agent{
 boolIsStarted(){return !!client_}
 
 // IO thread started, and client connected
-boolIsConnected();
 boolIsWaitingForConnect();
 
 voidWaitForDisconnect();

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
-boolStartSession(int session_id, const std::string& target_id) override;
+voidStartSession(int session_id, const std::string& target_id) override;
 // kSendMessage
 voidMessageReceived(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;
-boolIsConnected(){return connected_}
 voidServerDone() override{
  io_->ServerDone();
  }
 
+voidAssignTransport(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_;
 };
 
 voidInterruptCallback(v8::Isolate*, void* agent){
@@ -226,10 +229,6 @@ void InspectorIo::Stop(){
 DispatchMessages();
 }
 
-boolInspectorIo::IsConnected(){
-return delegate_ != nullptr && delegate_->IsConnected();
-}
-
 boolInspectorIo::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 (constauto& 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>(
+newIoSessionDelegate(this));
+ agent->Connect(session_delegate_.get());
+return TransportAction::kAcceptSession;
+}
+
 voidInspectorIo::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>(
-newIoSessionDelegate(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){}
 
 
-boolInspectorIoDelegate::StartSession(int session_id,
+voidInspectorIoDelegate::StartSession(int session_id,
 const std::string& target_id){
-if (connected_)
-returnfalse;
- connected_ = true;
- session_id_++;
- io_->PostIncomingMessage(InspectorAction::kStartSession, session_id, "");
-returntrue;
+ session_id_ = session_id;
+ InspectorAction action = InspectorAction::kStartSession;
+if (waiting_){
+ action = InspectorAction::kStartSessionUnconditionally;
+ server_->AcceptSession(session_id);
+ }
+ io_->PostIncomingMessage(action, session_id, "");
 }
 
 voidInspectorIoDelegate::MessageReceived(int session_id,
@@ -464,7 +486,6 @@ void InspectorIoDelegate::MessageReceived(int session_id,
 }
 
 voidInspectorIoDelegate::EndSession(int session_id){
- connected_ = false;
  io_->PostIncomingMessage(InspectorAction::kEndSession, session_id, "");
 }
 

src/inspector_io.h

@@ -36,6 +36,7 @@ class InspectorIoDelegate;
 
 enumclassInspectorAction{
 kStartSession,
+kStartSessionUnconditionally, // First attach with --inspect-brk
 kEndSession,
 kSendMessage
 };
@@ -44,7 +45,9 @@ enum class InspectorAction{
 enumclassTransportAction{
 kKill,
 kSendMessage,
-kStop
+kStop,
+kAcceptSession,
+kDeclineSession
 };
 
 classInspectorIo{
@@ -61,7 +64,6 @@ class InspectorIo{
 voidStop();
 
 boolIsStarted();
-boolIsConnected();
 
 voidWaitForDisconnect();
 // Called from thread to queue an incoming message and trigger
@@ -124,6 +126,8 @@ class InspectorIo{
 voidWaitForFrontendMessageWhilePaused();
 // Broadcast incoming_message_cond_
 voidNotifyMessageReceived();
+// Attach session to an inspector. Either kAcceptSession or kDeclineSession
+ TransportAction Attach(int session_id);
 
 const DebugOptions options_;