Report the correct os_error in the SOCKET_READ_ERROR event

net/socket/tcp_client_socket_win.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/socket/tcp_client_socket_win.h"
 
 #include <mstcpip.h>
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
@@ skipping 939 matching lines @@
     LogConnectCompletion(rv);
     DoReadCallback(rv);
   }
 }
 
 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);
   waiting_read_ = false;
   int rv;
   if (ok) {
     base::StatsCounter read_bytes("tcp.read_bytes");
     read_bytes.Add(num_bytes);
     num_bytes_read_ += num_bytes;
     if (num_bytes > 0)
       use_history_.set_was_used_to_convey_data();
     net_log_.AddByteTransferEvent(NetLog::TYPE_SOCKET_BYTES_RECEIVED,
                                   num_bytes, core_->read_iobuffer_->data());
     rv = static_cast<int>(num_bytes);
   } else {
     int os_error = WSAGetLastError();
     rv = MapSystemError(os_error);
     net_log_.AddEvent(NetLog::TYPE_SOCKET_READ_ERROR,
                       CreateNetLogSocketErrorCallback(rv, os_error));
   }
+  WSAResetEvent(core_->read_overlapped_.hEvent);

[wtc, 2013/03/05 20:17:03]

This change is just defensive programming. Between the
WSAGetOverlappedResult() call that failed and the
WSAGetLastError() call, we should avoid any Win32 function
call that may change the last error.

   core_->read_iobuffer_ = NULL;
   core_->read_buffer_length_ = 0;
   DoReadCallback(rv);
 }
 
 void TCPClientSocketWin::DidCompleteWrite() {
   DCHECK(waiting_write_);
 
   DWORD num_bytes, flags;
   BOOL ok = WSAGetOverlappedResult(socket_, &core_->write_overlapped_,
@@ skipping 31 matching lines @@
 void TCPClientSocketWin::DidSignalRead() {
   DCHECK(waiting_read_);
   int os_error = 0;
   WSANETWORKEVENTS network_events;
   int rv = WSAEnumNetworkEvents(socket_, core_->read_overlapped_.hEvent,
                                 &network_events);
   if (rv == SOCKET_ERROR) {
     os_error = WSAGetLastError();
     rv = MapSystemError(os_error);
   } else if (network_events.lNetworkEvents & FD_READ) {
-    rv = DoRead(core_->read_iobuffer_, core_->read_buffer_length_,
-                read_callback_);
-    if (rv == ERR_IO_PENDING)
-      return;
-  } else if (network_events.lNetworkEvents & FD_CLOSE) {
-    if (network_events.iErrorCode[FD_CLOSE_BIT]) {
-      rv = MapSystemError(network_events.iErrorCode[FD_CLOSE_BIT]);
+    if (network_events.iErrorCode[FD_READ_BIT]) {

[wtc, 2013/03/05 20:17:03]

According to MSDN documentation and the sample code in the
book "Network Programming for Microsoft Windows", we also
need to check for a nonzero error code for the FD_READ event.

Note:

1. In my debugging this code path is never taken. But I
think we should add this code anyway.

2. I sometimes get both FD_READ and FD_CLOSE (0x21), so
the order in which we check for FD_READ and FD_CLOSE is
actually important. Perhaps we should add a comment about
that.

[rvargas (doing something else), 2013/03/05 23:14:07]

could you point me to that sample?
My reading of the doc doesn't tell me that this path is needed. On the other
hand, I would not bet against being able to find a client configuration that
actually returns an error here... and adding more error handling is never wrong
:)
This sounds like the real issue. It is totally fine to report multiple events
simultaneously, and in that case we should pick the one we care the most about
(if we don't go over all of them). In other words, we should invert the logic
and test first for close events.

[wtc, 2013/03/06 00:00:03]

I will bring the book to the office tomorrow.
I agree.  The MSDN page for WSAEnumNetworkEvents
http://msdn.microsoft.com/en-us/library/windows/desktop/ms741572%28v=vs.85%29...
only mentions FD_READ_BIT and FD_WRITE_BIT without
providing sample code, but I think that implies FD_READ_BIT
is a valid index for the iErrorCode array. In addition,
the MSDN page also documents WSAENETDOWN as the error code
that can be returned with the FD_READ event.
When I saw 0x21 in the debugger, the iErrorCode[FD_READ_BIT]
and iErrorCode[FD_CLOSE_BIT] were both zero (i.e., no errors).

I interpreted that (without verifying) as the peer sending
us some bytes and then closing the connection gracefully.
This is why I think we should read the bytes first, before
reporting the EOF (rv = 0) event to the caller.

If we report the FD_CLOSE (rv = 0) event first, I am worried
that we will fail to return some bytes to the caller. I'll
need to verify this with experiments.

Does this make sense?

[rvargas (doing something else), 2013/03/06 00:34:47]

That's ok. We agree that it is better to add the code anyway.
I was looking at that page, but I'm reading it as no_error for a bunch of bits,
until FD_WRITE. Of course, a list of returned errors should not be interpreted
as exhaustive :)

The reasoning for my interpretation is that it doesn't make much sense to tell
an app "hey, there's available data for read... not really, there's an error so
you have nothing to read"
Yes... in that both bits should mean that. So assuming there is no error
anywhere, the right thing to do is to read the last few bytes. However, if there
is an error somewhere, the fact that we process the read first and don't look at
the other one means we are ignoring the close related error.

Maybe the right thing to do is to go through both paths and not use an else.

[Pat Meenan, 2013/03/06 02:05:28]

In theory the FD_CLOSE is supposed to stay signaled and does not get reset:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms741576(v=vs.85).aspx

I wasn't comfortable issuing 2 callbacks to a single read call so in the case of
both being signaled the read should complete, call back with the data and the
close would be indicated whenever another socket operation was attempted (likely
another read).

+      os_error = network_events.iErrorCode[FD_READ_BIT];
+      rv = MapSystemError(os_error);
       net_log_.AddEvent(NetLog::TYPE_SOCKET_READ_ERROR,
                         CreateNetLogSocketErrorCallback(rv, os_error));

[wtc, 2013/03/05 20:17:03]

This is the bug. Note that we map
network_events.iErrorCode[FD_CLOSE_BIT] to |rv|, but
os_error still has the initial value of 0. This is why
the SOCKET_READ_ERROR event has os_error = 0.

     } else {
+      rv = DoRead(core_->read_iobuffer_, core_->read_buffer_length_,
+                  read_callback_);
+      if (rv == ERR_IO_PENDING)
+        return;
+    }
+  } else if (network_events.lNetworkEvents & FD_CLOSE) {
+    DCHECK_EQ(network_events.lNetworkEvents & ~FD_CLOSE, 0);
+    if (network_events.iErrorCode[FD_CLOSE_BIT]) {
+      os_error = network_events.iErrorCode[FD_CLOSE_BIT];
+      rv = MapSystemError(os_error);
+      net_log_.AddEvent(NetLog::TYPE_SOCKET_READ_ERROR,
+                        CreateNetLogSocketErrorCallback(rv, os_error));
+    } else {
       rv = 0;
     }
   } else {
     // This should not happen but I have seen cases where we will get
     // signaled but the network events flags are all clear (0).
     core_->WatchForRead();

[wtc, 2013/03/05 20:17:03]

I also saw this in my debugging. This occurs while I am not
browsing. So I suspect this is caused by the non-blocking
recv(MSG_PEEK) calls in IsConnected() and IsConnectedAndIdle().
I did not verify that.

This CL does not address this potential problem.

     return;
   }
   waiting_read_ = false;
   core_->read_iobuffer_ = NULL;
   core_->read_buffer_length_ = 0;
   DoReadCallback(rv);
 }
 
 }  // namespace net