JobController3: Move MarkAlternativeServiceBroken to job controller

net/http/http_stream_factory_impl_job_controller.cc

 // Copyright (c) 2016 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/http/http_stream_factory_impl_job_controller.h"
 
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/values.h"
@@ skipping 20 matching lines @@
     HttpStreamFactoryImpl* factory,
     HttpStreamRequest::Delegate* delegate,
     HttpNetworkSession* session,
     JobFactory* job_factory)
     : factory_(factory),
       session_(session),
       job_factory_(job_factory),
       request_(nullptr),
       delegate_(delegate),
       is_preconnect_(false),
+      alternative_service_is_broken_(false),
       job_bound_(false),
       main_job_is_blocked_(false),
       bound_job_(nullptr),
       can_start_alternative_proxy_job_(false),
       ptr_factory_(this) {
   DCHECK(factory);
 }
 
 HttpStreamFactoryImpl::JobController::~JobController() {
   main_job_.reset();
@@ skipping 161 matching lines @@
   request_->OnBidirectionalStreamImplReady(used_ssl_config, used_proxy_info,
                                            stream.release());
 }
 
 void HttpStreamFactoryImpl::JobController::OnWebSocketHandshakeStreamReady(
     Job* job,
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info,
     WebSocketHandshakeStreamBase* stream) {
   DCHECK(job);
-
   MarkRequestComplete(job->was_npn_negotiated(), job->negotiated_protocol(),
                       job->using_spdy());
 
   if (!request_)
     return;
   DCHECK(factory_->for_websockets_);
   DCHECK_EQ(HttpStreamRequest::HTTP_STREAM, request_->stream_type());
   DCHECK(stream);
 
   OnJobSucceeded(job);
   request_->OnWebSocketHandshakeStreamReady(used_ssl_config, used_proxy_info,
                                             stream);
 }
 
 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);
 
   if (!bound_job_) {
     if (main_job_ && alternative_job_) {
       // 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;
     } else {
       BindJob(job);
     }
   }
 
   request_->OnStreamFailed(status, used_ssl_config);
 }
@@ skipping 137 matching lines @@
   const bool was_npn_negotiated = job->was_npn_negotiated();
   const NextProto negotiated_protocol = job->negotiated_protocol();
   const bool using_spdy = job->using_spdy();
   const BoundNetLog net_log = job->net_log();
 
   // Cache this so we can still use it if the JobController is deleted.
   HttpStreamFactoryImpl* factory = factory_;
 
   // 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.
     if (!job_bound_) {
       BindJob(job);
     }
 
     MarkRequestComplete(was_npn_negotiated, negotiated_protocol, using_spdy);
 
     std::unique_ptr<HttpStream> stream;
@@ skipping 64 matching lines @@
       base::Bind(&NetLogHttpStreamJobDelayCallback, main_job_wait_time_));
 
   main_job_->Resume();
   main_job_wait_time_ = base::TimeDelta();
 }
 
 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;
 
   main_job_is_blocked_ = false;
 
   if (!main_job_->is_waiting())
     return;
 
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
@@ skipping 207 matching lines @@
     // because we *WANT* to cancel the unnecessary Jobs from other requests if
     // another Job completes first.
     // TODO(mbelshe): Revisit this when we implement ip connection pooling of
     // SpdySessions. Do we want to orphan the jobs for a different hostname so
     // they complete? Or do we want to prevent connecting a new SpdySession if
     // we've already got one available for a different hostname where the ip
     // address matches up?
     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;
   }
   DCHECK(bound_job_);
 }
 
 void HttpStreamFactoryImpl::JobController::MarkRequestComplete(
     bool was_npn_negotiated,
     NextProto negotiated_protocol,
     bool using_spdy) {
   if (request_)
     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_);
     factory_->OnJobControllerComplete(this);
   }
 }
 
 GURL HttpStreamFactoryImpl::JobController::ApplyHostMappingRules(
     const GURL& url,
     HostPortPair* endpoint) {
@@ skipping 231 matching lines @@
     // Check that QUIC is enabled globally, and it is not disabled.
     if (!session_->params().enable_quic ||
         session_->quic_stream_factory()->IsQuicDisabled()) {
       return false;
     }
   }
 
   return true;
 }
 }