Skip to content

Commit

Permalink
inspector: Fix crash for WS connection
Browse files Browse the repository at this point in the history 
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 <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
  • Loading branch information
@eugeneo
Eugene Ostroukhov authored and eugeneo committed Dec 11, 2017
1 parent e51fb90 commit 73ad3f9
Show file tree
Hide file tree
Showing 13 changed files with 1,022 additions and 1,024 deletions.
4 changes: 0 additions & 4 deletions src/inspector_agent.cc
View file
Expand Up @@ -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());
Expand Down
1 change: 0 additions & 1 deletion src/inspector_agent.h
View file
Expand Up @@ -48,7 +48,6 @@ class Agent {
bool IsStarted() { return !!client_; }

// IO thread started, and client connected
bool IsConnected();
bool IsWaitingForConnect();

void WaitForDisconnect();
Expand Down
73 changes: 47 additions & 26 deletions src/inspector_io.cc
View file
Expand Up @@ -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
Expand All @@ -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) {
Expand Down Expand Up @@ -226,10 +229,6 @@ void InspectorIo::Stop() {
DispatchMessages();
}

bool InspectorIo::IsConnected() {
return delegate_ != nullptr && delegate_->IsConnected();
}

bool InspectorIo::IsStarted() {
return platform_ != nullptr;
}
Expand Down Expand Up @@ -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();
Expand All @@ -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;
}
}
Expand All @@ -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()) {
Expand All @@ -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;
}

Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand Down Expand Up @@ -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,
Expand All @@ -464,7 +486,6 @@ void InspectorIoDelegate::MessageReceived(int session_id,
}

void InspectorIoDelegate::EndSession(int session_id) {
connected_ = false;
io_->PostIncomingMessage(InspectorAction::kEndSession, session_id, "");
}

Expand Down
8 changes: 6 additions & 2 deletions src/inspector_io.h
View file
Expand Up @@ -36,6 +36,7 @@ class InspectorIoDelegate;

enum class InspectorAction {
kStartSession,
kStartSessionUnconditionally, // First attach with --inspect-brk
kEndSession,
kSendMessage
};
Expand All @@ -44,7 +45,9 @@ enum class InspectorAction {
enum class TransportAction {
kKill,
kSendMessage,
kStop
kStop,
kAcceptSession,
kDeclineSession
};

class InspectorIo {
Expand All @@ -61,7 +64,6 @@ class InspectorIo {
void Stop();

bool IsStarted();
bool IsConnected();

void WaitForDisconnect();
// Called from thread to queue an incoming message and trigger
Expand Down Expand Up @@ -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_;

Expand Down

0 comments on commit 73ad3f9

Please sign in to comment.