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..0dad0f47f2d4bb2b66f146ae87a106d5d6be18a4 100644
--- a/net/socket/socket_test_util.h
+++ b/net/socket/socket_test_util.h
@@ -335,6 +335,11 @@ struct SSLSocketDataProvider {
   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.
@@ -638,6 +643,10 @@ class MockClientSocketFactory : public ClientSocketFactory {
     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,
@@ -654,10 +663,10 @@ class MockClientSocketFactory : public ClientSocketFactory {
       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 {
@@ -691,6 +700,9 @@ class MockClientSocket : public SSLClientSocket {
   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,
@@ -703,6 +715,11 @@ class MockClientSocket : public SSLClientSocket {
                                        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);
@@ -712,6 +729,9 @@ class MockClientSocket : public SSLClientSocket {
   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_;
 
@@ -950,6 +970,9 @@ class MockSSLClientSocket : public MockClientSocket, public AsyncSocket {
   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,
@@ -978,6 +1001,9 @@ class MockSSLClientSocket : public MockClientSocket, public AsyncSocket {
   bool is_protocol_negotiated_set_;
   NextProto protocol_negotiated_;
 
+  base::Closure success_callback_;
+  base::Closure error_callback_;
+
   DISALLOW_COPY_AND_ASSIGN(MockSSLClientSocket);
 };