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..aa5ef1c762a88c32504b4aa503583b79d03a5dfd 100644
--- a/net/socket/socket_test_util.h
+++ b/net/socket/socket_test_util.h
@@ -335,6 +335,10 @@ struct SSLSocketDataProvider {
   bool channel_id_sent;
   ServerBoundCertService* server_bound_cert_service;
   int connection_status;
+  bool is_in_session_cache_;

[Ryan Sleevi, 2014/07/10 19:51:05]

Probably needs a comment here as well, since connection_status has a bitflag for
indicating whether or not resumption happened.

[mshelley, 2014/07/10 22:07:14]

I just realized that this shouldn't even be here -- I originally intended to use
a field like this, but replaced it with the is_in_session_cache_ member of the
SSLSocketDataProvider because it was easier for me to set it from my unit test. 

+
+  // Denotes that this job may have pending jobs dependant upon its connection.
+  bool is_leader_;

[Ryan Sleevi, 2014/07/10 19:51:05]

So, I'm a little confused by this - is this bookkeeping for the
MockClientSocketFactory (which is responsible for determining if it's leader),
is this something to expose to tests that is set by the factory, or is this
something that the tests set to affect the factory?

If it's something test sets, it seems like the wrong layer.
If it's bookkeeping for the MCSF / for tests, it seems like the wrong layer, and
we should be 'hiding' that a bit more.

This is not a strong objection at present, but it is something I'm having a
little trouble understanding. 

[mshelley, 2014/07/10 22:07:15]

This is something that the test sets to affect the factory (or rather, to affect
the factory's bookkeeping). When you say that this seems like the wrong layer,
do you mean that this information shouldn't go in the SSLSocketDataProvider? 

[Ryan Sleevi, 2014/07/12 00:12:15]

What I meant is that your tests that use this also use CreatePool - i.e.
https://codereview.chromium.org/353713005/diff/300001/net/socket/ssl_client_s...

It seems like if anyone is going to determine who the leader is, it's the pool -
since that's very much what the pool does. So it's odd for me to see this. As
far as I can tell, it doesn't actually cause this job to be the leader (because
the SSLClientSocketPool is never going to check this)

The places where you check |is_leader_| seems like you could have handled  this
by having MockClientSocket::SetIsLeader() set some MockSSLClientSocket data to
measure that it's the leader.

That is, put differently, it seems like you're violating your test boundary,
because you're not relying on SetIsLeader to actually configure you as the
leader, but you're 'cheating' and using the test. This results in not testing
the expected behaviour.

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

[Ryan Sleevi, 2014/07/10 19:51:05]

Static variables are very bad, especially in tests.

If you find you needed to make it static because there is not a path from the
thing-that-needs-to-set-this to the MCSF, then we should fix it (either
establishing a path, for finding a better way to thread this information
through)

[mshelley, 2014/07/10 22:07:15]

Yeah I used the static variable because I wanted to have a way for all of the
sockets to know if the leading socket had connected, so that I could confirm
that the leading socket connected before all of the other sockets started
connecting. 

I put the static bool/function in MockClientSocketFactory because it was the
only mocked object that somewhat "connected" all of the sockets. Would it be
terrible style to just add a client_socket_factory_ field to MockClientSocket,
and then give each MCS a pointer to the MCSF so that it could access
leader_connected_ that way?

[Ryan Sleevi, 2014/07/12 00:12:15]

The SocketDataProvider has a path to the ClientSocket.
The factory has a path to the SocketDataProvider.

Having the ClientSocket have a path to the Factory thus is a circular
dependency, so yeah, it'd be somewhat terrible.

I feel like the checks for good_ordering_ and is_leader_ belong to your actual
unittests, and shouldn't be part of the MockSSLClientSocket. That is, what is
good/bad ordering is not an aspect of the MSSLCS (which doesn't set the policy
for leadership), but of your test / messenger.

+
   SocketDataProviderArray<SocketDataProvider>& mock_data() {
     return mock_data_;
   }
@@ -654,10 +664,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 bool leader_connected_;
+  std::vector<MockSSLClientSocket*> ssl_client_sockets_;
 };
 
 class MockClientSocket : public SSLClientSocket {
@@ -691,6 +704,11 @@ class MockClientSocket : public SSLClientSocket {
   virtual void SetOmniboxSpeculation() OVERRIDE {}
 
   // SSLClientSocket implementation.
+  virtual bool InSessionCache() const OVERRIDE;
+  virtual void WatchSessionForCompletion(const base::Closure& cb) OVERRIDE;
+  virtual void SetSocketFailureCallback(const base::Closure& cb) OVERRIDE;
+  virtual void OnSocketFailure() OVERRIDE;
+  virtual void SetIsLeader() OVERRIDE;
   virtual void GetSSLCertRequestInfo(SSLCertRequestInfo* cert_request_info)
       OVERRIDE;
   virtual int ExportKeyingMaterial(const base::StringPiece& label,
@@ -950,6 +968,10 @@ 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) 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,24 @@ class MockSSLClientSocket : public MockClientSocket, public AsyncSocket {
   virtual void set_channel_id_sent(bool channel_id_sent) OVERRIDE;
   virtual ServerBoundCertService* GetServerBoundCertService() const OVERRIDE;
 
+  // Returns a bool indicating whether or not the socket connected at
+  // the correct time relative to other sockets with the same host/port pair.

[Ryan Sleevi, 2014/07/10 19:51:05]

Just from reading this, "the correct time" is unclear here, and so I'm not sure
what it's supposed to reply, so I had to dig into the implementation.

[mshelley, 2014/07/10 22:07:15]

Done.

+  bool IsGoodOrdering() const;
+
  private:
   static void ConnectCallback(MockSSLClientSocket* ssl_client_socket,
                               const CompletionCallback& callback,
                               int rv);
 
+  base::Closure process_pending_jobs_callback_;
+
   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);
 };