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..b62230443709761ac30ae5041e7505d496fbaeca 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()> ConnectionCompleteCallback;

[Ryan Sleevi, 2014/08/14 01:20:20]

We have a typedef for this already - base::Closure

[mshelley, 2014/08/14 20:01:36]

Done.

 
-  SSLConnectJobMessenger();
+  // |connection_complete_callback| is used to inform the client socket pool
+  // that a connection monitored by the SSLConnectJobMessenger has completed.
+  SSLConnectJobMessenger(
+      const ConnectionCompleteCallback& connection_complete_callback);

[wtc, 2014/08/14 00:46:46]

DESIGN: instead of an abstract callback function, I would pass an
SSLClientSocketPool* pointer and the cache key, and have the
SSLConnectJobMessenger call socket_pool->DeleteSSLConnectJobMessenger(cache_key)
directly.

[Ryan Sleevi, 2014/08/14 01:20:20]

I recommended against this, to avoid introducing additional circular
dependencies for what is a header-visible class.

This may be unnecessary complexity for now, but the concern was about messengers
with different connection strategies, and wanting to decouple them as much as
possible from the appropriate bits.

Since ConnectJob depends on Messenger, and Pool depends on ConnectJob, I wanted
to avoid a circular dependency where Messenger depends on Pool (thus making Pool
depend on ConnectJob and ConnectJob depend, indirectly, on Pool)

[mshelley, 2014/08/14 20:01:37]

So is the benefit of passing in the pointer directly just that it's more
straightforward? I've left it as it is because I'm not sure if that concern
outweighs Ryan's concern about circular dependencies or not.

   ~SSLConnectJobMessenger();
 
   // Removes |socket| from the set of sockets being monitored. This
@@ -151,33 +155,47 @@ 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_;
+
+  ConnectionCompleteCallback connection_complete_callback_;
+
+  base::WeakPtrFactory<SSLConnectJobMessenger> weak_factory_;
 };
 
 // SSLConnectJob handles the SSL handshake after setting up the underlying
 // connection as specified in the params.
 class SSLConnectJob : public ConnectJob {
  public:
+  // Callback to allow the SSLConnectJob to obtain an SSLConnectJobMessenger to
+  // coordinate connecting. The SSLConnectJob will supply a unique identifer
+  // (ex: the SSL session cache key), with the expectation that the same
+  // Messenger
+  // will be returned for all such ConnectJobs.

[wtc, 2014/08/14 00:46:46]

Please reformat this comment block.

[mshelley, 2014/08/14 20:01:37]

Done.

+  //
+  // Note: It will only be called for situations where the SSL session cache
+  // does not already have a candidate session to resume.
+  typedef base::Callback<SSLConnectJobMessenger*(const std::string&)>
+      GetMessengerForUncachedSessionCallback;

[wtc, 2014/08/14 00:46:46]

Nit: I would shorten this type to "GetMessengerCallback".

[mshelley, 2014/08/14 20:01:37]

Done.

+
   // 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,
+      const GetMessengerForUncachedSessionCallback& uncached_session_callback,

[wtc, 2014/08/14 00:46:46]

Between the "get messenger" and the "uncached session", I think "get messenger"
is more important. So I suggest naming this parameter "get_messenger_callback".
Similarly, I think the uncached_session_callback_ member should be renamed
"get_messenger_callback_".

[wtc, 2014/08/14 00:46:46]

DESIGN: I would just pass a SSLClientSocketPool* pointer, and have the
SSLConnectJob call
socket_pool->AddSSLConnectJobMessenger(ssl_socket_->GetSessionCacheKey())
directly.

[Ryan Sleevi, 2014/08/14 01:20:20]

Same comment re: coupling.

To date, there's nothing that requires the SSLConnectJob depend on a Pool, and I
was hoping to avoid that. For example, one can supply a callback that returns
NULL to not implement this feature, or other (non-SocketPools) can use
messengers to coordinate session establishment outside of the use of a pool.

[mshelley, 2014/08/14 20:01:37]

Done.

+      Delegate* delegate,
+      NetLog* net_log);
   virtual ~SSLConnectJob();
 
   // ConnectJob methods.
@@ -245,6 +263,9 @@ class SSLConnectJob : public ConnectJob {
   SSLConnectJobMessenger* messenger_;
   HttpResponseInfo error_response_info_;
 
+  SSLConnectJob::GetMessengerForUncachedSessionCallback

[wtc, 2014/08/14 00:46:46]

You should be able to omit "SSLConnectJob::".

[mshelley, 2014/08/14 20:01:37]

Done.

+      uncached_session_callback_;
+
   base::WeakPtrFactory<SSLConnectJob> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SSLConnectJob);
@@ -330,8 +351,16 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
   // HigherLayeredPool implementation.
   virtual bool CloseOneIdleConnection() OVERRIDE;
 
+  // Creates an SSLConnectJobMessenger for the given ssl session |cache_key|

[wtc, 2014/08/14 00:46:45]

This comment is not accurate because the method may simply look up an existing
SSLConnectJobMessenger from |messenger_map_|. So, the
"AddSSLConnectJobMessenger" function name is also not accurate because "Add" is
not always done. I suggest using "Get" or "Lookup" instead. (We've also used
"GetOrCreate" in this situation if you don't mind the verbosity.)

[mshelley, 2014/08/14 20:01:37]

Done.

+  // and stores it in |messenger_map_|. Returns the new SSLConnectJobMessenger.
+  SSLConnectJobMessenger* AddSSLConnectJobMessenger(
+      const std::string& cache_key);
+  void DeleteSSLConnectJobMessenger(const std::string& cache_key);
+
  private:
   typedef ClientSocketPoolBase<SSLSocketParams> PoolBase;
+  // Maps SSLConnectJob cache keys to SSLConnectJobMessenger objects.
+  typedef std::map<std::string, SSLConnectJobMessenger*> MessengerMap;
 
   // SSLConfigService::Observer implementation.
 
@@ -347,7 +376,8 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
                          ClientSocketFactory* client_socket_factory,
                          HostResolver* host_resolver,
                          const SSLClientSocketContext& context,
-                         bool enable_ssl_connect_job_waiting,
+                         SSLConnectJob::GetMessengerForUncachedSessionCallback
+                             uncached_session_callback,

[wtc, 2014/08/14 00:46:46]

Rename this "get_messenger_callback".

[mshelley, 2014/08/14 20:01:36]

Done.

                          NetLog* net_log);
 
     virtual ~SSLConnectJobFactory();
@@ -361,9 +391,6 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
     virtual base::TimeDelta ConnectionTimeout() const OVERRIDE;
 
    private:
-    // Maps SSLConnectJob cache keys to SSLConnectJobMessenger objects.
-    typedef std::map<std::string, SSLConnectJobMessenger*> MessengerMap;
-
     TransportClientSocketPool* const transport_pool_;
     SOCKSClientSocketPool* const socks_pool_;
     HttpProxyClientSocketPool* const http_proxy_pool_;
@@ -371,13 +398,9 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
     HostResolver* const host_resolver_;
     const SSLClientSocketContext context_;
     base::TimeDelta timeout_;
-    bool enable_ssl_connect_job_waiting_;
+    SSLConnectJob::GetMessengerForUncachedSessionCallback
+        uncached_session_callback_;

[wtc, 2014/08/14 00:46:45]

Rename this "get_messenger_callback".

[mshelley, 2014/08/14 20:01:36]

Done.

     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 +410,8 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
   HttpProxyClientSocketPool* const http_proxy_pool_;
   PoolBase base_;
   const scoped_refptr<SSLConfigService> ssl_config_service_;
+  MessengerMap messenger_map_;
+  bool enable_ssl_connect_job_waiting_;
 
   DISALLOW_COPY_AND_ASSIGN(SSLClientSocketPool);
 };