SSL Connect Job Waiting

net/socket/ssl_client_socket_pool.h

Index: net/socket/ssl_client_socket_pool.h
diff --git a/net/socket/ssl_client_socket_pool.h b/net/socket/ssl_client_socket_pool.h
index e03b76ade6ab5946e2d981e7d7802b1cb70d6dba..96b65d7e08ec9498e3ab301cea7b87ddc71323c3 100644
--- a/net/socket/ssl_client_socket_pool.h
+++ b/net/socket/ssl_client_socket_pool.h
@@ -5,7 +5,9 @@
 #ifndef NET_SOCKET_SSL_CLIENT_SOCKET_POOL_H_
 #define NET_SOCKET_SSL_CLIENT_SOCKET_POOL_H_
 
+#include <map>
 #include <string>
+#include <vector>
 
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
@@ -17,6 +19,7 @@
 #include "net/socket/client_socket_pool_base.h"
 #include "net/socket/client_socket_pool_histograms.h"
 #include "net/socket/ssl_client_socket.h"
+#include "net/socket/ssl_client_socket_openssl.h"
 #include "net/ssl/ssl_config_service.h"
 
 namespace net {
@@ -98,19 +101,21 @@ class NET_EXPORT_PRIVATE SSLSocketParams
 // connection as specified in the params.
 class SSLConnectJob : public ConnectJob {
  public:
-  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,
-      Delegate* delegate,
-      NetLog* net_log);
+  typedef std::vector<SSLConnectJob*> PendingJobList;

[wtc, 2014/06/17 17:44:41]

Nit: it would be nice to document this type. For example, are the jobs in the
list supposed to be connecting to the same server? Are the jobs in the list
suspended? Does the owner of this list need to destroy the jobs?

[mshelley1, 2014/06/18 18:53:50]

Done.

+
+  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,
+                Delegate* delegate,
+                NetLog* net_log,
+                PendingJobList* pending);

[wtc, 2014/06/17 17:44:41]

1. Nit: I suggest naming this parameter "pending_jobs" or "pending_job_list".
"pending" sounds like the name of a boolean parameter.

2. I suggest moving this parameter before the "delegate" parameter. By
convention, the delegate/callback and net_log parameters are usually listed last
in the net code.

3. Please document whether this object owns the pending job list. If not,
document that the pending job list must outlive this object.

[mshelley1, 2014/06/18 18:53:50]

Done.

   virtual ~SSLConnectJob();
 
   // ConnectJob methods.
@@ -118,6 +123,8 @@ class SSLConnectJob : public ConnectJob {
 
   virtual void GetAdditionalErrorState(ClientSocketHandle * handle) OVERRIDE;
 
+  static NET_EXPORT void EnableJobWaiting(bool enable);

[wtc, 2014/06/17 17:44:41]

Nit: please document this method.

[mshelley1, 2014/06/18 18:53:50]

Done.

+
  private:
   enum State {
     STATE_TRANSPORT_CONNECT,
@@ -126,6 +133,8 @@ class SSLConnectJob : public ConnectJob {
     STATE_SOCKS_CONNECT_COMPLETE,
     STATE_TUNNEL_CONNECT,
     STATE_TUNNEL_CONNECT_COMPLETE,
+    STATE_CREATE_SSL_SOCKET,
+    STATE_CHECK_FOR_RESUME,
     STATE_SSL_CONNECT,
     STATE_SSL_CONNECT_COMPLETE,
     STATE_NONE,
@@ -142,9 +151,21 @@ class SSLConnectJob : public ConnectJob {
   int DoSOCKSConnectComplete(int result);
   int DoTunnelConnect();
   int DoTunnelConnectComplete(int result);
+  int DoCreateSSLSocket();
+  int DoCheckForResume();
   int DoSSLConnect();
   int DoSSLConnectComplete(int result);
 
+  // Decides how pending jobs should continue their connection
+  // based upon the success or failure of the leading job.
+  void ProcessPendingJobs(int result);
+  void ResumeSSLConnection();

[wtc, 2014/06/17 17:44:41]

Please document this method.

[mshelley1, 2014/06/18 18:53:51]

Done.

+  State GetNextConnectState(bool enabled);

[wtc, 2014/06/17 17:44:41]

Delete this unused method declaration.

[mshelley1, 2014/06/18 18:53:51]

Done.

+
+  std::string GetSessionCacheKey();

[wtc, 2014/06/17 17:44:41]

Delete this unused method.

[mshelley1, 2014/06/18 18:53:50]

Done.

+
+  static bool enable_job_waiting_;
+
   // Returns the initial state for the state machine based on the
   // |connection_type|.
   static State GetInitialState(SSLSocketParams::ConnectionType connection_type);
@@ -164,10 +185,11 @@ class SSLConnectJob : public ConnectJob {
   const SSLClientSocketContext context_;
 
   State next_state_;
-  CompletionCallback callback_;
+  CompletionCallback io_callback_;
   scoped_ptr<ClientSocketHandle> transport_socket_handle_;
   scoped_ptr<SSLClientSocket> ssl_socket_;
 
+  PendingJobList* pending_jobs_;
   HttpResponseInfo error_response_info_;
 
   DISALLOW_COPY_AND_ASSIGN(SSLConnectJob);
@@ -284,6 +306,8 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
     virtual base::TimeDelta ConnectionTimeout() const OVERRIDE;
 
    private:
+    typedef std::map<std::string, SSLConnectJob::PendingJobList*> PendingJobMap;

[wtc, 2014/06/17 17:44:41]

Please document this map. In particular, what is the std::string key.

[mshelley1, 2014/06/18 18:53:51]

Done.

+
     TransportClientSocketPool* const transport_pool_;
     SOCKSClientSocketPool* const socks_pool_;
     HttpProxyClientSocketPool* const http_proxy_pool_;
@@ -292,6 +316,7 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
     const SSLClientSocketContext context_;
     base::TimeDelta timeout_;
     NetLog* net_log_;
+    PendingJobMap* pending_jobs_;

[wtc, 2014/06/17 17:44:41]

Please document who owns pending_jobs_. If SSLClientSocketPool owns it, it
probably should be declared as

    PendingJobMap pending_jobs_;

or

    scoped_ptr<PendingJobMap> pending_jobs_;

Nit: the member name probably should have "map" in it.

[mshelley1, 2014/06/18 18:53:51]

Done.

 
     DISALLOW_COPY_AND_ASSIGN(SSLConnectJobFactory);
   };