Implements new, more robust design for communicating between SSLConnectJobs.

net/socket/ssl_client_socket_pool.cc

Index: net/socket/ssl_client_socket_pool.cc
diff --git a/net/socket/ssl_client_socket_pool.cc b/net/socket/ssl_client_socket_pool.cc
index 2c704658820bc06a8404679c2f796be269657387..3032a08bf49203ef17ae9d8565de00a437b79013 100644
--- a/net/socket/ssl_client_socket_pool.cc
+++ b/net/socket/ssl_client_socket_pool.cc
@@ -94,6 +94,50 @@ SSLSocketParams::GetHttpProxyConnectionParams() const {
   return http_proxy_params_;
 }
 
+bool SSLConnectJobMessenger::CanProceed(SSLClientSocket* ssl_socket) {
+  // If the session is in the session cache, or there are no connecting
+  // sockets allow the connection to proceed.
+  if (ssl_socket->InSessionCache() || connecting_sockets_.empty()) {
+    connecting_sockets_.push_back(ssl_socket);

[wtc, 2014/07/08 01:25:42]

IMPORTANT: it is better for CanProceed() to me a simply boolean query that has
no side effect, and let the caller of this function push ssl_socket to
connecting_sockets_ when CanProceed() returns true.

[mshelley, 2014/07/09 19:51:01]

Yeah I agree, moving connecting_sockets_ into MonitorConnectionResult should be
an appropriate solution.

I also thought about this a bit more, and came to the conclusion that the
current way that I'm handling this is fundamentally flawed because it fails to
differentiate between the two different cases in which CanProceed will return
true. 

If the socket's session is already in the session cache, then the socket should
proceed, but I don't need to monitor its connection, nor do I need to add it to
connecting_sockets_, because any connections that follow it will be allowed to
proceed because they too will be in the session cache. 

However, if the socket is not in the session cache, but connecting_sockets_ is
empty, then this socket should be allowed to proceed as a leading connection. In
this case, I do need to watch the connection nor do I need to add the socket to
connecting_sockets_.  

To correct this issue, I added an if statement before MonitorConnectionResult
that will allow the job to bypass this method if it is already in the session
cache. 

+    return true;
+  }
+  return false;
+}
+
+void SSLConnectJobMessenger::MonitorConnectionResult(
+    SSLClientSocket* ssl_socket) {
+  connecting_sockets_.push_back(ssl_socket);
+  ssl_socket->SetSocketFailureCallback(
+      base::Bind(&SSLConnectJobMessenger::OnJobFailed, base::Unretained(this)));
+  ssl_socket->WatchSessionForCompletion(base::Bind(
+      &SSLConnectJobMessenger::OnJobSucceeded, base::Unretained(this)));
+}
+
+void SSLConnectJobMessenger::AddPendingSocket(SSLClientSocket* socket,
+                                              const base::Closure& callback) {
+  pending_sockets_and_callbacks_.AddSocket(socket, callback);
+}
+
+void SSLConnectJobMessenger::OnJobSucceeded() {
+  SSLPendingSocketsAndCallbacks temp_list = pending_sockets_and_callbacks_;
+  pending_sockets_and_callbacks_.Clear();
+  connecting_sockets_.clear();
+  temp_list.RunAllJobs();
+}
+
+void SSLConnectJobMessenger::OnJobFailed() {
+  if (pending_sockets_and_callbacks_.Empty())
+    return;
+  base::Closure cb = pending_sockets_and_callbacks_.GetFirstCallback();

[mmenke, 2014/07/08 21:02:05]

nit:  Style guide discourages abbreviations - should just write out callback.

[mshelley, 2014/07/09 19:51:01]

Done.

+  connecting_sockets_.erase(connecting_sockets_.begin());
+  SSLClientSocket* ssl_socket = pending_sockets_and_callbacks_.GetFirstSocket();
+  ssl_socket->SetIsLeader();

[wtc, 2014/07/08 01:25:42]

Move the ssl_socket->SetIsLeader() call into MonitorConnectionResult(). Both of
the MonitorConnectionResult() callers call ssl_socket->SetIsLeader().

[mshelley, 2014/07/09 19:51:01]

Done.

+  connecting_sockets_.push_back(ssl_socket);
+  pending_sockets_and_callbacks_.EraseFirstEntry();
+  MonitorConnectionResult(ssl_socket);
+  cb.Run();
+}
+
 // Timeout for the SSL handshake portion of the connect.
 static const int kSSLHandshakeTimeoutInSeconds = 30;
 
@@ -107,6 +151,7 @@ SSLConnectJob::SSLConnectJob(const std::string& group_name,
                              ClientSocketFactory* client_socket_factory,
                              HostResolver* host_resolver,
                              const SSLClientSocketContext& context,
+                             SSLConnectJobMessenger* messenger,
                              Delegate* delegate,
                              NetLog* net_log)
     : ConnectJob(group_name,
@@ -127,8 +172,11 @@ SSLConnectJob::SSLConnectJob(const std::string& group_name,
                (params->privacy_mode() == PRIVACY_MODE_ENABLED
                     ? "pm/" + context.ssl_session_cache_shard
                     : context.ssl_session_cache_shard)),
-      callback_(base::Bind(&SSLConnectJob::OnIOComplete,
-                           base::Unretained(this))) {}
+      io_callback_(
+          base::Bind(&SSLConnectJob::OnIOComplete, base::Unretained(this))),
+      messenger_(messenger),
+      weak_factory_(this) {
+}
 
 SSLConnectJob::~SSLConnectJob() {}
 
@@ -144,6 +192,8 @@ LoadState SSLConnectJob::GetLoadState() const {
     case STATE_SOCKS_CONNECT_COMPLETE:
     case STATE_TUNNEL_CONNECT:
       return transport_socket_handle_->GetLoadState();
+    case STATE_CREATE_SSL_SOCKET:
+    case STATE_CHECK_FOR_RESUME:
     case STATE_SSL_CONNECT:
     case STATE_SSL_CONNECT_COMPLETE:
       return LOAD_STATE_SSL_HANDSHAKE;
@@ -165,6 +215,10 @@ void SSLConnectJob::GetAdditionalErrorState(ClientSocketHandle* handle) {
     handle->set_is_ssl_error(true);
 }
 
+base::WeakPtr<SSLConnectJob> SSLConnectJob::GetWeakPtr() {

[mmenke, 2014/07/08 21:02:05]

In general, weak pointers should not be publicly exposed, to keep lifetime
expectations clearer.  I don't think this function is even needed - looks like
the only consumer is SSLConnectJob itself.

[mshelley, 2014/07/09 19:51:01]

Done.

+  return weak_factory_.GetWeakPtr();
+}
+
 void SSLConnectJob::OnIOComplete(int result) {
   int rv = DoLoop(result);
   if (rv != ERR_IO_PENDING)
@@ -200,6 +254,12 @@ int SSLConnectJob::DoLoop(int result) {
       case STATE_TUNNEL_CONNECT_COMPLETE:
         rv = DoTunnelConnectComplete(rv);
         break;
+      case STATE_CREATE_SSL_SOCKET:
+        rv = DoCreateSSLSocket();
+        break;
+      case STATE_CHECK_FOR_RESUME:
+        rv = DoCheckForResume();
+        break;
       case STATE_SSL_CONNECT:
         DCHECK_EQ(OK, rv);
         rv = DoSSLConnect();
@@ -227,14 +287,16 @@ int SSLConnectJob::DoTransportConnect() {
   return transport_socket_handle_->Init(group_name(),
                                         direct_params,
                                         priority(),
-                                        callback_,
+                                        io_callback_,
                                         transport_pool_,
                                         net_log());
 }
 
 int SSLConnectJob::DoTransportConnectComplete(int result) {
-  if (result == OK)
-    next_state_ = STATE_SSL_CONNECT;
+  if (result != OK)
+    return result;
+
+  next_state_ = STATE_CREATE_SSL_SOCKET;
 
   return result;
 }
@@ -248,14 +310,16 @@ int SSLConnectJob::DoSOCKSConnect() {
   return transport_socket_handle_->Init(group_name(),
                                         socks_proxy_params,
                                         priority(),
-                                        callback_,
+                                        io_callback_,
                                         socks_pool_,
                                         net_log());
 }
 
 int SSLConnectJob::DoSOCKSConnectComplete(int result) {
-  if (result == OK)
-    next_state_ = STATE_SSL_CONNECT;
+  if (result != OK)
+    return result;
+
+  next_state_ = STATE_CREATE_SSL_SOCKET;
 
   return result;
 }
@@ -270,7 +334,7 @@ int SSLConnectJob::DoTunnelConnect() {
   return transport_socket_handle_->Init(group_name(),
                                         http_proxy_params,
                                         priority(),
-                                        callback_,
+                                        io_callback_,
                                         http_proxy_pool_,
                                         net_log());
 }
@@ -291,12 +355,15 @@ int SSLConnectJob::DoTunnelConnectComplete(int result) {
   if (result < 0)
     return result;
 
-  next_state_ = STATE_SSL_CONNECT;
+  next_state_ = STATE_CREATE_SSL_SOCKET;
   return result;
 }
 
-int SSLConnectJob::DoSSLConnect() {
-  next_state_ = STATE_SSL_CONNECT_COMPLETE;
+int SSLConnectJob::DoCreateSSLSocket() {
+  if (SSLClientSocket::GetEnableJobWaiting())
+    next_state_ = STATE_CHECK_FOR_RESUME;
+  else
+    next_state_ = STATE_SSL_CONNECT;
   // Reset the timeout to just the time allowed for the SSL handshake.
   ResetTimer(base::TimeDelta::FromSeconds(kSSLHandshakeTimeoutInSeconds));
 
@@ -314,14 +381,36 @@ int SSLConnectJob::DoSSLConnect() {
     connect_timing_.dns_end = socket_connect_timing.dns_end;
   }
 
-  connect_timing_.ssl_start = base::TimeTicks::Now();
-
   ssl_socket_ = client_socket_factory_->CreateSSLClientSocket(
       transport_socket_handle_.Pass(),
       params_->host_and_port(),
       params_->ssl_config(),
       context_);
-  return ssl_socket_->Connect(callback_);
+  return OK;
+}
+
+int SSLConnectJob::DoCheckForResume() {
+  next_state_ = STATE_SSL_CONNECT;
+  if (messenger_->CanProceed(ssl_socket_.get())) {
+    messenger_->MonitorConnectionResult(ssl_socket_.get());
+    ssl_socket_->SetIsLeader();
+    return OK;
+  }
+  messenger_->AddPendingSocket(
+      ssl_socket_.get(),
+      base::Bind(&net::SSLConnectJob::ResumeSSLConnection, GetWeakPtr()));
+  return ERR_IO_PENDING;
+}
+
+int SSLConnectJob::DoSSLConnect() {
+  next_state_ = STATE_SSL_CONNECT_COMPLETE;
+
+  connect_timing_.ssl_start = base::TimeTicks::Now();
+
+  int result = ssl_socket_->Connect(io_callback_);
+  if (result != OK && SSLClientSocket::GetEnableJobWaiting())
+    messenger_->OnJobFailed();

[wtc, 2014/07/08 01:25:42]

IMPORTANT: move lines 411-412 to DoSSLConnectComplete(), so that this is done
for both synchronous and asynchronous failures of ssl_socket_->Connect().

[mshelley, 2014/07/09 19:51:01]

Done.

+  return result;
 }
 
 int SSLConnectJob::DoSSLConnectComplete(int result) {
@@ -351,8 +440,9 @@ int SSLConnectJob::DoSSLConnectComplete(int result) {
       ssl_socket_->set_was_spdy_negotiated(true);
     }
   }
-  if (params_->want_spdy_over_npn() && !ssl_socket_->was_spdy_negotiated())
+  if (params_->want_spdy_over_npn() && !ssl_socket_->was_spdy_negotiated()) {
     return ERR_NPN_NEGOTIATION_FAILED;
+  }

[wtc, 2014/07/08 01:25:42]

Remove the curly braces, unless these were added by git cl format.

[mshelley, 2014/07/09 19:51:01]

Done.

 
   // Spdy might be turned on by default, or it might be over npn.
   bool using_spdy = params_->force_spdy_over_ssl() ||
@@ -411,9 +501,9 @@ int SSLConnectJob::DoSSLConnectComplete(int result) {
     }
 
     const std::string& host = params_->host_and_port().host();
-    bool is_google = host == "google.com" ||
-                     (host.size() > 11 &&
-                      host.rfind(".google.com") == host.size() - 11);
+    bool is_google =
+        host == "google.com" ||
+        (host.size() > 11 && host.rfind(".google.com") == host.size() - 11);
     if (is_google) {
       UMA_HISTOGRAM_CUSTOM_TIMES("Net.SSL_Connection_Latency_Google2",
                                  connect_duration,
@@ -449,6 +539,12 @@ int SSLConnectJob::DoSSLConnectComplete(int result) {
   return result;
 }
 
+void SSLConnectJob::ResumeSSLConnection() {
+  DCHECK_EQ(next_state_, STATE_SSL_CONNECT);
+  next_state_ = STATE_SSL_CONNECT;

[wtc, 2014/07/08 01:25:42]

Delete this line.

[mshelley, 2014/07/09 19:51:01]

Done.

+  OnIOComplete(OK);
+}
+
 SSLConnectJob::State SSLConnectJob::GetInitialState(
     SSLSocketParams::ConnectionType connection_type) {
   switch (connection_type) {
@@ -482,7 +578,8 @@ SSLClientSocketPool::SSLConnectJobFactory::SSLConnectJobFactory(
       client_socket_factory_(client_socket_factory),
       host_resolver_(host_resolver),
       context_(context),
-      net_log_(net_log) {
+      net_log_(net_log),
+      messenger_map_(new MessengerMap) {
   base::TimeDelta max_transport_timeout = base::TimeDelta();
   base::TimeDelta pool_timeout;
   if (transport_pool_)
@@ -520,21 +617,24 @@ SSLClientSocketPool::SSLClientSocketPool(
     : transport_pool_(transport_pool),
       socks_pool_(socks_pool),
       http_proxy_pool_(http_proxy_pool),
-      base_(this, max_sockets, max_sockets_per_group, histograms,
+      base_(this,
+            max_sockets,
+            max_sockets_per_group,
+            histograms,
             ClientSocketPool::unused_idle_socket_timeout(),
             ClientSocketPool::used_idle_socket_timeout(),
-            new SSLConnectJobFactory(transport_pool,
-                                     socks_pool,
-                                     http_proxy_pool,
-                                     client_socket_factory,
-                                     host_resolver,
-                                     SSLClientSocketContext(
-                                         cert_verifier,
-                                         server_bound_cert_service,
-                                         transport_security_state,
-                                         cert_transparency_verifier,
-                                         ssl_session_cache_shard),
-                                     net_log)),
+            new SSLConnectJobFactory(
+                transport_pool,
+                socks_pool,
+                http_proxy_pool,
+                client_socket_factory,
+                host_resolver,
+                SSLClientSocketContext(cert_verifier,
+                                       server_bound_cert_service,
+                                       transport_security_state,
+                                       cert_transparency_verifier,
+                                       ssl_session_cache_shard),
+                net_log)),
       ssl_config_service_(ssl_config_service) {
   if (ssl_config_service_.get())
     ssl_config_service_->AddObserver(this);
@@ -556,11 +656,35 @@ SSLClientSocketPool::SSLConnectJobFactory::NewConnectJob(
     const std::string& group_name,
     const PoolBase::Request& request,
     ConnectJob::Delegate* delegate) const {
-  return scoped_ptr<ConnectJob>(
-      new SSLConnectJob(group_name, request.priority(), request.params(),
-                        ConnectionTimeout(), transport_pool_, socks_pool_,
-                        http_proxy_pool_, client_socket_factory_,
-                        host_resolver_, context_, delegate, net_log_));
+  SSLConnectJobMessenger* messenger;
+  std::string cache_key = request.params()->host_and_port().ToString() + "\\" +
+                          context_.ssl_session_cache_shard;

[wtc, 2014/07/08 01:25:42]

This should be abstracted into a function. Ideally there should only be one
function that constructs the cache key.

[mshelley, 2014/07/09 19:51:01]

Done.

+
+  if (SSLClientSocket::GetEnableJobWaiting()) {
+    MessengerMap::const_iterator it = messenger_map_->find(cache_key);
+    if (it == messenger_map_->end()) {
+      std::pair<MessengerMap::iterator, bool> iter = messenger_map_->insert(
+          MessengerMap::value_type(cache_key, new SSLConnectJobMessenger()));
+      it = iter.first;
+    }
+    messenger = it->second;
+  } else {
+    messenger = NULL;
+  }
+
+  return scoped_ptr<ConnectJob>(new SSLConnectJob(group_name,
+                                                  request.priority(),
+                                                  request.params(),
+                                                  ConnectionTimeout(),
+                                                  transport_pool_,
+                                                  socks_pool_,
+                                                  http_proxy_pool_,
+                                                  client_socket_factory_,
+                                                  host_resolver_,
+                                                  context_,
+                                                  messenger,
+                                                  delegate,
+                                                  net_log_));
 }
 
 base::TimeDelta