Implements new, more robust design for communicating between SSLConnectJobs.

net/socket/socket_test_util.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NET_SOCKET_SOCKET_TEST_UTIL_H_
 #define NET_SOCKET_SOCKET_TEST_UTIL_H_
 
 #include <cstring>
 #include <deque>
 #include <string>
@@ skipping 317 matching lines @@
   std::string next_proto;
   std::string server_protos;
   bool was_npn_negotiated;
   NextProto protocol_negotiated;
   bool client_cert_sent;
   SSLCertRequestInfo* cert_request_info;
   scoped_refptr<X509Certificate> cert;
   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.
 // the response) will complete.
 class DelayedSocketData : public StaticSocketDataProvider {
  public:
   // |write_delay| the number of MockWrites to complete before allowing
   //               a MockRead to complete.
   // |reads| the list of MockRead completions.
   // |writes| the list of MockWrite completions.
@@ skipping 279 matching lines @@
 // socket types.
 class MockClientSocketFactory : public ClientSocketFactory {
  public:
   MockClientSocketFactory();
   virtual ~MockClientSocketFactory();
 
   void AddSocketDataProvider(SocketDataProvider* socket);
   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_;
   }
 
   // ClientSocketFactory
   virtual scoped_ptr<DatagramClientSocket> CreateDatagramClientSocket(
       DatagramSocket::BindType bind_type,
       const RandIntCallback& rand_int_cb,
       NetLog* net_log,
       const NetLog::Source& source) OVERRIDE;
   virtual scoped_ptr<StreamSocket> CreateTransportClientSocket(
       const AddressList& addresses,
       NetLog* net_log,
       const NetLog::Source& source) OVERRIDE;
   virtual scoped_ptr<SSLClientSocket> CreateSSLClientSocket(
       scoped_ptr<ClientSocketHandle> transport_socket,
       const HostPortPair& host_and_port,
       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 {
  public:
   // Value returned by GetTLSUniqueChannelBinding().
   static const char kTlsUnique[];
 
   // The BoundNetLog is needed to test LoadTimingInfo, which uses NetLog IDs as
   // unique socket IDs.
   explicit MockClientSocket(const BoundNetLog& net_log);
@@ skipping 13 matching lines @@
   virtual void Disconnect() OVERRIDE;
   virtual bool IsConnected() const OVERRIDE;
   virtual bool IsConnectedAndIdle() const OVERRIDE;
   virtual int GetPeerAddress(IPEndPoint* address) const OVERRIDE;
   virtual int GetLocalAddress(IPEndPoint* address) const OVERRIDE;
   virtual const BoundNetLog& NetLog() const OVERRIDE;
   virtual void SetSubresourceSpeculation() OVERRIDE {}
   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,
                                    bool has_context,
                                    const base::StringPiece& context,
                                    unsigned char* out,
                                    unsigned int outlen) OVERRIDE;
   virtual int GetTLSUniqueChannelBinding(std::string* out) OVERRIDE;
   virtual NextProtoStatus GetNextProto(std::string* proto,
                                        std::string* server_protos) OVERRIDE;
@@ skipping 239 matching lines @@
   virtual int Connect(const CompletionCallback& callback) OVERRIDE;
   virtual void Disconnect() OVERRIDE;
   virtual bool IsConnected() const OVERRIDE;
   virtual bool WasEverUsed() const OVERRIDE;
   virtual bool UsingTCPFastOpen() const OVERRIDE;
   virtual int GetPeerAddress(IPEndPoint* address) const OVERRIDE;
   virtual bool WasNpnNegotiated() const OVERRIDE;
   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,
                                        std::string* server_protos) OVERRIDE;
   virtual bool set_was_npn_negotiated(bool negotiated) OVERRIDE;
   virtual void set_protocol_negotiated(NextProto protocol_negotiated) OVERRIDE;
   virtual NextProto GetNegotiatedProtocol() const OVERRIDE;
 
   // This MockSocket does not implement the manual async IO feature.
   virtual void OnReadComplete(const MockRead& data) OVERRIDE;
   virtual void OnConnectComplete(const MockConnect& data) OVERRIDE;
 
   virtual bool WasChannelIDSent() const OVERRIDE;
   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);
 };
 
 class MockUDPClientSocket : public DatagramClientSocket, public AsyncSocket {
  public:
   MockUDPClientSocket(SocketDataProvider* data, net::NetLog* net_log);
   virtual ~MockUDPClientSocket();
 
   // Socket implementation.
@@ skipping 295 matching lines @@
 
 extern const char kSOCKS5OkRequest[];
 extern const int kSOCKS5OkRequestLength;
 
 extern const char kSOCKS5OkResponse[];
 extern const int kSOCKS5OkResponseLength;
 
 }  // namespace net
 
 #endif  // NET_SOCKET_SOCKET_TEST_UTIL_H_