Implements new, more robust design for communicating between SSLConnectJobs.

net/socket/socket_test_util.h

Index: net/socket/socket_test_util.h
diff --git a/net/socket/socket_test_util.h b/net/socket/socket_test_util.h
index 2918aad2dc55ec4f3b8ff5a63c0e8b584aa1af5c..a9868d85436e3284d3698aafd56e3076781b2096 100644
--- a/net/socket/socket_test_util.h
+++ b/net/socket/socket_test_util.h
@@ -29,6 +29,7 @@
 #include "net/socket/client_socket_handle.h"
 #include "net/socket/socks_client_socket_pool.h"
 #include "net/socket/ssl_client_socket.h"
+#include "net/socket/ssl_client_socket_openssl.h"

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

IMPORTANT: this header should not need to include
"net/socket/ssl_client_socket_openssl.h".

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

Done.

 #include "net/socket/ssl_client_socket_pool.h"
 #include "net/socket/transport_client_socket_pool.h"
 #include "net/ssl/ssl_config_service.h"
@@ -335,6 +336,8 @@ struct SSLSocketDataProvider {
   bool channel_id_sent;
   ServerBoundCertService* server_bound_cert_service;
   int connection_status;
+  bool is_in_session_cache_;
+  bool is_leader_;

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

is_leader_ should be documented because its meaning is not obvious.

I can guess what is_in_session_cache_ means. Since none of the other members are
documented, it is fine to not document is_in_session_cache_.

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

Done.

 };
 
 // A DataProvider where the client must write a request before the reads (e.g.
@@ -634,6 +637,12 @@ class MockClientSocketFactory : public ClientSocketFactory {
   void AddSSLSocketDataProvider(SSLSocketDataProvider* socket);
   void ResetNextMockIndexes();
 
+  // Tell the client socket factory that the leading job has connected.

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

Nit: Tell => Tells

[mshelley, 2014/07/09 19:50:59]

Done.

+  static void LeaderConnected();

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

Name this method "SetLeaderConnected".

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

Done.

+
+  // Returns |leader_connected_|.
+  static bool IsLeaderConnected();

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

IMPORTANT: it seems that the leader connect job is a notion of the client socket
pool rather than the client socket factory. So I don't know why we need to add
these two methods to MockClientSocketFactory.

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

I agree that it would make more sense to have these methods in the client socket
pool. However, in this case I'm not using a mock client socket pool, so I
couldn't add these methods there. The client socket factory seemed like the next
most logical place to keep track of this information. 

+
   SocketDataProviderArray<SocketDataProvider>& mock_data() {
     return mock_data_;
   }
@@ -654,10 +663,13 @@ class MockClientSocketFactory : public ClientSocketFactory {
       const SSLConfig& ssl_config,
       const SSLClientSocketContext& context) OVERRIDE;
   virtual void ClearSSLSessionCache() OVERRIDE;
+  virtual std::vector<MockSSLClientSocket*> GetSSLClientSockets();
 
  private:
   SocketDataProviderArray<SocketDataProvider> mock_data_;
   SocketDataProviderArray<SSLSocketDataProvider> mock_ssl_data_;
+  static NET_EXPORT bool leader_connected_;

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

Use NET_EXPORT_PRIVATE instead. But, is this really necessary?

[mshelley, 2014/07/09 19:50:59]

Done.

+  std::vector<MockSSLClientSocket*> ssl_client_sockets_;
 };
 
 class MockClientSocket : public SSLClientSocket {
@@ -691,6 +703,11 @@ class MockClientSocket : public SSLClientSocket {
   virtual void SetOmniboxSpeculation() OVERRIDE {}
 
   // SSLClientSocket implementation.
+  virtual bool InSessionCache() const OVERRIDE;
+  virtual void WatchSessionForCompletion(
+      const base::Closure& cb) const OVERRIDE;
+  virtual void SetSocketFailureCallback(const base::Closure& cb) OVERRIDE;
+  virtual void OnSocketFailure() OVERRIDE;
   virtual void GetSSLCertRequestInfo(SSLCertRequestInfo* cert_request_info)
       OVERRIDE;
   virtual int ExportKeyingMaterial(const base::StringPiece& label,
@@ -950,6 +967,11 @@ class MockSSLClientSocket : public MockClientSocket, public AsyncSocket {
   virtual bool GetSSLInfo(SSLInfo* ssl_info) OVERRIDE;
 
   // SSLClientSocket implementation.
+  virtual bool InSessionCache() const OVERRIDE;
+  virtual void WatchSessionForCompletion(
+      const base::Closure& cb) const OVERRIDE;
+  virtual void SetSocketFailureCallback(const base::Closure& cb) OVERRIDE;
+  virtual void OnSocketFailure() OVERRIDE;
   virtual void GetSSLCertRequestInfo(SSLCertRequestInfo* cert_request_info)
       OVERRIDE;
   virtual NextProtoStatus GetNextProto(std::string* proto,
@@ -966,17 +988,22 @@ class MockSSLClientSocket : public MockClientSocket, public AsyncSocket {
   virtual void set_channel_id_sent(bool channel_id_sent) OVERRIDE;
   virtual ServerBoundCertService* GetServerBoundCertService() const OVERRIDE;
 
+  bool IsGoodOrdering() const;

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

Document this method.

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

Done.

+
  private:
   static void ConnectCallback(MockSSLClientSocket* ssl_client_socket,
                               const CompletionCallback& callback,
                               int rv);
 
+  mutable base::Closure process_pending_jobs_callback_;

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

Why does this member need to be marked mutable? Can this be avoided?

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

This member was marked as mutable because I need to set its value inside of
WatchSessionForCompletion, which is a const method. Normally, the callback
passed into this method would be passed into the cache, and stored there to be
called upon the connection's completion. However, because this test doesn't
actually utilize the cache, I needed to set the member here. 

Instead of making this mutable I'll just change the base class so that the
method is not const.  

+
   scoped_ptr<ClientSocketHandle> transport_;
   SSLSocketDataProvider* data_;
   bool is_npn_state_set_;
   bool new_npn_value_;
   bool is_protocol_negotiated_set_;
   NextProto protocol_negotiated_;
+  bool good_ordering_;
 
   DISALLOW_COPY_AND_ASSIGN(MockSSLClientSocket);
 };