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 97 matching lines @@
     SocketAndCallback(SSLClientSocket* ssl_socket,
                       const base::Closure& job_resumption_callback);
     ~SocketAndCallback();
 
     SSLClientSocket* socket;
     base::Closure callback;
   };
 
   typedef std::vector<SocketAndCallback> SSLPendingSocketsAndCallbacks;
 
-  SSLConnectJobMessenger();
+  // |messenger_finished_callback| is used to inform the client socket pool
+  // that a connection monitored by the SSLConnectJobMessenger has completed.

[wtc, 2014/08/15 18:48:29]

Nit: this comment should ideally avoid explicit reference to the client socket
pool. Would be better to only describe "the messenger is finished" part.

+  SSLConnectJobMessenger(const base::Closure& messenger_finished_callback);
   ~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);
 
+  std::vector<SSLClientSocket*> connecting_sockets() {

[wtc, 2014/08/15 18:48:29]

IMPORTANT: I didn't realize that the removal of CanProceed() requires us to give
access to the private connecting_sockets_ member.

It's bad to add this getter method. Let's resurrect the CanProceed() method. We
probably should keep its |ssl_socket| parameter for generality even though it is
currently unused. If you decide to delete the |ssl_socket| parameter, please
update the comment.

+    return connecting_sockets_;
+  };
+
  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_;
+
+  base::Closure messenger_finished_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.
+  //
+  // 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&)>
+      GetMessengerCallback;
+
   // 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,
+                const GetMessengerCallback& get_messenger_callback,
                 Delegate* delegate,
                 NetLog* net_log);
   virtual ~SSLConnectJob();
 
   // ConnectJob methods.
   virtual LoadState GetLoadState() const OVERRIDE;
 
   virtual void GetAdditionalErrorState(ClientSocketHandle * handle) OVERRIDE;
 
  private:
@@ skipping 49 matching lines @@
   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_;
 
+  GetMessengerCallback get_messenger_callback_;
+
   base::WeakPtrFactory<SSLConnectJob> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SSLConnectJob);
 };
 
 class NET_EXPORT_PRIVATE SSLClientSocketPool
     : public ClientSocketPool,
       public HigherLayeredPool,
       public SSLConfigService::Observer {
  public:
@@ skipping 65 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;
 
+  // Gets the SSLConnectJobMessenger for the given ssl session |cache_key|. If
+  // none exits, it creates one and stores it in |messenger_map_|.
+  SSLConnectJobMessenger* GetOrCreateSSLConnectJobMessenger(
+      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.
 
   // 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,
+        const SSLConnectJob::GetMessengerCallback& get_messenger_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::GetMessengerCallback get_messenger_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_;
+  bool enable_ssl_connect_job_waiting_;
 
   DISALLOW_COPY_AND_ASSIGN(SSLClientSocketPool);
 };
 
 }  // namespace net
 
 #endif  // NET_SOCKET_SSL_CLIENT_SOCKET_POOL_H_