Change the HttpStreamFactory::JobController to mark alternative service broken...

Change the HttpStreamFactory::JobController to mark alternative service broken until default network changes.

Change the HttpStreamFactory::JobController to mark alternative service broken until default network changes iff TCP succeeds, QUIC fails on the defaut network but succeeds on the alternate network. The HttpServerProperties will clear the brokenness until default network changes.

Bug: 790547
Change-Id: Ib33def42b474898879bf9e39567632ed273c5ca2
Reviewed-on: https://chromium-review.googlesource.com/1212142
Commit-Queue: Zhongyi Shi <zhongyi@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593486}

net/http/http_proxy_client_socket_wrapper.cc

@@ -649,6 +649,7 @@ int HttpProxyClientSocketWrapper::DoQuicProxyCreateSession() {
       initial_socket_tag_, ssl_params_->ssl_config().GetCertVerifyFlags(),
       GURL("https://" + proxy_server.ToString()), net_log_,
       &quic_net_error_details_,
+      /*failed_on_default_network_callback=*/CompletionOnceCallback(),
       base::Bind(&HttpProxyClientSocketWrapper::OnIOComplete,
                  base::Unretained(this)));
 }

net/http/http_stream_factory_job.cc

@@ -901,7 +901,10 @@ int HttpStreamFactory::Job::DoInitConnectionImpl() {
     int rv = quic_request_.Request(
         destination, quic_version_, request_info_.privacy_mode, priority_,
         request_info_.socket_tag, ssl_config->GetCertVerifyFlags(), url,
-        net_log_, &net_error_details_, io_callback_);
+        net_log_, &net_error_details_,
+        base::BindOnce(&Job::OnFailedOnDefaultNetwork,
+                       ptr_factory_.GetWeakPtr()),
+        io_callback_);
     if (rv == OK) {
       using_existing_quic_session_ = true;
     } else if (rv == ERR_IO_PENDING) {
@@ -999,6 +1002,12 @@ void HttpStreamFactory::Job::OnQuicHostResolution(int result) {
   delegate_->OnConnectionInitialized(this, result);
 }
 
+void HttpStreamFactory::Job::OnFailedOnDefaultNetwork(int result) {
+  DCHECK_EQ(job_type_, ALTERNATIVE);
+  DCHECK(using_quic_);
+  delegate_->OnFailedOnDefaultNetwork(this);
+}
+
 int HttpStreamFactory::Job::DoInitConnectionComplete(int result) {
   net_log_.EndEvent(NetLogEventType::HTTP_STREAM_JOB_INIT_CONNECTION);
   if (job_type_ == PRECONNECT) {

net/http/http_stream_factory_job.h

@@ -83,6 +83,9 @@ class HttpStreamFactory::Job {
                                 int status,
                                 const SSLConfig& used_ssl_config) = 0;
 
+    // Invoked when |job| fails on the default network.
+    virtual void OnFailedOnDefaultNetwork(Job* job) = 0;
+
     // Invoked when |job| has a certificate error for the HttpStreamRequest.
     virtual void OnCertificateError(Job* job,
                                     int status,
@@ -316,6 +319,9 @@ class HttpStreamFactory::Job {
   // resolution, not the result of host resolution itself.
   void OnQuicHostResolution(int result);
 
+  // Invoked when the underlying connection fails on the default network.
+  void OnFailedOnDefaultNetwork(int result);
+
   // Each of these methods corresponds to a State value.  Those with an input
   // argument receive the result from the previous state.  If a method returns
   // ERR_IO_PENDING, then the result from OnIOComplete will be passed to the

net/http/http_stream_factory_job_controller.cc

@@ -78,7 +78,9 @@ HttpStreamFactory::JobController::JobController(
       is_websocket_(is_websocket),
       enable_ip_based_pooling_(enable_ip_based_pooling),
       enable_alternative_services_(enable_alternative_services),
+      main_job_net_error_(OK),
       alternative_job_net_error_(OK),
+      alternative_job_failed_on_default_network_(false),
       job_bound_(false),
       main_job_is_blocked_(false),
       main_job_is_resumed_(false),
@@ -209,6 +211,7 @@ void HttpStreamFactory::JobController::OnStreamReadyOnPooledConnection(
 
   main_job_.reset();
   alternative_job_.reset();
+  ResetErrorStatusForJobs();
 
   factory_->OnStreamReady(proxy_info, request_info_.privacy_mode);
 
@@ -226,6 +229,7 @@ void HttpStreamFactory::JobController::
 
   main_job_.reset();
   alternative_job_.reset();
+  ResetErrorStatusForJobs();
 
   delegate_->OnBidirectionalStreamImplReady(used_ssl_config, used_proxy_info,
                                             std::move(stream));
@@ -322,6 +326,9 @@ void HttpStreamFactory::JobController::OnStreamFailed(
     } else {
       OnAlternativeServiceJobFailed(status);
     }
+  } else {
+    DCHECK_EQ(main_job_.get(), job);
+    main_job_net_error_ = status;
   }
 
   MaybeResumeMainJob(job, base::TimeDelta());
@@ -365,6 +372,11 @@ void HttpStreamFactory::JobController::OnStreamFailed(
   delegate_->OnStreamFailed(status, *job->net_error_details(), used_ssl_config);
 }
 
+void HttpStreamFactory::JobController::OnFailedOnDefaultNetwork(Job* job) {
+  DCHECK_EQ(job->job_type(), ALTERNATIVE);
+  alternative_job_failed_on_default_network_ = true;
+}
+
 void HttpStreamFactory::JobController::OnCertificateError(
     Job* job,
     int status,
@@ -482,8 +494,6 @@ void HttpStreamFactory::JobController::OnNewSpdySessionReady(
 
   // Notify |request_|.
   if (!is_preconnect_ && !is_job_orphaned) {
-    if (job->job_type() == MAIN && alternative_job_net_error_ != OK)
-      ReportBrokenAlternativeService();
 
     DCHECK(request_);
 
@@ -528,6 +538,7 @@ void HttpStreamFactory::JobController::OnNewSpdySessionReady(
 void HttpStreamFactory::JobController::OnPreconnectsComplete(Job* job) {
   DCHECK_EQ(main_job_.get(), job);
   main_job_.reset();
+  ResetErrorStatusForJobs();
   factory_->OnPreconnectsCompleteInternal();
   MaybeNotifyFactoryOfCompletion();
 }
@@ -579,6 +590,12 @@ void HttpStreamFactory::JobController::ResumeMainJob() {
   main_job_wait_time_ = base::TimeDelta();
 }
 
+void HttpStreamFactory::JobController::ResetErrorStatusForJobs() {
+  main_job_net_error_ = OK;
+  alternative_job_net_error_ = OK;
+  alternative_job_failed_on_default_network_ = false;
+}
+
 void HttpStreamFactory::JobController::MaybeResumeMainJob(
     Job* job,
     const base::TimeDelta& delay) {
@@ -919,11 +936,17 @@ void HttpStreamFactory::JobController::OrphanUnboundJob() {
     return;
   }
 
-  if (bound_job_->job_type() == ALTERNATIVE && main_job_) {
-    // |request_| is bound to the alternative job. This means that the main job
+  if (bound_job_->job_type() == ALTERNATIVE && main_job_ &&
+      !alternative_job_failed_on_default_network_) {
+    // |request_| is bound to the alternative job and the alternative job
+    // succeeds on the default network. This means that the main job
     // is no longer needed, so cancel it now. Pending ConnectJobs will return
     // established sockets to socket pools if applicable.
     // https://crbug.com/757548.
+    // The main job still needs to run if the alternative job succeeds on the
+    // alternate network in order to figure out whether QUIC should be marked as
+    // broken until the default network changes.
+    DCHECK_EQ(OK, alternative_job_net_error_);
     main_job_.reset();
   }
 }
@@ -931,9 +954,6 @@ void HttpStreamFactory::JobController::OrphanUnboundJob() {
 void HttpStreamFactory::JobController::OnJobSucceeded(Job* job) {
   DCHECK(job);
 
-  if (job->job_type() == MAIN && alternative_job_net_error_ != OK)
-    ReportBrokenAlternativeService();
-
   if (!bound_job_) {
     if (main_job_ && alternative_job_)
       ReportAlternateProtocolUsage(job);
@@ -958,14 +978,6 @@ void HttpStreamFactory::JobController::OnAlternativeServiceJobFailed(
   DCHECK_NE(kProtoUnknown, alternative_service_info_.protocol());
 
   alternative_job_net_error_ = net_error;
-
-  if (IsJobOrphaned(alternative_job_.get())) {
-    // If |request_| is gone, then it must have been successfully served by
-    // |main_job_|.
-    // If |request_| is bound to a different job, then it is being
-    // successfully served by the main job.
-    ReportBrokenAlternativeService();
-  }
 }
 
 void HttpStreamFactory::JobController::OnAlternativeProxyJobFailed(
@@ -988,17 +1000,41 @@ void HttpStreamFactory::JobController::OnAlternativeProxyJobFailed(
   }
 }
 
-void HttpStreamFactory::JobController::ReportBrokenAlternativeService() {
+void HttpStreamFactory::JobController::MaybeReportBrokenAlternativeService() {
+  // If alternative job succeeds on the default network, no brokenness to
+  // report.
+  if (alternative_job_net_error_ == OK &&
+      !alternative_job_failed_on_default_network_)
+    return;
+
+  // No brokenness to report if the main job fails.
+  if (main_job_net_error_ != OK)
+    return;
+
   DCHECK(alternative_service_info_.protocol() != kProtoUnknown);
-  DCHECK_NE(OK, alternative_job_net_error_);
 
-  int error_to_report = alternative_job_net_error_;
-  alternative_job_net_error_ = OK;
-  base::UmaHistogramSparse("Net.AlternateServiceFailed", -error_to_report);
+  if (alternative_job_failed_on_default_network_ &&
+      alternative_job_net_error_ == OK) {
+    // Alternative job failed on the default network but succeeds on the
+    // non-default network, mark alternative service broken until the default
+    // network changes.
+    session_->http_server_properties()
+        ->MarkAlternativeServiceBrokenUntilDefaultNetworkChanges(
+            alternative_service_info_.alternative_service());
+    // Reset error status for Jobs after reporting brokenness.
+    ResetErrorStatusForJobs();
+    return;
+  }
+
+  // Report brokenness if alternative job failed.
+  base::UmaHistogramSparse("Net.AlternateServiceFailed",
+                           -alternative_job_net_error_);
 
-  if (error_to_report == ERR_NETWORK_CHANGED ||
-      error_to_report == ERR_INTERNET_DISCONNECTED) {
+  if (alternative_job_net_error_ == ERR_NETWORK_CHANGED ||
+      alternative_job_net_error_ == ERR_INTERNET_DISCONNECTED) {
     // No need to mark alternative service as broken.
+    // Reset error status for Jobs.
+    ResetErrorStatusForJobs();
     return;
   }
 
@@ -1006,9 +1042,17 @@ void HttpStreamFactory::JobController::ReportBrokenAlternativeService() {
       BROKEN_ALTERNATE_PROTOCOL_LOCATION_HTTP_STREAM_FACTORY_JOB_ALT);
   session_->http_server_properties()->MarkAlternativeServiceBroken(
       alternative_service_info_.alternative_service());
+  // Reset error status for Jobs after reporting brokenness.
+  ResetErrorStatusForJobs();
 }
 
 void HttpStreamFactory::JobController::MaybeNotifyFactoryOfCompletion() {
+  if (!main_job_ && !alternative_job_) {
+    // Both jobs are gone, report brokenness if apply. Error status for Jobs
+    // will be reset after reporting to avoid redundant reporting.
+    MaybeReportBrokenAlternativeService();
+  }
+
   if (!request_ && !main_job_ && !alternative_job_) {
     DCHECK(!bound_job_);
     factory_->OnJobControllerComplete(this);
@@ -1306,6 +1350,7 @@ int HttpStreamFactory::JobController::ReconsiderProxyAfterError(Job* job,
   bound_job_ = nullptr;
   alternative_job_.reset();
   main_job_.reset();
+  ResetErrorStatusForJobs();
   // Also resets states that related to the old main job. In particular,
   // cancels |resume_main_job_callback_| so there won't be any delayed
   // ResumeMainJob() left in the task queue.

net/http/http_stream_factory_job_controller.h

@@ -111,6 +111,9 @@ class HttpStreamFactory::JobController
                       int status,
                       const SSLConfig& used_ssl_config) override;
 
+  // Invoked when |job| fails on the default network.
+  void OnFailedOnDefaultNetwork(Job* job) override;
+
   // Invoked when |job| has a certificate error for the Request.
   void OnCertificateError(Job* job,
                           int status,
@@ -244,9 +247,14 @@ class HttpStreamFactory::JobController
   // net error of the failed alternative proxy job.
   void OnAlternativeProxyJobFailed(int net_error);
 
-  // Called to report to http_server_properties to mark alternative service
-  // broken.
-  void ReportBrokenAlternativeService();
+  // Called when all Jobs complete. Reports alternative service brokenness to
+  // HttpServerProperties if apply and resets net errors afterwards:
+  // - report broken if the main job has no error and the alternative job has an
+  //   error;
+  // - report broken until default network change if the main job has no error,
+  //   the alternative job has no error, but the alternative job failed on the
+  //   default network.
+  void MaybeReportBrokenAlternativeService();
 
   void MaybeNotifyFactoryOfCompletion();
 
@@ -262,6 +270,11 @@ class HttpStreamFactory::JobController
   // Resumes the main job immediately.
   void ResumeMainJob();
 
+  // Reset error status to default value for Jobs:
+  // - reset |main_job_net_error_| and |alternative_job_net_error_| to OK;
+  // - reset |alternative_job_failed_on_default_network_| to false.
+  void ResetErrorStatusForJobs();
+
   AlternativeServiceInfo GetAlternativeServiceInfoFor(
       const HttpRequestInfo& request_info,
       HttpStreamRequest::Delegate* delegate,
@@ -344,8 +357,13 @@ class HttpStreamFactory::JobController
   // (or by |main_job_| if |is_preconnect_|.)
   AlternativeServiceInfo alternative_service_info_;
 
-  // Net error code of the failed alternative job. Set to OK by default.
+  // Error status used for alternative service brokenness reporting.
+  // Net error code of the main job. Set to OK by default.
+  int main_job_net_error_;
+  // Net error code of the alternative job. Set to OK by default.
   int alternative_job_net_error_;
+  // Set to true if the alternative job failed on the default network.
+  bool alternative_job_failed_on_default_network_;
 
   // True if a Job has ever been bound to the |request_|.
   bool job_bound_;

net/quic/quic_stream_factory.cc

@@ -623,6 +623,10 @@ int QuicStreamFactory::Job::DoConfirmConnection(int rv) {
         net_log_.AddEvent(
             NetLogEventType::
                 QUIC_STREAM_FACTORY_JOB_RETRY_ON_ALTERNATE_NETWORK);
+        // Notify requests that connection on the default network failed.
+        for (auto* request : stream_requests_) {
+          request->OnConnectionFailedOnDefaultNetwork();
+        }
         DVLOG(1) << "Retry connection on alternate network";
         session_ = nullptr;
         io_state_ = STATE_CONNECT;
@@ -661,16 +665,18 @@ QuicStreamRequest::~QuicStreamRequest() {
     factory_->CancelRequest(this);
 }
 
-int QuicStreamRequest::Request(const HostPortPair& destination,
-                               quic::QuicTransportVersion quic_version,
-                               PrivacyMode privacy_mode,
-                               RequestPriority priority,
-                               const SocketTag& socket_tag,
-                               int cert_verify_flags,
-                               const GURL& url,
-                               const NetLogWithSource& net_log,
-                               NetErrorDetails* net_error_details,
-                               CompletionOnceCallback callback) {
+int QuicStreamRequest::Request(
+    const HostPortPair& destination,
+    quic::QuicTransportVersion quic_version,
+    PrivacyMode privacy_mode,
+    RequestPriority priority,
+    const SocketTag& socket_tag,
+    int cert_verify_flags,
+    const GURL& url,
+    const NetLogWithSource& net_log,
+    NetErrorDetails* net_error_details,
+    CompletionOnceCallback failed_on_default_network_callback,
+    CompletionOnceCallback callback) {
   DCHECK_NE(quic_version, quic::QUIC_VERSION_UNSUPPORTED);
   DCHECK(net_error_details);
   DCHECK(callback_.is_null());
@@ -678,6 +684,8 @@ int QuicStreamRequest::Request(const HostPortPair& destination,
   DCHECK(factory_);
 
   net_error_details_ = net_error_details;
+  failed_on_default_network_callback_ =
+      std::move(failed_on_default_network_callback);
   session_key_ =
       QuicSessionKey(HostPortPair::FromURL(url), privacy_mode, socket_tag);
 
@@ -709,6 +717,11 @@ void QuicStreamRequest::SetSession(
   session_ = move(session);
 }
 
+void QuicStreamRequest::OnConnectionFailedOnDefaultNetwork() {
+  if (!failed_on_default_network_callback_.is_null())
+    base::ResetAndReturn(&failed_on_default_network_callback_).Run(OK);
+}
+
 void QuicStreamRequest::OnRequestComplete(int rv) {
   factory_ = nullptr;
   base::ResetAndReturn(&callback_).Run(rv);
@@ -1324,6 +1337,14 @@ void QuicStreamFactory::OnNetworkMadeDefault(NetworkHandle network) {
   if (!migrate_sessions_on_network_change_v2_)
     return;
 
+  // Clear alternative services that were marked as broken until default network
+  // changes.
+  if (retry_on_alternate_network_before_handshake_ &&
+      default_network_ != NetworkChangeNotifier::kInvalidNetworkHandle &&
+      network != default_network_) {
+    http_server_properties_->OnDefaultNetworkChanged();
+  }
+
   DCHECK_NE(NetworkChangeNotifier::kInvalidNetworkHandle, network);
   default_network_ = network;
   ScopedConnectionMigrationEventLog scoped_event_log(net_log_,

net/quic/quic_stream_factory.h

@@ -123,6 +123,7 @@ class NET_EXPORT_PRIVATE QuicStreamRequest {
               const GURL& url,
               const NetLogWithSource& net_log,
               NetErrorDetails* net_error_details,
+              CompletionOnceCallback failed_on_default_network_callback,
               CompletionOnceCallback callback);
 
   // This function must be called after Request() returns ERR_IO_PENDING.
@@ -144,6 +145,11 @@ class NET_EXPORT_PRIVATE QuicStreamRequest {
 
   void OnRequestComplete(int rv);
 
+  // Called when the original connection created on the default network for
+  // |this| fails and a new connection has been created on the alternate
+  // network.
+  void OnConnectionFailedOnDefaultNetwork();
+
   // Helper method that calls |factory_|'s GetTimeDelayForWaitingJob(). It
   // returns the amount of time waiting job should be delayed.
   base::TimeDelta GetTimeDelayForWaitingJob() const;
@@ -165,6 +171,7 @@ class NET_EXPORT_PRIVATE QuicStreamRequest {
   QuicSessionKey session_key_;
   NetLogWithSource net_log_;
   CompletionOnceCallback callback_;
+  CompletionOnceCallback failed_on_default_network_callback_;
   NetErrorDetails* net_error_details_;  // Unowned.
   std::unique_ptr<QuicChromiumClientSession::Handle> session_;
 

net/quic/quic_stream_factory_fuzzer.cc

@@ -159,7 +159,9 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
       env->host_port_pair,
       data_provider.PickValueInArray(quic::kSupportedTransportVersions),
       PRIVACY_MODE_DISABLED, DEFAULT_PRIORITY, SocketTag(), kCertVerifyFlags,
-      GURL(kUrl), env->net_log, &net_error_details, callback.callback());
+      GURL(kUrl), env->net_log, &net_error_details,
+      /*failed_on_default_network_callback=*/CompletionOnceCallback(),
+      callback.callback());
 
   callback.WaitForResult();
   std::unique_ptr<QuicChromiumClientSession::Handle> session =

net/quic/test_task_runner.cc

@@ -50,6 +50,15 @@ const std::vector<PostedTask>& TestTaskRunner::GetPostedTasks() const {
   return tasks_;
 }
 
+quic::QuicTime::Delta TestTaskRunner::NextPendingTaskDelay() {
+  if (tasks_.empty())
+    return quic::QuicTime::Delta::Infinite();
+
+  auto next = FindNextTask();
+  return quic::QuicTime::Delta::FromMicroseconds(
+      (next->GetTimeToRun() - NowInTicks(*clock_)).InMicroseconds());
+}
+
 void TestTaskRunner::RunNextTask() {
   std::vector<PostedTask>::iterator next = FindNextTask();
   DCHECK(next != tasks_.end());
@@ -60,6 +69,24 @@ void TestTaskRunner::RunNextTask() {
   std::move(task.task).Run();
 }
 
+void TestTaskRunner::FastForwardBy(quic::QuicTime::Delta delta) {
+  DCHECK_GE(delta, quic::QuicTime::Delta::Zero());
+
+  quic::QuicTime end_timestamp = clock_->Now() + delta;
+
+  while (NextPendingTaskDelay() <= end_timestamp - clock_->Now()) {
+    RunNextTask();
+  }
+
+  if (clock_->Now() != end_timestamp)
+    clock_->AdvanceTime(end_timestamp - clock_->Now());
+
+  while (NextPendingTaskDelay() <= quic::QuicTime::Delta::Zero()) {
+    RunNextTask();
+  }
+  return;
+}
+
 void TestTaskRunner::RunUntilIdle() {
   while (!tasks_.empty())
     RunNextTask();

net/quic/test_task_runner.h

@@ -14,6 +14,7 @@
 #include "base/sequenced_task_runner.h"
 #include "base/task_runner.h"
 #include "base/test/test_pending_task.h"
+#include "net/third_party/quic/core/quic_time.h"
 
 namespace quic {
 class MockClock;
@@ -40,10 +41,19 @@ class TestTaskRunner : public base::SequencedTaskRunner {
 
   const std::vector<PostedTask>& GetPostedTasks() const;
 
+  // Returns the delay for next task to run. If there is no pending task,
+  // return QuicTime::Delta::Infinite().
+  quic::QuicTime::Delta NextPendingTaskDelay();
+
   // Finds the next task to run, advances the time to the correct time
   // and then runs the task.
   void RunNextTask();
 
+  // Fast forwards virtual time by |delta|, causing all tasks with a remaining
+  // delay less than or equal to |delta| to be executed. |delta| must be
+  // non-negative.
+  void FastForwardBy(quic::QuicTime::Delta delta);
+
   // While there are posted tasks, finds the next task to run, advances the
   // time to the correct time and then runs the task.
   void RunUntilIdle();