Implements new, more robust design for communicating between SSLConnectJobs.

net/socket/ssl_client_socket_unittest.cc

 // 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.
 
 #include "net/socket/ssl_client_socket.h"
 
 #include "base/callback_helpers.h"
 #include "base/memory/ref_counted.h"
 #include "base/run_loop.h"
 #include "net/base/address_list.h"
@@ skipping 574 matching lines @@
       OVERRIDE {}
   virtual int GetCertCount() OVERRIDE { return 0; }
   virtual void SetForceKeepSessionState() OVERRIDE {}
 };
 
 class SSLClientSocketTest : public PlatformTest {
  public:
   SSLClientSocketTest()
       : socket_factory_(ClientSocketFactory::GetDefaultFactory()),
         cert_verifier_(new MockCertVerifier),
-        transport_security_state_(new TransportSecurityState) {
+        transport_security_state_(new TransportSecurityState),
+        ran_completion_callback_(false) {
     cert_verifier_->set_default_result(OK);
     context_.cert_verifier = cert_verifier_.get();
     context_.transport_security_state = transport_security_state_.get();
   }
 
+  void RecordCompletionCallbackRun() { ran_completion_callback_ = true; }

[wtc, 2014/07/30 21:56:57]

In ssl_client_socket_openssl_unittest.cc, this method is named
"RecordCompletedHandshake". Please use the same name. (The name should contain
"Handshake" to avoid confusion with the "TestCompletionCallback callback_"
member.)

+
  protected:
   // Sets up a TCP connection to a HTTPS server. To actually do the SSL
   // handshake, follow up with call to CreateAndConnectSSLClientSocket() below.
   bool ConnectToTestServer(SpawnedTestServer::SSLOptions& ssl_options) {
     test_server_.reset(new SpawnedTestServer(
         SpawnedTestServer::TYPE_HTTPS, ssl_options, base::FilePath()));
     if (!test_server_->Start()) {
       LOG(ERROR) << "Could not start SpawnedTestServer";
       return false;
     }
@@ skipping 42 matching lines @@
     *result = callback_.GetResult(sock_->Connect(callback_.callback()));
     return true;
   }
 
   ClientSocketFactory* socket_factory_;
   scoped_ptr<MockCertVerifier> cert_verifier_;
   scoped_ptr<TransportSecurityState> transport_security_state_;
   SSLClientSocketContext context_;
   scoped_ptr<SSLClientSocket> sock_;
   CapturingNetLog log_;
+  bool ran_completion_callback_;

[wtc, 2014/07/30 21:56:58]

Same here: this member should be renamed "ran_handshake_completion_callback_" to
avoid confusion with the "TestCompletionCallback callback_" member on line 671.

 
  private:
   scoped_ptr<StreamSocket> transport_;
   scoped_ptr<SpawnedTestServer> test_server_;
   TestCompletionCallback callback_;
   AddressList addr_;
 };
 
 // Verifies the correctness of GetSSLCertRequestInfo.
 class SSLClientSocketCertRequestInfoTest : public SSLClientSocketTest {
@@ skipping 262 matching lines @@
   CapturingNetLog log;
   scoped_ptr<StreamSocket> transport(
       new TCPClientSocket(addr, &log, NetLog::Source()));
   int rv = transport->Connect(callback.callback());
   if (rv == ERR_IO_PENDING)
     rv = callback.WaitForResult();
   EXPECT_EQ(OK, rv);
 
   scoped_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
       transport.Pass(), test_server.host_port_pair(), kDefaultSSLConfig));
 

[wtc, 2014/07/30 21:56:58]

Why don't you call sock->SetHandshakeCompletionCallback here?

-  EXPECT_FALSE(sock->IsConnected());

[wtc, 2014/07/30 21:56:57]

Why did you delete this EXPECT_FALSE?

-
   rv = sock->Connect(callback.callback());
 
   CapturingNetLog::CapturedEntryList entries;
   log.GetEntries(&entries);
   EXPECT_TRUE(LogContainsBeginEvent(entries, 5, NetLog::TYPE_SSL_CONNECT));
   if (rv == ERR_IO_PENDING)
     rv = callback.WaitForResult();
 
   EXPECT_EQ(ERR_CERT_COMMON_NAME_INVALID, rv);
 
@@ skipping 202 matching lines @@
   // Disable TLS False Start to avoid handshake non-determinism.
   SSLConfig ssl_config;
   ssl_config.false_start_enabled = false;
 
   SynchronousErrorStreamSocket* raw_transport = transport.get();
   scoped_ptr<SSLClientSocket> sock(
       CreateSSLClientSocket(transport.PassAs<StreamSocket>(),
                             test_server.host_port_pair(),
                             ssl_config));
 
+#if defined(USE_OPENSSL)
+  sock->SetHandshakeCompletionCallback(
+      base::Bind(&SSLClientSocketTest::RecordCompletionCallbackRun,
+                 base::Unretained(this)));
+#endif
+
   rv = callback.GetResult(sock->Connect(callback.callback()));
   EXPECT_EQ(OK, rv);
   EXPECT_TRUE(sock->IsConnected());
 
   const char request_text[] = "GET / HTTP/1.0\r\n\r\n";
   static const int kRequestTextSize =
       static_cast<int>(arraysize(request_text) - 1);
   scoped_refptr<IOBuffer> request_buffer(new IOBuffer(kRequestTextSize));
   memcpy(request_buffer->data(), request_text, kRequestTextSize);
 
@@ skipping 10 matching lines @@
   // rv != ERR_IO_PENDING is insufficient, as ERR_IO_PENDING is a legitimate
   // result when using a dedicated task runner for NSS.
   rv = callback.GetResult(sock->Read(buf.get(), 4096, callback.callback()));
 
 #if !defined(USE_OPENSSL)
   // SSLClientSocketNSS records the error exactly
   EXPECT_EQ(ERR_CONNECTION_RESET, rv);
 #else
   // SSLClientSocketOpenSSL treats any errors as a simple EOF.
   EXPECT_EQ(0, rv);
+  EXPECT_TRUE(ran_completion_callback_);
 #endif
 }
 
 // Tests that the SSLClientSocket properly handles when the underlying transport
 // asynchronously returns an error code while writing data - such as if an
 // intermediary terminates the socket connection uncleanly.
 // This is a regression test for http://crbug.com/249848
 TEST_F(SSLClientSocketTest, Write_WithSynchronousError) {
   SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
                                 SpawnedTestServer::kLocalhost,
@@ skipping 19 matching lines @@
 
   // Disable TLS False Start to avoid handshake non-determinism.
   SSLConfig ssl_config;
   ssl_config.false_start_enabled = false;
 
   scoped_ptr<SSLClientSocket> sock(
       CreateSSLClientSocket(transport.PassAs<StreamSocket>(),
                             test_server.host_port_pair(),
                             ssl_config));
 
+#if defined(USE_OPENSSL)
+  sock->SetHandshakeCompletionCallback(
+      base::Bind(&SSLClientSocketTest::RecordCompletionCallbackRun,
+                 base::Unretained(this)));
+#endif
+
   rv = callback.GetResult(sock->Connect(callback.callback()));
   EXPECT_EQ(OK, rv);
   EXPECT_TRUE(sock->IsConnected());
 
   const char request_text[] = "GET / HTTP/1.0\r\n\r\n";
   static const int kRequestTextSize =
       static_cast<int>(arraysize(request_text) - 1);
   scoped_refptr<IOBuffer> request_buffer(new IOBuffer(kRequestTextSize));
   memcpy(request_buffer->data(), request_text, kRequestTextSize);
 
@@ skipping 22 matching lines @@
   // checking that rv != ERR_IO_PENDING is insufficient, as ERR_IO_PENDING
   // is a legitimate result when using a dedicated task runner for NSS.
   rv = callback.GetResult(rv);
 
 #if !defined(USE_OPENSSL)
   // SSLClientSocketNSS records the error exactly
   EXPECT_EQ(ERR_CONNECTION_RESET, rv);
 #else
   // SSLClientSocketOpenSSL treats any errors as a simple EOF.
   EXPECT_EQ(0, rv);
+  EXPECT_TRUE(ran_completion_callback_);
 #endif
 }
 
 // Test the full duplex mode, with Read and Write pending at the same time.
 // This test also serves as a regression test for http://crbug.com/29815.
 TEST_F(SSLClientSocketTest, Read_FullDuplex) {
   SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
                                 SpawnedTestServer::kLocalhost,
                                 base::FilePath());
   ASSERT_TRUE(test_server.Start());
@@ skipping 204 matching lines @@
 
   // Disable TLS False Start to avoid handshake non-determinism.
   SSLConfig ssl_config;
   ssl_config.false_start_enabled = false;
 
   scoped_ptr<SSLClientSocket> sock(
       CreateSSLClientSocket(transport.PassAs<StreamSocket>(),
                             test_server.host_port_pair(),
                             ssl_config));
 
+#if defined(USE_OPENSSL)
+  sock->SetHandshakeCompletionCallback(
+      base::Bind(&SSLClientSocketTest::RecordCompletionCallbackRun,
+                 base::Unretained(this)));
+#endif
+
   rv = callback.GetResult(sock->Connect(callback.callback()));
   EXPECT_EQ(OK, rv);
   EXPECT_TRUE(sock->IsConnected());
 
   // Send a request so there is something to read from the socket.
   const char request_text[] = "GET / HTTP/1.0\r\n\r\n";
   static const int kRequestTextSize =
       static_cast<int>(arraysize(request_text) - 1);
   scoped_refptr<IOBuffer> request_buffer(new IOBuffer(kRequestTextSize));
   memcpy(request_buffer->data(), request_text, kRequestTextSize);
@@ skipping 38 matching lines @@
       ASSERT_LT(0, long_request_buffer->BytesRemaining());
     }
   } while (rv > 0);
 
 #if !defined(USE_OPENSSL)
   // NSS records the error exactly.
   EXPECT_EQ(ERR_CONNECTION_RESET, rv);
 #else
   // OpenSSL treats the reset as a generic protocol error.
   EXPECT_EQ(ERR_SSL_PROTOCOL_ERROR, rv);
+  EXPECT_TRUE(ran_completion_callback_);
 #endif
 
   // Release the read. Some bytes should go through.
   raw_transport->UnblockReadResult();
   rv = read_callback.WaitForResult();
 
   // Per the fix for http://crbug.com/249848, write failures currently break
   // reads. Change this assertion if they're changed to not collide.
   EXPECT_EQ(ERR_CONNECTION_RESET, rv);
 }
@@ skipping 254 matching lines @@
   scoped_ptr<StreamSocket> transport(
       new MockTCPClientSocket(addr, NULL, &data));
   int rv = transport->Connect(callback.callback());
   if (rv == ERR_IO_PENDING)
     rv = callback.WaitForResult();
   EXPECT_EQ(OK, rv);
 
   scoped_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
       transport.Pass(), test_server.host_port_pair(), kDefaultSSLConfig));
 
+#if defined(USE_OPENSSL)
+  sock->SetHandshakeCompletionCallback(
+      base::Bind(&SSLClientSocketTest::RecordCompletionCallbackRun,
+                 base::Unretained(this)));
+#endif
+
   rv = sock->Connect(callback.callback());
   if (rv == ERR_IO_PENDING)
     rv = callback.WaitForResult();
   EXPECT_EQ(ERR_SSL_PROTOCOL_ERROR, rv);
+#if defined(USE_OPENSSL)
+  EXPECT_TRUE(ran_completion_callback_);
+#endif
 }
 
 TEST_F(SSLClientSocketTest, CipherSuiteDisables) {
   // Rather than exhaustively disabling every RC4 ciphersuite defined at
   // http://www.iana.org/assignments/tls-parameters/tls-parameters.xml,
   // only disabling those cipher suites that the test server actually
   // implements.
   const uint16 kCiphersToDisable[] = {0x0005,  // TLS_RSA_WITH_RC4_128_SHA
   };
 
@@ skipping 668 matching lines @@
 
   // TODO(haavardm@opera.com): Due to differences in threading, Linux returns
   // ERR_UNEXPECTED while Mac and Windows return ERR_PROTOCOL_ERROR. Accept all
   // error codes for now.
   // http://crbug.com/373670
   EXPECT_NE(OK, rv);
   EXPECT_FALSE(sock_->IsConnected());
 }
 
 }  // namespace net