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..a68f092915f8ab007a3760ebc5319c5527965f9b 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,101 @@ class NET_EXPORT_PRIVATE SSLSocketParams
   DISALLOW_COPY_AND_ASSIGN(SSLSocketParams);
 };
 
+// SSLConnectJobMessenger handles communication between concurrent
+// SSLConnectJobs

[Ryan Sleevi, 2014/06/26 01:47:16]

Surely there's more to say here ;)

[mshelley, 2014/07/01 02:35:23]

Done.

+class NET_EXPORT SSLConnectJobMessenger {
+ public:
+  // SSLPendingSocketsAndCallbacks manages the callback and socket
+  // associated with a pending ssl connection.
+  class SSLPendingSocketsAndCallbacks {

[Ryan Sleevi, 2014/06/26 01:47:16]

This class should be private (and forward declared), since it doesn't seem to be
part of any of the interface under test.

[mshelley, 2014/07/01 02:35:23]

Done.

+   public:
+    SSLPendingSocketsAndCallbacks() {
+      pending_sockets_ = ScopedVector<scoped_ptr<SSLClientSocket> >();
+      callbacks_ = std::vector<base::Closure>();

[Ryan Sleevi, 2014/06/26 01:47:16]

This isn't needed; callbacks_ default ctor will run fine.

[mshelley, 2014/07/01 02:35:22]

Done.

+    }
+
+    SSLPendingSocketsAndCallbacks(SSLPendingSocketsAndCallbacks& ref) {

[Ryan Sleevi, 2014/06/26 01:47:16]

This is a pretty dangerous constructor. Why was it necessary?

Further pedantry:
Normally, if you're going to allow copying, you implement the copy-ctor (which
is const-ref), the assignment operator (operator=, which takes in a const-ref).
If you want to allow swapping, you actually implement a method called swap
(except there are a whole bunch of special, hidden rules in C++ you actually
need to follow)

[mshelley, 2014/07/01 02:35:22]

My intent was to implement the copy constructor -- I forgot the const part
though clearly! I need to be able to copy the object so that I can make a copy,
clear the object, and then run the callbacks stored in the copy. Are the
special, hidden rules that you're referring to the "swap and copy idiom"? I just
implemented the assignment operator according to that, but wasn't positive if
that was what you meant.

On 2014/06/26 01:47:16, Ryan Sleevi wrote:

[Ryan Sleevi, 2014/07/08 22:34:01]

The style guide actively discourages this (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C...
)

That said, if you are going to do it, yes, there are special rules. Scott
Meyers' Effective C++ (which we have in the break room, but I'm happy to bring a
copy if it's grown legs).

Further, if you're going to declare a copy ctor, you need to play by the Rule of
Three ( http://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming) )

In general, we discourage this. This is covered a bit more in the discussion of
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...
, which mentions CopyFrom() as an alternative to operator=.

Note that by introducing CopyCtor/operator=, new opportunities for performance
bugs and impicit type conversions get introduced, hence the concern.

+      pending_sockets_.swap(ref.pending_sockets_);
+      callbacks_.swap(ref.callbacks_);
+    }
+
+    SSLClientSocket* push_back(scoped_ptr<SSLClientSocket> socket,
+                               const base::Closure& cb) {

[Ryan Sleevi, 2014/06/26 01:47:16]

naming: AddSocket?

[mshelley, 2014/07/01 02:35:22]

Done.

+      pending_sockets_.push_back(&socket);
+      callbacks_.push_back(cb);
+      return socket.release();
+    }
+
+    void clear() {

[Ryan Sleevi, 2014/06/26 01:47:16]

style: clear() -> Clear()

[mshelley, 2014/07/01 02:35:22]

Done.

+      pending_sockets_.clear();
+      callbacks_.clear();
+    }
+
+    // Tells all pending connections to resume.
+    void RunAllJobs() {
+      for (std::vector<base::Closure>::iterator it = callbacks_.begin();
+           it != callbacks_.end();
+           it++)
+        it->Run();

[Ryan Sleevi, 2014/06/26 01:47:16]

style: braces on multi-lines.

[mshelley, 2014/07/01 02:35:22]

Isn't the body of this loop only one line though?
On 2014/06/26 01:47:16, Ryan Sleevi wrote:

+    }
+
+    scoped_ptr<SSLClientSocket> GetFirstSocket() {
+      return pending_sockets_[0]->Pass();

[Ryan Sleevi, 2014/06/26 01:47:16]

safety: DCHECK(!pending_sockets_.empty()) ?

[mshelley, 2014/07/01 02:35:22]

Done.

+    }
+
+    base::Closure GetFirstCallback() { return callbacks_[0]; }

[Ryan Sleevi, 2014/06/26 01:47:16]

ditto safety

[mshelley, 2014/07/01 02:35:23]

Done.

+
+    void EraseFirstEntry() {

[Ryan Sleevi, 2014/06/26 01:47:16]

DANGER: if .begin() == .end(), this will explode.

[mshelley, 2014/07/01 02:35:23]

Done.

+      pending_sockets_.erase(pending_sockets_.begin());
+      callbacks_.erase(callbacks_.begin());
+    }
+
+   private:
+    ScopedVector<scoped_ptr<SSLClientSocket> > pending_sockets_;
+    std::vector<base::Closure> callbacks_;
+  };
+
+  // Sets rv to true if the given |ssl_socket| should continue its
+  // SSL connection.

[Ryan Sleevi, 2014/06/26 01:47:16]

STYLE: We don't pass output values as-ref; always as pointers.

That is, input values as const-ref (or, if intrinsic, by-val), input/output and
outputs as pointers (typically).

This also doesn't document what the returned Socket is, or who owns it.

[mshelley, 2014/07/01 02:35:23]

Done.

+  SSLClientSocket* CanProceed(scoped_ptr<SSLClientSocket> ssl_socket, int& rv);
+
+  // Informs the session cache that it should notify the SSLConnectJobMessenger
+  // upon the completion of |ssl_socket|'s connection.
+  SSLClientSocket* NotifyOfCompletion(scoped_ptr<SSLClientSocket> ssl_socket,
+                                      const base::Closure& cb);

[Ryan Sleevi, 2014/06/26 01:47:16]

Ditto here. You're transferring ownership, but what is it you're returning?

[mshelley, 2014/07/01 02:35:23]

Done.

+
+  // 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 pending_sockets_and_callbacks_;
+  ScopedVector<scoped_ptr<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 pending_jobs_list

[wtc, 2014/06/27 00:36:49]

This comment needs to be updated because there is no |own pending_jobs_list| in
this CL.

[mshelley, 2014/07/01 02:35:23]

Done.

+  // 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 +199,13 @@ class SSLConnectJob : public ConnectJob {
 
   virtual void GetAdditionalErrorState(ClientSocketHandle * handle) OVERRIDE;
 
+  base::WeakPtr<SSLConnectJob> GetWeakPtr();
+
+  // Enable SSLConnectJob waiting if |enable| is true.
+  static NET_EXPORT void EnableJobWaiting(bool enable);
+
+  static bool GetEnableJobWaiting();
+
  private:
   enum State {
     STATE_TRANSPORT_CONNECT,
@@ -126,6 +214,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 +232,16 @@ 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();
+
+  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,12 +261,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 +384,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*> PendingJobMap;
+
     TransportClientSocketPool* const transport_pool_;
     SOCKSClientSocketPool* const socks_pool_;
     HttpProxyClientSocketPool* const http_proxy_pool_;
@@ -292,6 +395,7 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool
     const SSLClientSocketContext context_;
     base::TimeDelta timeout_;
     NetLog* net_log_;
+    scoped_ptr<PendingJobMap> pending_jobs_map_;
 
     DISALLOW_COPY_AND_ASSIGN(SSLConnectJobFactory);
   };