SSLClientSocket::IsConnected should care for internal buffers

net/socket_stream/socket_stream.cc

Index: net/socket_stream/socket_stream.cc
diff --git a/net/socket_stream/socket_stream.cc b/net/socket_stream/socket_stream.cc
index 73d5a2b9cdc6d70ab60a17b9ddacc5b2065adbd2..478d102b7c2e6e505fe837ed3f837067b5d711f8 100644
--- a/net/socket_stream/socket_stream.cc
+++ b/net/socket_stream/socket_stream.cc
@@ -1067,7 +1067,8 @@ int SocketStream::DoReadWrite(int result) {
     next_state_ = STATE_CLOSE;
     return result;
   }
-  if (!socket_.get() || !socket_->IsConnected()) {

[Ryan Sleevi, 2012/12/15 01:03:04]

I have to agree with Wan-Teh: I think this function is the root of the bug.

You should not check IsConnected() - you should attempt to read or write and let
the EOF signalling from those methods indicate success or failure.

Could you explain more about this?

[Takashi Toyoshima, 2012/12/21 07:43:03]

Yes. Removing IsConnected() check is easy and right fix for this bug. I tried it
as Patch Set 1 and confirmed that it fixed the problem.

This code was written by ukai@ as the initial commit for SocketStream. I guess
he aimed early close detection here, or just followed other implementations,
e.g., ftp_network_transaction.cc.

It means that without fixing IsConnected() behavior, other protocol may keep on
having the same problem.

+  //if (!socket_.get() || !socket_->IsConnected()) {

[tyoshino (SeeGerritForStatus), 2012/12/14 15:13:54]

To be removed before submission?

[Takashi Toyoshima, 2012/12/21 06:39:03]

Oops, this change is not expected, but just for test.
But, As Wan-Teh and Ryan said, I'll change direction.

+  if (!socket_.get()) {
     next_state_ = STATE_CLOSE;
     return ERR_CONNECTION_CLOSED;
   }