Mark QUIC broken when the network blackholes after the handshake

net/quic/chromium/quic_stream_factory.cc

Index: net/quic/chromium/quic_stream_factory.cc
diff --git a/net/quic/chromium/quic_stream_factory.cc b/net/quic/chromium/quic_stream_factory.cc
index f7e7fcd9cd46d0b8e4a815f7611b8061c67014a3..63338bc2734aba76af44dae9bafddc94dcd72414 100644
--- a/net/quic/chromium/quic_stream_factory.cc
+++ b/net/quic/chromium/quic_stream_factory.cc
@@ -666,7 +666,10 @@ int QuicStreamFactory::Job::DoConnectComplete(int rv) {
 QuicStreamRequest::QuicStreamRequest(
     QuicStreamFactory* factory,
     HttpServerProperties* http_server_properties)
-    : factory_(factory), http_server_properties_(http_server_properties) {}
+    : factory_(factory),
+      http_server_properties_(http_server_properties),
+      mark_quic_broken_when_network_suspected_(
+          factory->mark_quic_broken_when_network_suspected()) {}
 
 QuicStreamRequest::~QuicStreamRequest() {
   if (factory_ && !callback_.is_null())
@@ -716,7 +719,9 @@ base::TimeDelta QuicStreamRequest::GetTimeDelayForWaitingJob() const {
 std::unique_ptr<QuicHttpStream> QuicStreamRequest::CreateStream() {
   if (!session_)
     return nullptr;
-  return base::MakeUnique<QuicHttpStream>(session_, http_server_properties_);
+  return base::MakeUnique<QuicHttpStream>(
+      session_, http_server_properties_,
+      mark_quic_broken_when_network_suspected_);
 }
 
 std::unique_ptr<BidirectionalStreamImpl>
@@ -757,6 +762,7 @@ QuicStreamFactory::QuicStreamFactory(
     int max_server_configs_stored_in_properties,
     bool close_sessions_on_ip_change,
     bool disable_quic_on_timeout_with_open_streams,
+    bool mark_quic_broken_when_network_suspected,
     int idle_connection_timeout_seconds,
     int reduced_ping_timeout_seconds,
     int packet_reader_yield_after_duration_milliseconds,
@@ -803,6 +809,8 @@ QuicStreamFactory::QuicStreamFactory(
       prefer_aes_(prefer_aes),
       disable_quic_on_timeout_with_open_streams_(
           disable_quic_on_timeout_with_open_streams),
+      mark_quic_broken_when_network_suspected_(
+          mark_quic_broken_when_network_suspected),
       consecutive_disabled_count_(0),
       need_to_evaluate_consecutive_disabled_count_(false),
       socket_receive_buffer_size_(socket_receive_buffer_size),
@@ -1191,8 +1199,9 @@ void QuicStreamFactory::OnCertVerifyJobComplete(CertVerifierJob* job, int rv) {
 
 std::unique_ptr<QuicHttpStream> QuicStreamFactory::CreateFromSession(
     QuicChromiumClientSession* session) {
-  return std::unique_ptr<QuicHttpStream>(
-      new QuicHttpStream(session->GetWeakPtr(), http_server_properties_));
+  return base::MakeUnique<QuicHttpStream>(
+      session->GetWeakPtr(), http_server_properties_,
+      mark_quic_broken_when_network_suspected_);

[Jana, 2017/04/06 02:29:18]

Is there an easy way to test this piece? That the param is passed correctly from
construction of QuicStreamFactory to construction of QuicHttpStream?

[Ryan Hamilton, 2017/04/06 03:43:39]

This happens in the QuicNetworkTransaction test where we have 2 different tests
in which we have post-handshake RTOs and post-handshake timeouts. In on version
we set mark_broken to true and in the other to false. We verify that the
HttpStream behaves as expected. So I think that does the needful. Otherwise we
could use peers to look at the privates of QuicHttpStream, but that doesn't seem
terribly awesome.

 }
 
 bool QuicStreamFactory::IsQuicDisabled() const {
@@ -1951,6 +1960,10 @@ void QuicStreamFactory::ProcessGoingAwaySession(
                                                server_id.host_port_pair());
   url::SchemeHostPort server("https", server_id.host_port_pair().host(),
                              server_id.host_port_pair().port());
+  // Do nothing if QUIC is currently marked as broken.
+  if (http_server_properties_->IsAlternativeServiceBroken(alternative_service))
+    return;

[Jana, 2017/04/06 02:29:18]

Did we not need this earlier because we wouldn't enter going away state when
QUIC was marked as broken (since that would only happen when the handshake
failed)?

[Ryan Hamilton, 2017/04/06 03:43:39]

It needs to happen because otherwise we'd un-mark QUIC as broken on line 1968.
:) The new QuicNetworkTransaction tests discovered this. Yay, tests.

[Jana, 2017/04/06 23:55:25]

Ah! Of course... Makes sense.

+
   if (session->IsCryptoHandshakeConfirmed()) {
     http_server_properties_->ConfirmAlternativeService(alternative_service);
     ServerNetworkStats network_stats;