SSLClientSocket::IsConnected should care for internal buffers

net/socket/ssl_client_socket_nss.cc

Index: net/socket/ssl_client_socket_nss.cc
diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc
index 809dbc5beb28ce36b36e1e76c8ef4141a24eb715..d7b47eb44033152624bd30c4f89bc711a7aca089 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -642,6 +642,11 @@ class SSLClientSocketNSS::Core : public base::RefCountedThreadSafe<Core> {
   int Read(IOBuffer* buf, int buf_len, const CompletionCallback& callback);
   int Write(IOBuffer* buf, int buf_len, const CompletionCallback& callback);
 
+  // Called on the network task runner.
+  bool IsClosed();
+  bool HasPendingAsyncOperation();
+  bool HasUnhandledReceivedData();
+
  private:
   friend class base::RefCountedThreadSafe<Core>;
   ~Core();
@@ -740,6 +745,10 @@ class SSLClientSocketNSS::Core : public base::RefCountedThreadSafe<Core> {
   void DoConnectCallback(int result);
   void DoReadCallback(int result);
   void DoWriteCallback(int result);
+  void UpdateNSSTaskRunnerStatus(int amount_in_read_buffer,
+                                 bool closed,
+                                 bool operation_is_read,
+                                 const base::Closure& callback);
 
   // Client channel ID handler.
   static SECStatus ClientChannelIDHandler(
@@ -817,6 +826,12 @@ class SSLClientSocketNSS::Core : public base::RefCountedThreadSafe<Core> {
   ServerBoundCertService* server_bound_cert_service_;
   ServerBoundCertService::RequestHandle domain_bound_cert_request_handle_;
 
+  // The information about NSS task runner.
+  int unhandled_buffer_size_;
+  bool nss_waiting_read_;
+  bool nss_waiting_write_;
+  bool nss_is_closed_;
+
   ////////////////////////////////////////////////////////////////////////////
   // Members that are ONLY accessed on the NSS task runner:
   ////////////////////////////////////////////////////////////////////////////
@@ -896,6 +911,10 @@ SSLClientSocketNSS::Core::Core(
       transport_(transport),
       weak_net_log_factory_(net_log),
       server_bound_cert_service_(server_bound_cert_service),
+      unhandled_buffer_size_(0),
+      nss_waiting_read_(false),
+      nss_waiting_write_(false),
+      nss_is_closed_(false),
       host_and_port_(host_and_port),
       ssl_config_(ssl_config),
       nss_fd_(NULL),
@@ -1104,6 +1123,7 @@ int SSLClientSocketNSS::Core::Read(IOBuffer* buf, int buf_len,
   DCHECK(user_connect_callback_.is_null());
   DCHECK(!user_read_buf_);
   DCHECK(nss_bufs_);
+  DCHECK(!nss_waiting_read_);
 
   user_read_buf_ = buf;
   user_read_buf_len_ = buf_len;
@@ -1111,6 +1131,7 @@ int SSLClientSocketNSS::Core::Read(IOBuffer* buf, int buf_len,
   int rv = DoReadLoop(OK);
   if (rv == ERR_IO_PENDING) {
     user_read_callback_ = callback;
+    nss_waiting_read_ = true;
   } else {
     user_read_buf_ = NULL;
     user_read_buf_len_ = 0;
@@ -1146,6 +1167,7 @@ int SSLClientSocketNSS::Core::Write(IOBuffer* buf, int buf_len,
   DCHECK(user_connect_callback_.is_null());
   DCHECK(!user_write_buf_);
   DCHECK(nss_bufs_);
+  DCHECK(!nss_waiting_write_);
 
   user_write_buf_ = buf;
   user_write_buf_len_ = buf_len;
@@ -1153,6 +1175,7 @@ int SSLClientSocketNSS::Core::Write(IOBuffer* buf, int buf_len,
   int rv = DoWriteLoop(OK);
   if (rv == ERR_IO_PENDING) {
     user_write_callback_ = callback;
+    nss_waiting_write_ = true;
   } else {
     user_write_buf_ = NULL;
     user_write_buf_len_ = 0;
@@ -1166,6 +1189,20 @@ int SSLClientSocketNSS::Core::Write(IOBuffer* buf, int buf_len,
   return rv;
 }
 
+bool SSLClientSocketNSS::Core::IsClosed() {
+  return nss_is_closed_;
+}
+
+bool SSLClientSocketNSS::Core::HasPendingAsyncOperation() {
+  DCHECK(OnNetworkTaskRunner());
+  return nss_waiting_read_ || nss_waiting_write_;
+}
+
+bool SSLClientSocketNSS::Core::HasUnhandledReceivedData() {
+  DCHECK(OnNetworkTaskRunner());
+  return unhandled_buffer_size_ != 0;
+}
+
 bool SSLClientSocketNSS::Core::OnNSSTaskRunner() const {
   return nss_task_runner_->RunsTasksOnCurrentThread();
 }
@@ -2287,10 +2324,21 @@ void SSLClientSocketNSS::Core::DoReadCallback(int rv) {
 
   user_read_buf_ = NULL;
   user_read_buf_len_ = 0;
-  base::Closure c = base::Bind(
-      base::ResetAndReturn(&user_read_callback_),
-      rv);
-  PostOrRunCallback(FROM_HERE, c);
+  char* buf;
+  int amount_in_read_buffer = memio_GetReadParams(nss_bufs_, &buf);

[wtc, 2013/01/16 00:34:16]

BUG(?): I believe this is wrong.

The return value of memio_GetReadParams() is the number of
bytes available to write into the read buffer, i.e., the
free space in the read buffer. So the amount of unhandled
data should be approximately
    kRecvBufferSize - memio_GetReadParams(nss_bufs_, &buf)

The reason I said "approximately" is that we may need to
subtract 1 because the read buffer is a circular buffer.

There is an internal function memio_buffer_used_contiguous(&secret->readbuf)
defined in net/base/nss_memio.c that we can use to get
the right value. We will need to expose it.

[Takashi Toyoshima, 2013/01/16 07:16:03]

Done.
I expose it as memio_GetReadableBufferSize().

+  // This will invoke the user callback with |rv| as result.
+  base::Closure user_cb = base::Bind(
+      base::ResetAndReturn(&user_read_callback_), rv);
+  // This is used to curry the |amount_int_read_buffer| and |user_cb| back to
+  // the network task runner.
+  base::Closure task = base::Bind(
+      &Core::UpdateNSSTaskRunnerStatus,
+      this,
+      amount_in_read_buffer,
+      rv <= 0,  // EOF or errors.
+      true,  // Operation is read.
+      user_cb);
+  PostOrRunCallback(FROM_HERE, task);
 }
 
 void SSLClientSocketNSS::Core::DoWriteCallback(int rv) {
@@ -2302,10 +2350,43 @@ void SSLClientSocketNSS::Core::DoWriteCallback(int rv) {
   // up front.
   user_write_buf_ = NULL;
   user_write_buf_len_ = 0;
-  base::Closure c = base::Bind(
-      base::ResetAndReturn(&user_write_callback_),
-      rv);
-  PostOrRunCallback(FROM_HERE, c);
+  char* buf;
+  // Update buffer status because DoWriteLoop called DoTransportIO which may
+  // perform read operations.
+  int amount_in_read_buffer = memio_GetReadParams(nss_bufs_, &buf);
+  // This will invoke the user callback with |rv| as result.
+  base::Closure user_cb = base::Bind(
+      base::ResetAndReturn(&user_write_callback_), rv);
+  // This is used to curry the |amount_int_read_buffer| and |user_cb| back to
+  // the network task runner.
+  base::Closure task = base::Bind(
+      &Core::UpdateNSSTaskRunnerStatus,
+      this,
+      amount_in_read_buffer,
+      rv < 0,  // Errors.
+      false,  // Operation is not read, but write.
+      user_cb);
+  PostOrRunCallback(FROM_HERE, task);
+}
+
+void SSLClientSocketNSS::Core::UpdateNSSTaskRunnerStatus(
+    int amount_in_read_buffer,
+    bool closed,
+    bool operation_is_read,
+    const base::Closure& callback) {
+  DCHECK(OnNetworkTaskRunner());
+  DCHECK(!callback.is_null());
+  DCHECK((operation_is_read && nss_waiting_read_) ||
+         (!operation_is_read && nss_waiting_write_));
+
+  unhandled_buffer_size_ = amount_in_read_buffer;
+  if (closed)
+    nss_is_closed_ = true;
+  if (operation_is_read)
+    nss_waiting_read_ = false;
+  else
+    nss_waiting_write_ = false;
+  callback.Run();
 }
 
 SECStatus SSLClientSocketNSS::Core::ClientChannelIDHandler(
@@ -2734,6 +2815,7 @@ SSLClientSocketNSS::SSLClientSocketNSS(
       server_bound_cert_service_(context.server_bound_cert_service),
       ssl_session_cache_shard_(context.ssl_session_cache_shard),
       completed_handshake_(false),
+      ssl_is_closed_(false),
       next_handshake_state_(STATE_NONE),
       nss_fd_(NULL),
       net_log_(transport_socket->socket()->NetLog()),
@@ -2922,29 +3004,22 @@ void SSLClientSocketNSS::Disconnect() {
 }
 
 bool SSLClientSocketNSS::IsConnected() const {
-  // Ideally, we should also check if we have received the close_notify alert
-  // message from the server, and return false in that case.  We're not doing
-  // that, so this function may return a false positive.  Since the upper
-  // layer (HttpNetworkTransaction) needs to handle a persistent connection
-  // closed by the server when we send a request anyway, a false positive in
-  // exchange for simpler code is a good trade-off.
   EnterFunction("");
-  bool ret = completed_handshake_ && transport_->socket()->IsConnected();
+  bool ret = completed_handshake_ &&
+             !ssl_is_closed_ &&
+             !core_->IsClosed() &&

[wtc, 2013/01/16 00:34:16]

Why do we need both ssl_is_closed_ and core_->IsClosed()?

[Takashi Toyoshima, 2013/01/16 07:16:03]

ssl_is_closed_ is for synchronous completion case, core_->IsClosed() is for
asynchronous completion case. Synchronous case should be handled for single
thread model.

But... these condition looks wrong?
Now I think HasPendingAsyncOperation should be handled at higher priority than
IsClosed() because pending async operation should be finished before the
detected EOF.

[Ryan Sleevi, 2013/01/18 00:03:24]

I'm not sure I understand this. Both synchronous and asynchronous use a core_
object. Why can that not be used consistently?

[Takashi Toyoshima, 2013/01/21 14:51:16]

OK, I move the logic for ssl_is_closed_ into core_ object.

+             (core_->HasPendingAsyncOperation() ||
+              core_->HasUnhandledReceivedData() ||
+              transport_->socket()->IsConnected());
   LeaveFunction("");
   return ret;
 }
 
 bool SSLClientSocketNSS::IsConnectedAndIdle() const {
-  // Unlike IsConnected, this method doesn't return a false positive.
-  //
-  // Strictly speaking, we should check if we have received the close_notify
-  // alert message from the server, and return false in that case.  Although
-  // the close_notify alert message means EOF in the SSL layer, it is just
-  // bytes to the transport layer below, so
-  // transport_->socket()->IsConnectedAndIdle() returns the desired false
-  // when we receive close_notify.
   EnterFunction("");
-  bool ret = completed_handshake_ && transport_->socket()->IsConnectedAndIdle();
+  bool ret = IsConnected() &&
+             !core_->HasUnhandledReceivedData() &&
+             transport_->socket()->IsConnectedAndIdle();
   LeaveFunction("");
   return ret;
 }
@@ -3016,6 +3091,8 @@ int SSLClientSocketNSS::Read(IOBuffer* buf, int buf_len,
 
   EnterFunction(buf_len);
   int rv = core_->Read(buf, buf_len, callback);
+  if (rv <= 0 && rv != ERR_IO_PENDING)
+    ssl_is_closed_ = true;
   LeaveFunction(rv);
 
   return rv;
@@ -3028,6 +3105,8 @@ int SSLClientSocketNSS::Write(IOBuffer* buf, int buf_len,
 
   EnterFunction(buf_len);
   int rv = core_->Write(buf, buf_len, callback);
+  if (rv < 0 && rv != ERR_IO_PENDING)
+    ssl_is_closed_ = true;
   LeaveFunction(rv);
 
   return rv;