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..2b71a122917eaf18113a75c6f4cfba3f6e5b9823 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;
+  // The input string to ConnectionCompleteCallback is the SSL session cache key
+  // associated with this messenger.
+  typedef base::Callback<void(const std::string&)> ConnectionCompleteCallback;

[Ryan Sleevi, 2014/08/13 01:02:15]

Usually we annotate callbacks like
typedef base::Callback<void(const std::string& /* session_cache_key */>)
ConnectionCompletionCallback

Then something like
// |session_cache_key| is the identifier of the SSL session that will
// be handled by this messenger.

[mshelley, 2014/08/13 08:10:00]

Done.

 
-  SSLConnectJobMessenger();
+  SSLConnectJobMessenger(
+      const ConnectionCompleteCallback& connection_complete_callback);

[Ryan Sleevi, 2014/08/13 01:02:15]

A bit of documentation is needed here to explain why/how someone would set the
connection_complete_callback, and what it means

For example, why is it necessary to expose (to the higher layer) the SSL session
key? Couldn't we manage it some other way?

Since we need to communicate to the creator when a session is completed, they
could use base::Bind() to smuggle whatever information they need through us.

   ~SSLConnectJobMessenger();
 
   // Removes |socket| from the set of sockets being monitored. This
@@ -151,33 +155,44 @@ 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_;

[Ryan Sleevi, 2014/08/13 01:02:15]

No longer needed?

[mshelley, 2014/08/13 08:09:59]

Done.

+
+  base::WeakPtrFactory<SSLConnectJobMessenger> weak_factory_;

[Ryan Sleevi, 2014/08/13 01:02:15]

WeakPtrFactory's should always be last.

[mshelley, 2014/08/13 08:09:59]

Done.

+
+  ConnectionCompleteCallback connection_complete_callback_;
 };
 
 // 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 SSLClientSocketPool.

[Ryan Sleevi, 2014/08/13 01:02:15]

// Callback to allow the SSLConnectJob to obtain an SSLConnectJobMessenger to
coordinate connecting.
// The SSLConnectJob will supply a unique identifer (ex: the SSL session ID),
with the expectation that the same Messenger will be returned for
// all such ConnectJobs.
// Note: It will only be called for situations where the SSL session cache
// does not already have a candidate session to resume.

[mshelley, 2014/08/13 08:09:59]

Done.

+  typedef base::Callback<SSLConnectJobMessenger*(const std::string&)>
+      GetMessengerForUncachedSessionCallback;
+
   // Note: the SSLConnectJob does not own |messenger| so it must outlive the
   // job.
-  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,
-                Delegate* delegate,
-                NetLog* net_log);
+  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,
+      GetMessengerForUncachedSessionCallback uncached_session_callback,

[Ryan Sleevi, 2014/08/13 01:02:15]

const-ref the callback.

[mshelley, 2014/08/13 08:09:59]

Done.

+      bool enable_ssl_connect_job_waiting,
+      Delegate* delegate,
+      NetLog* net_log);
   virtual ~SSLConnectJob();
 
   // ConnectJob methods.
@@ -219,6 +234,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 RunGetMessengerForUncachedSessionCallback(const 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 +266,10 @@ class SSLConnectJob : public ConnectJob {
 
   base::WeakPtrFactory<SSLConnectJob> weak_factory_;
 
+  SSLConnectJob::GetMessengerForUncachedSessionCallback
+      uncached_session_callback_;
+  bool enable_ssl_connect_job_waiting_;

[Ryan Sleevi, 2014/08/13 01:02:15]

1) Document
2) Is the bool really needed?

[mshelley, 2014/08/13 08:09:59]

It can be avoided -- previously I thought I needed it so that I only
created/retrieved a messenger for the job if the flag was enabled. I can just
return NULL if the flag is enabled instead though, which means only the pool
needs to know about the flag.

+
   DISALLOW_COPY_AND_ASSIGN(SSLConnectJob);
 };
 
@@ -330,8 +353,16 @@ 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(
+      const std::string& cache_key);
+  void DeleteSSLConnectJobMessenger(const std::string& cache_key);

[Ryan Sleevi, 2014/08/13 01:02:15]

what's cache_key? group_name? session id? Something else?

Whatever it is, ensure this header file consistently calls it the same thing

[mshelley, 2014/08/13 08:09:59]

Done.

+
  private:
   typedef ClientSocketPoolBase<SSLSocketParams> PoolBase;
+  // Maps SSLConnectJob cache keys to SSLConnectJobMessenger objects.
+  typedef std::map<std::string, SSLConnectJobMessenger*> MessengerMap;
 
   // SSLConfigService::Observer implementation.
 
@@ -348,6 +379,8 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
                          HostResolver* host_resolver,
                          const SSLClientSocketContext& context,
                          bool enable_ssl_connect_job_waiting,
+                         SSLConnectJob::GetMessengerForUncachedSessionCallback
+                             uncached_session_callback,
                          NetLog* net_log);
 
     virtual ~SSLConnectJobFactory();
@@ -372,12 +405,9 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
     const SSLClientSocketContext context_;
     base::TimeDelta timeout_;
     bool enable_ssl_connect_job_waiting_;
+    SSLConnectJob::GetMessengerForUncachedSessionCallback
+        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 +417,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);
 };