SSL Connect Job Waiting

net/socket/ssl_client_socket_pool.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/socket/ssl_client_socket_pool.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
@@ skipping 90 matching lines @@
                              RequestPriority priority,
                              const scoped_refptr<SSLSocketParams>& params,
                              const base::TimeDelta& timeout_duration,
                              TransportClientSocketPool* transport_pool,
                              SOCKSClientSocketPool* socks_pool,
                              HttpProxyClientSocketPool* http_proxy_pool,
                              ClientSocketFactory* client_socket_factory,
                              HostResolver* host_resolver,
                              const SSLClientSocketContext& context,
                              Delegate* delegate,
-                             NetLog* net_log)
+                             NetLog* net_log,
+                             PendingJobList* pending)
     : ConnectJob(group_name,
                  timeout_duration,
                  priority,
                  delegate,
                  BoundNetLog::Make(net_log, NetLog::SOURCE_CONNECT_JOB)),
       params_(params),
       transport_pool_(transport_pool),
       socks_pool_(socks_pool),
       http_proxy_pool_(http_proxy_pool),
       client_socket_factory_(client_socket_factory),
       host_resolver_(host_resolver),
       context_(context.cert_verifier,
                context.server_bound_cert_service,
                context.transport_security_state,
                context.cert_transparency_verifier,
                (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:
       if (transport_socket_handle_->socket())
         return LOAD_STATE_ESTABLISHING_PROXY_TUNNEL;
       // else, fall through.
     case STATE_TRANSPORT_CONNECT:
     case STATE_TRANSPORT_CONNECT_COMPLETE:
     case STATE_SOCKS_CONNECT:
     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;
     default:
       NOTREACHED();
       return LOAD_STATE_IDLE;
   }
 }
 
 void SSLConnectJob::GetAdditionalErrorState(ClientSocketHandle* handle) {
   // Headers in |error_response_info_| indicate a proxy tunnel setup
   // problem. See DoTunnelConnectComplete.
   if (error_response_info_.headers.get()) {
     handle->set_pending_http_proxy_connection(
         transport_socket_handle_.release());
   }
   handle->set_ssl_error_response_info(error_response_info_);
   if (!connect_timing_.ssl_start.is_null())
     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)
     NotifyDelegateOfCompletion(rv);  // Deletes |this|.
 }
 
 int SSLConnectJob::DoLoop(int result) {
   DCHECK_NE(next_state_, STATE_NONE);
 
   int rv = result;
@@ skipping 15 matching lines @@
       case STATE_SOCKS_CONNECT_COMPLETE:
         rv = DoSOCKSConnectComplete(rv);
         break;
       case STATE_TUNNEL_CONNECT:
         DCHECK_EQ(OK, rv);
         rv = DoTunnelConnect();
         break;
       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();
         break;
       case STATE_SSL_CONNECT_COMPLETE:
         rv = DoSSLConnectComplete(rv);
         break;
       default:
         NOTREACHED() << "bad state";
         rv = ERR_FAILED;
         break;
     }
   } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
 
   return rv;
 }
 
 int SSLConnectJob::DoTransportConnect() {
   DCHECK(transport_pool_);
 
   next_state_ = STATE_TRANSPORT_CONNECT_COMPLETE;
   transport_socket_handle_.reset(new ClientSocketHandle());
   scoped_refptr<TransportSocketParams> direct_params =
       params_->GetDirectConnectionParams();
   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;
 }
 
 int SSLConnectJob::DoSOCKSConnect() {
   DCHECK(socks_pool_);
   next_state_ = STATE_SOCKS_CONNECT_COMPLETE;
   transport_socket_handle_.reset(new ClientSocketHandle());
   scoped_refptr<SOCKSSocketParams> socks_proxy_params =
       params_->GetSocksProxyConnectionParams();
   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;
 }
 
 int SSLConnectJob::DoTunnelConnect() {
   DCHECK(http_proxy_pool_);
   next_state_ = STATE_TUNNEL_CONNECT_COMPLETE;
 
   transport_socket_handle_.reset(new ClientSocketHandle());
   scoped_refptr<HttpProxySocketParams> http_proxy_params =
       params_->GetHttpProxyConnectionParams();
   return transport_socket_handle_->Init(group_name(),
                                         http_proxy_params,
                                         priority(),
-                                        callback_,
+                                        io_callback_,
                                         http_proxy_pool_,
                                         net_log());
 }
 
 int SSLConnectJob::DoTunnelConnectComplete(int result) {
   // Extract the information needed to prompt for appropriate proxy
   // authentication so that when ClientSocketPoolBaseHelper calls
   // |GetAdditionalErrorState|, we can easily set the state.
   if (result == ERR_SSL_CLIENT_AUTH_CERT_NEEDED) {
     error_response_info_ = transport_socket_handle_->ssl_error_response_info();
   } else if (result == ERR_PROXY_AUTH_REQUESTED ||
              result == ERR_HTTPS_PROXY_TUNNEL_RESPONSE) {
     StreamSocket* socket = transport_socket_handle_->socket();
     HttpProxyClientSocket* tunnel_socket =
         static_cast<HttpProxyClientSocket*>(socket);
     error_response_info_ = *tunnel_socket->GetConnectResponseInfo();
   }
   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));
 
   // If the handle has a fresh socket, get its connect start and DNS times.
   // This should always be the case.
   const LoadTimingInfo::ConnectTiming& socket_connect_timing =
       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) {
   connect_timing_.ssl_end = base::TimeTicks::Now();
 
   SSLClientSocket::NextProtoStatus status =
       SSLClientSocket::kNextProtoUnsupported;
   std::string proto;
   std::string server_protos;
   // GetNextProto will fail and and trigger a NOTREACHED if we pass in a socket
@@ skipping 10 matching lines @@
     ssl_socket_->set_protocol_negotiated(protocol_negotiated);
     // If we negotiated a SPDY version, it must have been present in
     // SSLConfig::next_protos.
     // TODO(mbelshe): Verify this.
     if (protocol_negotiated >= kProtoSPDYMinimumVersion &&
         protocol_negotiated <= kProtoSPDYMaximumVersion) {
       ssl_socket_->set_was_spdy_negotiated(true);
     }
   }
   if (params_->want_spdy_over_npn() && !ssl_socket_->was_spdy_negotiated())
     return ERR_NPN_NEGOTIATION_FAILED;

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

IMPORTANT: Do we need to wake up the pending jobs in this case?

We need to check every return statement in the DoSSLConnectComplete function and
see what we should do about the pending jobs before returning.

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

Yes we do -- I believe I can just call the ProcessPendingJobs function to
correctly handle it.

 
   // Spdy might be turned on by default, or it might be over npn.
   bool using_spdy = params_->force_spdy_over_ssl() ||
       params_->want_spdy_over_npn();
 
   if (result == OK ||
       ssl_socket_->IgnoreCertError(result, params_->load_flags())) {
     DCHECK(!connect_timing_.ssl_start.is_null());
     base::TimeDelta connect_duration =
         connect_timing_.ssl_end - connect_timing_.ssl_start;
@@ skipping 38 matching lines @@
                                  100);
     } else if (ssl_info.handshake_type == SSLInfo::HANDSHAKE_FULL) {
       UMA_HISTOGRAM_CUSTOM_TIMES("Net.SSL_Connection_Latency_Full_Handshake",
                                  connect_duration,
                                  base::TimeDelta::FromMilliseconds(1),
                                  base::TimeDelta::FromMinutes(1),
                                  100);
     }
 
     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,
                                  base::TimeDelta::FromMilliseconds(1),
                                  base::TimeDelta::FromMinutes(1),
                                  100);
       if (ssl_info.handshake_type == SSLInfo::HANDSHAKE_RESUME) {
         UMA_HISTOGRAM_CUSTOM_TIMES("Net.SSL_Connection_Latency_Google_"
                                        "Resume_Handshake",
                                    connect_duration,
@@ skipping 12 matching lines @@
   }
 
   if (result == OK || IsCertificateError(result)) {
     SetSocket(ssl_socket_.PassAs<StreamSocket>());
   } else if (result == ERR_SSL_CLIENT_AUTH_CERT_NEEDED) {
     error_response_info_.cert_request_info = new SSLCertRequestInfo;
     ssl_socket_->GetSSLCertRequestInfo(
         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) {
     case SSLSocketParams::DIRECT:
       return STATE_TRANSPORT_CONNECT;
     case SSLSocketParams::HTTP_PROXY:
       return STATE_TUNNEL_CONNECT;
     case SSLSocketParams::SOCKS_PROXY:
       return STATE_SOCKS_CONNECT;
   }
@@ skipping 13 matching lines @@
     ClientSocketFactory* client_socket_factory,
     HostResolver* host_resolver,
     const SSLClientSocketContext& context,
     NetLog* net_log)
     : transport_pool_(transport_pool),
       socks_pool_(socks_pool),
       http_proxy_pool_(http_proxy_pool),
       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_)
     max_transport_timeout = transport_pool_->ConnectionTimeout();
   if (socks_pool_) {
     pool_timeout = socks_pool_->ConnectionTimeout();
     if (pool_timeout > max_transport_timeout)
       max_transport_timeout = pool_timeout;
   }
   if (http_proxy_pool_) {
@@ skipping 17 matching lines @@
     const std::string& ssl_session_cache_shard,
     ClientSocketFactory* client_socket_factory,
     TransportClientSocketPool* transport_pool,
     SOCKSClientSocketPool* socks_pool,
     HttpProxyClientSocketPool* http_proxy_pool,
     SSLConfigService* ssl_config_service,
     NetLog* net_log)
     : 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);
   if (transport_pool_)
     base_.AddLowerLayeredPool(transport_pool_);
   if (socks_pool_)
     base_.AddLowerLayeredPool(socks_pool_);
   if (http_proxy_pool_)
     base_.AddLowerLayeredPool(http_proxy_pool_);
 }
 
 SSLClientSocketPool::~SSLClientSocketPool() {
   if (ssl_config_service_.get())
     ssl_config_service_->RemoveObserver(this);
 }
 
 scoped_ptr<ConnectJob>
 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
 SSLClientSocketPool::SSLConnectJobFactory::ConnectionTimeout() const {
   return timeout_;
 }
 
 int SSLClientSocketPool::RequestSocket(const std::string& group_name,
                                        const void* socket_params,
                                        RequestPriority priority,
@@ skipping 103 matching lines @@
   if (base_.CloseOneIdleSocket())
     return true;
   return base_.CloseOneIdleConnectionInHigherLayeredPool();
 }
 
 void SSLClientSocketPool::OnSSLConfigChanged() {
   FlushWithError(ERR_NETWORK_CHANGED);
 }
 
 }  // namespace net