SSLClientSocket::IsConnected should care for internal buffers

net/socket_stream/socket_stream.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.
 //
 // TODO(ukai): code is similar with http_network_transaction.cc.  We should
 //   think about ways to share code, if possible.
 
 #include "net/socket_stream/socket_stream.h"
 
 #include <set>
@@ skipping 1049 matching lines @@
     next_state_ = STATE_CLOSE;
   }
   return result;
 }
 
 int SocketStream::DoReadWrite(int result) {
   if (result < OK) {
     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;
   }
 
   // If client has requested close(), and there's nothing to write, then
   // let's close the socket.
   // We don't care about receiving data after the socket is closed.
   if (closing_ && !write_buf_ && pending_write_bufs_.empty()) {
     socket_->Disconnect();
     next_state_ = STATE_CLOSE;
@@ skipping 192 matching lines @@
 
 SSLConfigService* SocketStream::ssl_config_service() const {
   return context_->ssl_config_service();
 }
 
 ProxyService* SocketStream::proxy_service() const {
   return context_->proxy_service();
 }
 
 }  // namespace net