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..c6bc74bc1acd9b3240cc56082cbcc2def9e89c75 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,24 @@ 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;
+}
+
 void SSLClientSocketOpenSSL::GetSSLCertRequestInfo(
     SSLCertRequestInfo* cert_request_info) {
   cert_request_info->host_and_port = host_and_port_;
@@ -416,6 +419,15 @@ int SSLClientSocketOpenSSL::Connect(const CompletionCallback& callback) {
 
   // Set up new ssl object.
   int rv = Init();
+
+  if (!handshake_completion_callback_.is_null()) {

[wtc, 2014/07/31 02:02:24]

1. Don't separate the Init() call from the check for its failure (lines
431-434).

2. Why is it necessary to call
context->session_cache()->SetSessionAddedCallback() after the Init() call?

[mshelley, 2014/07/31 20:11:37]

Init() initializes ssl_, which is what I map to callbacks in the session cache.
If i call SetSessionAddedCallback before Init(), then I'm just mapping to an
empty pointer, which means the cache will never notify the messenger when a
session completes successfully.

+    SSLContext* context = SSLContext::GetInstance();
+    context->session_cache()->SetSessionAddedCallback(
+        ssl_,
+        base::Bind(&SSLClientSocketOpenSSL::OnHandshakeCompletion,
+                   base::Unretained(this)));
+  }
+
   if (rv != OK) {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_SSL_CONNECT, rv);
     return rv;
@@ -430,13 +442,21 @@ 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() {
+  // If a handshake was pending (Connect() had been called), notify interested
+  // parties that it's been aborted now. If the handshake had already completed,f
+  // this is a no-op.
+  OnHandshakeCompletion();
   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_);
@@ -610,6 +630,11 @@ int SSLClientSocketOpenSSL::Read(IOBuffer* buf,
       was_ever_used_ = true;
     user_read_buf_ = NULL;
     user_read_buf_len_ = 0;
+    if (rv <= 0) {
+      // Failure of a read attempt may indicate a failed false start
+      // connection.
+      OnHandshakeCompletion();
+    }
   }
 
   return rv;
@@ -630,6 +655,11 @@ int SSLClientSocketOpenSSL::Write(IOBuffer* buf,
       was_ever_used_ = true;
     user_write_buf_ = NULL;
     user_write_buf_len_ = 0;
+    if (rv < 0) {
+      // Failure of a write attempt may indicate a failed false start
+      // connection.
+      OnHandshakeCompletion();
+    }
   }
 
   return rv;
@@ -658,7 +688,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 +792,11 @@ void SSLClientSocketOpenSSL::DoReadCallback(int rv) {
     was_ever_used_ = true;
   user_read_buf_ = NULL;
   user_read_buf_len_ = 0;
+  if (rv <= 0) {
+    // Failure of a read attempt may indicate a failed false start
+    // connection.
+    OnHandshakeCompletion();
+  }
   base::ResetAndReturn(&user_read_callback_).Run(rv);
 }
 
@@ -772,9 +807,23 @@ void SSLClientSocketOpenSSL::DoWriteCallback(int rv) {
     was_ever_used_ = true;
   user_write_buf_ = NULL;
   user_write_buf_len_ = 0;
+  if (rv < 0) {
+    // Failure of a write attempt may indicate a failed false start
+    // connection.
+    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;
@@ -902,6 +951,8 @@ int SSLClientSocketOpenSSL::DoVerifyCertComplete(int result) {
 }
 
 void SSLClientSocketOpenSSL::DoConnectCallback(int rv) {
+  if (rv < OK)
+    OnHandshakeCompletion();
   if (!user_connect_callback_.is_null()) {
     CompletionCallback c = user_connect_callback_;
     user_connect_callback_.Reset();
@@ -1015,6 +1066,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 +1168,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());