This CL changes the lifespan of SSLConnectJobMessengers so that they are created only when needed,

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 86 matching lines @@
 
 SSLConnectJobMessenger::SocketAndCallback::SocketAndCallback(
     SSLClientSocket* ssl_socket,
     const base::Closure& job_resumption_callback)
     : socket(ssl_socket), callback(job_resumption_callback) {
 }
 
 SSLConnectJobMessenger::SocketAndCallback::~SocketAndCallback() {
 }
 
-SSLConnectJobMessenger::SSLConnectJobMessenger() : weak_factory_(this) {
+SSLConnectJobMessenger::SSLConnectJobMessenger(
+    const base::Closure& connection_complete_callback)
+    : connection_complete_callback_(connection_complete_callback),
+      weak_factory_(this) {
 }
 
 SSLConnectJobMessenger::~SSLConnectJobMessenger() {
 }
 
 void SSLConnectJobMessenger::RemovePendingSocket(SSLClientSocket* ssl_socket) {
   // Sockets do not need to be removed from connecting_sockets_ because
   // OnSSLHandshakeCompleted 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) {

[wtc, 2014/08/14 23:26:26]

1. Remove the unused |ssl_socket| parameter.

2. Nit: the CanProceed() method is now so simple that it probably should be
removed. I remember both mmenke and I suggested this before.

[mshelley, 2014/08/15 16:53:06]

Done.

-  // 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();
+  // If there are no connecting sockets allow the connection to proceed.
+  return connecting_sockets_.empty();
 }
 
 void SSLConnectJobMessenger::MonitorConnectionResult(
     SSLClientSocket* ssl_socket) {
   connecting_sockets_.push_back(ssl_socket);
   ssl_socket->SetHandshakeCompletionCallback(
       base::Bind(&SSLConnectJobMessenger::OnSSLHandshakeCompleted,
                  weak_factory_.GetWeakPtr()));
 }
 
 void SSLConnectJobMessenger::AddPendingSocket(SSLClientSocket* ssl_socket,
                                               const base::Closure& callback) {
   DCHECK(!connecting_sockets_.empty());
   pending_sockets_and_callbacks_.push_back(
       SocketAndCallback(ssl_socket, callback));
 }
 
 void SSLConnectJobMessenger::OnSSLHandshakeCompleted() {
   connecting_sockets_.clear();
   SSLPendingSocketsAndCallbacks temp_list;
   temp_list.swap(pending_sockets_and_callbacks_);
+  base::Closure connection_complete_callback = connection_complete_callback_;
   RunAllCallbacks(temp_list);
+  connection_complete_callback.Run();

[wtc, 2014/08/14 23:26:26]

IMPORTANT: I think we should run connection_complete_callback before calling
RunAllCallbacks.

The reason is that RunAllCallbacks() could potentially cause new connections
that use the current cache key. So we need connection_complete_callback to
remove the current messenger from the messenger map, to prevent the current
messenger from being used for the new connections.

Does this make sense?

[mshelley, 2014/08/15 16:53:06]

Done.

 }
 
 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;
 
 SSLConnectJob::SSLConnectJob(const std::string& group_name,
                              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,
-                             SSLConnectJobMessenger* messenger,
+                             const GetMessengerCallback& get_messenger_callback,
                              Delegate* delegate,
                              NetLog* net_log)
     : 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.channel_id_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)),
       io_callback_(
           base::Bind(&SSLConnectJob::OnIOComplete, base::Unretained(this))),
-      messenger_(messenger),
+      messenger_(NULL),
+      get_messenger_callback_(get_messenger_callback),
       weak_factory_(this) {
 }
 
 SSLConnectJob::~SSLConnectJob() {
   if (ssl_socket_.get() && messenger_)
     messenger_->RemovePendingSocket(ssl_socket_.get());
 }
 
 LoadState SSLConnectJob::GetLoadState() const {
   switch (next_state_) {
@@ skipping 183 matching lines @@
     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;
   }
 
   ssl_socket_ = client_socket_factory_->CreateSSLClientSocket(
       transport_socket_handle_.Pass(),
       params_->host_and_port(),
       params_->ssl_config(),
       context_);
+
+  if (!ssl_socket_->InSessionCache()) {
+    messenger_ = get_messenger_callback_.Run(ssl_socket_->GetSessionCacheKey());
+  }

[wtc, 2014/08/14 23:26:26]

Nit: omit the curly braces.

[mshelley, 2014/08/15 16:53:06]

Done.

+
   return OK;
 }
 
 int SSLConnectJob::DoCheckForResume() {
   next_state_ = STATE_SSL_CONNECT;
-  if (!messenger_)
+
+  if (ssl_socket_->InSessionCache() || !messenger_)

[wtc, 2014/08/14 23:26:26]

Remove the redundant ssl_socket_->InSessionCache() test. We just tested it in
the previous state (line 411).

[mshelley, 2014/08/15 16:53:06]

Done.

     return OK;
 
-  // TODO(mshelley): Remove duplicate InSessionCache() calls.
   if (messenger_->CanProceed(ssl_socket_.get())) {
-    if (!ssl_socket_->InSessionCache())
-      messenger_->MonitorConnectionResult(ssl_socket_.get());
+    messenger_->MonitorConnectionResult(ssl_socket_.get());
+    // The SSLConnectJob no longer needs access to the messenger after this
+    // point.
+    messenger_ = NULL;

[wtc, 2014/08/14 23:26:26]

I suspect you set messenger_ to NULL because we will crash otherwise?

[mshelley, 2014/08/15 16:53:06]

Yep -- I check whether the messenger_ is valid before calling
RemovePendingSocket in the SSLConenctJob destructor. If I don't set it to NULL
here, then I end up calling that method on a deleted messenger_.

     return OK;
   }
+
   messenger_->AddPendingSocket(ssl_socket_.get(),
                                base::Bind(&SSLConnectJob::ResumeSSLConnection,
                                           weak_factory_.GetWeakPtr()));

[wtc, 2014/08/14 23:26:26]

In this case, the right time to set messenger_ to NULL is actually quite subtle.
I believe your code is correct. I wonder if we should document it.

[mshelley, 2014/08/15 16:53:06]

Probably, I'll add documentation.

+
   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_);
 }
@@ skipping 117 matching lines @@
     error_response_info_.cert_request_info = new SSLCertRequestInfo;
     ssl_socket_->GetSSLCertRequestInfo(
         error_response_info_.cert_request_info.get());
   }
 
   return result;
 }
 
 void SSLConnectJob::ResumeSSLConnection() {
   DCHECK_EQ(next_state_, STATE_SSL_CONNECT);
+  messenger_ = NULL;
   OnIOComplete(OK);
 }
 
 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;
   }
   NOTREACHED();
   return STATE_NONE;
 }
 
 int SSLConnectJob::ConnectInternal() {
   next_state_ = GetInitialState(params_->GetConnectionType());
   return DoLoop(OK);
 }
 
 SSLClientSocketPool::SSLConnectJobFactory::SSLConnectJobFactory(
     TransportClientSocketPool* transport_pool,
     SOCKSClientSocketPool* socks_pool,
     HttpProxyClientSocketPool* http_proxy_pool,
     ClientSocketFactory* client_socket_factory,
     HostResolver* host_resolver,
     const SSLClientSocketContext& context,
-    bool enable_ssl_connect_job_waiting,
+    SSLConnectJob::GetMessengerCallback get_messenger_callback,
     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),
-      enable_ssl_connect_job_waiting_(enable_ssl_connect_job_waiting),
-      net_log_(net_log),
-      messenger_map_(new MessengerMap) {
+      get_messenger_callback_(get_messenger_callback),
+      net_log_(net_log) {
   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_) {
     pool_timeout = http_proxy_pool_->ConnectionTimeout();
     if (pool_timeout > max_transport_timeout)
       max_transport_timeout = pool_timeout;
   }
   timeout_ = max_transport_timeout +
       base::TimeDelta::FromSeconds(kSSLHandshakeTimeoutInSeconds);
 }
 
 SSLClientSocketPool::SSLConnectJobFactory::~SSLConnectJobFactory() {
-  STLDeleteValues(messenger_map_.get());
 }
 
 SSLClientSocketPool::SSLClientSocketPool(
     int max_sockets,
     int max_sockets_per_group,
     ClientSocketPoolHistograms* histograms,
     HostResolver* host_resolver,
     CertVerifier* cert_verifier,
     ChannelIDService* channel_id_service,
     TransportSecurityState* transport_security_state,
@@ skipping 19 matching lines @@
                 transport_pool,
                 socks_pool,
                 http_proxy_pool,
                 client_socket_factory,
                 host_resolver,
                 SSLClientSocketContext(cert_verifier,
                                        channel_id_service,
                                        transport_security_state,
                                        cert_transparency_verifier,
                                        ssl_session_cache_shard),
-                enable_ssl_connect_job_waiting,
+                base::Bind(
+                    &SSLClientSocketPool::GetOrCreateSSLConnectJobMessenger,
+                    base::Unretained(this)),

[wtc, 2014/08/14 23:26:26]

Please confirm that we don't need to use a weak pointer here. (I didn't check
this.)

[mshelley, 2014/08/15 16:53:06]

We do not -- the pool owns the SSLConnectJobs, therefore it will always be valid
when this callback is run.

                 net_log)),
-      ssl_config_service_(ssl_config_service) {
+      ssl_config_service_(ssl_config_service),
+      enable_ssl_connect_job_waiting_(enable_ssl_connect_job_waiting) {
   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() {
+  STLDeleteContainerPairSecondPointers(messenger_map_.begin(),
+                                       messenger_map_.end());
   if (ssl_config_service_.get())
     ssl_config_service_->RemoveObserver(this);
 }
 
-scoped_ptr<ConnectJob>
-SSLClientSocketPool::SSLConnectJobFactory::NewConnectJob(
+scoped_ptr<ConnectJob> SSLClientSocketPool::SSLConnectJobFactory::NewConnectJob(
     const std::string& group_name,
     const PoolBase::Request& request,
     ConnectJob::Delegate* delegate) const {
-  SSLConnectJobMessenger* messenger = NULL;
-  if (enable_ssl_connect_job_waiting_) {
-    std::string cache_key = SSLClientSocket::CreateSessionCacheKey(
-        request.params()->host_and_port(), 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,
+                                                  get_messenger_callback_,
                                                   delegate,
                                                   net_log_));
 }
 
-base::TimeDelta
-SSLClientSocketPool::SSLConnectJobFactory::ConnectionTimeout() const {
+base::TimeDelta SSLClientSocketPool::SSLConnectJobFactory::ConnectionTimeout()
+    const {
   return timeout_;
 }
 
 int SSLClientSocketPool::RequestSocket(const std::string& group_name,
                                        const void* socket_params,
                                        RequestPriority priority,
                                        ClientSocketHandle* handle,
                                        const CompletionCallback& callback,
                                        const BoundNetLog& net_log) {
   const scoped_refptr<SSLSocketParams>* casted_socket_params =
@@ skipping 94 matching lines @@
     HigherLayeredPool* higher_pool) {
   base_.RemoveHigherLayeredPool(higher_pool);
 }
 
 bool SSLClientSocketPool::CloseOneIdleConnection() {
   if (base_.CloseOneIdleSocket())
     return true;
   return base_.CloseOneIdleConnectionInHigherLayeredPool();
 }
 
+SSLConnectJobMessenger* SSLClientSocketPool::GetOrCreateSSLConnectJobMessenger(
+    const std::string& cache_key) {
+  if (!enable_ssl_connect_job_waiting_)
+    return NULL;
+  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(
+                base::Bind(&SSLClientSocketPool::DeleteSSLConnectJobMessenger,
+                           base::Unretained(this),

[wtc, 2014/08/14 23:26:26]

Please confirm that we don't need a weak pointer here. (I didn't check this.)

[mshelley, 2014/08/15 16:53:06]

We do not, the pool also owns the SSLConnectJobMessengers, therefore it will
always be valid when this callback is run.

+                           cache_key))));
+    it = iter.first;
+  }
+  return it->second;
+}
+
+void SSLClientSocketPool::DeleteSSLConnectJobMessenger(
+    const std::string& cache_key) {
+  MessengerMap::iterator it = messenger_map_.find(cache_key);
+  CHECK(it != messenger_map_.end());

[wtc, 2014/08/14 23:26:26]

You can also assert CHECK_EQ(this, it->second).

[mshelley, 2014/08/15 16:53:06]

I'm not sure if I understand how that would work -- wouldn't that be comparing a
SSLClientSocketPool* to a SSLConnectJobMessenger*?

[wtc, 2014/08/15 18:48:29]

Ah, you're right. I thought |this| was a pointer to the SSLConnectJobMessenger
object we're about to delete. I'm not sure if we should pass that pointer to
this method for extra validation, so let's leave this unchanged.

+  delete it->second;
+  messenger_map_.erase(it);
+}
+
 void SSLClientSocketPool::OnSSLConfigChanged() {
   FlushWithError(ERR_NETWORK_CHANGED);
 }
 
 }  // namespace net