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

net/quic/chromium/quic_chromium_client_session.cc

Index: net/quic/chromium/quic_chromium_client_session.cc
diff --git a/net/quic/chromium/quic_chromium_client_session.cc b/net/quic/chromium/quic_chromium_client_session.cc
index d783551a80145c58032a9ce31a09b2446bc0f9d6..d1d218550204bf5bbe4cdf340724960d2ff7ac43 100644
--- a/net/quic/chromium/quic_chromium_client_session.cc
+++ b/net/quic/chromium/quic_chromium_client_session.cc
@@ -670,6 +670,13 @@ int QuicChromiumClientSession::CryptoConnect(
   DCHECK(flow_controller());
   crypto_stream_->CryptoConnect();
 
+  // Check if the connection is still open, issues during CryptoConnect like
+  // packet write error could cause the connection to be torn down. Return
+  // synchronously with ERR_QUIC_HANDSHAKE_FAILED so that QuicStreamFactory::Job

[xunjieli, 2017/03/23 17:43:37]

mentioning QuicStreamFacory::Job here is a layering violation.

I think what happens here is crypto_stream_->CryptoConnect()'s method signature
doesn't indicate that it can return error synchronously. The calling code
(QuicChromiumClientSession) failed to handle the sync error. 

If you agree, I think this API (QuicCryptoClientStream::CryptoConnect()) needs
to be changed to make the API contract clearer.

[Zhongyi Shi, 2017/03/23 19:20:32]

QuicCryptoClientStream::CryptoConnect() is shared with internal code, which
doesn't support completion callback IIUC. It might be a good idea to change the
API to return a status code for the CryptoConnect here, which will introduce
changes in both internal and external code base. But let's keep it simple here
and have the leak fixed first. Helen, could we move the discussion to
crbug.com/704649?

+  // could fail without hanging.
+  if (!connection()->connected())
+    return ERR_QUIC_HANDSHAKE_FAILED;
+
   if (IsCryptoHandshakeConfirmed()) {
     connect_timing_.connect_end = base::TimeTicks::Now();
     return OK;