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..6c3c871278ef238b5c7aacac688a736b997fc4fd 100644
--- a/net/socket/ssl_client_socket_pool.cc
+++ b/net/socket/ssl_client_socket_pool.cc
@@ -97,6 +97,8 @@ SSLSocketParams::GetHttpProxyConnectionParams() const {
 // Timeout for the SSL handshake portion of the connect.
 static const int kSSLHandshakeTimeoutInSeconds = 30;
 
+// Note: the SSLConnectJob does not own pending_jobs_list
+// so it must outlive the job.

[wtc, 2014/06/19 19:38:11]

Please move this comment to the .h file. Ideally someone should be able to read
just the .h file and understand how to use the class. So API contracts are
usually documented in the .h file.

[mshelley1, 2014/06/24 17:03:59]

Done.

 SSLConnectJob::SSLConnectJob(const std::string& group_name,
                              RequestPriority priority,
                              const scoped_refptr<SSLSocketParams>& params,
@@ -107,6 +109,7 @@ SSLConnectJob::SSLConnectJob(const std::string& group_name,
                              ClientSocketFactory* client_socket_factory,
                              HostResolver* host_resolver,
                              const SSLClientSocketContext& context,
+                             PendingJobList* pending_jobs_list,
                              Delegate* delegate,
                              NetLog* net_log)
     : ConnectJob(group_name,
@@ -127,11 +130,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))),
+      pending_jobs_(pending_jobs_list) {
+}
 
 SSLConnectJob::~SSLConnectJob() {}
 
+// static
+bool SSLConnectJob::enable_job_waiting_ = false;
+
 LoadState SSLConnectJob::GetLoadState() const {
   switch (next_state_) {
     case STATE_TUNNEL_CONNECT_COMPLETE:
@@ -144,6 +152,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 +175,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 +214,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 +247,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;

[wtc, 2014/06/19 19:38:11]

Nit: keep this method and DoSOCKSConnectComplete (lines 278-282) the same. Pick
the form that you find easier to understand. We value consistency highly.

[mshelley1, 2014/06/24 17:03:59]

Done.

 
   return result;
 }
@@ -248,14 +270,14 @@ 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;
+    next_state_ = STATE_CREATE_SSL_SOCKET;
 
   return result;
 }
@@ -270,7 +292,7 @@ int SSLConnectJob::DoTunnelConnect() {
   return transport_socket_handle_->Init(group_name(),
                                         http_proxy_params,
                                         priority(),
-                                        callback_,
+                                        io_callback_,
                                         http_proxy_pool_,
                                         net_log());
 }
@@ -290,13 +312,16 @@ int SSLConnectJob::DoTunnelConnectComplete(int result) {
   }
   if (result < 0)
     return result;
+  next_state_ = STATE_CREATE_SSL_SOCKET;
 

[wtc, 2014/06/19 19:38:10]

Nit: reverse lines 315 and 316.

[mshelley1, 2014/06/24 17:03:59]

Done.

-  next_state_ = STATE_SSL_CONNECT;
   return result;
 }
 
-int SSLConnectJob::DoSSLConnect() {
-  next_state_ = STATE_SSL_CONNECT_COMPLETE;
+int SSLConnectJob::DoCreateSSLSocket() {
+  if (enable_job_waiting_)
+    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 +339,42 @@ int SSLConnectJob::DoSSLConnect() {
     connect_timing_.dns_end = socket_connect_timing.dns_end;
   }

[wtc, 2014/06/19 19:38:10]

Please confirm that you think lines 325-340 should NOT be moved to
DoSSLConnect(). I believe it is correct to leave them here.

[mshelley1, 2014/06/24 17:03:59]

Yes -- I don't completely understand why, but leaving those lines in
DoSSLConnect() led to segfaults.

 
-  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() {
+  // If the group is in the cache, continue with the session resumption

[wtc, 2014/06/19 19:38:10]

Nit: say "If the group's session is in the cache" or "If the group has a session
in the cache".

[mshelley1, 2014/06/24 17:03:59]

Done.

+  // SSL handshake.
+  if (ssl_socket_->InSessionCache()) {
+    next_state_ = STATE_SSL_CONNECT;
+    return OK;
+  }
+
+  // If there are pending jobs, wait.
+  if (!pending_jobs_->empty()) {
+    pending_jobs_->push_back(this);
+    next_state_ = STATE_CHECK_FOR_RESUME;

[wtc, 2014/06/19 19:38:11]

I think we should change next_state_ to STATE_SSL_CONNECT here, and in
ResumeSSLConnection, simply DCHECK that next_state_ is equal to
STATE_SSL_CONNECT.

[mshelley1, 2014/06/24 17:03:59]

Done.

+    return ERR_IO_PENDING;
+  }
+
+  // If there are no pending jobs, continue the full SSL handshake
+  // because a resumption handshake is not possible.
+  pending_jobs_->push_back(this);

[wtc, 2014/06/19 19:38:11]

I got confused by this before, so a comment might be useful. I didn't understand
why we need to add |this| to pending_jobs_ even though |this| is continuing to
STATE_SSL_CONNECT. The non-obvious reason is that this will cause other jobs to
wait.

[mshelley1, 2014/06/24 17:03:59]

Done.

+  next_state_ = STATE_SSL_CONNECT;
+  return OK;
+}
+
+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) {
@@ -351,8 +404,11 @@ 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()) {
+    if (enable_job_waiting_)
+      ProcessPendingJobs(ERR_NPN_NEGOTIATION_FAILED);
     return ERR_NPN_NEGOTIATION_FAILED;
+  }
 
   // Spdy might be turned on by default, or it might be over npn.
   bool using_spdy = params_->force_spdy_over_ssl() ||
@@ -411,9 +467,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 +502,41 @@ int SSLConnectJob::DoSSLConnectComplete(int result) {
         error_response_info_.cert_request_info.get());
   }
 
+  if (enable_job_waiting_)
+    ProcessPendingJobs(result);
+
   return result;
 }
 
+void SSLConnectJob::ProcessPendingJobs(int result) {
+  // If there is a pending list
+  if (!pending_jobs_->empty()) {
+    // The first element of the vector will always be the leading job.
+    PendingJobList::iterator it = pending_jobs_->begin();
+
+    if (result == OK) {
+      // If the connection was successful, tell all pending jobs to proceed.
+      for (PendingJobList::iterator job = it + 1; job != pending_jobs_->end();
+           ++job) {
+        (*job)->ResumeSSLConnection();
+      }
+      pending_jobs_->clear();
+    } else {
+      // If the connection failed, tell one (if any) of the remaining jobs
+      // to proceed as the new leader.
+      if (it + 1 != pending_jobs_->end())
+        (*(it + 1))->ResumeSSLConnection();
+      pending_jobs_->erase(it);

[wtc, 2014/06/19 19:38:10]

We should figure out how to erase the first job or clear all jobs of
pending_jobs_, *before* calling ResumeSSLConnection. This may require copying
(or rather, swapping with a temporary PendingJobList variable).

You can make this change in a follow-up CL.

[mshelley1, 2014/06/24 17:03:59]

Done.

+    }
+  }
+}
+
+void SSLConnectJob::ResumeSSLConnection() {
+  DCHECK_EQ(next_state_, STATE_CHECK_FOR_RESUME);
+  next_state_ = STATE_SSL_CONNECT;
+  OnIOComplete(OK);
+}
+
 SSLConnectJob::State SSLConnectJob::GetInitialState(
     SSLSocketParams::ConnectionType connection_type) {
   switch (connection_type) {
@@ -482,7 +570,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_jobs_map_(new PendingJobMap()) {
   base::TimeDelta max_transport_timeout = base::TimeDelta();
   base::TimeDelta pool_timeout;
   if (transport_pool_)
@@ -520,21 +609,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 +648,25 @@ 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_));
+  SSLConnectJob::PendingJobList* pending_job_list;

[wtc, 2014/06/19 19:38:10]

This code should also be controlled by the enable_job_waiting_ boolean. It would
be better to move that static bool member into the
SSLClientSocketPool::SSLConnectJobFactory class or even the SSLClientSocketPool
class, and expose it to the SSLConnectJob class.

As a short-term solution, add a getter method for
SSLConnectJob::enable_job_waiting_ and check it here.

[mshelley1, 2014/06/24 17:03:59]

Done.

+  std::pair<PendingJobMap::iterator, bool> it =
+      pending_jobs_map_->insert(PendingJobMap::value_type(
+          group_name, new SSLConnectJob::PendingJobList()));

[wtc, 2014/06/19 19:38:11]

IMPORTANT: there are two memory leaks of PendingJobLists. We should at least fix
the first leak before committing this CL.

1. If the map already has an entry for the group_name, the new PendingJobList we
create here is immediately leaked.

Can you try to come up with a solution? I think you can check the bool
(it.second) returned by pending_jobs_map_->insert(), and only allocate a new
PendingJobList and store it in it.first->second if the bool (it.second) is true.

2. We need to delete all the PendingJobLists in the map when the map is
destroyed. This needs to be done very carefully because some SSLConnectJobs may
still be using the PendingJobLists. Please add a TODO comment about this leak.

[mshelley1, 2014/06/24 17:03:59]

Done.

+  pending_job_list = it.first->second;

[wtc, 2014/06/19 19:38:11]

Nit: Delete the declaration of pending_job_list on line 651 and declare it here:

  SSLConnectJob::PendingJobList* pending_job_list = pending_job_list;

[mshelley1, 2014/06/24 17:03:59]

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