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 87b1212995dffe3c212806d44325c55a6c72ec99..457ce066e974ad752084bd737ca70ee5233e7928 100644
--- a/net/socket/ssl_client_socket_pool.h
+++ b/net/socket/ssl_client_socket_pool.h
@@ -142,6 +142,9 @@ class NET_EXPORT_PRIVATE SSLConnectJobMessenger {
   // and runs that socket's resumption callback.
   void ConnectNewLeader();
 
+  // Returns true if this SSLConnectJobMessenger is still in use.
+  bool IsNeeded();

[Ryan Sleevi, 2014/07/25 21:40:07]

What does "in use" mean? By who? Better to expand on this (e.g. do you mean
whether or not callbacks are pending? If we're still monitoring sockets?
Something else?)

[mshelley, 2014/07/28 21:05:32]

Done.

+
  private:
   // Runs all callbacks stored in |pending_sockets_and_callbacks_|.
   void RunAllJobs(
@@ -156,6 +159,10 @@ class NET_EXPORT_PRIVATE SSLConnectJobMessenger {
 // connection as specified in the params.
 class SSLConnectJob : public ConnectJob {
  public:
+  typedef base::Callback<SSLConnectJobMessenger*(std::string)>

[Ryan Sleevi, 2014/07/25 21:40:07]

What is the ownership semantics of the Messenger passed back? Does the caller
own it? Or is it a pointer to an existing object? What's the lifetime of that
object.

[mshelley, 2014/07/28 21:05:32]

Done.

+      AddMessengerCallback;
+  typedef base::Callback<void(std::string)> DeleteMessengerCallback;
+
   // Note: the SSLConnectJob does not own |messenger|.
   // so it must outlive the job.
   SSLConnectJob(const std::string& group_name,
@@ -169,6 +176,8 @@ class SSLConnectJob : public ConnectJob {
                 HostResolver* host_resolver,
                 const SSLClientSocketContext& context,
                 SSLConnectJobMessenger* messenger,
+                AddMessengerCallback add_messenger_calblack,
+                DeleteMessengerCallback delete_messenger_callback,
                 Delegate* delegate,
                 NetLog* net_log);
   virtual ~SSLConnectJob();
@@ -212,6 +221,14 @@ class SSLConnectJob : public ConnectJob {
   // Tells a waiting SSLConnectJob to resume its SSL connection.
   void ResumeSSLConnection();
 
+  // Retrieves an SSLConnectJobMessenger from the SSLConnectJobFactory and
+  // stores it in the messenger_ field.
+  void GetMessenger(std::string cache_key);
+
+  // Deletes this job's SSLConnectJobMessenger from the SSLConnectJobFactory's
+  // map of messengers, and deletes the messenger itself.
+  void DeleteMessenger(std::string cache_key);

[Ryan Sleevi, 2014/07/25 21:40:07]

This seems like a layering violation. The SSLConnectJob shouldn't be responsible
for the lifetime management of the Messenger.

It might help to write out a bit about what the expected lifetimes are of the
objects (just in this CL as a description/reply to this), so then we can make
sure the code matches those lifetimes. I, perhaps naievely, assumed the Pool
managed the Messenger. In which case, Sockets should talk to the Pool, but
whether or not a Messenger is deleted or not is opaque to the Socket.

[mshelley, 2014/07/28 21:05:32]

Right now, the SSLConnectJobFactory manages the lifetime of messengers.
Basically, a messenger should be given to an SSL Connect Job whenever that
connection is not in the session cache. This is because whenever a session isn't
in the cache, the job must be either a leading job, or a future pending job (so
it needs a messenger). Thus, messengers are created whenever a job isn't in the
cache, and a messenger doesn't already exist for that job's cache key. 

Messengers are deleted whenever a connection finishes and there are no pending
jobs left in the messenger.

Since this is determined by the SSLConnectJobs, I put the connect job factory in
charge of creating/owning messengers because then giving SSLConnectJobs
callbacks to control creating/deleting messengers would be relatively
straightforward. 

I can easily change this so that the pool manages this though, that does seem
more logical to me.

+
   // Returns the initial state for the state machine based on the
   // |connection_type|.
   static State GetInitialState(SSLSocketParams::ConnectionType connection_type);
@@ -240,6 +257,9 @@ class SSLConnectJob : public ConnectJob {
 
   base::WeakPtrFactory<SSLConnectJob> weak_factory_;
 
+  AddMessengerCallback add_messenger_callback_;
+  DeleteMessengerCallback delete_messenger_callback_;

[Ryan Sleevi, 2014/07/25 21:40:07]

What are these callbacks?

Connect() can be called multiple times, but general principal is not to re-use a
callback, so how does that affect how this works?

[mshelley, 2014/07/28 21:05:32]

These are callbacks to methods of the SSLConnectJobFactory, which actually
controls creating/deleting messengers. The SSLConnectJob methods GetMessenger &
DeleteMessenger just run these callbacks.

+
   DISALLOW_COPY_AND_ASSIGN(SSLConnectJob);
 };
 
@@ -345,11 +365,18 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
 
     virtual ~SSLConnectJobFactory() {}
 
+    // Creates an SSLConnectJobMessenger for the given |cache_key| and stores it
+    // in |messenger_map_|. Returns the new SSLConnectJobMessenger.
+    SSLConnectJobMessenger* AddSSLConnectJobMessenger(std::string cache_key);
+
+    // Deletes the entry for the given |cache_key| from the |messenger_map_|.
+    void DeleteSSLConnectJobMessenger(std::string cache_key);
+
     // ClientSocketPoolBase::ConnectJobFactory methods.
     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;
 
@@ -365,7 +392,7 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
     const SSLClientSocketContext context_;
     base::TimeDelta timeout_;
     NetLog* net_log_;
-    scoped_ptr<MessengerMap> messenger_map_;
+    MessengerMap messenger_map_;
 
     DISALLOW_COPY_AND_ASSIGN(SSLConnectJobFactory);
   };