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..99b9c20b64b2a4c40c6561e20868e968c02e5d8a 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),
+      can_start_alternative_proxy_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,40 @@ 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);
+
+  ProxyServer alternative_proxy_server;
+  if (!ShouldCreateAlternativeProxyServerJob(job, job->proxy_info(),
+                                             request_info.url,
+                                             &alternative_proxy_server)) {
+    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());
+
+  can_start_alternative_proxy_job_ = false;
+  main_job_is_blocked_ = true;

[Zhongyi Shi, 2016/08/20 06:15:40]

I am a little bit concerned on starting the alternative job while the main job
is actually not IO_PENDING.  Can we post a task here to start the alternative
job?

[tbansal1, 2016/08/22 15:42:33]

Done.

+  alternative_job_->Start(request_->stream_type());
+}
+
 void HttpStreamFactoryImpl::JobController::OnNewSpdySessionReady(
     Job* job,
     const base::WeakPtr<SpdySession>& spdy_session,
@@ -602,11 +639,13 @@ 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;
     alternative_job_->Start(request_->stream_type());
+  } else {

[Zhongyi Shi, 2016/08/20 06:15:40]

Rather than wait until later check, how about set the variable to true if the
request is using HTTP?

[tbansal1, 2016/08/22 15:42:33]

I do not understand this comment. It seems that here we should just be checking
if the alt service job was created or not. And,  the URL is http or not can be
checked later.

[Zhongyi Shi, 2016/08/23 05:42:33]

Okay. I thought the url scheme check can be part of the meta bit to decide
whether start a proxy server job or not. But fine, you're right, not too much
overhead to delay to ShouldCreate..Job().

[tbansal1, 2016/08/23 20:56:40]

Acknowledged.

+    can_start_alternative_proxy_job_ = true;
   }
   // 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
@@ -895,4 +934,72 @@ 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/20 06:15:40]

nit: DCHECK(!alternative_proxy_server);

[tbansal1, 2016/08/22 15:42:33]

Done.

+  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;
+  }
+
+  if (!can_start_alternative_proxy_job_) {

[Zhongyi Shi, 2016/08/20 06:15:40]

I think this is more like a meta bit of information. Probably better to move it
to the very beginning of this method. An early return is always preferred. 

[tbansal1, 2016/08/22 15:42:33]

Done.

+    // Either an alternative service job or an alternative proxy server job has
+    // already been started.
+    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;
+}
 }