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 92f6c37f4ef564aeb959f6e16ef969674a432b63..97fdc290e75b295741eba2ffe1408c0ada7ce14d 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -637,6 +637,10 @@ 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 HasPendingAsyncOperation();
+  bool HasUnhandledData();

[wtc, 2013/01/14 20:50:08]

It may be good to stress that this is unhandled received data
(as opposed to the data that we are trying to send).

Perhaps the word "unhandled" already implies received data.

[Takashi Toyoshima, 2013/01/15 13:46:03]

Done.

+
  private:
   friend class base::RefCountedThreadSafe<Core>;
   ~Core();
@@ -735,6 +739,9 @@ class SSLClientSocketNSS::Core : public base::RefCountedThreadSafe<Core> {
   void DoConnectCallback(int result);
   void DoReadCallback(int result);
   void DoWriteCallback(int result);
+  void UpdateNSSStatus(int amount_in_read_buffer,
+                       bool operation_is_read,
+                       const base::Closure& callback);
 
   // Client channel ID handler.
   static SECStatus ClientChannelIDHandler(
@@ -812,6 +819,11 @@ 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 status.

[wtc, 2013/01/14 20:50:08]

Nit: I think "status" can be removed from this comment without
losing much. Or the comment can say "The NSS task runner status".

It seems important to note that these members are accessed
on the network task runner.

[Takashi Toyoshima, 2013/01/15 13:46:03]

OK, I remove the last "status".

We have a comment at line 802 to 804 and I think it means that following members
(including these status variables) can be accessed on the network task runner.
So I omitted the comment and just placed them here.

+  int unhandled_buffer_size_;
+  bool waiting_read_;
+  bool waiting_write_;

[wtc, 2013/01/14 20:50:08]

It may be a good idea to name these members nss_waiting_read_
and nss_waiting_write_ to stress the fact that they represent
the NSS task runner status.

[Takashi Toyoshima, 2013/01/15 13:46:03]

Done.

+
   ////////////////////////////////////////////////////////////////////////////
   // Members that are ONLY accessed on the NSS task runner:
   ////////////////////////////////////////////////////////////////////////////
@@ -892,6 +904,9 @@ SSLClientSocketNSS::Core::Core(
       weak_net_log_factory_(net_log),
       server_bound_cert_service_(server_bound_cert_service),
       domain_bound_cert_request_handle_(NULL),
+      unhandled_buffer_size_(0),
+      waiting_read_(false),
+      waiting_write_(false),
       host_and_port_(host_and_port),
       ssl_config_(ssl_config),
       nss_fd_(NULL),
@@ -1104,9 +1119,11 @@ int SSLClientSocketNSS::Core::Read(IOBuffer* buf, int buf_len,
   DCHECK(user_connect_callback_.is_null());
   DCHECK(!user_read_buf_);
   DCHECK(nss_bufs_);
+  DCHECK(!waiting_read_);
 
   user_read_buf_ = buf;
   user_read_buf_len_ = buf_len;
+  waiting_read_ = true;
 
   int rv = DoReadLoop(OK);
   if (rv == ERR_IO_PENDING) {
@@ -1114,6 +1131,7 @@ int SSLClientSocketNSS::Core::Read(IOBuffer* buf, int buf_len,
   } else {
     user_read_buf_ = NULL;
     user_read_buf_len_ = 0;
+    waiting_read_ = false;
 
     if (!OnNetworkTaskRunner()) {
       PostOrRunCallback(FROM_HERE, base::Bind(callback, rv));
@@ -1146,9 +1164,11 @@ int SSLClientSocketNSS::Core::Write(IOBuffer* buf, int buf_len,
   DCHECK(user_connect_callback_.is_null());
   DCHECK(!user_write_buf_);
   DCHECK(nss_bufs_);
+  DCHECK(!waiting_write_);
 
   user_write_buf_ = buf;
   user_write_buf_len_ = buf_len;
+  waiting_write_ = true;
 
   int rv = DoWriteLoop(OK);
   if (rv == ERR_IO_PENDING) {
@@ -1156,6 +1176,7 @@ int SSLClientSocketNSS::Core::Write(IOBuffer* buf, int buf_len,
   } else {
     user_write_buf_ = NULL;
     user_write_buf_len_ = 0;
+    waiting_write_ = false;
 
     if (!OnNetworkTaskRunner()) {
       PostOrRunCallback(FROM_HERE, base::Bind(callback, rv));
@@ -1166,6 +1187,16 @@ int SSLClientSocketNSS::Core::Write(IOBuffer* buf, int buf_len,
   return rv;
 }
 
+bool SSLClientSocketNSS::Core::HasPendingAsyncOperation() {
+  DCHECK(OnNetworkTaskRunner());
+  return waiting_read_ || waiting_write_;
+}
+
+bool SSLClientSocketNSS::Core::HasUnhandledData() {
+  DCHECK(OnNetworkTaskRunner());
+  return unhandled_buffer_size_ != 0;
+}
+
 bool SSLClientSocketNSS::Core::OnNSSTaskRunner() const {
   return nss_task_runner_->RunsTasksOnCurrentThread();
 }
@@ -2257,10 +2288,20 @@ 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);
+  // 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::UpdateNSSStatus,
+      this,
+      amount_in_read_buffer,
+      true,  // Operation is read.
+      user_cb);
+  PostOrRunCallback(FROM_HERE, task);
 }
 
 void SSLClientSocketNSS::Core::DoWriteCallback(int rv) {
@@ -2272,10 +2313,39 @@ 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::UpdateNSSStatus,
+      this,
+      amount_in_read_buffer,
+      false,  // Operation is not read, but write.
+      user_cb);
+  PostOrRunCallback(FROM_HERE, task);
+}
+
+void SSLClientSocketNSS::Core::UpdateNSSStatus(

[wtc, 2013/01/14 20:50:08]

Document this method either here or in the declaration.

I think this method could have a more precise name. I don't
have a good suggestion, but even "UpdateNSSTaskRunnerStatus"
would be clearer than "UpdateNSSStatus".

[Takashi Toyoshima, 2013/01/15 13:46:03]

OK. I use "UpdateNSSTaskRunnerStatus" for now.
I may change it again if I find better name.

+    int amount_in_read_buffer,
+    bool operation_is_read,
+    const base::Closure& callback) {
+  DCHECK(OnNetworkTaskRunner());
+  DCHECK(!callback.is_null());
+  DCHECK((operation_is_read && waiting_read_) ||
+         (!operation_is_read && waiting_write_));
+
+  unhandled_buffer_size_ = amount_in_read_buffer;
+  if (operation_is_read)
+    waiting_read_ = false;
+  else
+    waiting_write_ = false;
+  callback.Run();
 }
 
 SECStatus SSLClientSocketNSS::Core::ClientChannelIDHandler(
@@ -2898,7 +2968,17 @@ bool SSLClientSocketNSS::IsConnected() const {
   // 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;
+
+  if (!completed_handshake_)
+    ret = false;
+  else if (core_->HasPendingAsyncOperation())
+    ret = true;
+  else if (core_->HasUnhandledData())

[Ryan Sleevi, 2013/01/09 19:01:52]

BUG: I think there's a bug here. This fails to consider receiving a CloseNotify,
which causes a weird situation in which Read() will return EOF, but then
IsConnected() will continue returning true.

It's not enough to check that there's unhandled data. You need to check that
there's unhandled data and there hasn't been an exceptional return from (Read,
Write) that should cause such data to be ignored.

[Takashi Toyoshima, 2013/01/10 07:46:29]

OK, I'll restore ssl_recv_eof_, then remove above comments.

[Takashi Toyoshima, 2013/01/15 13:46:03]

Try to fix this in the next CL.

+    ret = true;
+  else
+    ret = transport_->socket()->IsConnected();

[wtc, 2013/01/14 20:50:08]

I would probably write this as a single conditional expression,
or like this:
  if (!completed_handshake_) {
    ret = false;
  } else {
    ret = core_->HasPendingAsyncOperation() ||
          core_->HasUnhandledData() ||
          transport_->socket()->IsConnected();
  }

This comment also applies to the SSLClientSocketNSS::IsConnectedAndIdle()
method.

[Takashi Toyoshima, 2013/01/15 13:46:03]

Done.

[Takashi Toyoshima, 2013/01/15 13:46:03]

Done.

+
   LeaveFunction("");
   return ret;
 }
@@ -2913,7 +2993,17 @@ bool SSLClientSocketNSS::IsConnectedAndIdle() const {
   // transport_->socket()->IsConnectedAndIdle() returns the desired false
   // when we receive close_notify.
   EnterFunction("");
-  bool ret = completed_handshake_ && transport_->socket()->IsConnectedAndIdle();
+  bool ret;
+
+  if (!completed_handshake_)
+    ret = false;
+  else if (core_->HasPendingAsyncOperation())
+    ret = true;  // This is probably wrong, but mirrors TCP.
+  else if (core_->HasUnhandledData())

[Ryan Sleevi, 2013/01/09 19:01:52]

Same here.

Design wise, you can probably simplify this:

if (!IsConnected())
  return false
if (core_->HasUnhandledData())
  return false;
return transport_->socket()->IsConnectedAndIdle();

[Takashi Toyoshima, 2013/01/10 07:46:29]

I tried to keep EnterFunction() and LeaveFunction() to work here. But can I
remove them? If so, I can simplify it as you suggested.

[Ryan Sleevi, 2013/01/10 08:01:23]

Just substitute the return for ret = ... & elseifs, like you have. Sorry for
being shorthand lazy here :)

[Takashi Toyoshima, 2013/01/10 08:17:14]

Ah, I see. You mean that I should reuse IsConnected() function here. I agreed.

[Takashi Toyoshima, 2013/01/15 13:46:03]

I applied Wan-Teh's suggestion after applying Ryan's idea.

+    ret = false;
+  else
+    ret = transport_->socket()->IsConnectedAndIdle();
+
   LeaveFunction("");
   return ret;
 }