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;
+  // Indicates that the socket should block in the Connect method.
+  bool should_block_in_connect;
+  // Indicates that the socket previously blocked in the Connect method.
+  bool blocked_in_connect;

[Ryan Sleevi, 2014/07/18 22:01:16]

So, as mentioned previously, the SSLSocketDataProvider should only hold data
that affects the behaviour of the socket (as seen by tests), and never internal
state for the mock sockets.

For example, what does blocked_in_connect==true mean if
should_block_in_connect==false ?

I think should_block is fine (in that it affects external visibility), but
blocked_in seems like state that should be on the socket.

[mshelley, 2014/07/21 23:00:08]

Done.

+  bool is_in_session_cache;
 };
 
 // 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 283 matching lines @@
   virtual ~MockClientSocketFactory();
 
   void AddSocketDataProvider(SocketDataProvider* socket);
   void AddSSLSocketDataProvider(SSLSocketDataProvider* socket);
   void ResetNextMockIndexes();
 
   SocketDataProviderArray<SocketDataProvider>& mock_data() {
     return mock_data_;
   }
 
+  virtual std::vector<MockSSLClientSocket*> ssl_client_sockets() const {

[Ryan Sleevi, 2014/07/18 22:01:16]

So, thoughts:
1) If it's inline, it shouldn't be virtual.
2) It should probably be const-ref, rather than a copy.
3) It needs documentation to the effect that this can be unsafe - because each
of the returned SSLSockets is owned by the caller of the factory, and they can
delete at will!

[mshelley, 2014/07/21 23:00:08]

Done.

+    return ssl_client_sockets_;
+  }
+
   // 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;
-
  private:
   SocketDataProviderArray<SocketDataProvider> mock_data_;
   SocketDataProviderArray<SSLSocketDataProvider> mock_ssl_data_;
+  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 SetHandshakeSuccessCallback(const base::Closure& cb) OVERRIDE;
+  virtual void SetHandshakeFailureCallback(const base::Closure& cb) OVERRIDE;

[Ryan Sleevi, 2014/07/18 22:01:17]

For future cleanup, I suspect it may make more sense to make this a
net::CompletionCallback, as discussed in the past (with either OK or an error)

[mshelley, 2014/07/21 23:00:08]

Yeah that makes sense -- it's acceptable to make that change in a followup cl?

   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;
   virtual ServerBoundCertService* GetServerBoundCertService() const OVERRIDE;
 
+  // Returns a callback that resumes the connection of a socket that was
+  // paused for testing. |resumption_callback_| should be set before invoking
+  // this method.

[Ryan Sleevi, 2014/07/18 22:01:16]

This comments seems out of date? You no longer have a |resumption_callback_|
member (I'm guessing it's |connect_callback_|)

[mshelley, 2014/07/21 23:00:07]

Done.

+  CompletionCallback connect_callback();
+
  protected:
   virtual ~MockClientSocket();
   void RunCallbackAsync(const CompletionCallback& callback, int result);
   void RunCallback(const CompletionCallback& callback, int result);
 
   // SSLClientSocket implementation.
   virtual scoped_refptr<X509Certificate> GetUnverifiedServerCertificateChain()
       const OVERRIDE;
 
+  // Callback to be used to resume the connection of a paused socket.
+  CompletionCallback connect_callback_;

[Ryan Sleevi, 2014/07/18 22:01:16]

So it's a little weird to have this member that has to be set by derived
classes. Will have to dig in more to see about suggesting an alternative.

+
   // True if Connect completed successfully and Disconnect hasn't been called.
   bool connected_;
 
   // Address of the "remote" peer we're connected to.
   IPEndPoint peer_addr_;
 
   BoundNetLog net_log_;
 
   base::WeakPtrFactory<MockClientSocket> weak_factory_;
 
@@ skipping 218 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 SetHandshakeSuccessCallback(const base::Closure& cb) OVERRIDE;
+  virtual void SetHandshakeFailureCallback(const base::Closure& cb) 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;
 
  private:
   static void ConnectCallback(MockSSLClientSocket* ssl_client_socket,
                               const CompletionCallback& callback,
                               int rv);
 
   scoped_ptr<ClientSocketHandle> transport_;
   SSLSocketDataProvider* data_;
   bool is_npn_state_set_;
   bool new_npn_value_;
   bool is_protocol_negotiated_set_;
   NextProto protocol_negotiated_;
 
+  base::Closure success_callback_;
+  base::Closure error_callback_;
+
   DISALLOW_COPY_AND_ASSIGN(MockSSLClientSocket);
 };
 
 class MockUDPClientSocket : public DatagramClientSocket, public AsyncSocket {
  public:
   MockUDPClientSocket(SocketDataProvider* data, net::NetLog* net_log);
   virtual ~MockUDPClientSocket();
 
   // Socket implementation.
   virtual int Read(IOBuffer* buf,
@@ skipping 294 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_