This CL adds unit tests for false start connections.

net/socket/ssl_client_socket_unittest.cc

Index: net/socket/ssl_client_socket_unittest.cc
diff --git a/net/socket/ssl_client_socket_unittest.cc b/net/socket/ssl_client_socket_unittest.cc
index 28083fcf8c0dcbbcd180315597ed4c0dc3f8948e..b87b0b7b6a267e48fe02600385ed21de1f77f655 100644
--- a/net/socket/ssl_client_socket_unittest.cc
+++ b/net/socket/ssl_client_socket_unittest.cc
@@ -358,6 +358,9 @@ class FakeBlockingStreamSocket : public WrappedStreamSocket {
   // Waits for the blocked Write() call to be scheduled.
   void WaitForWrite();
 
+  // Returns the wrapped stream socket.
+  StreamSocket* transport() { return transport_.get(); }
+
  private:
   // Handles completion from the underlying transport read.
   void OnReadCompleted(int result);
@@ -796,6 +799,10 @@ class SSLClientSocketCertRequestInfoTest : public SSLClientSocketTest {
 };
 
 class SSLClientSocketFalseStartTest : public SSLClientSocketTest {
+ public:
+  SSLClientSocketFalseStartTest()
+      : monitor_handshake_callback_(false), expect_false_start_error_(false) {}
+
  protected:
   // Creates an SSLClientSocket with |client_config| attached to a
   // FakeBlockingStreamSocket, returning both in |*out_raw_transport| and
@@ -814,11 +821,10 @@ class SSLClientSocketFalseStartTest : public SSLClientSocketTest {
       const SSLConfig& client_config,
       TestCompletionCallback* callback,
       FakeBlockingStreamSocket** out_raw_transport,
+      scoped_ptr<StreamSocket> real_transport,

[davidben, 2014/08/07 21:24:19]

Nit: Switch the order of out_raw_transport and real_transport. (So that the
input parameters all come before the output ones.)

       scoped_ptr<SSLClientSocket>* out_sock) {
     CHECK(test_server());
 
-    scoped_ptr<StreamSocket> real_transport(
-        new TCPClientSocket(addr(), NULL, NetLog::Source()));
     scoped_ptr<FakeBlockingStreamSocket> transport(
         new FakeBlockingStreamSocket(real_transport.Pass()));
     int rv = callback->GetResult(transport->Connect(callback->callback()));
@@ -830,6 +836,12 @@ class SSLClientSocketFalseStartTest : public SSLClientSocketTest {
                               test_server()->host_port_pair(),
                               client_config);
 
+    if (monitor_handshake_callback_) {
+      sock->SetHandshakeCompletionCallback(
+          base::Bind(&SSLClientSocketTest::RecordCompletedHandshake,
+                     base::Unretained(this)));
+    }
+
     // Connect. Stop before the client processes the first server leg
     // (ServerHello, etc.)
     raw_transport->BlockReadResult();
@@ -845,6 +857,12 @@ class SSLClientSocketFalseStartTest : public SSLClientSocketTest {
     raw_transport->UnblockReadResult();
     raw_transport->WaitForWrite();
 
+    if (expect_false_start_error_) {

[davidben, 2014/08/07 21:24:19]

So this is a little odd in that, if |expect_false_start_error_| is true, it
assumes that |real_transport| is a SynchronousErrorStreamSocket rather than a
generic StreamSocket. I was actually thinking you'd call SetNextReadError
outside of CreateAndConnectUntilServerFinishedReceived (Aaah that function name.
I should have picked something shorter. :-) ).

But now that I look at UnblockWrite, that probably wouldn't work. Sorry, my
fault. I did not realize that, by calling UnblockWrite, we'd likely immediately
run the OpenSSL state machine (by virtue of running the callback on line 499),
causing SSLClientSocket to call Read no the transport earlier.

Option 1: I suspect if you change that line to

  // Defer running the callback by an event loop iteration so the
  // caller has a chance to affect the next read.
  base::MessageLoop::current()->PostTask(
      FROM_HERE, base::Bind(
          base::ResetAndReturn(&pending_write_callback_), rv));

it wouldn't break other tests and you could move the SetNextReadError call to
TestFalseStart...

Option 2: ...but I'm fine with just having
CreateAndConnectUntilServerFinishedReceived also handle the false start error
case. (I.e. disregard my suggestion of passing real_transport in as a parameter.
I didn't realize UnblockWrite worked that way.) In that case you can rename
expect_false_start_error_ to maybe fail_handshake_after_false_start_, always
wrap real_transport in a SynchronousErrorStreamSocket before wrapping in a
FakeBlockingStreamSocket, and then do all your SetNextReadError logic in there.
Then you wouldn't need to change the other tests.

+      SynchronousErrorStreamSocket* error_socket =
+          static_cast<SynchronousErrorStreamSocket*>(
+              raw_transport->transport());
+      error_socket->SetNextReadError(ERR_CONNECTION_RESET);
+    }
     // And, finally, release that and block the next server leg
     // (ChangeCipherSpec, Finished).
     raw_transport->BlockReadResult();
@@ -862,8 +880,18 @@ class SSLClientSocketFalseStartTest : public SSLClientSocketTest {
     TestCompletionCallback callback;
     FakeBlockingStreamSocket* raw_transport = NULL;
     scoped_ptr<SSLClientSocket> sock;
-    ASSERT_NO_FATAL_FAILURE(CreateAndConnectUntilServerFinishedReceived(
-        client_config, &callback, &raw_transport, &sock));
+    scoped_ptr<StreamSocket> real_transport = scoped_ptr<StreamSocket>(
+        new TCPClientSocket(addr(), NULL, NetLog::Source()));
+    if (expect_false_start_error_)
+      real_transport.reset(
+          new SynchronousErrorStreamSocket(real_transport.Pass()));
+
+    ASSERT_NO_FATAL_FAILURE(
+        CreateAndConnectUntilServerFinishedReceived(client_config,
+                                                    &callback,
+                                                    &raw_transport,
+                                                    real_transport.Pass(),
+                                                    &sock));
 
     if (expect_false_start) {
       // When False Starting, the handshake should complete before receiving the
@@ -896,7 +924,10 @@ class SSLClientSocketFalseStartTest : public SSLClientSocketTest {
       // After releasing reads, the connection proceeds.
       raw_transport->UnblockReadResult();
       rv = callback.GetResult(rv);
-      EXPECT_LT(0, rv);
+      if (expect_false_start_error_)
+        EXPECT_EQ(ERR_CONNECTION_RESET, rv);
+      else
+        EXPECT_LT(0, rv);
     } else {
       // False Start is not enabled, so the handshake will not complete because
       // the server second leg is blocked.
@@ -904,6 +935,13 @@ class SSLClientSocketFalseStartTest : public SSLClientSocketTest {
       EXPECT_FALSE(callback.have_result());
     }
   }
+
+  // Indicates that the socket's handshake completion callback should
+  // be monitored.
+  bool monitor_handshake_callback_;
+  // Indicates that this test's handshake should fail after the client
+  // "finished" message is sent.
+  bool expect_false_start_error_;
 };
 
 class SSLClientSocketChannelIDTest : public SSLClientSocketTest {
@@ -2770,6 +2808,35 @@ TEST_F(SSLClientSocketTest, HandshakeCallbackIsRun_WithNoneSessionCache) {
   EXPECT_TRUE(sock->IsConnected());
   EXPECT_TRUE(ran_handshake_completion_callback_);
 }
+
+TEST_F(SSLClientSocketFalseStartTest,
+       HandshakeCallbackIsRun_WithFalseStartFailure) {
+  // False Start requires NPN and a forward-secret cipher suite.
+  SpawnedTestServer::SSLOptions server_options;
+  server_options.key_exchanges =
+      SpawnedTestServer::SSLOptions::KEY_EXCHANGE_DHE_RSA;
+  server_options.enable_npn = true;
+  SSLConfig client_config;
+  client_config.next_protos.push_back("http/1.1");
+  monitor_handshake_callback_ = true;
+  expect_false_start_error_ = true;
+  ASSERT_NO_FATAL_FAILURE(TestFalseStart(server_options, client_config, true));
+  ASSERT_TRUE(ran_handshake_completion_callback_);
+}
+
+TEST_F(SSLClientSocketFalseStartTest,
+       HandshakeCallbackIsRun_WithFalseStartSuccess) {
+  // False Start requires NPN and a forward-secret cipher suite.
+  SpawnedTestServer::SSLOptions server_options;
+  server_options.key_exchanges =
+      SpawnedTestServer::SSLOptions::KEY_EXCHANGE_DHE_RSA;
+  server_options.enable_npn = true;
+  SSLConfig client_config;
+  client_config.next_protos.push_back("http/1.1");
+  monitor_handshake_callback_ = true;
+  ASSERT_NO_FATAL_FAILURE(TestFalseStart(server_options, client_config, true));
+  ASSERT_TRUE(ran_handshake_completion_callback_);
+}
 #endif  // defined(USE_OPENSSL)
 
 TEST_F(SSLClientSocketFalseStartTest, FalseStartEnabled) {
@@ -2854,8 +2921,14 @@ TEST_F(SSLClientSocketFalseStartTest, NoSessionResumptionBeforeFinish) {
   TestCompletionCallback callback;
   FakeBlockingStreamSocket* raw_transport1;
   scoped_ptr<SSLClientSocket> sock1;
-  ASSERT_NO_FATAL_FAILURE(CreateAndConnectUntilServerFinishedReceived(
-      client_config, &callback, &raw_transport1, &sock1));
+  scoped_ptr<StreamSocket> real_transport(
+      new TCPClientSocket(addr(), NULL, NetLog::Source()));
+  ASSERT_NO_FATAL_FAILURE(
+      CreateAndConnectUntilServerFinishedReceived(client_config,
+                                                  &callback,
+                                                  &raw_transport1,
+                                                  real_transport.Pass(),
+                                                  &sock1));
   // Although raw_transport1 has the server Finished blocked, the handshake
   // still completes.
   EXPECT_EQ(OK, callback.WaitForResult());