QUIC: mark QUIC handshake failed if connection is closed after CryptoConnect

net/quic/chromium/quic_stream_factory_test.cc

Index: net/quic/chromium/quic_stream_factory_test.cc
diff --git a/net/quic/chromium/quic_stream_factory_test.cc b/net/quic/chromium/quic_stream_factory_test.cc
index d6fd5aeb42ec0c74be6e856b963bf382beebf52f..bfaa832c5e51b2dbb8a33d62b764ec8628491232 100644
--- a/net/quic/chromium/quic_stream_factory_test.cc
+++ b/net/quic/chromium/quic_stream_factory_test.cc
@@ -316,6 +316,12 @@ class QuicStreamFactoryTestBase {
     return QuicStreamFactoryPeer::HasActiveSession(factory_.get(), server_id);
   }
 
+  bool HasActiveJob(const HostPortPair& host_port_pair,
+                    const PrivacyMode privacy_mode) {
+    QuicServerId server_id(host_port_pair, privacy_mode);
+    return QuicStreamFactoryPeer::HasActiveJob(factory_.get(), server_id);
+  }
+
   bool HasActiveCertVerifierJob(const QuicServerId& server_id) {
     return QuicStreamFactoryPeer::HasActiveCertVerifierJob(factory_.get(),
                                                            server_id);
@@ -1803,6 +1809,29 @@ TEST_P(QuicStreamFactoryTest, CloseAllSessions) {
   EXPECT_TRUE(socket_data2.AllWriteDataConsumed());
 }
 
+// Regression test for crbug.com/700617. Test a write error during the
+// crypto handshake will not hang QuicStreamFactory::Job and should
+// report QUIC_HANDSHAKE_FAILED to upper layers.
+TEST_P(QuicStreamFactoryTest, WriteErrorInCryptoConnect) {
+  Initialize();
+  crypto_client_stream_factory_.set_use_normal_crypto_stream();
+
+  MockQuicData socket_data;
+  socket_data.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
+  socket_data.AddWrite(SYNCHRONOUS, ERR_ADDRESS_UNREACHABLE);
+  socket_data.AddSocketDataToFactory(&socket_factory_);
+
+  // Create request and QuicHttpStream.
+  QuicStreamRequest request(factory_.get());
+  EXPECT_EQ(ERR_IO_PENDING,

[Ryan Hamilton, 2017/03/22 02:38:48]

I'm a bit surprised that this returns ERR_IO_PENDING instead of
QUIC_HANDSHAKE_FAILED. Is that because DNS resolution (in this test) in async?
If so, can you make another version of this test which does
host_resolver_.set_synchronous_mode(true); (look for other uses in this file)
and verify that it fails immediately?

[Zhongyi Shi, 2017/03/22 07:00:11]

Done.

+            request.Request(host_port_pair_, privacy_mode_,
+                            /*cert_verify_flags=*/0, url_, "GET", net_log_,
+                            callback_.callback()));
+  EXPECT_EQ(ERR_QUIC_HANDSHAKE_FAILED, callback_.WaitForResult());
+  EXPECT_FALSE(HasActiveSession(host_port_pair_));
+  EXPECT_FALSE(HasActiveJob(host_port_pair_, privacy_mode_));

[Ryan Hamilton, 2017/03/22 02:38:48]

If I understand the bug correctly, in the old code, if we had tried to make a
new request at this point, the request would have hung. So instead of checking
with the peer that there is no active job can you make another request, and this
time let it succeed?

[Zhongyi Shi, 2017/03/22 07:00:11]

Done. Good point! 

I have checked those regression test will fail w/o the fix.

[Ryan Hamilton, 2017/03/22 14:13:01]

It looks like this is still testing for an active not, instead of testing that a
new QuicStreamRequest will not hang.
Great!

[Zhongyi Shi, 2017/03/22 18:53:09]

Done.

+}
+
 TEST_P(QuicStreamFactoryTest, OnIPAddressChanged) {
   close_sessions_on_ip_change_ = true;
   Initialize();