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;
+  typedef base::Callback<void(std::string, std::string)>
+      ConnectionCompleteCallback;

[wtc, 2014/08/12 14:51:01]

Please document what the two string inputs to ConnectionCompleteCallback are.

Also, they probably should be const std::string references.

[mshelley, 2014/08/12 21:47:01]

Done.

 
-  SSLConnectJobMessenger();
+  SSLConnectJobMessenger(
+      const ConnectionCompleteCallback& connection_complete_callback,
+      std::string group_name);

[Ryan Sleevi, 2014/08/12 00:27:43]

Do you really need |group_name| to be exposed at all? It seems unused in the
implementation. If so, you can just smuggle it with base::Bind currying.

And if that made no sense, throw something on my calendar :)

[wtc, 2014/08/12 14:51:01]

If |group_name| is needed, it should be a const reference.

[mshelley, 2014/08/12 21:47:02]

Done.

   ~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);
 
   // Configures the SSLConnectJobMessenger to begin monitoring |ssl_socket|'s
   // connection status. After a successful connection, or an error,
   // the messenger will determine which sockets that have been added
   // via AddPendingSocket() to allow to proceed.
   void MonitorConnectionResult(SSLClientSocket* ssl_socket);
 
   // Adds |socket| to the list of sockets waiting to Connect(). When
   // the messenger has determined that it's an appropriate time for |socket|
   // to connect, it will invoke |callback|.
   //
   // Note: It is an error to call AddPendingSocket() without having first
   // called MonitorConnectionResult() and configuring a socket that WILL
   // have Connect() called on it.
   void AddPendingSocket(SSLClientSocket* ssl_socket,
                         const base::Closure& callback);
 
+  // Returns true if this SSLConnectJobMessenger still has pending
+  // connection callbacks.
+  bool IsNeeded();

[Ryan Sleevi, 2014/08/12 00:27:43]

Let's rename this then, to better match that it does

bool HasPendingConnections

?

[mshelley, 2014/08/12 21:47:01]

Realized I don't actually need this anymore, because we always resume all
pending connections whenever the leader completes. Thus we know that the
messenger is not needed as soon as the leader calls RunAllCallbacks in
OnSSLHandshakeCompleted.

+
  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_;
+
+  base::WeakPtrFactory<SSLConnectJobMessenger> weak_factory_;
+
+  const ConnectionCompleteCallback connection_complete_callback_;

[Ryan Sleevi, 2014/08/12 00:27:42]

Why const?

[mshelley, 2014/08/12 21:47:01]

Done.

 };
 
 // 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 SSLConnectJobFactory.
+  typedef base::Callback<SSLConnectJobMessenger*(std::string, std::string)>
+      UncachedSessionCallback;

[Ryan Sleevi, 2014/08/12 00:27:43]

I guess I'm still unclear what this means, or how to use it properly, just from
reading the header.

[mshelley, 2014/08/12 21:47:02]

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,
+                UncachedSessionCallback uncached_session_callback,

[Ryan Sleevi, 2014/08/12 00:27:43]

This still feels a bit circular. Perhaps we should chat in person?

[mshelley, 2014/08/12 21:47:02]

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;
 
+  void RemoveMessenger(const std::string& cache_key);
+
  private:
   enum State {
     STATE_TRANSPORT_CONNECT,
     STATE_TRANSPORT_CONNECT_COMPLETE,
     STATE_SOCKS_CONNECT,
     STATE_SOCKS_CONNECT_COMPLETE,
     STATE_TUNNEL_CONNECT,
     STATE_TUNNEL_CONNECT_COMPLETE,
     STATE_CREATE_SSL_SOCKET,
     STATE_CHECK_FOR_RESUME,
@@ skipping 14 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 RunUncachedSessionCallback(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::UncachedSessionCallback uncached_session_callback_;
+  bool enable_ssl_connect_job_waiting_;
+
   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(std::string cache_key,
+                                                    std::string group_name);
+  void DeleteSSLConnectJobMessenger(std::string cache_key,
+                                    std::string group_name);
+
  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,
-                         NetLog* net_log);
+    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::UncachedSessionCallback 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;
+        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_;
     bool enable_ssl_connect_job_waiting_;
+    SSLConnectJob::UncachedSessionCallback 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_