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..69a939dd1e9c758469d68f31e16be16b3d62419b 100644
--- a/net/socket/ssl_client_socket_pool.cc
+++ b/net/socket/ssl_client_socket_pool.cc
@@ -94,6 +94,69 @@ SSLSocketParams::GetHttpProxyConnectionParams() const {
   return http_proxy_params_;
 }
 
+void SSLConnectJobMessenger::RemoveSocket(SSLClientSocket* ssl_socket) {
+  // Sockets do not need to be removed from connecting_sockets_ because
+  // either OnJobSucceeded or OnJobFailed will do this.
+  for (SSLPendingSocketsAndCallbacks::iterator it =
+           pending_sockets_and_callbacks_.begin();
+       it != pending_sockets_and_callbacks_.end();
+       ++it) {
+    if (it->socket == ssl_socket) {
+      pending_sockets_and_callbacks_.erase(it);
+      return;
+    }
+  }
+}
+
+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.
+  return ssl_socket->InSessionCache() || connecting_sockets_.empty();
+}
+
+void SSLConnectJobMessenger::MonitorConnectionResult(
+    SSLClientSocket* ssl_socket) {
+  connecting_sockets_.push_back(ssl_socket);
+  // TODO(mshelley): Both of these callbacks will use WeakPtr in future CL.
+  ssl_socket->SetHandshakeFailureCallback(
+      base::Bind(&SSLConnectJobMessenger::OnJobFailed, base::Unretained(this)));
+  ssl_socket->SetHandshakeSuccessCallback(base::Bind(
+      &SSLConnectJobMessenger::OnJobSucceeded, base::Unretained(this)));
+}
+
+void SSLConnectJobMessenger::AddPendingSocket(SSLClientSocket* socket,
+                                              const base::Closure& callback) {
+  pending_sockets_and_callbacks_.push_back(SocketAndCallback(socket, callback));
+}
+
+void SSLConnectJobMessenger::OnJobSucceeded() {
+  SSLPendingSocketsAndCallbacks temp_list = pending_sockets_and_callbacks_;
+  pending_sockets_and_callbacks_.clear();
+  connecting_sockets_.clear();
+  RunAllCallbacks(temp_list);
+}
+
+void SSLConnectJobMessenger::OnJobFailed() {
+  if (pending_sockets_and_callbacks_.empty())
+    return;
+  base::Closure callback = pending_sockets_and_callbacks_[0].callback;
+  connecting_sockets_.erase(connecting_sockets_.begin());
+  SSLClientSocket* ssl_socket = pending_sockets_and_callbacks_[0].socket;
+  pending_sockets_and_callbacks_.erase(pending_sockets_and_callbacks_.begin());
+  MonitorConnectionResult(ssl_socket);
+  callback.Run();
+}
+
+void SSLConnectJobMessenger::RunAllCallbacks(
+    const SSLPendingSocketsAndCallbacks& pending_sockets_and_callbacks) {
+  for (std::vector<SocketAndCallback>::const_iterator it =
+           pending_sockets_and_callbacks.begin();
+       it != pending_sockets_and_callbacks.end();
+       ++it) {
+    it->callback.Run();
+  }
+}
+
 // Timeout for the SSL handshake portion of the connect.
 static const int kSSLHandshakeTimeoutInSeconds = 30;
 
@@ -107,6 +170,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,10 +191,16 @@ 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() {}
+SSLConnectJob::~SSLConnectJob() {
+  if (ssl_socket_.get() != NULL && messenger_ != NULL)
+    messenger_->RemoveSocket(ssl_socket_.get());
+}
 
 LoadState SSLConnectJob::GetLoadState() const {
   switch (next_state_) {
@@ -144,6 +214,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;
@@ -200,6 +272,12 @@ int SSLConnectJob::DoLoop(int result) {
       case STATE_TUNNEL_CONNECT_COMPLETE:
         rv = DoTunnelConnectComplete(rv);
         break;
+      case STATE_CREATE_SSL_SOCKET:
+        rv = DoCreateSSLSocket();

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

Not sure if this has been discussed, but one potential problem with this
approach is that if the socket we're delaying all other connections on stalls
for a while, the server may start timing out all the other sockets while we're
waiting.  Some servers may have very aggressive timeouts.

I'm not positive this will be a problem, but it's something to be on the lookout
for.

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

In the past we have talked about timing the leading socket's connection, and
allowing the pending sockets to proceed prematurely if the leading socket
stalls/takes too long. Perhaps that would prevent/minimize this issue?

I guess the tricky part with that solution would be determining what qualifies
as "too long". 

[mmenke, 2014/07/17 21:27:55]

In bugs, I've heard of idle timeouts as low as 10 seconds, at least on
never-used connections.

At the socket pool level, we start a backup job if a connection attempt takes
more than 250 milliseconds.  Assuming an SSL handshake is going to take two
round trips, after establishing the connection, that gives us an estimate of 500
milliseconds.  Of course, there's more potential cost to guess wrong here than
there is to connecting another socket.

Anyhow, nothing to worry about in this CL, though I think we should probably add
a histogram to try and catch that case before enabling the new behavior via a
field trial.

+        break;
+      case STATE_CHECK_FOR_RESUME:
+        rv = DoCheckForResume();
+        break;
       case STATE_SSL_CONNECT:
         DCHECK_EQ(OK, rv);
         rv = DoSSLConnect();
@@ -227,14 +305,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 +328,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 +352,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 +373,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 (messenger_ != NULL)
+    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 +399,35 @@ 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;
+  // TODO(mshelley): Remove duplicate InSessionCache() calls.
+  if (messenger_->CanProceed(ssl_socket_.get())) {
+    if (!ssl_socket_->InSessionCache())
+      messenger_->MonitorConnectionResult(ssl_socket_.get());
+    return OK;
+  }
+  messenger_->AddPendingSocket(
+      ssl_socket_.get(),
+      base::Bind(&net::SSLConnectJob::ResumeSSLConnection,
+                 weak_factory_.GetWeakPtr()));
+  return ERR_IO_PENDING;
+}
+
+int SSLConnectJob::DoSSLConnect() {
+  next_state_ = STATE_SSL_CONNECT_COMPLETE;
+
+  connect_timing_.ssl_start = base::TimeTicks::Now();
+
+  return ssl_socket_->Connect(io_callback_);
 }
 
 int SSLConnectJob::DoSSLConnectComplete(int result) {
@@ -411,9 +517,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 +555,11 @@ int SSLConnectJob::DoSSLConnectComplete(int result) {
   return result;
 }
 
+void SSLConnectJob::ResumeSSLConnection() {
+  DCHECK_EQ(next_state_, STATE_SSL_CONNECT);
+  OnIOComplete(OK);
+}
+
 SSLConnectJob::State SSLConnectJob::GetInitialState(
     SSLSocketParams::ConnectionType connection_type) {
   switch (connection_type) {
@@ -482,7 +593,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_)
@@ -501,6 +613,17 @@ SSLClientSocketPool::SSLConnectJobFactory::SSLConnectJobFactory(
       base::TimeDelta::FromSeconds(kSSLHandshakeTimeoutInSeconds);
 }
 
+SSLClientSocketPool::SSLConnectJobFactory::~SSLConnectJobFactory() {
+  for (MessengerMap::iterator it = messenger_map_->begin();
+       it != messenger_map_->end();
+       ++it)
+    delete it->second;
+  messenger_map_->erase(messenger_map_->begin(), messenger_map_->end());
+}
+
+// static
+bool SSLClientSocketPool::enable_connect_job_waiting_ = false;
+
 SSLClientSocketPool::SSLClientSocketPool(
     int max_sockets,
     int max_sockets_per_group,
@@ -520,21 +643,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 +682,33 @@ 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 = NULL;
+  if (SSLClientSocketPool::get_enable_connect_job_waiting()) {
+    std::string cache_key = SSLClientSocket::FormatSessionCacheKey(
+        request.params()->host_and_port().ToString(),
+        context_.ssl_session_cache_shard);
+    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;
+  }
+
+  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
@@ -679,6 +827,16 @@ bool SSLClientSocketPool::CloseOneIdleConnection() {
   return base_.CloseOneIdleConnectionInHigherLayeredPool();
 }
 
+// static
+void SSLClientSocketPool::set_enable_connect_job_waiting(bool enable) {
+  enable_connect_job_waiting_ = enable;
+}
+
+// static
+bool SSLClientSocketPool::get_enable_connect_job_waiting() {
+  return enable_connect_job_waiting_;
+}
+
 void SSLClientSocketPool::OnSSLConfigChanged() {
   FlushWithError(ERR_NETWORK_CHANGED);
 }