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

net/quic/chromium/quic_chromium_client_session.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/quic/chromium/quic_chromium_client_session.h"
 
 #include <utility>
 
 #include "base/callback_helpers.h"
 #include "base/location.h"
@@ skipping 652 matching lines @@
   return OK;
 }
 
 int QuicChromiumClientSession::CryptoConnect(
     const CompletionCallback& callback) {
   connect_timing_.connect_start = base::TimeTicks::Now();
   RecordHandshakeState(STATE_STARTED);
   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;
   }
 
   // Unless we require handshake confirmation, activate the session if
   // we have established initial encryption.
   if (!require_confirmation_ && IsEncryptionEstablished())
     return OK;
 
@@ skipping 837 matching lines @@
 }
 
 size_t QuicChromiumClientSession::EstimateMemoryUsage() const {
   // TODO(xunjieli): Estimate |crypto_stream_|, QuicSpdySession's
   // QuicHeaderList, QuicSession's QuiCWriteBlockedList, open streams and
   // unacked packet map.
   return base::trace_event::EstimateMemoryUsage(packet_readers_);
 }
 
 }  // namespace net