Race TCP connection to proxies with QUIC connections

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 f526618acdb36b55c222b2ece8f30284d7099b3b..bf1c0b9fdba055e73c6479d7e9c5e51d40340602 100644
--- a/net/http/http_stream_factory_impl_job_controller.cc
+++ b/net/http/http_stream_factory_impl_job_controller.cc
@@ -9,8 +9,10 @@
 #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 {
@@ -38,6 +40,7 @@ HttpStreamFactoryImpl::JobController::JobController(
       job_bound_(false),
       main_job_is_blocked_(false),
       bound_job_(nullptr),
+      started_alternative_proxy_server_job_(false),
       ptr_factory_(this) {
   DCHECK(factory);
 }
@@ -109,7 +112,7 @@ void HttpStreamFactoryImpl::JobController::Preconnect(
   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);
 }
 
@@ -355,6 +358,45 @@ void HttpStreamFactoryImpl::JobController::OnNeedsProxyAuth(
                              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,
@@ -602,7 +644,7 @@ void HttpStreamFactoryImpl::JobController::CreateJobs(
     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;
@@ -895,4 +937,66 @@ HttpStreamFactoryImpl::JobController::GetAlternativeServiceForInternal(
 
   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;
+}
 }