Race TCP connection to proxies with QUIC connections

net/http/http_stream_factory_impl_job.cc

Index: net/http/http_stream_factory_impl_job.cc
diff --git a/net/http/http_stream_factory_impl_job.cc b/net/http/http_stream_factory_impl_job.cc
index d492754969355f7eb8593711eac4d9a1bcc1d001..9e9c20339cc5e46961327f48e2ebef69ea6803d2 100644
--- a/net/http/http_stream_factory_impl_job.cc
+++ b/net/http/http_stream_factory_impl_job.cc
@@ -25,6 +25,7 @@
 #include "base/values.h"
 #include "build/build_config.h"
 #include "net/base/port_util.h"
+#include "net/base/proxy_delegate.h"
 #include "net/cert/cert_verifier.h"
 #include "net/http/bidirectional_stream_impl.h"
 #include "net/http/http_basic_stream.h"
@@ -161,6 +162,7 @@ HttpStreamFactoryImpl::Job::Job(Delegate* delegate,
           destination,
           origin_url,
           AlternativeService(),
+          ProxyServer(),
           net_log) {}
 
 HttpStreamFactoryImpl::Job::Job(Delegate* delegate,
@@ -173,6 +175,7 @@ HttpStreamFactoryImpl::Job::Job(Delegate* delegate,
                                 HostPortPair destination,
                                 GURL origin_url,
                                 AlternativeService alternative_service,
+                                const ProxyServer& alternative_proxy_server,
                                 NetLog* net_log)
     : request_info_(request_info),
       priority_(priority),
@@ -188,6 +191,7 @@ HttpStreamFactoryImpl::Job::Job(Delegate* delegate,
       origin_url_(origin_url),
       alternative_service_(alternative_service),
       delegate_(delegate),
+      alternative_proxy_server_(alternative_proxy_server),
       job_type_(job_type),
       using_ssl_(false),
       using_spdy_(false),
@@ -205,6 +209,15 @@ HttpStreamFactoryImpl::Job::Job(Delegate* delegate,
       stream_type_(HttpStreamRequest::BIDIRECTIONAL_STREAM),
       ptr_factory_(this) {
   DCHECK(session);
+  // The job can't have alternative service and alternative proxy server set at
+  // the same time since alternative services are used for requests that are
+  // fetched directly, while the alternative proxy server is used for requests
+  // that should be fetched using proxy.
+  DCHECK(alternative_service_.protocol == UNINITIALIZED_ALTERNATE_PROTOCOL ||
+         !alternative_proxy_server_.is_valid());
+  DCHECK(!alternative_proxy_server_.is_valid() ||
+         !(IsSpdyAlternative() || IsQuicAlternative()));
+
   if (IsSpdyAlternative()) {
     DCHECK(origin_url_.SchemeIs("https"));
   }
@@ -701,6 +714,12 @@ int HttpStreamFactoryImpl::Job::DoResolveProxy() {
     return OK;
   }
 
+  // If an alternative proxy server was provided, use that.
+  if (alternative_proxy_server_.is_valid()) {

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

nit: DCHECK_EQ(job_type_, ALTERNATIVE);

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

good idea, I added a similar DCHECK in the constructor of the Job above.

+    proxy_info_.UseProxyServer(alternative_proxy_server_);
+    return OK;
+  }
+
   return session_->proxy_service()->ResolveProxy(
       origin_url_, request_info_.method, &proxy_info_, io_callback_,
       &pac_request_, session_->params().proxy_delegate, net_log_);
@@ -738,6 +757,11 @@ int HttpStreamFactoryImpl::Job::DoResolveProxyComplete(int result) {
   }
 
   next_state_ = STATE_WAIT;
+
+  delegate_->OnResolveProxyComplete(this, request_info_, priority_,
+                                    server_ssl_config_, proxy_ssl_config_,
+                                    stream_type_);
+
   return OK;
 }
 
@@ -1394,6 +1418,14 @@ int HttpStreamFactoryImpl::Job::ReconsiderProxyAfterError(int error) {
   if (request_info_.load_flags & LOAD_BYPASS_PROXY)
     return error;
 
+  // Alternative proxy server job should not use fallback proxies, and instead
+  // return. This would resume the main job (if possible) which may try the
+  // fallback proxies.
+  if (alternative_proxy_server_.is_valid()) {
+    DCHECK_EQ(STATE_NONE, next_state_);
+    return error;
+  }
+
   if (proxy_info_.is_https() && proxy_ssl_config_.send_client_cert) {
     session_->ssl_client_auth_cache()->Remove(
         proxy_info_.proxy_server().host_port_pair());
@@ -1474,8 +1506,16 @@ void HttpStreamFactoryImpl::Job::ReportJobSucceededForRequest() {
 
 void HttpStreamFactoryImpl::Job::MarkOtherJobComplete(const Job& job) {
   DCHECK_EQ(STATUS_RUNNING, other_job_status_);
+  DCHECK(!other_job_alternative_proxy_server_.is_valid());
+
   other_job_status_ = job.job_status_;
   other_job_alternative_service_ = job.alternative_service_;
+  other_job_alternative_proxy_server_ = job.alternative_proxy_server_;
+
+  // At most one job can have a valid |alternative_proxy_server_|.
+  DCHECK(!alternative_proxy_server_.is_valid() ||
+         !other_job_alternative_proxy_server_.is_valid());
+
   MaybeMarkAlternativeServiceBroken();
 }
 
@@ -1483,6 +1523,8 @@ void HttpStreamFactoryImpl::Job::MaybeMarkAlternativeServiceBroken() {
   if (job_status_ == STATUS_RUNNING || other_job_status_ == STATUS_RUNNING)
     return;
 
+  MaybeNotifyAlternativeProxyServerBroken();
+
   if (IsSpdyAlternative() || IsQuicAlternative()) {
     if (job_status_ == STATUS_BROKEN && other_job_status_ == STATUS_SUCCEEDED) {
       HistogramBrokenAlternateProtocolLocation(
@@ -1503,6 +1545,47 @@ void HttpStreamFactoryImpl::Job::MaybeMarkAlternativeServiceBroken() {
   }
 }
 
+void HttpStreamFactoryImpl::Job::MaybeNotifyAlternativeProxyServerBroken()
+    const {
+  if (!alternative_proxy_server_.is_valid() &&
+      !other_job_alternative_proxy_server_.is_valid()) {

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

nit:
DCHECK_EQ(alternative_service_.protocol != UNINITIALIZED_ALTERNATE_PROTOCOL ||
other_job_alternative_service_.protocol != UNINITIALIZED_ALTERNATE_PROTOCOL);

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

Isn't this the same as the DCHECKs in lInes 1558-1560 below?

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

You won't be able to run the checks on line 1558-1560 if you are early return
here, right?

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

I don't think those two checks has any correlation. Checking on line 1558-1560
doesn't guarantee the checks in this early return. I was trying to confirm that
one of the two jobs are using alternative service. 

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

I think what you meant was:
DCHECK(alternative_service_.protocol == UNINITIALIZED_ALTERNATE_PROTOCOL ||
         other_job_alternative_service_.protocol ==
             UNINITIALIZED_ALTERNATE_PROTOCOL);

Done.

+    // Neither of the two jobs used an alternative proxy server.
+    return;
+  }
+
+  // Neither this job, nor the other job should have used the alternative
+  // service.
+  DCHECK_EQ(UNINITIALIZED_ALTERNATE_PROTOCOL, alternative_service_.protocol);
+  DCHECK_EQ(UNINITIALIZED_ALTERNATE_PROTOCOL,
+            other_job_alternative_service_.protocol);
+
+  ProxyDelegate* proxy_delegate = session_->params().proxy_delegate;
+  if (!proxy_delegate)
+    return;
+
+  if (alternative_proxy_server_.is_valid()) {
+    // |this| connected to the alternative proxy server.
+    if ((job_status_ == STATUS_BROKEN || job_status_ == STATUS_FAILED) &&
+        other_job_status_ == STATUS_SUCCEEDED) {
+      // Notify ProxyDelegate.
+      proxy_delegate->OnAlternativeProxyBroken(alternative_proxy_server_);
+    }
+    return;
+  }
+
+  if (other_job_alternative_proxy_server_.is_valid()) {
+    // Other job connected to the alternative proxy server.
+    if (job_status_ == STATUS_SUCCEEDED &&
+        (other_job_status_ == STATUS_BROKEN ||
+         other_job_status_ == STATUS_FAILED)) {
+      // Notify ProxyDelegate.
+      proxy_delegate->OnAlternativeProxyBroken(
+          other_job_alternative_proxy_server_);
+    }
+    return;
+  }
+}
+
 ClientSocketPoolManager::SocketGroupType
 HttpStreamFactoryImpl::Job::GetSocketGroup() const {
   std::string scheme = origin_url_.scheme();