Create a detachable core for the TCPClientSocketWin object

so that part of the socket can outlive the socket destruction.

This is the second part of removing infinite waits from
the IO loop.

BUG=9258
TEST=none

Review URL: http://codereview.chromium.org/113291

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@15895 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 197 insertions, 132 deletions

diff --git a/net/base/tcp_client_socket_win.cc b/net/base/tcp_client_socket_win.cc
index 70da428..77df3ff 100644
--- a/net/base/tcp_client_socket_win.cc
+++ b/net/base/tcp_client_socket_win.cc
@@ -74,6 +74,135 @@ int MapWinsockError(DWORD err) {
 
 //-----------------------------------------------------------------------------
 
+// This class encapsulates all the state that has to be preserved as long as
+// there is a network IO operation in progress. If the owner TCPClientSocketWin
+// is destroyed while an operation is in progress, the Core is detached and it
+// lives until the operation completes and the OS doesn't reference any resource
+// declared on this class anymore.
+class TCPClientSocketWin::Core : public base::RefCounted<Core> {
+ public:
+  explicit Core(TCPClientSocketWin* socket);
+  ~Core();
+
+  // Start watching for the end of a read or write operation.
+  void WatchForRead();
+  void WatchForWrite();
+
+  // The TCPClientSocketWin is going away.
+  void Detach() { socket_ = NULL; }
+
+  // The separate OVERLAPPED variables for asynchronous operation.
+  // |read_overlapped_| is used for both Connect() and Read().
+  // |write_overlapped_| is only used for Write();
+  OVERLAPPED read_overlapped_;
+  OVERLAPPED write_overlapped_;
+
+  // The buffers used in Read() and Write().
+  WSABUF read_buffer_;
+  WSABUF write_buffer_;
+  scoped_refptr<IOBuffer> read_iobuffer_;
+  scoped_refptr<IOBuffer> write_iobuffer_;
+
+ private:
+  class ReadDelegate : public base::ObjectWatcher::Delegate {
+   public:
+    explicit ReadDelegate(Core* core) : core_(core) {}
+    virtual ~ReadDelegate() {}
+
+    // base::ObjectWatcher::Delegate methods:
+    virtual void OnObjectSignaled(HANDLE object);
+
+   private:
+    Core* const core_;
+  };
+
+  class WriteDelegate : public base::ObjectWatcher::Delegate {
+   public:
+    explicit WriteDelegate(Core* core) : core_(core) {}
+    virtual ~WriteDelegate() {}
+
+    // base::ObjectWatcher::Delegate methods:
+    virtual void OnObjectSignaled(HANDLE object);
+
+   private:
+    Core* const core_;
+  };
+
+  // The socket that created this object.
+  TCPClientSocketWin* socket_;
+
+  // |reader_| handles the signals from |read_watcher_|.
+  ReadDelegate reader_;
+  // |writer_| handles the signals from |write_watcher_|.
+  WriteDelegate writer_;
+
+  // |read_watcher_| watches for events from Connect() and Read().
+  base::ObjectWatcher read_watcher_;
+  // |write_watcher_| watches for events from Write();
+  base::ObjectWatcher write_watcher_;
+
+  DISALLOW_COPY_AND_ASSIGN(Core);
+};
+
+TCPClientSocketWin::Core::Core(
+    TCPClientSocketWin* socket)
+    : socket_(socket),
+      ALLOW_THIS_IN_INITIALIZER_LIST(reader_(this)),
+      ALLOW_THIS_IN_INITIALIZER_LIST(writer_(this)) {
+  memset(&read_overlapped_, 0, sizeof(read_overlapped_));
+  memset(&write_overlapped_, 0, sizeof(write_overlapped_));
+}
+
+TCPClientSocketWin::Core::~Core() {
+  // Make sure the message loop is not watching this object anymore.
+  read_watcher_.StopWatching();
+  write_watcher_.StopWatching();
+
+  WSACloseEvent(read_overlapped_.hEvent);
+  memset(&read_overlapped_, 0, sizeof(read_overlapped_));
+  WSACloseEvent(write_overlapped_.hEvent);
+  memset(&write_overlapped_, 0, sizeof(write_overlapped_));
+}
+
+void TCPClientSocketWin::Core::WatchForRead() {
+  // We grab an extra reference because there is an IO operation in progress.
+  // Balanced in ReadDelegate::OnObjectSignaled().
+  AddRef();
+  read_watcher_.StartWatching(read_overlapped_.hEvent, &reader_);
+}
+
+void TCPClientSocketWin::Core::WatchForWrite() {
+  // We grab an extra reference because there is an IO operation in progress.
+  // Balanced in WriteDelegate::OnObjectSignaled().
+  AddRef();
+  write_watcher_.StartWatching(write_overlapped_.hEvent, &writer_);
+}
+
+void TCPClientSocketWin::Core::ReadDelegate::OnObjectSignaled(
+    HANDLE object) {
+  DCHECK_EQ(object, core_->read_overlapped_.hEvent);
+  if (core_->socket_) {
+    if (core_->socket_->waiting_connect_) {
+      core_->socket_->DidCompleteConnect();
+    } else {
+      core_->socket_->DidCompleteRead();
+    }
+  }
+
+  core_->Release();
+}
+
+void TCPClientSocketWin::Core::WriteDelegate::OnObjectSignaled(
+    HANDLE object) {
+  DCHECK_EQ(object, core_->write_overlapped_.hEvent);
+  if (core_->socket_)
+    core_->socket_->DidCompleteWrite();
+
+  core_->Release();
+}
+
+//-----------------------------------------------------------------------------
+
 TCPClientSocketWin::TCPClientSocketWin(const AddressList& addresses)
     : socket_(INVALID_SOCKET),
       addresses_(addresses),
@@ -81,12 +210,8 @@ TCPClientSocketWin::TCPClientSocketWin(const AddressList& addresses)
       waiting_connect_(false),
       waiting_read_(false),
       waiting_write_(false),
-      ALLOW_THIS_IN_INITIALIZER_LIST(reader_(this)),
-      ALLOW_THIS_IN_INITIALIZER_LIST(writer_(this)),
       read_callback_(NULL),
       write_callback_(NULL) {
-  memset(&read_overlapped_, 0, sizeof(read_overlapped_));
-  memset(&write_overlapped_, 0, sizeof(write_overlapped_));
   EnsureWinsockInit();
 }
 
@@ -107,17 +232,20 @@ int TCPClientSocketWin::Connect(CompletionCallback* callback) {
   if (rv != OK)
     return rv;
 
+  DCHECK(!core_);
+  core_ = new Core(this);
+
   // WSACreateEvent creates a manual-reset event object.
-  read_overlapped_.hEvent = WSACreateEvent();
+  core_->read_overlapped_.hEvent = WSACreateEvent();
   // WSAEventSelect sets the socket to non-blocking mode as a side effect.
   // Our connect() and recv() calls require that the socket be non-blocking.
-  WSAEventSelect(socket_, read_overlapped_.hEvent, FD_CONNECT);
+  WSAEventSelect(socket_, core_->read_overlapped_.hEvent, FD_CONNECT);
 
-  write_overlapped_.hEvent = WSACreateEvent();
+  core_->write_overlapped_.hEvent = WSACreateEvent();
 
   if (!connect(socket_, ai->ai_addr, static_cast<int>(ai->ai_addrlen))) {
     // Connected without waiting!
-    WaitForAndResetEvent(read_overlapped_.hEvent);
+    WaitForAndResetEvent(core_->read_overlapped_.hEvent);
     TRACE_EVENT_END("socket.connect", this, "");
     return OK;
   }
@@ -128,7 +256,7 @@ int TCPClientSocketWin::Connect(CompletionCallback* callback) {
     return MapWinsockError(err);
   }
 
-  read_watcher_.StartWatching(read_overlapped_.hEvent, &reader_);
+  core_->WatchForRead();
   waiting_connect_ = true;
   read_callback_ = callback;
   return ERR_IO_PENDING;
@@ -140,10 +268,6 @@ void TCPClientSocketWin::Disconnect() {
 
   TRACE_EVENT_INSTANT("socket.disconnect", this, "");
 
-  // Make sure the message loop is not watching this object anymore.
-  read_watcher_.StopWatching();
-  write_watcher_.StopWatching();
-
   // Note: don't use CancelIo to cancel pending IO because it doesn't work
   // when there is a Winsock layered service provider.
 
@@ -157,33 +281,23 @@ void TCPClientSocketWin::Disconnect() {
   closesocket(socket_);
   socket_ = INVALID_SOCKET;
 
-  if (waiting_read_ || waiting_write_) {
-    base::TimeTicks start = base::TimeTicks::Now();
-
-    // Wait for pending IO to be aborted.
-    if (waiting_read_)
-      WaitForSingleObject(read_overlapped_.hEvent, INFINITE);
-    if (waiting_write_)
-      WaitForSingleObject(write_overlapped_.hEvent, INFINITE);
-
-    // We want to see if we block the message loop for too long.
-    UMA_HISTOGRAM_TIMES("AsyncIO.ClientSocketDisconnect",
-                        base::TimeTicks::Now() - start);
-  }
-
-  WSACloseEvent(read_overlapped_.hEvent);
-  memset(&read_overlapped_, 0, sizeof(read_overlapped_));
-  WSACloseEvent(write_overlapped_.hEvent);
-  memset(&write_overlapped_, 0, sizeof(write_overlapped_));
-
   // Reset for next time.
   current_ai_ = addresses_.head();
 
+  if (waiting_connect_) {
+    // We closed the socket, so this notification will never come.
+    // From MSDN' WSAEventSelect documentation:
+    // "Closing a socket with closesocket also cancels the association and
+    // selection of network events specified in WSAEventSelect for the socket".
+    core_->Release();
+  }
+
   waiting_read_ = false;
   waiting_write_ = false;
-  read_iobuffer_ = NULL;
-  write_iobuffer_ = NULL;
   waiting_connect_ = false;
+
+  core_->Detach();
+  core_ = NULL;
 }
 
 bool TCPClientSocketWin::IsConnected() const {
@@ -223,19 +337,19 @@ int TCPClientSocketWin::Read(IOBuffer* buf,
   DCHECK_NE(socket_, INVALID_SOCKET);
   DCHECK(!waiting_read_);
   DCHECK(!read_callback_);
-  DCHECK(!read_iobuffer_);
+  DCHECK(!core_->read_iobuffer_);
 
-  read_buffer_.len = buf_len;
-  read_buffer_.buf = buf->data();
+  core_->read_buffer_.len = buf_len;
+  core_->read_buffer_.buf = buf->data();
 
   TRACE_EVENT_BEGIN("socket.read", this, "");
   // TODO(wtc): Remove the CHECK after enough testing.
-  CHECK(WaitForSingleObject(read_overlapped_.hEvent, 0) == WAIT_TIMEOUT);
+  CHECK(WaitForSingleObject(core_->read_overlapped_.hEvent, 0) == WAIT_TIMEOUT);
   DWORD num, flags = 0;
-  int rv = WSARecv(
-      socket_, &read_buffer_, 1, &num, &flags, &read_overlapped_, NULL);
+  int rv = WSARecv(socket_, &core_->read_buffer_, 1, &num, &flags,
+                   &core_->read_overlapped_, NULL);
   if (rv == 0) {
-    WaitForAndResetEvent(read_overlapped_.hEvent);
+    WaitForAndResetEvent(core_->read_overlapped_.hEvent);
     TRACE_EVENT_END("socket.read", this, StringPrintf("%d bytes", num));
 
     // Because of how WSARecv fills memory when used asynchronously, Purify
@@ -244,15 +358,15 @@ int TCPClientSocketWin::Read(IOBuffer* buf,
     // individual bytes. We override that in PURIFY builds to avoid the false
     // error reports.
     // See bug 5297.
-    base::MemoryDebug::MarkAsInitialized(read_buffer_.buf, num);
+    base::MemoryDebug::MarkAsInitialized(core_->read_buffer_.buf, num);
     return static_cast<int>(num);
   }
   int err = WSAGetLastError();
   if (err == WSA_IO_PENDING) {
-    read_watcher_.StartWatching(read_overlapped_.hEvent, &reader_);
+    core_->WatchForRead();
     waiting_read_ = true;
     read_callback_ = callback;
-    read_iobuffer_ = buf;
+    core_->read_iobuffer_ = buf;
     return ERR_IO_PENDING;
   }
   return MapWinsockError(err);
@@ -265,28 +379,29 @@ int TCPClientSocketWin::Write(IOBuffer* buf,
   DCHECK(!waiting_write_);
   DCHECK(!write_callback_);
   DCHECK_GT(buf_len, 0);
-  DCHECK(!write_iobuffer_);
+  DCHECK(!core_->write_iobuffer_);
 
-  write_buffer_.len = buf_len;
-  write_buffer_.buf = buf->data();
+  core_->write_buffer_.len = buf_len;
+  core_->write_buffer_.buf = buf->data();
 
   TRACE_EVENT_BEGIN("socket.write", this, "");
   // TODO(wtc): Remove the CHECK after enough testing.
-  CHECK(WaitForSingleObject(write_overlapped_.hEvent, 0) == WAIT_TIMEOUT);
+  CHECK(
+      WaitForSingleObject(core_->write_overlapped_.hEvent, 0) == WAIT_TIMEOUT);
   DWORD num;
-  int rv =
-      WSASend(socket_, &write_buffer_, 1, &num, 0, &write_overlapped_, NULL);
+  int rv = WSASend(socket_, &core_->write_buffer_, 1, &num, 0,
+                   &core_->write_overlapped_, NULL);
   if (rv == 0) {
-    WaitForAndResetEvent(write_overlapped_.hEvent);
+    WaitForAndResetEvent(core_->write_overlapped_.hEvent);
     TRACE_EVENT_END("socket.write", this, StringPrintf("%d bytes", num));
     return static_cast<int>(num);
   }
   int err = WSAGetLastError();
   if (err == WSA_IO_PENDING) {
-    write_watcher_.StartWatching(write_overlapped_.hEvent, &writer_);
+    core_->WatchForWrite();
     waiting_write_ = true;
     write_callback_ = callback;
-    write_iobuffer_ = buf;
+    core_->write_iobuffer_ = buf;
     return ERR_IO_PENDING;
   }
   return MapWinsockError(err);
@@ -377,13 +492,15 @@ void TCPClientSocketWin::DoWriteCallback(int rv) {
 }
 
 void TCPClientSocketWin::DidCompleteConnect() {
+  DCHECK(waiting_connect_);
   int result;
 
   TRACE_EVENT_END("socket.connect", this, "");
   waiting_connect_ = false;
 
   WSANETWORKEVENTS events;
-  int rv = WSAEnumNetworkEvents(socket_, read_overlapped_.hEvent, &events);
+  int rv = WSAEnumNetworkEvents(socket_, core_->read_overlapped_.hEvent,
+                                &events);
   if (rv == SOCKET_ERROR) {
     NOTREACHED();
     result = MapWinsockError(WSAGetLastError());
@@ -413,42 +530,29 @@ void TCPClientSocketWin::DidCompleteConnect() {
     DoReadCallback(result);
 }
 
-void TCPClientSocketWin::ReadDelegate::OnObjectSignaled(HANDLE object) {
-  DCHECK_EQ(object, tcp_socket_->read_overlapped_.hEvent);
-
-  if (tcp_socket_->waiting_connect_) {
-    tcp_socket_->DidCompleteConnect();
-  } else {
-    DCHECK(tcp_socket_->waiting_read_);
-    DWORD num_bytes, flags;
-    BOOL ok = WSAGetOverlappedResult(
-        tcp_socket_->socket_, &tcp_socket_->read_overlapped_, &num_bytes,
-        FALSE, &flags);
-    WSAResetEvent(object);
-    TRACE_EVENT_END("socket.read", tcp_socket_,
-                    StringPrintf("%d bytes", num_bytes));
-    tcp_socket_->waiting_read_ = false;
-    tcp_socket_->read_iobuffer_ = NULL;
-    tcp_socket_->DoReadCallback(
-        ok ? num_bytes : MapWinsockError(WSAGetLastError()));
-  }
+void TCPClientSocketWin::DidCompleteRead() {
+  DCHECK(waiting_read_);
+  DWORD num_bytes, flags;
+  BOOL ok = WSAGetOverlappedResult(socket_, &core_->read_overlapped_,
+                                   &num_bytes, FALSE, &flags);
+  WSAResetEvent(core_->read_overlapped_.hEvent);
+  TRACE_EVENT_END("socket.read", this, StringPrintf("%d bytes", num_bytes));
+  waiting_read_ = false;
+  core_->read_iobuffer_ = NULL;
+  DoReadCallback(ok ? num_bytes : MapWinsockError(WSAGetLastError()));
 }
 
-void TCPClientSocketWin::WriteDelegate::OnObjectSignaled(HANDLE object) {
-  DCHECK_EQ(object, tcp_socket_->write_overlapped_.hEvent);
-  DCHECK(tcp_socket_->waiting_write_);
+void TCPClientSocketWin::DidCompleteWrite() {
+  DCHECK(waiting_write_);
 
   DWORD num_bytes, flags;
-  BOOL ok = WSAGetOverlappedResult(
-      tcp_socket_->socket_, &tcp_socket_->write_overlapped_, &num_bytes,
-      FALSE, &flags);
-  WSAResetEvent(object);
-  TRACE_EVENT_END("socket.write", tcp_socket_,
-                  StringPrintf("%d bytes", num_bytes));
-  tcp_socket_->waiting_write_ = false;
-  tcp_socket_->write_iobuffer_ = NULL;
-  tcp_socket_->DoWriteCallback(
-      ok ? num_bytes : MapWinsockError(WSAGetLastError()));
+  BOOL ok = WSAGetOverlappedResult(socket_, &core_->write_overlapped_,
+                                   &num_bytes, FALSE, &flags);
+  WSAResetEvent(core_->write_overlapped_.hEvent);
+  TRACE_EVENT_END("socket.write", this, StringPrintf("%d bytes", num_bytes));
+  waiting_write_ = false;
+  core_->write_iobuffer_ = NULL;
+  DoWriteCallback(ok ? num_bytes : MapWinsockError(WSAGetLastError()));
 }
 
 }  // namespace net
diff --git a/net/base/tcp_client_socket_win.h b/net/base/tcp_client_socket_win.h
index 0199bfc..87a5a83 100644
--- a/net/base/tcp_client_socket_win.h
+++ b/net/base/tcp_client_socket_win.h
@@ -36,36 +36,14 @@ class TCPClientSocketWin : public ClientSocket {
   virtual int Write(IOBuffer* buf, int buf_len, CompletionCallback* callback);
 
  private:
-  class ReadDelegate : public base::ObjectWatcher::Delegate {
-   public:
-    explicit ReadDelegate(TCPClientSocketWin* tcp_socket)
-        : tcp_socket_(tcp_socket) { }
-    virtual ~ReadDelegate() { }
-
-    // base::ObjectWatcher::Delegate methods:
-    virtual void OnObjectSignaled(HANDLE object);
-
-   private:
-    TCPClientSocketWin* const tcp_socket_;
-  };
-
-  class WriteDelegate : public base::ObjectWatcher::Delegate {
-   public:
-    explicit WriteDelegate(TCPClientSocketWin* tcp_socket)
-        : tcp_socket_(tcp_socket) { }
-    virtual ~WriteDelegate() { }
-
-    // base::ObjectWatcher::Delegate methods:
-    virtual void OnObjectSignaled(HANDLE object);
-
-   private:
-    TCPClientSocketWin* const tcp_socket_;
-  };
+  class Core;
 
   int CreateSocket(const struct addrinfo* ai);
   void DoReadCallback(int rv);
   void DoWriteCallback(int rv);
   void DidCompleteConnect();
+  void DidCompleteRead();
+  void DidCompleteWrite();
 
   SOCKET socket_;
 
@@ -80,27 +58,10 @@ class TCPClientSocketWin : public ClientSocket {
   bool waiting_read_;
   bool waiting_write_;
 
-  // The separate OVERLAPPED variables for asynchronous operation.
-  // |read_overlapped_| is used for both Connect() and Read().
-  // |write_overlapped_| is only used for Write();
-  OVERLAPPED read_overlapped_;
-  OVERLAPPED write_overlapped_;
-
-  // The buffers used in Read() and Write().
-  WSABUF read_buffer_;
-  WSABUF write_buffer_;
-  scoped_refptr<IOBuffer> read_iobuffer_;
-  scoped_refptr<IOBuffer> write_iobuffer_;
-
-  // |reader_| handles the signals from |read_watcher_|.
-  ReadDelegate reader_;
-  // |writer_| handles the signals from |write_watcher_|.
-  WriteDelegate writer_;
-
-  // |read_watcher_| watches for events from Connect() and Read().
-  base::ObjectWatcher read_watcher_;
-  // |write_watcher_| watches for events from Write();
-  base::ObjectWatcher write_watcher_;
+  // The core of the socket that can live longer than the socket itself. We pass
+  // resources to the Windows async IO functions and we have to make sure that
+  // they are not destroyed while the OS still references them.
+  scoped_refptr<Core> core_;
 
   // External callback; called when connect or read is complete.
   CompletionCallback* read_callback_;