Mark QUIC broken when the network blackholes after the handshake

net/quic/chromium/quic_stream_factory.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_stream_factory.h"
 
 #include <algorithm>
 #include <tuple>
 #include <utility>
 
@@ skipping 648 matching lines @@
   }
 
   factory_->ActivateSession(key_, session_);
 
   return OK;
 }
 
 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())
     factory_->CancelRequest(this);
 }
 
 int QuicStreamRequest::Request(const HostPortPair& destination,
                                PrivacyMode privacy_mode,
                                int cert_verify_flags,
                                const GURL& url,
@@ skipping 29 matching lines @@
 
 base::TimeDelta QuicStreamRequest::GetTimeDelayForWaitingJob() const {
   if (!factory_)
     return base::TimeDelta();
   return factory_->GetTimeDelayForWaitingJob(server_id_);
 }
 
 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>
 QuicStreamRequest::CreateBidirectionalStreamImpl() {
   if (!session_)
     return nullptr;
   return base::MakeUnique<BidirectionalStreamQuicImpl>(session_);
 }
 
 QuicStreamFactory::QuicStreamFactory(
@@ skipping 20 matching lines @@
     float load_server_info_timeout_srtt_multiplier,
     bool enable_connection_racing,
     bool enable_non_blocking_io,
     bool disable_disk_cache,
     bool prefer_aes,
     int socket_receive_buffer_size,
     bool delay_tcp_race,
     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,
     bool migrate_sessions_on_network_change,
     bool migrate_sessions_early,
     bool allow_server_migration,
     bool force_hol_blocking,
     bool race_cert_verification,
     bool do_not_fragment,
     bool estimate_initial_rtt,
@@ skipping 26 matching lines @@
           always_require_handshake_confirmation),
       disable_connection_pooling_(disable_connection_pooling),
       load_server_info_timeout_srtt_multiplier_(
           load_server_info_timeout_srtt_multiplier),
       enable_connection_racing_(enable_connection_racing),
       enable_non_blocking_io_(enable_non_blocking_io),
       disable_disk_cache_(disable_disk_cache),
       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),
       delay_tcp_race_(delay_tcp_race),
       ping_timeout_(QuicTime::Delta::FromSeconds(kPingTimeoutSecs)),
       reduced_ping_timeout_(
           QuicTime::Delta::FromSeconds(reduced_ping_timeout_seconds)),
       yield_after_packets_(kQuicYieldAfterPacketsRead),
       yield_after_duration_(QuicTime::Delta::FromMilliseconds(
           packet_reader_yield_after_duration_milliseconds)),
@@ skipping 368 matching lines @@
   active_jobs_.erase(server_id);
   job_requests_map_.erase(server_id);
 }
 
 void QuicStreamFactory::OnCertVerifyJobComplete(CertVerifierJob* job, int rv) {
   active_cert_verifier_jobs_.erase(job->server_id());
 }
 
 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 {
   return status_ != OPEN;
 }
 
 bool QuicStreamFactory::OnHandshakeConfirmed(
     QuicChromiumClientSession* session) {
   if (!IsQuicDisabled())
     return false;
@@ skipping 738 matching lines @@
     const QuicServerId& server_id,
     bool session_was_active) {
   if (!http_server_properties_)
     return;
 
   const QuicConnectionStats& stats = session->connection()->GetStats();
   const AlternativeService alternative_service(kProtoQUIC,
                                                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;
     network_stats.srtt = base::TimeDelta::FromMicroseconds(stats.srtt_us);
     network_stats.bandwidth_estimate = stats.estimated_bandwidth;
     http_server_properties_->SetServerNetworkStats(server, network_stats);
     return;
   }
 
   http_server_properties_->ClearServerNetworkStats(server);
@@ skipping 23 matching lines @@
 void QuicStreamFactory::OpenFactory() {
   status_ = OPEN;
 }
 
 void QuicStreamFactory::MaybeClearConsecutiveDisabledCount() {
   if (status_ == OPEN)
     consecutive_disabled_count_ = 0;
 }
 
 }  // namespace net