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>

2 files changed, 256 insertions, 362 deletions

diff --git a/test/cctest/test_inspector_socket.cc b/test/cctest/test_inspector_socket.cc
index d1f5b4a98a..943109b8a5 100644
--- a/test/cctest/test_inspector_socket.cc
+++ b/test/cctest/test_inspector_socket.cc
@@ -1,57 +1,17 @@
 #include "inspector_socket.h"
 #include "gtest/gtest.h"
 
+#include <queue>
+
 #define PORT 9444
 
 namespace {
 
 using node::inspector::InspectorSocket;
-using node::inspector::inspector_from_stream;
-using node::inspector::inspector_handshake_event;
-using node::inspector::kInspectorHandshakeFailed;
-using node::inspector::kInspectorHandshakeHttpGet;
-using node::inspector::kInspectorHandshakeUpgraded;
-using node::inspector::kInspectorHandshakeUpgrading;
 
 static const int MAX_LOOP_ITERATIONS = 10000;
 
-#define SPIN_WHILE(condition)                                                  \
-  {                                                                            \
-    Timeout timeout(&loop);                                                    \
-    while ((condition) && !timeout.timed_out) {                              \
-      uv_run(&loop, UV_RUN_NOWAIT);                                            \
-    }                                                                          \
-    ASSERT_FALSE((condition));                                                 \
-  }
-
-static bool connected = false;
-static bool inspector_ready = false;
-static int handshake_events = 0;
-static enum inspector_handshake_event last_event = kInspectorHandshakeHttpGet;
 static uv_loop_t loop;
-static uv_tcp_t server, client_socket;
-static InspectorSocket inspector;
-static std::string last_path;  // NOLINT(runtime/string)
-static void (*handshake_delegate)(enum inspector_handshake_event state,
-                                  const std::string& path,
-                                  bool* should_continue);
-static const char SERVER_CLOSE_FRAME[] = {'\x88', '\x00'};
-
-
-struct read_expects {
-  const char* expected;
-  size_t expected_len;
-  size_t pos;
-  bool read_expected;
-  bool callback_called;
-};
-
-static const char HANDSHAKE_REQ[] = "GET /ws/path HTTP/1.1\r\n"
-                                    "Host: localhost:9222\r\n"
-                                    "Upgrade: websocket\r\n"
-                                    "Connection: Upgrade\r\n"
-                                    "Sec-WebSocket-Key: aaa==\r\n"
-                                    "Sec-WebSocket-Version: 13\r\n\r\n";
 
 class Timeout {
  public:
@@ -86,16 +46,176 @@ class Timeout {
   uv_timer_t timer_;
 };
 
-static void stop_if_stop_path(enum inspector_handshake_event state,
-                              const std::string& path, bool* cont) {
-  *cont = path.empty() || path != "/close";
+#define SPIN_WHILE(condition)                                                  \
+  {                                                                            \
+    Timeout timeout(&loop);                                                    \
+    while ((condition) && !timeout.timed_out) {                                \
+      uv_run(&loop, UV_RUN_NOWAIT);                                            \
+    }                                                                          \
+    ASSERT_FALSE((condition));                                                 \
+  }
+
+enum inspector_handshake_event {
+  kInspectorHandshakeHttpGet,
+  kInspectorHandshakeUpgraded,
+  kInspectorHandshakeNoEvents
+};
+
+struct expectations {
+  std::string actual_data;
+  size_t actual_offset;
+  size_t actual_end;
+  int err_code;
+};
+
+static bool waiting_to_close = true;
+
+void handle_closed(uv_handle_t* handle) {
+  waiting_to_close = false;
 }
 
-static bool connected_cb(InspectorSocket* socket,
-                         enum inspector_handshake_event state,
-                         const std::string& path) {
-  inspector_ready = state == kInspectorHandshakeUpgraded;
-  last_event = state;
+static void really_close(uv_handle_t* handle) {
+  waiting_to_close = true;
+  if (!uv_is_closing(handle)) {
+    uv_close(handle, handle_closed);
+    SPIN_WHILE(waiting_to_close);
+  }
+}
+
+static void buffer_alloc_cb(uv_handle_t* stream, size_t len, uv_buf_t* buf) {
+  buf->base = new char[len];
+  buf->len = len;
+}
+
+class TestInspectorDelegate;
+
+static TestInspectorDelegate* delegate = nullptr;
+
+// Gtest asserts can't be used in dtor directly.
+static void assert_is_delegate(TestInspectorDelegate* d) {
+  GTEST_ASSERT_EQ(delegate, d);
+}
+
+class TestInspectorDelegate : public InspectorSocket::Delegate {
+ public:
+  using delegate_fn = void(*)(inspector_handshake_event, const std::string&,
+                              bool* should_continue);
+
+  TestInspectorDelegate() : inspector_ready(false),
+                            last_event(kInspectorHandshakeNoEvents),
+                            handshake_events(0),
+                            handshake_delegate_(stop_if_stop_path),
+                            fail_on_ws_frame_(false) { }
+
+  ~TestInspectorDelegate() {
+    assert_is_delegate(this);
+    delegate = nullptr;
+  }
+
+  void OnHttpGet(const std::string& path) override {
+    process(kInspectorHandshakeHttpGet, path);
+  }
+
+  void OnSocketUpgrade(const std::string& path,
+                       const std::string& ws_key) override {
+    ws_key_ = ws_key;
+    process(kInspectorHandshakeUpgraded, path);
+  }
+
+  void OnWsFrame(const std::vector<char>& buffer) override {
+    ASSERT_FALSE(fail_on_ws_frame_);
+    frames.push(buffer);
+  }
+
+  void SetDelegate(delegate_fn d) {
+    handshake_delegate_ = d;
+  }
+
+  void SetInspector(InspectorSocket::Pointer inspector) {
+    socket_ = std::move(inspector);
+  }
+
+  void Write(const char* buf, size_t len) {
+    socket_->Write(buf, len);
+  }
+
+  void ExpectReadError() {
+    SPIN_WHILE(frames.empty() || !frames.back().empty());
+  }
+
+  void ExpectData(const char* data, size_t len) {
+    const char* cur = data;
+    const char* end = data + len;
+    while (cur < end) {
+      SPIN_WHILE(frames.empty());
+      const std::vector<char>& frame = frames.front();
+      EXPECT_FALSE(frame.empty());
+      auto c = frame.begin();
+      for (; c < frame.end() && cur < end; c++) {
+        GTEST_ASSERT_EQ(*cur, *c) << "Character #" << cur - data;
+        cur = cur + 1;
+      }
+      EXPECT_EQ(c, frame.end());
+      frames.pop();
+    }
+  }
+
+  void FailOnWsFrame() {
+    fail_on_ws_frame_ = true;
+  }
+
+  void WaitForDispose() {
+    SPIN_WHILE(delegate != nullptr);
+  }
+
+  void Close() {
+    socket_.reset();
+  }
+
+  bool inspector_ready;
+  std::string last_path;  // NOLINT(runtime/string)
+  inspector_handshake_event last_event;
+  int handshake_events;
+  std::queue<std::vector<char>> frames;
+
+ private:
+  static void stop_if_stop_path(enum inspector_handshake_event state,
+                              const std::string& path, bool* cont) {
+    *cont = path.empty() || path != "/close";
+  }
+
+  void process(inspector_handshake_event event, const std::string& path);
+
+  bool disposed_ = false;
+  delegate_fn handshake_delegate_;
+  InspectorSocket::Pointer socket_;
+  std::string ws_key_;
+  bool fail_on_ws_frame_;
+};
+
+static bool connected = false;
+static uv_tcp_t server, client_socket;
+static const char SERVER_CLOSE_FRAME[] = {'\x88', '\x00'};
+
+struct read_expects {
+  const char* expected;
+  size_t expected_len;
+  size_t pos;
+  bool read_expected;
+  bool callback_called;
+};
+
+static const char HANDSHAKE_REQ[] = "GET /ws/path HTTP/1.1\r\n"
+                                    "Host: localhost:9222\r\n"
+                                    "Upgrade: websocket\r\n"
+                                    "Connection: Upgrade\r\n"
+                                    "Sec-WebSocket-Key: aaa==\r\n"
+                                    "Sec-WebSocket-Version: 13\r\n\r\n";
+
+void TestInspectorDelegate::process(inspector_handshake_event event,
+                                    const std::string& path) {
+  inspector_ready = event == kInspectorHandshakeUpgraded;
+  last_event = event;
   if (path.empty()) {
     last_path = "@@@ Nothing received @@@";
   } else {
@@ -103,15 +223,23 @@ static bool connected_cb(InspectorSocket* socket,
   }
   handshake_events++;
   bool should_continue = true;
-  handshake_delegate(state, path, &should_continue);
-  return should_continue;
+  handshake_delegate_(event, path, &should_continue);
+  if (should_continue) {
+    if (inspector_ready)
+      socket_->AcceptUpgrade(ws_key_);
+  } else {
+    socket_->CancelHandshake();
+  }
 }
 
 static void on_new_connection(uv_stream_t* server, int status) {
   GTEST_ASSERT_EQ(0, status);
   connected = true;
-  inspector_accept(server, static_cast<InspectorSocket*>(server->data),
-                   connected_cb);
+  delegate = new TestInspectorDelegate();
+  delegate->SetInspector(
+      InspectorSocket::Accept(server,
+                              InspectorSocket::DelegatePointer(delegate)));
+  GTEST_ASSERT_NE(nullptr, delegate);
 }
 
 void write_done(uv_write_t* req, int status) { req->data = nullptr; }
@@ -129,11 +257,6 @@ static void do_write(const char* data, int len) {
   SPIN_WHILE(req.data);
 }
 
-static void buffer_alloc_cb(uv_handle_t* stream, size_t len, uv_buf_t* buf) {
-  buf->base = new char[len];
-  buf->len = len;
-}
-
 static void check_data_cb(read_expects* expectation, ssize_t nread,
                           const uv_buf_t* buf, bool* retval) {
   *retval = false;
@@ -207,102 +330,6 @@ static void expect_on_client(const char* data, size_t len) {
   SPIN_WHILE(!expectation.read_expected);
 }
 
-struct expectations {
-  std::string actual_data;
-  size_t actual_offset;
-  size_t actual_end;
-  int err_code;
-};
-
-static void grow_expects_buffer(uv_handle_t* stream, size_t size, uv_buf_t* b) {
-  expectations* expects = static_cast<expectations*>(
-      inspector_from_stream(stream)->data);
-  size_t end = expects->actual_end;
-  // Grow the buffer in chunks of 64k.
-  size_t new_length = (end + size + 65535) & ~((size_t) 0xFFFF);
-  expects->actual_data.resize(new_length);
-  *b = uv_buf_init(&expects->actual_data[end], new_length - end);
-}
-
-// static void dump_hex(const char* buf, size_t len) {
-//   const char* ptr = buf;
-//   const char* end = ptr + len;
-//   const char* cptr;
-//   char c;
-//   int i;
-
-//   while (ptr < end) {
-//     cptr = ptr;
-//     for (i = 0; i < 16 && ptr < end; i++) {
-//       printf("%2.2X  ", *(ptr++));
-//     }
-//     for (i = 72 - (i * 4); i > 0; i--) {
-//       printf(" ");
-//     }
-//     for (i = 0; i < 16 && cptr < end; i++) {
-//       c = *(cptr++);
-//       printf("%c", (c > 0x19) ? c : '.');
-//     }
-//     printf("\n");
-//   }
-//   printf("\n\n");
-// }
-
-static void save_read_data(uv_stream_t* stream, ssize_t nread,
-                           const uv_buf_t* buf) {
-  expectations* expects = static_cast<expectations*>(
-      inspector_from_stream(stream)->data);
-  expects->err_code = nread < 0 ? nread : 0;
-  if (nread > 0) {
-    expects->actual_end += nread;
-  }
-}
-
-static void setup_inspector_expecting() {
-  if (inspector.data) {
-    return;
-  }
-  expectations* expects = new expectations();
-  inspector.data = expects;
-  inspector_read_start(&inspector, grow_expects_buffer, save_read_data);
-}
-
-static void expect_on_server(const char* data, size_t len) {
-  setup_inspector_expecting();
-  expectations* expects = static_cast<expectations*>(inspector.data);
-  for (size_t i = 0; i < len;) {
-    SPIN_WHILE(expects->actual_offset == expects->actual_end);
-    for (; i < len && expects->actual_offset < expects->actual_end; i++) {
-      char actual = expects->actual_data[expects->actual_offset++];
-      char expected = data[i];
-      if (expected != actual) {
-        fprintf(stderr, "Character %zu:\n", i);
-        GTEST_ASSERT_EQ(expected, actual);
-      }
-    }
-  }
-  expects->actual_end -= expects->actual_offset;
-  if (!expects->actual_end) {
-    memmove(&expects->actual_data[0],
-            &expects->actual_data[expects->actual_offset],
-            expects->actual_end);
-  }
-  expects->actual_offset = 0;
-}
-
-static void inspector_record_error_code(uv_stream_t* stream, ssize_t nread,
-                                        const uv_buf_t* buf) {
-  InspectorSocket *inspector = inspector_from_stream(stream);
-  // Increment instead of assign is to ensure the function is only called once
-  *(static_cast<int*>(inspector->data)) += nread;
-}
-
-static void expect_server_read_error() {
-  setup_inspector_expecting();
-  expectations* expects = static_cast<expectations*>(inspector.data);
-  SPIN_WHILE(expects->err_code != UV_EPROTO);
-}
-
 static void expect_handshake() {
   const char UPGRADE_RESPONSE[] =
       "HTTP/1.1 101 Switching Protocols\r\n"
@@ -320,35 +347,6 @@ static void expect_handshake_failure() {
   expect_on_client(UPGRADE_RESPONSE, sizeof(UPGRADE_RESPONSE) - 1);
 }
 
-static bool waiting_to_close = true;
-
-void handle_closed(uv_handle_t* handle) { waiting_to_close = false; }
-
-static void really_close(uv_handle_t* handle) {
-  waiting_to_close = true;
-  if (!uv_is_closing(handle)) {
-    uv_close(handle, handle_closed);
-    SPIN_WHILE(waiting_to_close);
-  }
-}
-
-// Called when the test leaves inspector socket in active state
-static void manual_inspector_socket_cleanup() {
-  EXPECT_EQ(0, uv_is_active(
-                   reinterpret_cast<uv_handle_t*>(&inspector.tcp)));
-  really_close(reinterpret_cast<uv_handle_t*>(&inspector.tcp));
-  delete inspector.ws_state;
-  inspector.ws_state = nullptr;
-  delete inspector.http_parsing_state;
-  inspector.http_parsing_state = nullptr;
-  inspector.buffer.clear();
-}
-
-static void assert_both_sockets_closed() {
-  SPIN_WHILE(uv_is_active(reinterpret_cast<uv_handle_t*>(&client_socket)));
-  SPIN_WHILE(uv_is_active(reinterpret_cast<uv_handle_t*>(&inspector.tcp)));
-}
-
 static void on_connection(uv_connect_t* connect, int status) {
   GTEST_ASSERT_EQ(0, status);
   connect->data = connect;
@@ -357,16 +355,10 @@ static void on_connection(uv_connect_t* connect, int status) {
 class InspectorSocketTest : public ::testing::Test {
  protected:
   virtual void SetUp() {
-    inspector.reinit();
-    handshake_delegate = stop_if_stop_path;
-    handshake_events = 0;
     connected = false;
-    inspector_ready = false;
-    last_event = kInspectorHandshakeHttpGet;
     GTEST_ASSERT_EQ(0, uv_loop_init(&loop));
     server = uv_tcp_t();
     client_socket = uv_tcp_t();
-    server.data = &inspector;
     sockaddr_in addr;
     uv_tcp_init(&loop, &server);
     uv_tcp_init(&loop, &client_socket);
@@ -386,13 +378,7 @@ class InspectorSocketTest : public ::testing::Test {
 
   virtual void TearDown() {
     really_close(reinterpret_cast<uv_handle_t*>(&client_socket));
-    EXPECT_TRUE(inspector.buffer.empty());
-    expectations* expects = static_cast<expectations*>(inspector.data);
-    if (expects != nullptr) {
-      GTEST_ASSERT_EQ(expects->actual_end, expects->actual_offset);
-      delete expects;
-      inspector.data = nullptr;
-    }
+    SPIN_WHILE(delegate != nullptr);
     const int err = uv_loop_close(&loop);
     if (err != 0) {
       uv_print_all_handles(&loop, stderr);
@@ -403,22 +389,22 @@ class InspectorSocketTest : public ::testing::Test {
 
 TEST_F(InspectorSocketTest, ReadsAndWritesInspectorMessage) {
   ASSERT_TRUE(connected);
-  ASSERT_FALSE(inspector_ready);
+  ASSERT_FALSE(delegate->inspector_ready);
   do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
-  SPIN_WHILE(!inspector_ready);
+  SPIN_WHILE(!delegate->inspector_ready);
   expect_handshake();
 
   // 2. Brief exchange
   const char SERVER_MESSAGE[] = "abcd";
   const char CLIENT_FRAME[] = {'\x81', '\x04', 'a', 'b', 'c', 'd'};
-  inspector_write(&inspector, SERVER_MESSAGE, sizeof(SERVER_MESSAGE) - 1);
+  delegate->Write(SERVER_MESSAGE, sizeof(SERVER_MESSAGE) - 1);
   expect_on_client(CLIENT_FRAME, sizeof(CLIENT_FRAME));
 
   const char SERVER_FRAME[] = {'\x81', '\x84', '\x7F', '\xC2', '\x66',
                                '\x31', '\x4E', '\xF0', '\x55', '\x05'};
   const char CLIENT_MESSAGE[] = "1234";
   do_write(SERVER_FRAME, sizeof(SERVER_FRAME));
-  expect_on_server(CLIENT_MESSAGE, sizeof(CLIENT_MESSAGE) - 1);
+  delegate->ExpectData(CLIENT_MESSAGE, sizeof(CLIENT_MESSAGE) - 1);
 
   // 3. Close
   const char CLIENT_CLOSE_FRAME[] = {'\x88', '\x80', '\x2D',
@@ -487,53 +473,34 @@ TEST_F(InspectorSocketTest, BufferEdgeCases) {
       "{\"id\":17,\"method\":\"Network.canEmulateNetworkConditions\"}"};
 
   do_write(MULTIPLE_REQUESTS, sizeof(MULTIPLE_REQUESTS));
-  expect_on_server(EXPECT, sizeof(EXPECT) - 1);
-  inspector_read_stop(&inspector);
-  manual_inspector_socket_cleanup();
+  delegate->ExpectData(EXPECT, sizeof(EXPECT) - 1);
 }
 
 TEST_F(InspectorSocketTest, AcceptsRequestInSeveralWrites) {
   ASSERT_TRUE(connected);
-  ASSERT_FALSE(inspector_ready);
+  ASSERT_FALSE(delegate->inspector_ready);
   // Specifically, break up the request in the "Sec-WebSocket-Key" header name
   // and value
   const int write1 = 95;
   const int write2 = 5;
   const int write3 = sizeof(HANDSHAKE_REQ) - write1 - write2 - 1;
   do_write(const_cast<char*>(HANDSHAKE_REQ), write1);
-  ASSERT_FALSE(inspector_ready);
+  ASSERT_FALSE(delegate->inspector_ready);
   do_write(const_cast<char*>(HANDSHAKE_REQ) + write1, write2);
-  ASSERT_FALSE(inspector_ready);
+  ASSERT_FALSE(delegate->inspector_ready);
   do_write(const_cast<char*>(HANDSHAKE_REQ) + write1 + write2, write3);
-  SPIN_WHILE(!inspector_ready);
+  SPIN_WHILE(!delegate->inspector_ready);
   expect_handshake();
-  inspector_read_stop(&inspector);
   GTEST_ASSERT_EQ(uv_is_active(reinterpret_cast<uv_handle_t*>(&client_socket)),
                   0);
-  manual_inspector_socket_cleanup();
 }
 
 TEST_F(InspectorSocketTest, ExtraTextBeforeRequest) {
-  last_event = kInspectorHandshakeUpgraded;
-  char UNCOOL_BRO[] = "Uncool, bro: Text before the first req\r\n";
-  do_write(const_cast<char*>(UNCOOL_BRO), sizeof(UNCOOL_BRO) - 1);
-
-  ASSERT_FALSE(inspector_ready);
-  do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
-  SPIN_WHILE(last_event != kInspectorHandshakeFailed);
-  expect_handshake_failure();
-  assert_both_sockets_closed();
-}
-
-TEST_F(InspectorSocketTest, ExtraLettersBeforeRequest) {
-  char UNCOOL_BRO[] = "Uncool!!";
+  delegate->last_event = kInspectorHandshakeUpgraded;
+  char UNCOOL_BRO[] = "Text before the first req, shouldn't be her\r\n";
   do_write(const_cast<char*>(UNCOOL_BRO), sizeof(UNCOOL_BRO) - 1);
-
-  ASSERT_FALSE(inspector_ready);
-  do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
-  SPIN_WHILE(last_event != kInspectorHandshakeFailed);
   expect_handshake_failure();
-  assert_both_sockets_closed();
+  GTEST_ASSERT_EQ(nullptr, delegate);
 }
 
 TEST_F(InspectorSocketTest, RequestWithoutKey) {
@@ -544,87 +511,65 @@ TEST_F(InspectorSocketTest, RequestWithoutKey) {
                                 "Sec-WebSocket-Version: 13\r\n\r\n";
 
   do_write(const_cast<char*>(BROKEN_REQUEST), sizeof(BROKEN_REQUEST) - 1);
-  SPIN_WHILE(last_event != kInspectorHandshakeFailed);
   expect_handshake_failure();
-  assert_both_sockets_closed();
 }
 
 TEST_F(InspectorSocketTest, KillsConnectionOnProtocolViolation) {
   ASSERT_TRUE(connected);
-  ASSERT_FALSE(inspector_ready);
+  ASSERT_FALSE(delegate->inspector_ready);
   do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
-  SPIN_WHILE(!inspector_ready);
-  ASSERT_TRUE(inspector_ready);
+  SPIN_WHILE(!delegate->inspector_ready);
+  ASSERT_TRUE(delegate->inspector_ready);
   expect_handshake();
   const char SERVER_FRAME[] = "I'm not a good WS frame. Nope!";
   do_write(SERVER_FRAME, sizeof(SERVER_FRAME));
-  expect_server_read_error();
+  SPIN_WHILE(delegate != nullptr);
   GTEST_ASSERT_EQ(uv_is_active(reinterpret_cast<uv_handle_t*>(&client_socket)),
                   0);
 }
 
 TEST_F(InspectorSocketTest, CanStopReadingFromInspector) {
   ASSERT_TRUE(connected);
-  ASSERT_FALSE(inspector_ready);
+  ASSERT_FALSE(delegate->inspector_ready);
   do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
   expect_handshake();
-  ASSERT_TRUE(inspector_ready);
+  ASSERT_TRUE(delegate->inspector_ready);
 
   // 2. Brief exchange
   const char SERVER_FRAME[] = {'\x81', '\x84', '\x7F', '\xC2', '\x66',
                                '\x31', '\x4E', '\xF0', '\x55', '\x05'};
   const char CLIENT_MESSAGE[] = "1234";
   do_write(SERVER_FRAME, sizeof(SERVER_FRAME));
-  expect_on_server(CLIENT_MESSAGE, sizeof(CLIENT_MESSAGE) - 1);
+  delegate->ExpectData(CLIENT_MESSAGE, sizeof(CLIENT_MESSAGE) - 1);
 
-  inspector_read_stop(&inspector);
   do_write(SERVER_FRAME, sizeof(SERVER_FRAME));
   GTEST_ASSERT_EQ(uv_is_active(
                       reinterpret_cast<uv_handle_t*>(&client_socket)), 0);
-  manual_inspector_socket_cleanup();
-}
-
-static int inspector_closed = 0;
-
-void inspector_closed_cb(InspectorSocket *inspector, int code) {
-  inspector_closed++;
 }
 
 TEST_F(InspectorSocketTest, CloseDoesNotNotifyReadCallback) {
-  inspector_closed = 0;
   do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
   expect_handshake();
 
-  int error_code = 0;
-  inspector.data = &error_code;
-  inspector_read_start(&inspector, buffer_alloc_cb,
-                       inspector_record_error_code);
-  inspector_close(&inspector, inspector_closed_cb);
+  delegate->Close();
   char CLOSE_FRAME[] = {'\x88', '\x00'};
   expect_on_client(CLOSE_FRAME, sizeof(CLOSE_FRAME));
-  EXPECT_EQ(0, inspector_closed);
   const char CLIENT_CLOSE_FRAME[] = {'\x88', '\x80', '\x2D',
                                      '\x0E', '\x1E', '\xFA'};
+  delegate->FailOnWsFrame();
   do_write(CLIENT_CLOSE_FRAME, sizeof(CLIENT_CLOSE_FRAME));
-  EXPECT_NE(UV_EOF, error_code);
-  SPIN_WHILE(inspector_closed == 0);
-  EXPECT_EQ(1, inspector_closed);
-  inspector.data = nullptr;
+  SPIN_WHILE(delegate != nullptr);
 }
 
 TEST_F(InspectorSocketTest, CloseWorksWithoutReadEnabled) {
-  inspector_closed = 0;
   do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
   expect_handshake();
-  inspector_close(&inspector, inspector_closed_cb);
+  delegate->Close();
   char CLOSE_FRAME[] = {'\x88', '\x00'};
   expect_on_client(CLOSE_FRAME, sizeof(CLOSE_FRAME));
-  EXPECT_EQ(0, inspector_closed);
   const char CLIENT_CLOSE_FRAME[] = {'\x88', '\x80', '\x2D',
                                      '\x0E', '\x1E', '\xFA'};
   do_write(CLIENT_CLOSE_FRAME, sizeof(CLIENT_CLOSE_FRAME));
-  SPIN_WHILE(inspector_closed == 0);
-  EXPECT_EQ(1, inspector_closed);
 }
 
 // Make sure buffering works
@@ -641,26 +586,24 @@ static void send_in_chunks(const char* data, size_t len) {
 }
 
 static const char TEST_SUCCESS[] = "Test Success\n\n";
+static int ReportsHttpGet_eventsCount = 0;
 
 static void ReportsHttpGet_handshake(enum inspector_handshake_event state,
                                      const std::string& path, bool* cont) {
   *cont = true;
   enum inspector_handshake_event expected_state = kInspectorHandshakeHttpGet;
   std::string expected_path;
-  switch (handshake_events) {
+  switch (delegate->handshake_events) {
   case 1:
     expected_path = "/some/path";
     break;
   case 2:
     expected_path = "/respond/withtext";
-    inspector_write(&inspector, TEST_SUCCESS, sizeof(TEST_SUCCESS) - 1);
+    delegate->Write(TEST_SUCCESS, sizeof(TEST_SUCCESS) - 1);
     break;
   case 3:
     expected_path = "/some/path2";
     break;
-  case 5:
-    expected_state = kInspectorHandshakeFailed;
-    break;
   case 4:
     expected_path = "/close";
     *cont = false;
@@ -670,10 +613,11 @@ static void ReportsHttpGet_handshake(enum inspector_handshake_event state,
   }
   EXPECT_EQ(expected_state, state);
   EXPECT_EQ(expected_path, path);
+  ReportsHttpGet_eventsCount = delegate->handshake_events;
 }
 
 TEST_F(InspectorSocketTest, ReportsHttpGet) {
-  handshake_delegate = ReportsHttpGet_handshake;
+  delegate->SetDelegate(ReportsHttpGet_handshake);
 
   const char GET_REQ[] = "GET /some/path HTTP/1.1\r\n"
                          "Host: localhost:9222\r\n"
@@ -688,7 +632,6 @@ TEST_F(InspectorSocketTest, ReportsHttpGet) {
   send_in_chunks(WRITE_REQUEST, sizeof(WRITE_REQUEST) - 1);
 
   expect_on_client(TEST_SUCCESS, sizeof(TEST_SUCCESS) - 1);
-
   const char GET_REQS[] = "GET /some/path2 HTTP/1.1\r\n"
                           "Host: localhost:9222\r\n"
                           "Sec-WebSocket-Key: aaa==\r\n"
@@ -698,53 +641,50 @@ TEST_F(InspectorSocketTest, ReportsHttpGet) {
                           "Sec-WebSocket-Key: aaa==\r\n"
                           "Sec-WebSocket-Version: 13\r\n\r\n";
   send_in_chunks(GET_REQS, sizeof(GET_REQS) - 1);
-
   expect_handshake_failure();
-  EXPECT_EQ(5, handshake_events);
+  EXPECT_EQ(4, ReportsHttpGet_eventsCount);
+  EXPECT_EQ(nullptr, delegate);
 }
 
-static void
-HandshakeCanBeCanceled_handshake(enum inspector_handshake_event state,
-                                 const std::string& path, bool* cont) {
-  switch (handshake_events - 1) {
+static int HandshakeCanBeCanceled_eventCount = 0;
+
+static
+void HandshakeCanBeCanceled_handshake(enum inspector_handshake_event state,
+                                      const std::string& path, bool* cont) {
+  switch (delegate->handshake_events - 1) {
   case 0:
-    EXPECT_EQ(kInspectorHandshakeUpgrading, state);
+    EXPECT_EQ(kInspectorHandshakeUpgraded, state);
     EXPECT_EQ("/ws/path", path);
     break;
-  case 1:
-    EXPECT_EQ(kInspectorHandshakeFailed, state);
-    EXPECT_TRUE(path.empty());
-    break;
   default:
     EXPECT_TRUE(false);
     break;
   }
   *cont = false;
+  HandshakeCanBeCanceled_eventCount = delegate->handshake_events;
 }
 
 TEST_F(InspectorSocketTest, HandshakeCanBeCanceled) {
-  handshake_delegate = HandshakeCanBeCanceled_handshake;
+  delegate->SetDelegate(HandshakeCanBeCanceled_handshake);
 
   do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
 
   expect_handshake_failure();
-  EXPECT_EQ(2, handshake_events);
+  EXPECT_EQ(1, HandshakeCanBeCanceled_eventCount);
+  EXPECT_EQ(nullptr, delegate);
 }
 
 static void GetThenHandshake_handshake(enum inspector_handshake_event state,
                                        const std::string& path, bool* cont) {
   *cont = true;
   std::string expected_path = "/ws/path";
-  switch (handshake_events - 1) {
+  switch (delegate->handshake_events - 1) {
   case 0:
     EXPECT_EQ(kInspectorHandshakeHttpGet, state);
     expected_path = "/respond/withtext";
-    inspector_write(&inspector, TEST_SUCCESS, sizeof(TEST_SUCCESS) - 1);
+    delegate->Write(TEST_SUCCESS, sizeof(TEST_SUCCESS) - 1);
     break;
   case 1:
-    EXPECT_EQ(kInspectorHandshakeUpgrading, state);
-    break;
-  case 2:
     EXPECT_EQ(kInspectorHandshakeUpgraded, state);
     break;
   default:
@@ -755,7 +695,7 @@ static void GetThenHandshake_handshake(enum inspector_handshake_event state,
 }
 
 TEST_F(InspectorSocketTest, GetThenHandshake) {
-  handshake_delegate = GetThenHandshake_handshake;
+  delegate->SetDelegate(GetThenHandshake_handshake);
   const char WRITE_REQUEST[] = "GET /respond/withtext HTTP/1.1\r\n"
                                "Host: localhost:9222\r\n\r\n";
   send_in_chunks(WRITE_REQUEST, sizeof(WRITE_REQUEST) - 1);
@@ -764,15 +704,7 @@ TEST_F(InspectorSocketTest, GetThenHandshake) {
 
   do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
   expect_handshake();
-  EXPECT_EQ(3, handshake_events);
-  manual_inspector_socket_cleanup();
-}
-
-static void WriteBeforeHandshake_inspector_delegate(inspector_handshake_event e,
-                                                    const std::string& path,
-                                                    bool* cont) {
-  if (e == kInspectorHandshakeFailed)
-    inspector_closed = 1;
+  EXPECT_EQ(2, delegate->handshake_events);
 }
 
 TEST_F(InspectorSocketTest, WriteBeforeHandshake) {
@@ -780,51 +712,31 @@ TEST_F(InspectorSocketTest, WriteBeforeHandshake) {
   const char MESSAGE2[] = "Message 2";
   const char EXPECTED[] = "Message 1Message 2";
 
-  inspector_write(&inspector, MESSAGE1, sizeof(MESSAGE1) - 1);
-  inspector_write(&inspector, MESSAGE2, sizeof(MESSAGE2) - 1);
+  delegate->Write(MESSAGE1, sizeof(MESSAGE1) - 1);
+  delegate->Write(MESSAGE2, sizeof(MESSAGE2) - 1);
   expect_on_client(EXPECTED, sizeof(EXPECTED) - 1);
-  inspector_closed = 0;
-  handshake_delegate = WriteBeforeHandshake_inspector_delegate;
   really_close(reinterpret_cast<uv_handle_t*>(&client_socket));
-  SPIN_WHILE(inspector_closed == 0);
-}
-
-static void CleanupSocketAfterEOF_close_cb(InspectorSocket* inspector,
-                                           int status) {
-  *(static_cast<bool*>(inspector->data)) = true;
-}
-
-static void CleanupSocketAfterEOF_read_cb(uv_stream_t* stream, ssize_t nread,
-                                          const uv_buf_t* buf) {
-  EXPECT_EQ(UV_EOF, nread);
-  InspectorSocket* insp = inspector_from_stream(stream);
-  inspector_close(insp, CleanupSocketAfterEOF_close_cb);
+  SPIN_WHILE(delegate != nullptr);
 }
 
 TEST_F(InspectorSocketTest, CleanupSocketAfterEOF) {
   do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
   expect_handshake();
 
-  inspector_read_start(&inspector, buffer_alloc_cb,
-                       CleanupSocketAfterEOF_read_cb);
-
   for (int i = 0; i < MAX_LOOP_ITERATIONS; ++i) {
     uv_run(&loop, UV_RUN_NOWAIT);
   }
 
   uv_close(reinterpret_cast<uv_handle_t*>(&client_socket), nullptr);
-  bool flag = false;
-  inspector.data = &flag;
-  SPIN_WHILE(!flag);
-  inspector.data = nullptr;
+  SPIN_WHILE(delegate != nullptr);
 }
 
 TEST_F(InspectorSocketTest, EOFBeforeHandshake) {
   const char MESSAGE[] = "We'll send EOF afterwards";
-  inspector_write(&inspector, MESSAGE, sizeof(MESSAGE) - 1);
+  delegate->Write(MESSAGE, sizeof(MESSAGE) - 1);
   expect_on_client(MESSAGE, sizeof(MESSAGE) - 1);
   uv_close(reinterpret_cast<uv_handle_t*>(&client_socket), nullptr);
-  SPIN_WHILE(last_event != kInspectorHandshakeFailed);
+  SPIN_WHILE(delegate != nullptr);
 }
 
 static void fill_message(std::string* buffer) {
@@ -843,9 +755,9 @@ static void mask_message(const std::string& message,
 
 TEST_F(InspectorSocketTest, Send1Mb) {
   ASSERT_TRUE(connected);
-  ASSERT_FALSE(inspector_ready);
+  ASSERT_FALSE(delegate->inspector_ready);
   do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
-  SPIN_WHILE(!inspector_ready);
+  SPIN_WHILE(!delegate->inspector_ready);
   expect_handshake();
 
   // 2. Brief exchange
@@ -860,7 +772,7 @@ TEST_F(InspectorSocketTest, Send1Mb) {
   std::string expected(EXPECTED_FRAME_HEADER, sizeof(EXPECTED_FRAME_HEADER));
   expected.append(message);
 
-  inspector_write(&inspector, &message[0], message.size());
+  delegate->Write(&message[0], message.size());
   expect_on_client(&expected[0], expected.size());
 
   char MASK[4] = {'W', 'h', 'O', 'a'};
@@ -874,9 +786,8 @@ TEST_F(InspectorSocketTest, Send1Mb) {
   outgoing.resize(outgoing.size() + message.size());
   mask_message(message, &outgoing[sizeof(FRAME_TO_SERVER_HEADER)], MASK);
 
-  setup_inspector_expecting();  // Buffer on the client side.
   do_write(&outgoing[0], outgoing.size());
-  expect_on_server(&message[0], message.size());
+  delegate->ExpectData(&message[0], message.size());
 
   // 3. Close
   const char CLIENT_CLOSE_FRAME[] = {'\x88', '\x80', '\x2D',
@@ -887,53 +798,33 @@ TEST_F(InspectorSocketTest, Send1Mb) {
                          reinterpret_cast<uv_handle_t*>(&client_socket)));
 }
 
-static ssize_t err;
-
-void ErrorCleansUpTheSocket_cb(uv_stream_t* stream, ssize_t read,
-                               const uv_buf_t* buf) {
-  err = read;
-}
-
 TEST_F(InspectorSocketTest, ErrorCleansUpTheSocket) {
-  inspector_closed = 0;
   do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
   expect_handshake();
   const char NOT_A_GOOD_FRAME[] = {'H', 'e', 'l', 'l', 'o'};
-  err = 42;
-  inspector_read_start(&inspector, buffer_alloc_cb,
-                       ErrorCleansUpTheSocket_cb);
   do_write(NOT_A_GOOD_FRAME, sizeof(NOT_A_GOOD_FRAME));
-  SPIN_WHILE(err > 0);
-  EXPECT_EQ(UV_EPROTO, err);
-}
-
-static void ServerClosedByClient_cb(InspectorSocket* socket, int code) {
-  *static_cast<bool*>(socket->data) = true;
+  SPIN_WHILE(delegate != nullptr);
 }
 
-TEST_F(InspectorSocketTest, NoCloseResponseFromClinet) {
+TEST_F(InspectorSocketTest, NoCloseResponseFromClient) {
   ASSERT_TRUE(connected);
-  ASSERT_FALSE(inspector_ready);
+  ASSERT_FALSE(delegate->inspector_ready);
   do_write(const_cast<char*>(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1);
-  SPIN_WHILE(!inspector_ready);
+  SPIN_WHILE(!delegate->inspector_ready);
   expect_handshake();
 
   // 2. Brief exchange
   const char SERVER_MESSAGE[] = "abcd";
   const char CLIENT_FRAME[] = {'\x81', '\x04', 'a', 'b', 'c', 'd'};
-  inspector_write(&inspector, SERVER_MESSAGE, sizeof(SERVER_MESSAGE) - 1);
+  delegate->Write(SERVER_MESSAGE, sizeof(SERVER_MESSAGE) - 1);
   expect_on_client(CLIENT_FRAME, sizeof(CLIENT_FRAME));
 
-  bool closed = false;
-
-  inspector.data = &closed;
-  inspector_close(&inspector, ServerClosedByClient_cb);
+  delegate->Close();
   expect_on_client(SERVER_CLOSE_FRAME, sizeof(SERVER_CLOSE_FRAME));
   uv_close(reinterpret_cast<uv_handle_t*>(&client_socket), nullptr);
-  SPIN_WHILE(!closed);
-  inspector.data = nullptr;
   GTEST_ASSERT_EQ(0, uv_is_active(
-                         reinterpret_cast<uv_handle_t*>(&client_socket)));
+                  reinterpret_cast<uv_handle_t*>(&client_socket)));
+  delegate->WaitForDispose();
 }
 
 }  // anonymous namespace
diff --git a/test/cctest/test_inspector_socket_server.cc b/test/cctest/test_inspector_socket_server.cc
index ab74917234..49a3ca9f95 100644
--- a/test/cctest/test_inspector_socket_server.cc
+++ b/test/cctest/test_inspector_socket_server.cc
@@ -95,16 +95,17 @@ class TestInspectorServerDelegate : public SocketServerDelegate {
     server_ = server;
   }
 
-  bool StartSession(int session_id, const std::string& target_id) override {
+  void StartSession(int session_id, const std::string& target_id) override {
     buffer_.clear();
     CHECK_NE(targets_.end(),
              std::find(targets_.begin(), targets_.end(), target_id));
     if (target_id == UNCONNECTABLE_TARGET_ID) {
-      return false;
+      server_->DeclineSession(session_id);
+      return;
     }
     connected++;
     session_id_ = session_id;
-    return true;
+    server_->AcceptSession(session_id);
   }
 
   void MessageReceived(int session_id, const std::string& message) override {
@@ -350,12 +351,13 @@ class ServerHolder {
 
 class ServerDelegateNoTargets : public SocketServerDelegate {
  public:
+  ServerDelegateNoTargets() : server_(nullptr) { }
   void Connect(InspectorSocketServer* server) { }
   void MessageReceived(int session_id, const std::string& message) override { }
   void EndSession(int session_id) override { }
 
-  bool StartSession(int session_id, const std::string& target_id) override {
-    return false;
+  void StartSession(int session_id, const std::string& target_id) override {
+    server_->DeclineSession(session_id);
   }
 
   std::vector<std::string> GetTargetIds() override {
@@ -375,6 +377,9 @@ class ServerDelegateNoTargets : public SocketServerDelegate {
   }
 
   bool done = false;
+
+ private:
+  InspectorSocketServer* server_;
 };
 
 static void TestHttpRequest(int port, const std::string& path,
@@ -407,7 +412,6 @@ TEST_F(InspectorSocketServerTest, InspectorSessions) {
   well_behaved_socket.Write(WsHandshakeRequest(MAIN_TARGET_ID));
   well_behaved_socket.Expect(WS_HANDSHAKE_RESPONSE);
 
-
   EXPECT_EQ(1, delegate.connected);
 
   well_behaved_socket.Write("\x81\x84\x7F\xC2\x66\x31\x4E\xF0\x55\x05");
@@ -416,7 +420,6 @@ TEST_F(InspectorSocketServerTest, InspectorSessions) {
   delegate.Write("5678");
 
   well_behaved_socket.Expect("\x81\x4" "5678");
-
   well_behaved_socket.Write(CLIENT_CLOSE_FRAME);
   well_behaved_socket.Expect(SERVER_CLOSE_FRAME);