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

net/socket/ssl_client_socket_pool.h

 // 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.
 
 #ifndef NET_SOCKET_SSL_CLIENT_SOCKET_POOL_H_
 #define NET_SOCKET_SSL_CLIENT_SOCKET_POOL_H_
 
 #include <map>
 #include <string>
 #include <vector>
@@ skipping 124 matching lines @@
   void OnJobSucceeded();
 
   // Posts a task to process pending callbacks when a socket encounters an error
   // while completing its connection.
   void OnJobFailed();
 
   // Determines which pending socket should be the next leading connection,
   // 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(
       std::vector<SocketAndCallback>& pending_sockets_and_callbacks);
 
   base::WeakPtrFactory<SSLConnectJobMessenger> weak_factory_;
   SSLPendingSocketsAndCallbacks pending_sockets_and_callbacks_;
   std::vector<SSLClientSocket*> connecting_sockets_;
 };
 
 // SSLConnectJob handles the SSL handshake after setting up the underlying
 // 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,
                 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,
+                AddMessengerCallback add_messenger_calblack,
+                DeleteMessengerCallback delete_messenger_callback,
                 Delegate* delegate,
                 NetLog* net_log);
   virtual ~SSLConnectJob();
 
   // ConnectJob methods.
   virtual LoadState GetLoadState() const OVERRIDE;
 
   virtual void GetAdditionalErrorState(ClientSocketHandle * handle) OVERRIDE;
 
  private:
@@ skipping 23 matching lines @@
   int DoTunnelConnect();
   int DoTunnelConnectComplete(int result);
   int DoCreateSSLSocket();
   int DoCheckForResume();
   int DoSSLConnect();
   int DoSSLConnectComplete(int result);
 
   // 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);
 
   // Starts the SSL connection process.  Returns OK on success and
   // ERR_IO_PENDING if it cannot immediately service the request.
   // Otherwise, it returns a net error code.
   virtual int ConnectInternal() OVERRIDE;
 
   scoped_refptr<SSLSocketParams> params_;
   TransportClientSocketPool* const transport_pool_;
   SOCKSClientSocketPool* const socks_pool_;
   HttpProxyClientSocketPool* const http_proxy_pool_;
   ClientSocketFactory* const client_socket_factory_;
   HostResolver* const host_resolver_;
 
   const SSLClientSocketContext context_;
 
   State next_state_;
   CompletionCallback io_callback_;
   scoped_ptr<ClientSocketHandle> transport_socket_handle_;
   scoped_ptr<SSLClientSocket> ssl_socket_;
 
   SSLConnectJobMessenger* messenger_;
   HttpResponseInfo error_response_info_;
 
   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);
 };
 
 class NET_EXPORT_PRIVATE SSLClientSocketPool
     : public ClientSocketPool,
       public HigherLayeredPool,
       public SSLConfigService::Observer {
  public:
   typedef SSLSocketParams SocketParams;
 
@@ skipping 85 matching lines @@
         TransportClientSocketPool* transport_pool,
         SOCKSClientSocketPool* socks_pool,
         HttpProxyClientSocketPool* http_proxy_pool,
         ClientSocketFactory* client_socket_factory,
         HostResolver* host_resolver,
         const SSLClientSocketContext& context,
         NetLog* net_log);
 
     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;
 
    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_;
     ClientSocketFactory* const client_socket_factory_;
     HostResolver* const host_resolver_;
     const SSLClientSocketContext context_;
     base::TimeDelta timeout_;
     NetLog* net_log_;
-    scoped_ptr<MessengerMap> messenger_map_;
+    MessengerMap messenger_map_;
 
     DISALLOW_COPY_AND_ASSIGN(SSLConnectJobFactory);
   };
 
   TransportClientSocketPool* const transport_pool_;
   SOCKSClientSocketPool* const socks_pool_;
   HttpProxyClientSocketPool* const http_proxy_pool_;
   PoolBase base_;
   const scoped_refptr<SSLConfigService> ssl_config_service_;
 
   DISALLOW_COPY_AND_ASSIGN(SSLClientSocketPool);
 };
 
 }  // namespace net
 
 #endif  // NET_SOCKET_SSL_CLIENT_SOCKET_POOL_H_