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 96 matching lines @@
   struct SocketAndCallback {
     SocketAndCallback(SSLClientSocket* ssl_socket,
                       const base::Closure& job_resumption_callback);
     ~SocketAndCallback();
 
     SSLClientSocket* socket;
     base::Closure callback;
   };
 
   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
   // guarantees that |job_resumption_callback| will not be called for
   // the socket.
   void RemovePendingSocket(SSLClientSocket* ssl_socket);
 
   // Returns true if |ssl_socket|'s Connect() method should be called.
   bool CanProceed(SSLClientSocket* ssl_socket);
 
@@ skipping 15 matching lines @@
 
  private:
   // Processes pending callbacks when a socket completes its SSL handshake --
   // either successfully or unsuccessfully.
   void OnSSLHandshakeCompleted();
 
   // Runs all callbacks stored in |pending_sockets_and_callbacks_|.
   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.
   virtual LoadState GetLoadState() const OVERRIDE;
 
   virtual void GetAdditionalErrorState(ClientSocketHandle * handle) OVERRIDE;
 
  private:
   enum State {
     STATE_TRANSPORT_CONNECT,
@@ skipping 21 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();
 
+  // 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);
 
   // 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_;
 
+  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);
 };
 
 class NET_EXPORT_PRIVATE SSLClientSocketPool
     : public ClientSocketPool,
       public HigherLayeredPool,
       public SSLConfigService::Observer {
  public:
   typedef SSLSocketParams SocketParams;
 
@@ skipping 63 matching lines @@
   // LowerLayeredPool implementation.
   virtual bool IsStalled() const OVERRIDE;
 
   virtual void AddHigherLayeredPool(HigherLayeredPool* higher_pool) OVERRIDE;
 
   virtual void RemoveHigherLayeredPool(HigherLayeredPool* higher_pool) OVERRIDE;
 
   // 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.
 
   // When the user changes the SSL config, we flush all idle sockets so they
   // won't get re-used.
   virtual void OnSSLConfigChanged() OVERRIDE;
 
   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,
+                         SSLConnectJob::GetMessengerForUncachedSessionCallback
+                             uncached_session_callback,
                          NetLog* net_log);
 
     virtual ~SSLConnectJobFactory();
 
     // ClientSocketPoolBase::ConnectJobFactory methods.
     virtual scoped_ptr<ConnectJob> NewConnectJob(
         const std::string& group_name,
         const PoolBase::Request& request,
         ConnectJob::Delegate* delegate) const 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_;
     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);
   };
 
   TransportClientSocketPool* const transport_pool_;
   SOCKSClientSocketPool* const socks_pool_;
   HttpProxyClientSocketPool* const http_proxy_pool_;
   PoolBase base_;
   const scoped_refptr<SSLConfigService> ssl_config_service_;
+  MessengerMap messenger_map_;
 
   DISALLOW_COPY_AND_ASSIGN(SSLClientSocketPool);
 };
 
 }  // namespace net
 
 #endif  // NET_SOCKET_SSL_CLIENT_SOCKET_POOL_H_