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

net/socket/ssl_client_socket_pool.h

Index: net/socket/ssl_client_socket_pool.h
diff --git a/net/socket/ssl_client_socket_pool.h b/net/socket/ssl_client_socket_pool.h
index cfc7c0dfbabd3832d89d29ebc511b145d64226fd..8dac9fc24392e78cb497575741854c45b2a5a684 100644
--- a/net/socket/ssl_client_socket_pool.h
+++ b/net/socket/ssl_client_socket_pool.h
@@ -114,8 +114,12 @@ class SSLConnectJobMessenger {
   };
 
   typedef std::vector<SocketAndCallback> SSLPendingSocketsAndCallbacks;
+  typedef base::Callback<void(std::string, std::string)>
+      ConnectionCompleteCallback;

[wtc, 2014/08/12 14:51:01]

Please document what the two string inputs to ConnectionCompleteCallback are.

Also, they probably should be const std::string references.

[mshelley, 2014/08/12 21:47:01]

Done.

 
-  SSLConnectJobMessenger();
+  SSLConnectJobMessenger(
+      const ConnectionCompleteCallback& connection_complete_callback,
+      std::string group_name);

[Ryan Sleevi, 2014/08/12 00:27:43]

Do you really need |group_name| to be exposed at all? It seems unused in the
implementation. If so, you can just smuggle it with base::Bind currying.

And if that made no sense, throw something on my calendar :)

[wtc, 2014/08/12 14:51:01]

If |group_name| is needed, it should be a const reference.

[mshelley, 2014/08/12 21:47:02]

Done.

   ~SSLConnectJobMessenger();
 
   // Removes |socket| from the set of sockets being monitored. This
@@ -142,6 +146,10 @@ class SSLConnectJobMessenger {
   void AddPendingSocket(SSLClientSocket* ssl_socket,
                         const base::Closure& callback);
 
+  // Returns true if this SSLConnectJobMessenger still has pending
+  // connection callbacks.
+  bool IsNeeded();

[Ryan Sleevi, 2014/08/12 00:27:43]

Let's rename this then, to better match that it does

bool HasPendingConnections

?

[mshelley, 2014/08/12 21:47:01]

Realized I don't actually need this anymore, because we always resume all
pending connections whenever the leader completes. Thus we know that the
messenger is not needed as soon as the leader calls RunAllCallbacks in
OnSSLHandshakeCompleted.

+
  private:
   // Processes pending callbacks when a socket completes its SSL handshake --
   // either successfully or unsuccessfully.
@@ -151,18 +159,27 @@ class SSLConnectJobMessenger {
   void RunAllCallbacks(
       const SSLPendingSocketsAndCallbacks& pending_socket_and_callbacks);
 
-  base::WeakPtrFactory<SSLConnectJobMessenger> weak_factory_;
-
   SSLPendingSocketsAndCallbacks pending_sockets_and_callbacks_;
   // Note: this field is a vector to allow for future design changes. Currently,
   // this vector should only ever have one entry.
   std::vector<SSLClientSocket*> connecting_sockets_;
+
+  std::string group_name_;
+
+  base::WeakPtrFactory<SSLConnectJobMessenger> weak_factory_;
+
+  const ConnectionCompleteCallback connection_complete_callback_;

[Ryan Sleevi, 2014/08/12 00:27:42]

Why const?

[mshelley, 2014/08/12 21:47:01]

Done.

 };
 
 // SSLConnectJob handles the SSL handshake after setting up the underlying
 // connection as specified in the params.
 class SSLConnectJob : public ConnectJob {
  public:
+  // The returned messenger is a pointer to an existing messenger
+  // owned by the SSLConnectJobFactory.
+  typedef base::Callback<SSLConnectJobMessenger*(std::string, std::string)>
+      UncachedSessionCallback;

[Ryan Sleevi, 2014/08/12 00:27:43]

I guess I'm still unclear what this means, or how to use it properly, just from
reading the header.

[mshelley, 2014/08/12 21:47:02]

Done.

+
   // Note: the SSLConnectJob does not own |messenger| so it must outlive the
   // job.
   SSLConnectJob(const std::string& group_name,
@@ -175,7 +192,8 @@ class SSLConnectJob : public ConnectJob {
                 ClientSocketFactory* client_socket_factory,
                 HostResolver* host_resolver,
                 const SSLClientSocketContext& context,
-                SSLConnectJobMessenger* messenger,
+                UncachedSessionCallback uncached_session_callback,

[Ryan Sleevi, 2014/08/12 00:27:43]

This still feels a bit circular. Perhaps we should chat in person?

[mshelley, 2014/08/12 21:47:02]

Done.

+                bool enable_ssl_connect_job_waiting,
                 Delegate* delegate,
                 NetLog* net_log);
   virtual ~SSLConnectJob();
@@ -185,6 +203,8 @@ class SSLConnectJob : public ConnectJob {
 
   virtual void GetAdditionalErrorState(ClientSocketHandle * handle) OVERRIDE;
 
+  void RemoveMessenger(const std::string& cache_key);
+
  private:
   enum State {
     STATE_TRANSPORT_CONNECT,
@@ -219,6 +239,10 @@ class SSLConnectJob : public ConnectJob {
   // Tells a waiting SSLConnectJob to resume its SSL connection.
   void ResumeSSLConnection();
 
+  // Runs a callback informing the SSLClientSocketPool that this
+  // SSLConnectJob's SSL session is not in the session cache.
+  void RunUncachedSessionCallback(std::string cache_key);
+
   // Returns the initial state for the state machine based on the
   // |connection_type|.
   static State GetInitialState(SSLSocketParams::ConnectionType connection_type);
@@ -247,6 +271,9 @@ class SSLConnectJob : public ConnectJob {
 
   base::WeakPtrFactory<SSLConnectJob> weak_factory_;
 
+  SSLConnectJob::UncachedSessionCallback uncached_session_callback_;
+  bool enable_ssl_connect_job_waiting_;
+
   DISALLOW_COPY_AND_ASSIGN(SSLConnectJob);
 };
 
@@ -330,8 +357,17 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
   // HigherLayeredPool implementation.
   virtual bool CloseOneIdleConnection() OVERRIDE;
 
+  // Creates an SSLConnectJobMessenger for the given |cache_key| and stores it
+  // in |messenger_map_|. Returns the new SSLConnectJobMessenger.
+  SSLConnectJobMessenger* AddSSLConnectJobMessenger(std::string cache_key,
+                                                    std::string group_name);
+  void DeleteSSLConnectJobMessenger(std::string cache_key,
+                                    std::string group_name);
+
  private:
   typedef ClientSocketPoolBase<SSLSocketParams> PoolBase;
+  // Maps SSLConnectJob cache keys to SSLConnectJobMessenger objects.
+  typedef std::map<std::string, SSLConnectJobMessenger*> MessengerMap;
 
   // SSLConfigService::Observer implementation.
 
@@ -341,14 +377,16 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
 
   class SSLConnectJobFactory : public PoolBase::ConnectJobFactory {
    public:
-    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,
-                         NetLog* net_log);
+    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::UncachedSessionCallback uncached_session_callback,
+        NetLog* net_log);
 
     virtual ~SSLConnectJobFactory();
 
@@ -356,7 +394,7 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
     virtual scoped_ptr<ConnectJob> NewConnectJob(
         const std::string& group_name,
         const PoolBase::Request& request,
-        ConnectJob::Delegate* delegate) const OVERRIDE;
+        ConnectJob::Delegate* delegate) OVERRIDE;
 
     virtual base::TimeDelta ConnectionTimeout() const OVERRIDE;
 
@@ -372,12 +410,8 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
     const SSLClientSocketContext context_;
     base::TimeDelta timeout_;
     bool enable_ssl_connect_job_waiting_;
+    SSLConnectJob::UncachedSessionCallback uncached_session_callback_;
     NetLog* net_log_;
-    // |messenger_map_| is currently a pointer so that an element can be
-    // added to it inside of the const method NewConnectJob. In the future,
-    // elements will be added in a different method.
-    // TODO(mshelley) Change this to a non-pointer.
-    scoped_ptr<MessengerMap> messenger_map_;
 
     DISALLOW_COPY_AND_ASSIGN(SSLConnectJobFactory);
   };
@@ -387,6 +421,7 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
   HttpProxyClientSocketPool* const http_proxy_pool_;
   PoolBase base_;
   const scoped_refptr<SSLConfigService> ssl_config_service_;
+  MessengerMap messenger_map_;
 
   DISALLOW_COPY_AND_ASSIGN(SSLClientSocketPool);
 };