Implements new, more robust design for communicating between SSLConnectJobs.

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..12680a843d8c9486caae87afc62862b55926038f 100644
--- a/net/socket/ssl_client_socket_pool.h
+++ b/net/socket/ssl_client_socket_pool.h
@@ -5,10 +5,13 @@
 #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"
+#include "base/memory/scoped_vector.h"
 #include "base/time/time.h"
 #include "net/base/privacy_mode.h"
 #include "net/dns/host_resolver.h"
@@ -94,23 +97,135 @@ class NET_EXPORT_PRIVATE SSLSocketParams
   DISALLOW_COPY_AND_ASSIGN(SSLSocketParams);
 };
 
+// SSLConnectJobMessenger handles communication between concurrent
+// SSLConnectJobs.

[wtc, 2014/07/08 01:25:42]

Please point out these SSLConnectJobs have the same SSL session cache key.

[mshelley, 2014/07/09 19:51:01]

Done.

+//
+// SSLConnectJobMessengers tell the session cache when a certain
+// connection should be monitored for success or failure, and
+// tell SSLConnectJobs when to pause or resume thier connections.
+class NET_EXPORT SSLConnectJobMessenger {

[wtc, 2014/07/08 01:25:43]

Use NET_EXPORT_PRIVATE instead.

[mshelley, 2014/07/09 19:51:01]

Done.

+ public:
+  // Sets rv to true if the given |ssl_socket| should continue its

[wtc, 2014/07/08 01:25:43]

By "Sets rv to true", did you mean "Returns true"? There is no |rv| in this
function.

[mshelley, 2014/07/09 19:51:02]

Done.

+  // SSL connection.
+  bool CanProceed(SSLClientSocket* ssl_socket);
+
+  // Informs the session cache that it should notify the SSLConnectJobMessenger
+  // upon the completion of |ssl_socket|'s connection.
+  void MonitorConnectionResult(SSLClientSocket* ssl_socket);
+
+  // Adds the socket and its associated callback to the SSLConnectJobMessenger's
+  // list of pending sockets and callbacks.
+  void AddPendingSocket(SSLClientSocket* socket, const base::Closure& callback);
+
+  // Processes pending callbacks when a socket successfully completes
+  // its connection.
+  void OnJobSucceeded();
+
+  // Processes pending callbacks when a socket encounters an error
+  // while completing its connection.
+  void OnJobFailed();
+
+ private:
+  // SSLPendingSocketsAndCallbacks manages the callback and socket
+  // associated with a pending ssl connection.
+  class SSLPendingSocketsAndCallbacks {
+   public:
+    SSLPendingSocketsAndCallbacks() {}
+
+    SSLPendingSocketsAndCallbacks(const SSLPendingSocketsAndCallbacks& ref) {
+      std::vector<SSLClientSocket*>::const_iterator socket_it =
+          ref.pending_sockets_.begin();
+      std::vector<base::Closure>::const_iterator callback_it =
+          ref.callbacks_.begin();
+      while (socket_it != ref.pending_sockets_.end() &&
+             callback_it != ref.callbacks_.end()) {

[wtc, 2014/07/08 01:25:43]

BUG: this while loop is wrong. You need two while loops to copy pending_sockets_
and callbacks_, respectively.

But you should be able to just copy the two std::vectors directly:

  pending_sockets_ = ref.pending_sockets_;
  callbacks_ = ref.callbacks_;

By the way, the parameter of the copy constructor is usually named |other|.

[mshelley, 2014/07/09 19:51:01]

Done.

+        pending_sockets_.push_back(*socket_it);
+        callbacks_.push_back(*callback_it);
+      }
+    }
+
+    SSLPendingSocketsAndCallbacks& operator=(
+        const SSLPendingSocketsAndCallbacks& ref) {
+      SSLPendingSocketsAndCallbacks temp(ref);
+      temp.Swap(*this);

[wtc, 2014/07/08 01:25:43]

I don't know why the Swap method is needed. You should be able to just do
member-wise copy.

[mshelley, 2014/07/09 19:51:02]

Done.

+      return *this;
+    }
+
+    void Swap(SSLPendingSocketsAndCallbacks ref) throw() {

[wtc, 2014/07/08 01:25:43]

Do we need "throw()" here? This is the first time I see it used in the Chromium
source tree.

[mshelley, 2014/07/09 19:51:01]

Done.

+      std::swap(pending_sockets_, this->pending_sockets_);
+      std::swap(callbacks_, this->callbacks_);
+    }
+
+    void AddSocket(SSLClientSocket* socket, const base::Closure& cb) {
+      pending_sockets_.push_back(socket);
+      callbacks_.push_back(cb);
+    }
+
+    void Clear() {
+      pending_sockets_.clear();
+      callbacks_.clear();
+    }
+
+    // Tells all pending connections to resume.
+    void RunAllJobs() {
+      for (std::vector<base::Closure>::iterator it = callbacks_.begin();

[wtc, 2014/07/08 01:25:42]

Use const_iterator?

[mshelley, 2014/07/09 19:51:01]

Done.

+           it != callbacks_.end();
+           it++)
+        it->Run();
+    }
+
+    SSLClientSocket* GetFirstSocket() {
+      DCHECK(!pending_sockets_.empty());
+      return pending_sockets_[0];
+    }
+
+    base::Closure GetFirstCallback() {
+      DCHECK(!callbacks_.empty());
+      return callbacks_[0];
+    }
+
+    bool Empty() {
+      if (pending_sockets_.empty() || callbacks_.empty())
+        return true;
+      return false;
+    }
+
+    void EraseFirstEntry() {
+      if (pending_sockets_.begin() == pending_sockets_.end() ||
+          callbacks_.begin() == callbacks_.end())
+        return;
+      pending_sockets_.erase(pending_sockets_.begin());
+      callbacks_.erase(callbacks_.begin());
+    }
+
+   private:
+    std::vector<SSLClientSocket*> pending_sockets_;
+    std::vector<base::Closure> callbacks_;

[wtc, 2014/07/08 01:25:42]

IMPORTANT: it seems that you should instead define a simple struct:

  struct SocketAndCallback {
    SSLClientSocket* socket;
    base::Closure callback;
  };

Then SSLPendingSocketsAndCallbacks is just std::vector<SocketAndCallback>.

[mshelley, 2014/07/09 19:51:01]

Done.

+  };
+
+  SSLPendingSocketsAndCallbacks pending_sockets_and_callbacks_;
+  std::vector<SSLClientSocket*> connecting_sockets_;
+};
+
 // SSLConnectJob handles the SSL handshake after setting up the underlying
 // 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);
+  // 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);
   virtual ~SSLConnectJob();
 
   // ConnectJob methods.
@@ -118,6 +233,8 @@ class SSLConnectJob : public ConnectJob {
 
   virtual void GetAdditionalErrorState(ClientSocketHandle * handle) OVERRIDE;
 
+  base::WeakPtr<SSLConnectJob> GetWeakPtr();
+
  private:
   enum State {
     STATE_TRANSPORT_CONNECT,
@@ -126,6 +243,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 +261,14 @@ class SSLConnectJob : public ConnectJob {
   int DoSOCKSConnectComplete(int result);
   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();
+
   // Returns the initial state for the state machine based on the
   // |connection_type|.
   static State GetInitialState(SSLSocketParams::ConnectionType connection_type);
@@ -164,12 +288,15 @@ 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_;
 
+  SSLConnectJobMessenger* messenger_;
   HttpResponseInfo error_response_info_;
 
+  base::WeakPtrFactory<SSLConnectJob> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(SSLConnectJob);
 };
 
@@ -284,6 +411,9 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
     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_;
@@ -292,6 +422,7 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
     const SSLClientSocketContext context_;
     base::TimeDelta timeout_;
     NetLog* net_log_;
+    scoped_ptr<MessengerMap> messenger_map_;

[mmenke, 2014/07/08 21:02:05]

There doesn't seem to be a need for this to be a scoped_ptr.

[mshelley, 2014/07/09 19:51:01]

The idea was that I didn't want it to be a plain pointer because that would
potentially lead to it being leaked, and I didn't want it to be a non-pointer
because then I wouldn't be able to insert to the map inside of NewConnectJob in
ssl_client_socket_pool.cc (which is a const method). I considered changing
NewConnectJob to not be const, but that would impact several other classes
derived from the same base class as SSLConenctJobFactory so I figured that using
a scoped_ptr was a simpler solution.

 
     DISALLOW_COPY_AND_ASSIGN(SSLConnectJobFactory);
   };