Implements new, more robust design for communicating between SSLConnectJobs.

net/socket/ssl_client_socket_openssl.cc

Index: net/socket/ssl_client_socket_openssl.cc
diff --git a/net/socket/ssl_client_socket_openssl.cc b/net/socket/ssl_client_socket_openssl.cc
index 4ff8d438e965b1fdf0b5b7f8b4ecdb75e1d14d66..6beb9e2da0e301053800492f29e634f32688b281 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -87,15 +87,6 @@ int GetNetSSLVersion(SSL* ssl) {
   }
 }
 
-// Compute a unique key string for the SSL session cache. |socket| is an
-// input socket object. Return a string.
-std::string GetSocketSessionCacheKey(const SSLClientSocketOpenSSL& socket) {
-  std::string result = socket.host_and_port().ToString();
-  result.append("/");
-  result.append(socket.ssl_session_cache_shard());
-  return result;
-}
-
 }  // namespace
 
 class SSLClientSocketOpenSSL::SSLContext {
@@ -139,7 +130,7 @@ class SSLClientSocketOpenSSL::SSLContext {
   static std::string GetSessionCacheKey(const SSL* ssl) {
     SSLClientSocketOpenSSL* socket = GetInstance()->GetClientSocketFromSSL(ssl);
     DCHECK(socket);
-    return GetSocketSessionCacheKey(*socket);
+    return socket->GetSessionCacheKey();
   }
 
   static SSLSessionCacheOpenSSL::Config kDefaultSessionCacheConfig;
@@ -360,12 +351,29 @@ SSLClientSocketOpenSSL::SSLClientSocketOpenSSL(
       npn_status_(kNextProtoUnsupported),
       channel_id_request_return_value_(ERR_UNEXPECTED),
       channel_id_xtn_negotiated_(false),
-      net_log_(transport_->socket()->NetLog()) {}
+      net_log_(transport_->socket()->NetLog()) {
+}
 
 SSLClientSocketOpenSSL::~SSLClientSocketOpenSSL() {
   Disconnect();
 }
 
+bool SSLClientSocketOpenSSL::InSessionCache() const {
+  SSLContext* context = SSLContext::GetInstance();
+  std::string cache_key = GetSessionCacheKey();
+  return context->session_cache()->SSLSessionIsInCache(cache_key);
+}
+
+void SSLClientSocketOpenSSL::SetHandshakeCompletionCallback(
+    const base::Closure& callback) {
+  handshake_completion_callback_ = callback;
+  SSLContext* context = SSLContext::GetInstance();
+  context->session_cache()->SetSessionAddedCallback(
+      ssl_,
+      base::Bind(&SSLClientSocketOpenSSL::OnHandshakeCompletion,
+                 base::Unretained(this)));
+}
+
 void SSLClientSocketOpenSSL::GetSSLCertRequestInfo(
     SSLCertRequestInfo* cert_request_info) {
   cert_request_info->host_and_port = host_and_port_;
@@ -430,13 +438,18 @@ int SSLClientSocketOpenSSL::Connect(const CompletionCallback& callback) {
     user_connect_callback_ = callback;
   } else {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_SSL_CONNECT, rv);
+    if (rv < OK)
+      OnHandshakeCompletion();
   }
 
   return rv > OK ? OK : rv;
 }
 
 void SSLClientSocketOpenSSL::Disconnect() {
+  OnHandshakeCompletion();

[Ryan Sleevi, 2014/07/25 01:36:36]

This surprises me, since we traditionally don't call callbacks during
Disconnect() ( it's actually documented like that in
http://src.chromium.org/viewvc/chrome/trunk/src/net/socket/socket.h /
http://src.chromium.org/viewvc/chrome/trunk/src/net/socket/stream_socket.h )

[mshelley, 2014/07/26 00:58:27]

The idea here was that we want to ensure that if a socket is randomly deleted,
its messenger will be notified so that it can delete any pending jobs dependent
upon that socket's connection.

Would it be more appropriate to make this call before Disconnect in the
destructor? 

[Ryan Sleevi, 2014/07/26 02:22:32]

Right, I got that part.
I think it's OK to call here, but it means you need to beef up the documentation
for this class (and setting the handshake callback), so that you're explicitly
documenting in the reader the 'edge' cases that can come up (similar to the
previous comments regarding ordering of callback execution), and what is or is
not 'acceptable' behaviour to do in such a callback.

   if (ssl_) {
+    SSLContext* context = SSLContext::GetInstance();
+    context->session_cache()->RemoveSessionAddedCallback(ssl_);
     // Calling SSL_shutdown prevents the session from being marked as
     // unresumable.
     SSL_shutdown(ssl_);
@@ -606,8 +619,13 @@ int SSLClientSocketOpenSSL::Read(IOBuffer* buf,
   if (rv == ERR_IO_PENDING) {
     user_read_callback_ = callback;
   } else {
-    if (rv > 0)
+    if (rv > 0) {
       was_ever_used_ = true;
+    } else if (rv < 0) {
+      // Failure of a read attempt may indicate a failed false start
+      // connection.
+      OnHandshakeCompletion();
+    }

[Ryan Sleevi, 2014/07/25 01:36:36]

I would actually move this to line 632

if (rv != ERR_IO_PENDING && rv < 0)
  OnHandshakeCompletion()

In this case, we shouldn't call the callback until the state updates on line
629/630 and 654/655 have happened (in part, this matches lines 786/787 and
798/799 in maintaining the ordering of variable assignments before callbacks)

[mshelley, 2014/07/26 00:58:27]

Done.

     user_read_buf_ = NULL;
     user_read_buf_len_ = 0;
   }
@@ -626,8 +644,13 @@ int SSLClientSocketOpenSSL::Write(IOBuffer* buf,
   if (rv == ERR_IO_PENDING) {
     user_write_callback_ = callback;
   } else {
-    if (rv > 0)
+    if (rv > 0) {
       was_ever_used_ = true;
+    } else {
+      // Failure of a write attempt may indicate a failed false start
+      // connection.
+      OnHandshakeCompletion();
+    }
     user_write_buf_ = NULL;
     user_write_buf_len_ = 0;
   }
@@ -658,7 +681,7 @@ int SSLClientSocketOpenSSL::Init() {
     return ERR_UNEXPECTED;
 
   trying_cached_session_ = context->session_cache()->SetSSLSessionWithKey(
-      ssl_, GetSocketSessionCacheKey(*this));
+      ssl_, GetSessionCacheKey());
 
   BIO* ssl_bio = NULL;
   // 0 => use default buffer sizes.
@@ -762,6 +785,8 @@ void SSLClientSocketOpenSSL::DoReadCallback(int rv) {
     was_ever_used_ = true;
   user_read_buf_ = NULL;
   user_read_buf_len_ = 0;
+  if (rv < 0)
+    OnHandshakeCompletion();
   base::ResetAndReturn(&user_read_callback_).Run(rv);
 }
 
@@ -772,9 +797,20 @@ void SSLClientSocketOpenSSL::DoWriteCallback(int rv) {
     was_ever_used_ = true;
   user_write_buf_ = NULL;
   user_write_buf_len_ = 0;
+  if (rv < 0)
+    OnHandshakeCompletion();
   base::ResetAndReturn(&user_write_callback_).Run(rv);
 }
 
+std::string SSLClientSocketOpenSSL::GetSessionCacheKey() const {
+  return CreateSessionCacheKey(host_and_port_, ssl_session_cache_shard_);
+}
+
+void SSLClientSocketOpenSSL::OnHandshakeCompletion() {
+  if (!handshake_completion_callback_.is_null())
+    base::ResetAndReturn(&handshake_completion_callback_).Run();
+}
+
 bool SSLClientSocketOpenSSL::DoTransportIO() {
   bool network_moved = false;
   int rv;
@@ -907,6 +943,8 @@ void SSLClientSocketOpenSSL::DoConnectCallback(int rv) {
     user_connect_callback_.Reset();
     c.Run(rv > OK ? OK : rv);
   }
+  if (rv < OK)
+    OnHandshakeCompletion();

[Ryan Sleevi, 2014/07/25 01:36:36]

This surprises me, as it's inverted from your order of events on line 788/799
and 800/801, in which you call your callback than the user's callback.

All the more reason for the expanded comment/documentation on the exact
sequencing of events.

[mshelley, 2014/07/26 00:58:27]

Ah yes, this is wrong; it should be called before user_connect_callback_ is run.

 }
 
 X509Certificate* SSLClientSocketOpenSSL::UpdateServerCert() {
@@ -915,7 +953,6 @@ X509Certificate* SSLClientSocketOpenSSL::UpdateServerCert() {
 
   if (!server_cert_chain_->IsValid())
     DVLOG(1) << "UpdateServerCert received invalid certificate chain from peer";
-
   return server_cert_.get();
 }
 
@@ -1015,6 +1052,7 @@ int SSLClientSocketOpenSSL::DoHandshakeLoop(int last_io_result) {
       rv = OK;  // This causes us to stay in the loop.
     }
   } while (rv != ERR_IO_PENDING && next_handshake_state_ != STATE_NONE);
+
   return rv;
 }
 
@@ -1116,7 +1154,6 @@ int SSLClientSocketOpenSSL::DoPayloadRead() {
 int SSLClientSocketOpenSSL::DoPayloadWrite() {
   crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
   int rv = SSL_write(ssl_, user_write_buf_->data(), user_write_buf_len_);
-
   if (rv >= 0) {
     net_log_.AddByteTransferEvent(NetLog::TYPE_SSL_SOCKET_BYTES_SENT, rv,
                                   user_write_buf_->data());