JobController3: Move MarkAlternativeServiceBroken to job controller

net/http/http_stream_factory_impl_job_controller.cc

Index: net/http/http_stream_factory_impl_job_controller.cc
diff --git a/net/http/http_stream_factory_impl_job_controller.cc b/net/http/http_stream_factory_impl_job_controller.cc
index c1efb61c4db40774d5d6e596e561b2fc464e07af..9bf3bfcd74ffa097ed3490c3d9053b7d45c819a1 100644
--- a/net/http/http_stream_factory_impl_job_controller.cc
+++ b/net/http/http_stream_factory_impl_job_controller.cc
@@ -38,6 +38,7 @@ HttpStreamFactoryImpl::JobController::JobController(
       request_(nullptr),
       delegate_(delegate),
       is_preconnect_(false),
+      alternative_service_is_broken_(false),
       job_bound_(false),
       main_job_is_blocked_(false),
       bound_job_(nullptr),
@@ -165,6 +166,7 @@ void HttpStreamFactoryImpl::JobController::OnStreamReady(
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info) {
   DCHECK(job);
+  MaybeReportAlternativeServiceBroken(job);
 
   if (job_bound_ && bound_job_ != job) {
     // We have bound a job to the associated Request, |job| has been orphaned.
@@ -190,6 +192,7 @@ void HttpStreamFactoryImpl::JobController::OnBidirectionalStreamImplReady(
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info) {
   DCHECK(job);
+  MaybeReportAlternativeServiceBroken(job);
 
   if (job_bound_ && bound_job_ != job) {
     // We have bound a job to the associated Request, |job| has been orphaned.
@@ -219,6 +222,7 @@ void HttpStreamFactoryImpl::JobController::OnWebSocketHandshakeStreamReady(
     const ProxyInfo& used_proxy_info,
     WebSocketHandshakeStreamBase* stream) {
   DCHECK(job);
+  MaybeReportAlternativeServiceBroken(job);
 
   MarkRequestComplete(job->was_npn_negotiated(), job->negotiated_protocol(),
                       job->using_spdy());
@@ -256,13 +260,10 @@ void HttpStreamFactoryImpl::JobController::OnStreamFailed(
       // Hey, we've got other jobs! Maybe one of them will succeed, let's just
       // ignore this failure.
       factory_->request_map_.erase(job);
-      // Notify all the other jobs that this one failed.
       if (job->job_type() == MAIN) {
-        alternative_job_->MarkOtherJobComplete(*job);
         main_job_.reset();
       } else {
         DCHECK(job->job_type() == ALTERNATIVE);
-        main_job_->MarkOtherJobComplete(*job);
         alternative_job_.reset();
       }
       return;
@@ -404,6 +405,7 @@ void HttpStreamFactoryImpl::JobController::OnNewSpdySessionReady(
     bool direct) {
   DCHECK(job);
   DCHECK(job->using_spdy());
+  MaybeReportAlternativeServiceBroken(job);
 
   bool is_job_orphaned = job_bound_ && bound_job_ != job;
 
@@ -504,6 +506,9 @@ void HttpStreamFactoryImpl::JobController::MaybeResumeMainJob(
     Job* job,
     const base::TimeDelta& delay) {
   DCHECK(job == main_job_.get() || job == alternative_job_.get());
+  if (job == alternative_job_.get())
+    MaybeMarkAlternativeServiceBroken(job);
+
   if (!main_job_is_blocked_ || job != alternative_job_.get() || !main_job_)
     return;
 
@@ -732,16 +737,8 @@ void HttpStreamFactoryImpl::JobController::OnJobSucceeded(Job* job) {
     return;
   }
   if (!bound_job_) {
-    if (main_job_ && alternative_job_) {
+    if (main_job_ && alternative_job_)
       job->ReportJobSucceededForRequest();
-      // Notify all the other jobs that this one succeeded.
-      if (job->job_type() == MAIN) {
-        alternative_job_->MarkOtherJobComplete(*job);
-      } else {
-        DCHECK(job->job_type() == ALTERNATIVE);
-        main_job_->MarkOtherJobComplete(*job);
-      }
-    }
     BindJob(job);
     return;
   }
@@ -756,6 +753,43 @@ void HttpStreamFactoryImpl::JobController::MarkRequestComplete(
     request_->Complete(was_npn_negotiated, negotiated_protocol, using_spdy);
 }
 
+void HttpStreamFactoryImpl::JobController::MaybeMarkAlternativeServiceBroken(
+    Job* job) {
+  if (job->job_type() == MAIN || !job->alternative_broken())
+    return;
+
+  alternative_service_is_broken_ = true;
+  if (job->alternative_proxy_server().is_valid()) {
+    broken_alternative_proxy_server_ = job->alternative_proxy_server();
+  } else {
+    DCHECK(!broken_alternative_proxy_server_.is_valid());
+    broken_alternative_service_ = job->alternative_service();
+  }
+}
+
+void HttpStreamFactoryImpl::JobController::MaybeReportAlternativeServiceBroken(
+    Job* job) {
+  // Only report QUIC broken alternative service when main job succeeds.
+  if (job->job_type() == ALTERNATIVE)
+    return;

[Ryan Hamilton, 2016/09/13 23:23:15]

As discussed offline, I think we need to make sure that we mark QUIC broken when
TCP succeed and QUIC fails, regardless of the order they happen in. Perhaps we
need some Controller unit tests to verify this? (And that we don't mark broken
if both fail)

+
+  if (!alternative_service_is_broken_)
+    return;
+
+  if (broken_alternative_proxy_server_.is_valid()) {
+    ProxyDelegate* proxy_delegate = session_->params().proxy_delegate;
+    if (proxy_delegate)
+      proxy_delegate->OnAlternativeProxyBroken(
+          broken_alternative_proxy_server_);
+  } else {
+    HistogramBrokenAlternateProtocolLocation(
+        BROKEN_ALTERNATE_PROTOCOL_LOCATION_HTTP_STREAM_FACTORY_IMPL_JOB_ALT);
+    session_->http_server_properties()->MarkAlternativeServiceBroken(
+        broken_alternative_service_);
+  }
+  session_->quic_stream_factory()->OnTcpJobCompleted(true);
+}
+
 void HttpStreamFactoryImpl::JobController::MaybeNotifyFactoryOfCompletion() {
   if (!request_ && !main_job_ && !alternative_job_) {
     DCHECK(!bound_job_);