Race TCP connection to proxies with QUIC connections

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"
 #include "net/base/host_mapping_rules.h"
+#include "net/base/proxy_delegate.h"
 #include "net/http/bidirectional_stream_impl.h"
 #include "net/http/transport_security_state.h"
+#include "net/proxy/proxy_server.h"
 #include "net/spdy/spdy_session.h"
 
 namespace net {
 
 // Returns parameters associated with the delay of the HTTP stream job.
 std::unique_ptr<base::Value> NetLogHttpStreamJobDelayCallback(
     base::TimeDelta delay,
     NetLogCaptureMode /* capture_mode */) {
   std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
   dict->SetInteger("resume_after_ms", static_cast<int>(delay.InMilliseconds()));
   return std::move(dict);
 }
 
 HttpStreamFactoryImpl::JobController::JobController(
     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),
       job_bound_(false),
       main_job_is_blocked_(false),
       bound_job_(nullptr),
+      started_alternative_proxy_server_job_(false),
       ptr_factory_(this) {
   DCHECK(factory);
 }
 
 HttpStreamFactoryImpl::JobController::~JobController() {
   main_job_.reset();
   alternative_job_.reset();
   bound_job_ = nullptr;
 }
 
@@ skipping 51 matching lines @@
     ignore_result(ApplyHostMappingRules(request_info.url, &destination));
   }
 
   // Due to how the socket pools handle priorities and idle sockets, only IDLE
   // priority currently makes sense for preconnects. The priority for
   // preconnects is currently ignored (see RequestSocketsForPool()), but could
   // be used at some point for proxy resolution or something.
   main_job_.reset(job_factory_->CreateJob(
       this, PRECONNECT, session_, request_info, IDLE, server_ssl_config,
       proxy_ssl_config, destination, origin_url, alternative_service,
-      session_->net_log()));
+      ProxyServer(), session_->net_log()));
   main_job_->Preconnect(num_streams);
 }
 
 LoadState HttpStreamFactoryImpl::JobController::GetLoadState() const {
   DCHECK(request_);
   DCHECK(main_job_ || alternative_job_);
   if (bound_job_)
     return bound_job_->GetLoadState();
 
   // Just pick the first one.
@@ skipping 225 matching lines @@
   }
 
   if (!request_)
     return;
   if (!bound_job_)
     BindJob(job);
   request_->OnNeedsProxyAuth(proxy_response, used_ssl_config, used_proxy_info,
                              auth_controller);
 }
 
+void HttpStreamFactoryImpl::JobController::OnResolveProxyComplete(
+    Job* job,
+    const HttpRequestInfo& request_info,
+    RequestPriority priority,
+    const SSLConfig& server_ssl_config,
+    const SSLConfig& proxy_ssl_config,
+    HttpStreamRequest::StreamType stream_type) {
+  DCHECK(job);
+

[Zhongyi Shi, 2016/08/19 23:08:42]

It seems like this method is only expected to be called when job controller only
creates main job. And the main job want to run this code to create an alt_job
with alternative proxy server. 

How about this:

s/maybe_create_alt_proxy_job_/started_alternative_proxy_server_job_
default to false.

set maybe_create_alt_proxy_job_ to TRUE when CreateJobs creates ONLY main job.

In this method
if (！maybe_create_alt_proxy_job_)
  return;
DCHECK_EQ(job, main_job_.get());
DCHECK(!alternative_job_.get());
maybe_create_alt_proxy_job_ = false;
// Perform checkings on whether we should create alt proxy job.
...
// Create alt proxy job.

[tbansal1, 2016/08/20 01:53:39]

This is a good approach. Done what you suggested but used a slightly different
variable name.

+  ProxyServer alternative_proxy_server;
+  if (!ShouldCreateAlternativeProxyServerJob(job, job->proxy_info(),
+                                             request_info.url,
+                                             &alternative_proxy_server)) {
+    return;
+  }
+
+  if (started_alternative_proxy_server_job_) {
+    // For one request, at most one alternative proxy server job can be
+    // started. This helps in keeping the resource utilization down.
+    return;
+  }
+  DCHECK(main_job_);
+  DCHECK(!alternative_job_);
+  DCHECK(!main_job_is_blocked_);
+
+  HostPortPair destination(HostPortPair::FromURL(request_info.url));
+  GURL origin_url = ApplyHostMappingRules(request_info.url, &destination);
+
+  alternative_job_.reset(job_factory_->CreateJob(
+      this, ALTERNATIVE, session_, request_info, priority, server_ssl_config,
+      proxy_ssl_config, destination, origin_url, AlternativeService(),
+      alternative_proxy_server, job->net_log().net_log()));
+  AttachJob(alternative_job_.get());
+
+  started_alternative_proxy_server_job_ = true;
+  main_job_is_blocked_ = true;
+  alternative_job_->Start(request_->stream_type());
+}
+
 void HttpStreamFactoryImpl::JobController::OnNewSpdySessionReady(
     Job* job,
     const base::WeakPtr<SpdySession>& spdy_session,
     bool direct) {
   DCHECK(job);
   DCHECK(job->using_spdy());
 
   bool is_job_orphaned = job_bound_ && bound_job_ != job;
 
   // Cache these values in case the job gets deleted.
@@ skipping 227 matching lines @@
              << " port: " << alternative_service.host_port_pair().port() << ")";
 
     DCHECK(!request_info.url.SchemeIs("ftp"));
     HostPortPair alternative_destination(alternative_service.host_port_pair());
     ignore_result(
         ApplyHostMappingRules(request_info.url, &alternative_destination));
 
     alternative_job_.reset(job_factory_->CreateJob(
         this, ALTERNATIVE, session_, request_info, priority, server_ssl_config,
         proxy_ssl_config, alternative_destination, origin_url,
-        alternative_service, net_log.net_log()));
+        alternative_service, ProxyServer(), net_log.net_log()));
     AttachJob(alternative_job_.get());
 
     main_job_is_blocked_ = true;
     alternative_job_->Start(request_->stream_type());
   }
   // Even if |alternative_job| has already finished, it will not have notified
   // the request yet, since we defer that to the next iteration of the
   // MessageLoop, so starting |main_job_| is always safe.
   main_job_->Start(request_->stream_type());
 }
@@ skipping 272 matching lines @@
     if (first_alternative_service.protocol == UNINITIALIZED_ALTERNATE_PROTOCOL)
       first_alternative_service = alternative_service;
   }
 
   // Ask delegate to mark QUIC as broken for the origin.
   if (quic_advertised && quic_all_broken && delegate != nullptr)
     delegate->OnQuicBroken();
 
   return first_alternative_service;
 }
+
+bool HttpStreamFactoryImpl::JobController::
+    ShouldCreateAlternativeProxyServerJob(
+        Job* job,
+        const ProxyInfo& proxy_info,
+        const GURL& url,
+        ProxyServer* alternative_proxy_server) const {

[Zhongyi Shi, 2016/08/19 23:08:42]

This is only called on OnResolveProxyComplete() right? How about merge the two
into one？

With the change I proposed. You can avoid invoking this method when proxy
resolution is complete for alt job or preconnect job.

[tbansal1, 2016/08/20 01:53:39]

I can merge but that will make one single method too long, and this feels like a
good abstraction which can be separated out in a const-method. 

Also, in future, the Controller may want to do some other shiny things in
OnProxyResolveComplete(), so it may not be a good idea to make it too long (I
think).

+  if (job->job_type() == ALTERNATIVE) {
+    // If |job| is using alternative service, then alternative proxy server
+    // should not be used.
+    return false;
+  }
+
+  if (job->job_type() == PRECONNECT) {
+    // Preconnects should be fetched using only the main job to keep the
+    // resource utilization down.
+    return false;
+  }
+
+  if (proxy_info.is_empty() || proxy_info.is_direct() || proxy_info.is_quic()) {
+    // Alternative proxy server job can be created only if |job| fetches the
+    // |request_| through a non-QUIC proxy.
+    return false;
+  }
+
+  if (!url.SchemeIs(url::kHttpScheme)) {
+    // Only HTTP URLs can be fetched through alternative proxy server, since the
+    // alternative proxy server may not support fetching of URLs with other
+    // schemes.
+    return false;
+  }
+
+  ProxyDelegate* proxy_delegate = session_->params().proxy_delegate;
+  if (!proxy_delegate)
+    return false;
+
+  proxy_delegate->GetAlternativeProxy(url, proxy_info.proxy_server(),
+                                      alternative_proxy_server);
+
+  if (!alternative_proxy_server->is_valid())
+    return false;
+
+  DCHECK(!(*alternative_proxy_server == proxy_info.proxy_server()));
+
+  if (!alternative_proxy_server->is_https() &&
+      !alternative_proxy_server->is_quic()) {
+    // Alternative proxy server should be a secure server.
+    return false;
+  }
+
+  if (alternative_proxy_server->is_quic()) {
+    // Check that QUIC is enabled globally, and it is not disabled on
+    // the specified port.
+    if (!session_->params().enable_quic ||
+        session_->quic_stream_factory()->IsQuicDisabled(
+            alternative_proxy_server->host_port_pair().port())) {
+      return false;
+    }
+  }
+
+  return true;
 }
+}