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..11380b7220d932ac90ee086332af21e46b903755 100644
--- a/net/socket/ssl_client_socket_pool.cc
+++ b/net/socket/ssl_client_socket_pool.cc
@@ -108,7 +108,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 +128,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))) {}
+      io_callback_(
+          base::Bind(&SSLConnectJob::OnIOComplete, base::Unretained(this))),
+      pending_jobs_(pending) {
+}
 
 SSLConnectJob::~SSLConnectJob() {}
 
+bool SSLConnectJob::enable_job_waiting_ = false;

[wtc, 2014/06/17 17:44:40]

Nit: add a
// static
comment before this line. (This convention is not universally followed in the
Chromium source tree though.)

Nit: we seem to initialize static data members before the constructors. I didn't
find any recommendation about this in the Style Guide.

[mshelley1, 2014/06/18 18:53:49]

Done.

+
 LoadState SSLConnectJob::GetLoadState() const {
   switch (next_state_) {
     case STATE_TUNNEL_CONNECT_COMPLETE:
@@ -144,6 +149,7 @@ LoadState SSLConnectJob::GetLoadState() const {
     case STATE_SOCKS_CONNECT_COMPLETE:
     case STATE_TUNNEL_CONNECT:
       return transport_socket_handle_->GetLoadState();
+    case STATE_CHECK_FOR_RESUME:

[wtc, 2014/06/17 17:44:40]

Should we also add the STATE_CREATE_SSL_SOCKET state to this switch statement?

[mshelley1, 2014/06/18 18:53:49]

Done.

     case STATE_SSL_CONNECT:
     case STATE_SSL_CONNECT_COMPLETE:
       return LOAD_STATE_SSL_HANDSHAKE;
@@ -165,6 +171,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 +210,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 +243,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 +266,15 @@ 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) {
+    next_state_ = STATE_CREATE_SSL_SOCKET;
+  }

[wtc, 2014/06/17 17:44:40]

Nit: omit the curly braces if the if statement's condition and body are just one
line.

[mshelley1, 2014/06/18 18:53:50]

Done.

 
   return result;
 }
@@ -270,7 +289,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 +309,16 @@ int SSLConnectJob::DoTunnelConnectComplete(int result) {
   }
   if (result < 0)
     return result;
+  next_state_ = STATE_CREATE_SSL_SOCKET;
 
-  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));
 
@@ -306,22 +328,48 @@ int SSLConnectJob::DoSSLConnect() {
       transport_socket_handle_->connect_timing();
   if (!transport_socket_handle_->is_reused() &&
       !socket_connect_timing.connect_start.is_null()) {
-    // Overwriting |connect_start| serves two purposes - it adjusts timing so
-    // |connect_start| doesn't include dns times, and it adjusts the time so
-    // as not to include time spent waiting for an idle socket.
     connect_timing_.connect_start = socket_connect_timing.connect_start;
     connect_timing_.dns_start = socket_connect_timing.dns_start;
     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() {
+  // If the group is in the cache, continue.

[wtc, 2014/06/17 17:44:40]

This comment should point out that this SSL handshake will be a session
resumption handshake.

[mshelley1, 2014/06/18 18:53:49]

Done.

+  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;
+    return ERR_IO_PENDING;
+  }
+
+  // If there are no pending jobs, continue

[wtc, 2014/06/17 17:44:40]

Nit: it would be good to elaborate in this comment. Why do we continue in this
case? I guess this job would be the leading job?

[mshelley1, 2014/06/18 18:53:49]

Done.

+  pending_jobs_->push_back(this);
+  next_state_ = STATE_SSL_CONNECT;
+  return OK;
+}
+
+int SSLConnectJob::DoSSLConnect() {
+  next_state_ = STATE_SSL_CONNECT_COMPLETE;
+  // Overwriting |connect_start| serves two purposes - it adjusts timing so
+  // |connect_start| doesn't include dns times, and it adjusts the time so
+  // as not to include time spent waiting for an idle socket.

[wtc, 2014/06/17 17:44:40]

This comment was copied here by mistake. Please move it back to line 331, before
the line
    connect_timing_.connect_start = socket_connect_timing.connect_start;

[mshelley1, 2014/06/18 18:53:49]

Done.

+
+  connect_timing_.ssl_start = base::TimeTicks::Now();
+
+  return ssl_socket_->Connect(io_callback_);
 }
 
 int SSLConnectJob::DoSSLConnectComplete(int result) {
@@ -411,9 +459,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 +494,48 @@ int SSLConnectJob::DoSSLConnectComplete(int result) {
         error_response_info_.cert_request_info.get());
   }
 
+  if (enable_job_waiting_) {
+    ProcessPendingJobs(result);
+  }

[wtc, 2014/06/17 17:44:40]

Nit: omit curly braces.

[mshelley1, 2014/06/18 18:53:50]

Done.

+
   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 i = pending_jobs_->begin();

[wtc, 2014/06/17 17:44:40]

Nit: name the iterator |it| or |iter|.

[mshelley1, 2014/06/18 18:53:50]

Done.

+    // If the connection was successful, tell all pending jobs to proceed.
+    if (result == OK) {
+      for (PendingJobList::iterator job = i + 1; job != pending_jobs_->end();
+           ++job) {
+        (*job)->ResumeSSLConnection();
+      }
+      pending_jobs_->clear();
+    }
+    // If the connection failed, tell only one job to proceed as the new
+    // leader.
+    else if (i + 1 != pending_jobs_->end()) {
+      (*(i + 1))->ResumeSSLConnection();
+      pending_jobs_->erase(i);
+    }
+    // If there are no more jobs and the leader failed, delete the entry.
+    else

[wtc, 2014/06/17 17:44:40]

On lines 519 and 524, the else should follow the preceding closing '}'. This
requires moving the comments inside the blocks, like this:

    pending_jobs_->clear();
  } else if (i + 1 != pending_jobs_->end()) {
    // If the connection failed, tell only one job to proceed as the new
    // leader.
    ...
  } else {
    // If there are no more jobs and the leader failed, delete the entry.
    pending_jobs_->erase(i);
  }

Note that the final "else" uses curly braces even though its body has only one
line. In an if-else-if-else statement, if any branch needs curly braces, all
branches should use curly braces.

[mshelley1, 2014/06/18 18:53:49]

Done.

+      pending_jobs_->erase(i);

[wtc, 2014/06/17 17:44:40]

Nit: I think this if-else-if-else statement can be simplified. An easy
simplification is to combine the second and third cases:

  if (result == OK) {
    the first case, unchanged
  } else {
    pending_jobs_->erase(i++);
    if (i != pending_jobs_->end())
      (*i)->ResumeSSLConnection();
  }

If we work harder, this should also work:

  pending_jobs_->erase(i++);
  if (result == OK) {
    for (; i != pending_jobs_->end(); ++i)
      (*i)->ResumeSSLConnection();
    pending_jobs_->clear();
  } else if (i != pending_jobs_->end()) {
    (*i)->ResumeSSLConnection();
  }

Try to come up with a control structure that's both simple and easy to
understand.

[mshelley1, 2014/06/18 18:53:50]

I decided to go with your first suggestion. The second suggestion led to an
error -- I think because erasing i invalidates i++, but I'm not completely sure.

+  }
+}
+
+void SSLConnectJob::ResumeSSLConnection() {
+  next_state_ = STATE_SSL_CONNECT;

[wtc, 2014/06/17 17:44:40]

Please add
  DCHECK_EQ(next_state_, STATE_CHECK_FOR_RESUME);
before this line.

But I think we usually handle this by directly setting
  next_state_ = STATE_SSL_CONNECT;
on line 354. Then we don't need to change next_state_ here.

[mshelley1, 2014/06/18 18:53:50]

Done.

+  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 +569,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_(new PendingJobMap()) {

[wtc, 2014/06/17 17:44:40]

BUG: this PendingJobMap object is leaked. We should declare this member as a
non-pointer in the .h file:

  PendingJobMap pending_jobs_;

[mshelley1, 2014/06/18 18:53:49]

As you implied in your comment on the .h file, making this a scoped pointer is
also an appropriate solution correct?

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

Yes, it is. But I suggested a better solution.

   base::TimeDelta max_transport_timeout = base::TimeDelta();
   base::TimeDelta pool_timeout;
   if (transport_pool_)
@@ -520,21 +608,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 +647,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;
+  std::pair<PendingJobMap::iterator, bool> it =
+      pending_jobs_->insert(PendingJobMap::value_type(
+          group_name, new SSLConnectJob::PendingJobList()));

[wtc, 2014/06/17 17:44:40]

IMPORTANT: it seems wrong to create a new SSLConnectJob::PendingJobList every
time. It seems that we should first find an existing
SSLConnectJob::PendingJobList in pending_jobs_. If none exsts, then create a new
one.

[mshelley1, 2014/06/18 18:53:49]

If I understand the documentation for std::map::insert correctly, the insert
method will first check if there's an existing element with the given key, and
if there is it will just return an iterator to that key. If not, then it
actually creates a new entry and returns an iterator to that.
(http://www.cplusplus.com/reference/map/map/insert/)

[Ryan Sleevi, 2014/06/19 20:20:43]

So, it's a little complicated:

.insert() will always update the value of map[key] to the second argument of
your pair

if [key] was already in your map, it sets the second argument to true.
if [key] was not already in your map, it sets the second argument to false.

In both cases, it returns the first argument as an interator to map[key]

This means that in the event that map[key] exists, you're *overwriting* it with
your "new SSLConnectJob::PendingJobList" - creating a new pending list every
time.

You still want to do 

iterator it = map.find(key);
if (it == map.end()) {
  other_it = map.insert(...);
  it = other_it.first;
}

pending_job_list = it->second;

This ensures you re-use the existing entry (if one exists - your first .find),
or, if you have to insert a new entry, you're not having to invoke .find() again
to find the entry you *just* inserted.

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