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 74fe5d3b64d645d42609ad706fbc3612d764827d..ac4e49ccebedc77fb8dda13e6dfe205acf9ad41f 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),
@@ -219,7 +220,6 @@ void HttpStreamFactoryImpl::JobController::OnWebSocketHandshakeStreamReady(
     const ProxyInfo& used_proxy_info,
     WebSocketHandshakeStreamBase* stream) {
   DCHECK(job);
-
   MarkRequestComplete(job->was_npn_negotiated(), job->negotiated_protocol(),
                       job->using_spdy());
 
@@ -238,16 +238,20 @@ void HttpStreamFactoryImpl::JobController::OnStreamFailed(
     Job* job,
     int status,
     const SSLConfig& used_ssl_config) {
+  if (job->job_type() == ALTERNATIVE)
+    MarkAlternativeServiceBroken(job);
+
   MaybeResumeMainJob(job, base::TimeDelta());
 
-  if (job_bound_ && bound_job_ != job) {
-    // We have bound a job to the associated Request, |job| has been orphaned.
-    OnOrphanedJobComplete(job);
+  if ((job_bound_ && bound_job_ != job) || !request_) {
+    if (job->job_type() == ALTERNATIVE)
+      ReportAlternativeServiceBroken(job);
+    if (request_) {

[Ryan Hamilton, 2016/09/14 21:45:42]

I'm having a hard time following this. I think that this is trying to handle the
case where the main job has already succeeded. Is that right? 

[Zhongyi Shi, 2016/09/14 23:07:07]

Yup. I have moved the logic up to OnAlternativeJobFailed.

+      // We have bound a job to the associated Request, |job| has been orphaned.
+      OnOrphanedJobComplete(job);
+    }
     return;
   }
-
-  if (!request_)
-    return;
   DCHECK_NE(OK, status);
   DCHECK(job);
 
@@ -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;
@@ -420,6 +421,11 @@ void HttpStreamFactoryImpl::JobController::OnNewSpdySessionReady(
 
   // Notify |request_|.
   if (!is_preconnect_ && !is_job_orphaned) {
+    if (job->job_type() == MAIN && alternative_service_is_broken_) {
+      // Report alternative service marked as broken.
+      ReportAlternativeServiceBroken(job);
+    }
+
     DCHECK(request_);
 
     // The first case is the usual case.
@@ -504,6 +510,7 @@ void HttpStreamFactoryImpl::JobController::MaybeResumeMainJob(
     Job* job,
     const base::TimeDelta& delay) {
   DCHECK(job == main_job_.get() || job == alternative_job_.get());
+
   if (!main_job_is_blocked_ || job != alternative_job_.get() || !main_job_)
     return;
 
@@ -731,17 +738,14 @@ void HttpStreamFactoryImpl::JobController::OnJobSucceeded(Job* job) {
     CancelJobs();
     return;
   }
+
+  if (job->job_type() == MAIN && alternative_service_is_broken_) {
+    ReportAlternativeServiceBroken(job);
+  }
+
   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 +760,40 @@ void HttpStreamFactoryImpl::JobController::MarkRequestComplete(
     request_->Complete(was_npn_negotiated, negotiated_protocol, using_spdy);
 }
 
+void HttpStreamFactoryImpl::JobController::MarkAlternativeServiceBroken(
+    Job* job) {
+  DCHECK_EQ(job->job_type(), ALTERNATIVE);
+
+  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::ReportAlternativeServiceBroken(
+    Job* job) {

[Ryan Hamilton, 2016/09/14 21:45:42]

This |job| argument is unused, right? If so, it's nuke it.

[Zhongyi Shi, 2016/09/14 23:07:07]

Done.

+  DCHECK(broken_alternative_service_.protocol !=
+             UNINITIALIZED_ALTERNATE_PROTOCOL ||
+         broken_alternative_proxy_server_.is_valid());
+
+  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_);