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()> ConnectionCompleteCallback;

[Ryan Sleevi, 2014/08/14 01:20:20]

We have a typedef for this already - base::Closure

[mshelley, 2014/08/14 20:01:36]

Done.

 
-  SSLConnectJobMessenger();
+  // |connection_complete_callback| is used to inform the client socket pool
+  // that a connection monitored by the SSLConnectJobMessenger has completed.
+  SSLConnectJobMessenger(
+      const ConnectionCompleteCallback& connection_complete_callback);

[wtc, 2014/08/14 00:46:46]

DESIGN: instead of an abstract callback function, I would pass an
SSLClientSocketPool* pointer and the cache key, and have the
SSLConnectJobMessenger call socket_pool->DeleteSSLConnectJobMessenger(cache_key)
directly.

[Ryan Sleevi, 2014/08/14 01:20:20]

I recommended against this, to avoid introducing additional circular
dependencies for what is a header-visible class.

This may be unnecessary complexity for now, but the concern was about messengers
with different connection strategies, and wanting to decouple them as much as
possible from the appropriate bits.

Since ConnectJob depends on Messenger, and Pool depends on ConnectJob, I wanted
to avoid a circular dependency where Messenger depends on Pool (thus making Pool
depend on ConnectJob and ConnectJob depend, indirectly, on Pool)

[mshelley, 2014/08/14 20:01:37]

So is the benefit of passing in the pointer directly just that it's more
straightforward? I've left it as it is because I'm not sure if that concern
outweighs Ryan's concern about circular dependencies or not.

   ~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_;
+
+  ConnectionCompleteCallback connection_complete_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.

[wtc, 2014/08/14 00:46:46]

Please reformat this comment block.

[mshelley, 2014/08/14 20:01:37]

Done.

+  //
+  // 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&)>
+      GetMessengerForUncachedSessionCallback;

[wtc, 2014/08/14 00:46:46]

Nit: I would shorten this type to "GetMessengerCallback".

[mshelley, 2014/08/14 20:01:37]

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,
-                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,
+      const GetMessengerForUncachedSessionCallback& uncached_session_callback,

[wtc, 2014/08/14 00:46:46]

Between the "get messenger" and the "uncached session", I think "get messenger"
is more important. So I suggest naming this parameter "get_messenger_callback".
Similarly, I think the uncached_session_callback_ member should be renamed
"get_messenger_callback_".

[wtc, 2014/08/14 00:46:46]

DESIGN: I would just pass a SSLClientSocketPool* pointer, and have the
SSLConnectJob call
socket_pool->AddSSLConnectJobMessenger(ssl_socket_->GetSessionCacheKey())
directly.

[Ryan Sleevi, 2014/08/14 01:20:20]

Same comment re: coupling.

To date, there's nothing that requires the SSLConnectJob depend on a Pool, and I
was hoping to avoid that. For example, one can supply a callback that returns
NULL to not implement this feature, or other (non-SocketPools) can use
messengers to coordinate session establishment outside of the use of a pool.

[mshelley, 2014/08/14 20:01:37]

Done.

+      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 47 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_;
 
+  SSLConnectJob::GetMessengerForUncachedSessionCallback

[wtc, 2014/08/14 00:46:46]

You should be able to omit "SSLConnectJob::".

[mshelley, 2014/08/14 20:01:37]

Done.

+      uncached_session_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;
 
+  // Creates an SSLConnectJobMessenger for the given ssl session |cache_key|

[wtc, 2014/08/14 00:46:45]

This comment is not accurate because the method may simply look up an existing
SSLConnectJobMessenger from |messenger_map_|. So, the
"AddSSLConnectJobMessenger" function name is also not accurate because "Add" is
not always done. I suggest using "Get" or "Lookup" instead. (We've also used
"GetOrCreate" in this situation if you don't mind the verbosity.)

[mshelley, 2014/08/14 20:01:37]

Done.

+  // and stores it in |messenger_map_|. Returns the new SSLConnectJobMessenger.
+  SSLConnectJobMessenger* AddSSLConnectJobMessenger(
+      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,
+                         SSLConnectJob::GetMessengerForUncachedSessionCallback
+                             uncached_session_callback,

[wtc, 2014/08/14 00:46:46]

Rename this "get_messenger_callback".

[mshelley, 2014/08/14 20:01:36]

Done.

                          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_;

[wtc, 2014/08/14 00:46:45]

Rename this "get_messenger_callback".

[mshelley, 2014/08/14 20:01:36]

Done.

     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_