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..ccdb7084e8f00f40c6fce5b52ceacac07781e4f5 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -571,9 +571,9 @@ int MapNSSClientHandshakeError(PRErrorCode err) {
 //                               |-------------------------------V
 //                                                      BufferRecvComplete()
 //                                                               |
-//                                                       PostOrRunCallback()
+//                                                        DoReadCallback()
 //                               V-------------------------------|
-//                        PostOrRunCallback()
+//                         UpdateReadStatus()

[Ryan Sleevi, 2013/01/07 18:59:04]

NACK on this. The intention of PostOrRunCallback is to force synchronous
continuation in situations where there is not a dedicated SSL thread. This
change introduces an extra yield for the single-threaded case, which we
definitely don't want.

[Takashi Toyoshima, 2013/01/08 06:25:39]

Oh, I miss the case of single-threaded model and my understanding on
PostOrRunCallback method is not clear.
Please let me reconsider this change from the viewpoint of threads and follow
your original suggestion carefully.

 //         V---------------------|
 //    (Read Callback)
 //
@@ -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();
+
  private:
   friend class base::RefCountedThreadSafe<Core>;
   ~Core();
@@ -735,6 +739,10 @@ 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 CompletionCallback& callback,
+                       int rv);
 
   // Client channel ID handler.
   static SECStatus ClientChannelIDHandler(
@@ -812,6 +820,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.
+  int unhandled_buffer_size_;
+  bool waiting_read_;
+  bool waiting_write_;
+
   ////////////////////////////////////////////////////////////////////////////
   // Members that are ONLY accessed on the NSS task runner:
   ////////////////////////////////////////////////////////////////////////////
@@ -892,6 +905,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 +1120,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 +1132,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 +1165,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 +1177,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 +1188,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 +2289,19 @@ 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);
+  CompletionCallback cb = base::ResetAndReturn(&user_read_callback_);
+  // Curry the |amount_int_read_buffer| and |cb| back to the network task
+  // runner.
+  network_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&Core::UpdateNSSStatus,
+                 this,
+                 amount_in_read_buffer,
+                 true,  // Operation is read.
+                 cb,
+                 rv));

[Ryan Sleevi, 2013/01/07 18:59:04]

NACK: See above comments re: PostOrRunCallback. You should not be directly
calling PostTask here, since in the single-threaded case, OnNSSTaskRunner ==
OnNetworkTaskRunner() == true

[Takashi Toyoshima, 2013/01/08 06:25:39]

Done.

 }
 
 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);
+  CompletionCallback cb = base::ResetAndReturn(&user_write_callback_);
+  // Curry the |cb| back to the network task runner.
+  network_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&Core::UpdateNSSStatus,
+                 this,
+                 amount_in_read_buffer,
+                 false,  // Operation is not read, but write.
+                 cb,
+                 rv));

[Ryan Sleevi, 2013/01/07 18:59:04]

Also here

[Takashi Toyoshima, 2013/01/08 06:25:39]

Done.

+}
+
+void SSLClientSocketNSS::Core::UpdateNSSStatus(
+    int amount_in_read_buffer,
+    bool operation_is_read,
+    const CompletionCallback& callback,
+    int rv) {

[Ryan Sleevi, 2013/01/07 18:59:04]

If you're going to post a callback, you should post it as a base::Closure,
pre-bound to rv. The existing code (DoWriteCallback/DoReadCallback) already
handles the DCHECKs on |rv|

[Takashi Toyoshima, 2013/01/08 06:25:39]

Done.

+  DCHECK(OnNetworkTaskRunner());
+  DCHECK_NE(ERR_IO_PENDING, rv);
+  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;
+  PostOrRunCallback(FROM_HERE, base::Bind(callback, rv));

[Ryan Sleevi, 2013/01/07 18:59:04]

At this point, you're guaranteed to be OnNetworkTaskRunner (line 2337), so
there's no point in calling PostOrRunCallback.

Presuming the updated type to base::Closure

callback.Run()

[Takashi Toyoshima, 2013/01/08 06:25:39]

Done.

 }
 
 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())
+    ret = true;
+  else
+    ret = transport_->socket()->IsConnected();
+
   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())
+    ret = false;
+  else
+    ret = transport_->socket()->IsConnectedAndIdle();
+
   LeaveFunction("");
   return ret;
 }