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..07df7bb9e3b192a903a7ac4dac641a2435e934a0 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -87,14 +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;
-}
 

[mmenke, 2014/07/17 19:21:32]

nit:  Remove extra blank line.

[mshelley, 2014/07/17 21:12:48]

Done.

 }  // namespace
 
@@ -139,7 +131,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 +352,34 @@ 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::SetHandshakeSuccessCallback(
+    const base::Closure& callback) {
+  success_callback_ = callback;
+  SSLContext* context = SSLContext::GetInstance();
+  context->session_cache()->SetSessionAddedCallback(
+      ssl_,
+      base::Bind(&SSLClientSocketOpenSSL::OnHandshakeSuccess,
+                 base::Unretained(this)));
+}
+
+void SSLClientSocketOpenSSL::SetHandshakeFailureCallback(
+    const base::Closure& callback) {
+  error_callback_ = callback;
+}
+
 void SSLClientSocketOpenSSL::GetSSLCertRequestInfo(
     SSLCertRequestInfo* cert_request_info) {
   cert_request_info->host_and_port = host_and_port_;
@@ -432,11 +446,17 @@ int SSLClientSocketOpenSSL::Connect(const CompletionCallback& callback) {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_SSL_CONNECT, rv);
   }
 
+  if (rv < OK)
+    OnHandshakeFailure();
+
   return rv > OK ? OK : rv;
 }
 
 void SSLClientSocketOpenSSL::Disconnect() {
+  OnHandshakeFailure();
   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_);
@@ -658,7 +678,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.
@@ -775,6 +795,21 @@ void SSLClientSocketOpenSSL::DoWriteCallback(int rv) {
   base::ResetAndReturn(&user_write_callback_).Run(rv);
 }
 
+std::string SSLClientSocketOpenSSL::GetSessionCacheKey() const {
+  return FormatSessionCacheKey(host_and_port_.ToString(),
+                               ssl_session_cache_shard_);
+}
+
+void SSLClientSocketOpenSSL::OnHandshakeSuccess() {
+  error_callback_.Reset();
+  base::ResetAndReturn(&success_callback_).Run();
+}
+
+void SSLClientSocketOpenSSL::OnHandshakeFailure() {
+  if (!error_callback_.is_null())
+    base::ResetAndReturn(&error_callback_).Run();

[mmenke, 2014/07/17 19:21:32]

Clear the success_callback_, too?  Admittedly, shouldn't be possible for it to
end up being called after this point, anyways, but seems best to be safe.

[mshelley, 2014/07/17 21:12:49]

Done.

+}
+
 bool SSLClientSocketOpenSSL::DoTransportIO() {
   bool network_moved = false;
   int rv;
@@ -925,6 +960,8 @@ void SSLClientSocketOpenSSL::OnHandshakeIOComplete(int result) {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_SSL_CONNECT, rv);
     DoConnectCallback(rv);
   }
+  if (rv < OK)
+    OnHandShakeFailure();

[mmenke, 2014/07/17 19:21:32]

Instead of having two places that call into OnHandShakeFailure after a call to
DoHandshakeLoop, could have just one call at the end of DoHandshakeLoop, when rv
< OK && rv != ERR_IO_PENDING.

[mmenke, 2014/07/17 19:21:32]

If rv is ERR_IO_PENDING, we're still waiting for another response, so this is
not a handshake failure.  The test should go up into the previous if block.

We should probably also have a test for that case.

[mshelley, 2014/07/17 21:12:49]

Done.

 }
 
 void SSLClientSocketOpenSSL::OnSendComplete(int result) {
@@ -1066,6 +1103,10 @@ int SSLClientSocketOpenSSL::DoPayloadRead() {
                   user_read_buf_len_ - total_bytes_read);
     if (rv > 0)
       total_bytes_read += rv;
+    // Failure of a read attempt indicates a failed false start
+    // connection.

[mmenke, 2014/07/17 19:21:32]

This should be indented two more, and go after the else.

[mshelley, 2014/07/17 21:12:49]

Done.

+    else
+      OnHandshakeFailure();

[mmenke, 2014/07/17 19:21:32]

nit:  Use braces on both parts of an if statement, if one takes up more than one
line.

[mshelley, 2014/07/17 21:12:49]

Done.

   } while (total_bytes_read < user_read_buf_len_ && rv > 0);
 
   if (total_bytes_read == user_read_buf_len_) {
@@ -1116,13 +1157,15 @@ 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_);
-
+  // Failure of the second write attempt indicates a failed false start
+  // connection.
   if (rv >= 0) {
     net_log_.AddByteTransferEvent(NetLog::TYPE_SSL_SOCKET_BYTES_SENT, rv,
                                   user_write_buf_->data());
     return rv;
   }
 
+  OnHandshakeFailure();
   int err = SSL_get_error(ssl_, rv);
   return MapOpenSSLError(err, err_tracer);
 }