SSL Connect Job Waiting

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..9f1ef96e5d28faef389a1fa0aa27c54d55751889 100644
--- a/net/socket/ssl_client_socket_pool.cc
+++ b/net/socket/ssl_client_socket_pool.cc
@@ -18,6 +18,7 @@
 #include "net/socket/client_socket_handle.h"
 #include "net/socket/socks_client_socket_pool.h"
 #include "net/socket/ssl_client_socket.h"
+#include "net/socket/ssl_client_socket_openssl.h"

[wtc, 2014/06/13 22:47:24]

This needs to be put inside #if defined(USE_OPENSSL).

Chromium style recommends listing the conditional #include's last, after the
cross-platform #include's, so add the following after line 25:

#if defined(USE_OPENSSL)
#include "net/socket/ssl_client_socket_openssl.h"
#endif

See the Chromium Coding Style page:
http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code

[mshelley1, 2014/06/16 19:02:50]

Done.

 #include "net/socket/transport_client_socket_pool.h"
 #include "net/ssl/ssl_cert_request_info.h"
 #include "net/ssl/ssl_connection_status_flags.h"
@@ -108,7 +109,8 @@ SSLConnectJob::SSLConnectJob(const std::string& group_name,
                              HostResolver* host_resolver,
                              const SSLClientSocketContext& context,
                              Delegate* delegate,
-                             NetLog* net_log)
+                             NetLog* net_log,
+                             PendingJobList* pending)
     : ConnectJob(group_name,
                  timeout_duration,
                  priority,
@@ -127,11 +129,15 @@ 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))) {}
+      callback_(

[Ryan Sleevi, 2014/06/13 23:24:22]

nit: We typically call this io_callback_

[mshelley1, 2014/06/16 19:02:50]

Done.

+          base::Bind(&SSLConnectJob::OnIOComplete, base::Unretained(this))),
+      pending_(pending) {
+}
 
 SSLConnectJob::~SSLConnectJob() {}
 
+bool SSLConnectJob::enable_job_waiting_ = false;
+
 LoadState SSLConnectJob::GetLoadState() const {
   switch (next_state_) {
     case STATE_TUNNEL_CONNECT_COMPLETE:
@@ -144,6 +150,8 @@ LoadState SSLConnectJob::GetLoadState() const {
     case STATE_SOCKS_CONNECT_COMPLETE:
     case STATE_TUNNEL_CONNECT:
       return transport_socket_handle_->GetLoadState();
+    case STATE_CHECK_FOR_RESUME:
+    case STATE_WAIT:
     case STATE_SSL_CONNECT:
     case STATE_SSL_CONNECT_COMPLETE:
       return LOAD_STATE_SSL_HANDSHAKE;
@@ -165,6 +173,10 @@ void SSLConnectJob::GetAdditionalErrorState(ClientSocketHandle* handle) {
     handle->set_is_ssl_error(true);
 }
 
+void SSLConnectJob::EnableJobWaiting(bool enable) {
+  enable_job_waiting_ = enable;
+}
+
 void SSLConnectJob::OnIOComplete(int result) {
   int rv = DoLoop(result);
   if (rv != ERR_IO_PENDING)
@@ -200,6 +212,13 @@ int SSLConnectJob::DoLoop(int result) {
       case STATE_TUNNEL_CONNECT_COMPLETE:
         rv = DoTunnelConnectComplete(rv);
         break;
+      case STATE_CHECK_FOR_RESUME:
+        rv = DoCheckForResume();
+        break;
+      case STATE_WAIT:
+        next_state_ = STATE_CHECK_FOR_RESUME;
+        rv = ERR_IO_PENDING;

[Ryan Sleevi, 2014/06/13 23:24:22]

First Reaction: This is weird to have it as an state transition, since it only
serves to set the next transition.

Normally, you would have DoCheckForResume() set this state, and return
ERR_IO_PENDING there.

[mshelley1, 2014/06/16 19:02:50]

Done.

+        break;
       case STATE_SSL_CONNECT:
         DCHECK_EQ(OK, rv);
         rv = DoSSLConnect();
@@ -233,8 +252,12 @@ int SSLConnectJob::DoTransportConnect() {
 }
 
 int SSLConnectJob::DoTransportConnectComplete(int result) {
-  if (result == OK)
-    next_state_ = STATE_SSL_CONNECT;
+  if (result == OK) {

[Ryan Sleevi, 2014/06/13 23:24:22]

We try to handle errors before success cases;

if (result != OK)
  return result;

if (enable_job_waiting_)
  next_state_ = STATE_CHECK_FOR_RESUME;
else
  next_state_ = STATE_SSL_CONNECT;

return result;

[mshelley1, 2014/06/16 19:02:49]

Done.

+    if (enable_job_waiting_)
+      next_state_ = STATE_CHECK_FOR_RESUME;
+    else
+      next_state_ = STATE_SSL_CONNECT;
+  }
 
   return result;
 }
@@ -254,8 +277,12 @@ int SSLConnectJob::DoSOCKSConnect() {
 }
 
 int SSLConnectJob::DoSOCKSConnectComplete(int result) {
-  if (result == OK)
-    next_state_ = STATE_SSL_CONNECT;
+  if (result == OK) {
+    if (enable_job_waiting_)
+      next_state_ = STATE_CHECK_FOR_RESUME;
+    else
+      next_state_ = STATE_SSL_CONNECT;
+  }

[Ryan Sleevi, 2014/06/13 23:24:22]

Ditto ordering

[mshelley1, 2014/06/16 19:02:50]

Done.

 
   return result;
 }
@@ -290,9 +317,31 @@ int SSLConnectJob::DoTunnelConnectComplete(int result) {
   }
   if (result < 0)
     return result;
+  if (enable_job_waiting_)
+    next_state_ = STATE_CHECK_FOR_RESUME;
+  else
+    next_state_ = STATE_SSL_CONNECT;

[Ryan Sleevi, 2014/06/13 23:24:22]

STYLE: This is now the third time this conditional has been duplicated.

It suggests there should be a private helper function, to ensure state
transitions are the same (eg: it's the only one that should ever set state =
STATE_SSL_CONNECT, presumably)

next_state_ = GetNextConnectState() ?

[mshelley1, 2014/06/16 19:02:50]

Done.

+  return result;
+}
 
+int SSLConnectJob::DoCheckForResume() {
+  // If the group is in the cache, continue.
+  if (SSLClientSocketOpenSSL::InSessionCache(GetSessionCacheKey())) {
+    next_state_ = STATE_SSL_CONNECT;
+    return OK;
+  }
+
+  // If there are pending jobs, wait.
+  if (!pending_->empty()) {
+    pending_->push_back(this);
+    next_state_ = STATE_WAIT;
+    return OK;

[Ryan Sleevi, 2014/06/13 23:24:22]

This should be ERR_IO_PENDING

[mshelley1, 2014/06/16 19:02:50]

Done.

+  }
+
+  // If there are no pending jobs, continue
+  pending_->push_back(this);
   next_state_ = STATE_SSL_CONNECT;
-  return result;
+  return OK;
 }
 
 int SSLConnectJob::DoSSLConnect() {
@@ -411,9 +460,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,
@@ -446,9 +495,45 @@ int SSLConnectJob::DoSSLConnectComplete(int result) {
         error_response_info_.cert_request_info.get());
   }
 
+  if (enable_job_waiting_) {

[Ryan Sleevi, 2014/06/13 23:24:22]

This should probably all be moved into a helper function; it's a discrete set of
functionality tied to this job_waiting_, so centralizing it (and documenting
what it does) is a good structural cleanup.

[mshelley1, 2014/06/16 19:02:50]

Done.

+    // If there is a pending list
+    if (!pending_->empty()) {
+      // The first element of the vector will always be the leading job.
+      PendingJobList::iterator i = pending_->begin();
+      // If the connection was successful, tell all pending jobs to proceed.
+      if (result == OK) {
+        for (PendingJobList::iterator job = i + 1; job != pending_->end();
+             ++job) {
+          (*job)->DoSSL();
+        }
+        // pending_->erase(i);
+        pending_->clear();
+      }
+      // If the connection failed, tell only one job to proceed as the new
+      // leader.
+      else if (i + 1 != pending_->end()) {
+        (*(i + 1))->DoSSL();
+        pending_->erase(i);
+      }
+      // If there are no more jobs and the leader failed, delete the entry.
+      else
+        pending_->erase(i);
+    }
+  }
+
   return result;
 }
 
+void SSLConnectJob::DoSSL() {
+  next_state_ = STATE_SSL_CONNECT;
+  OnIOComplete(OK);
+}
+
+std::string SSLConnectJob::GetSessionCacheKey() {
+  return params_->host_and_port().ToString() + "/" +
+         context_.ssl_session_cache_shard;
+}
+
 SSLConnectJob::State SSLConnectJob::GetInitialState(
     SSLSocketParams::ConnectionType connection_type) {
   switch (connection_type) {
@@ -482,7 +567,8 @@ SSLClientSocketPool::SSLConnectJobFactory::SSLConnectJobFactory(
       client_socket_factory_(client_socket_factory),
       host_resolver_(host_resolver),
       context_(context),
-      net_log_(net_log) {
+      net_log_(net_log),
+      pending_(new PendingJobMap()) {
   base::TimeDelta max_transport_timeout = base::TimeDelta();
   base::TimeDelta pool_timeout;
   if (transport_pool_)
@@ -520,21 +606,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 +645,26 @@ 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_));
+  PendingJobMap::iterator p = pending_->find(group_name);
+  SSLConnectJob::PendingJobList* pending_job_list;
+  if (p == pending_->end()) {
+    (*pending_)[group_name] = new SSLConnectJob::PendingJobList();
+    pending_job_list = (*pending_)[group_name];

[Ryan Sleevi, 2014/06/13 23:24:22]

This is actually somewhat inefficient; it does two scans of the map's
red-black-tree

You can do

std::pair<PendingJobMap::iterator, bool> it =
pending_->insert(PendingJobMap::value_type(group_name, new
SSLConnectJob::PendingJobList()));
pending_job_list = it->first.second;


If the PendingJobMap::value_type constructor doesn't work, std::pair<const
std::string, SSLConnectJob::PendingJobList*>(...)
or

PendingJobMap::value_type value(group_name, new
SSLConnectJob::PendingJobList());
... it = pending_->insert(value);

[mshelley1, 2014/06/16 19:02:50]

Done.

+  } else
+    pending_job_list = p->second;

[Ryan Sleevi, 2014/06/13 23:24:22]

If one portion of an if statement includes braces, the other conditions should
too.

[mshelley1, 2014/06/16 19:02:50]

Done.

+  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_,
+                                                  pending_job_list));
 }
 
 base::TimeDelta