Race preconnects to HTTP2 proxies that support alternate proxies

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 c04c240092ef4a7ee9ebbd584066e6de1a345481..f883e3ec7708170d25397c8a367635630392dd39 100644
--- a/net/http/http_stream_factory_impl_job_controller.cc
+++ b/net/http/http_stream_factory_impl_job_controller.cc
@@ -98,6 +98,10 @@ void HttpStreamFactoryImpl::JobController::Preconnect(
   DCHECK(!alternative_job_);
 
   is_preconnect_ = true;
+
+  can_start_alternative_proxy_job_ =
+      session_->params().race_preconnects_to_http2_proxies;
+
   HostPortPair destination(HostPortPair::FromURL(request_info.url));
   GURL origin_url = ApplyHostMappingRules(request_info.url, &destination);
 
@@ -394,19 +398,29 @@ void HttpStreamFactoryImpl::JobController::OnResolveProxyComplete(
     return;
   }
 
-  DCHECK(main_job_);
-  DCHECK_EQ(MAIN, job->job_type());
+  DCHECK(is_preconnect_ || job->job_type() == MAIN);
+  DCHECK_EQ(main_job_.get(), job);

[Ryan Hamilton, 2017/01/24 19:36:59]

The previous line suggest that this job might not be the main job if
is_preconnect_ is true. If that's the case, isn't this DCHECK invalid? Or
alternatively, should is_preconnect_ be removed from the first DCHECK? (Or more
likely than either, am I confused?)

   DCHECK(!alternative_job_);
   DCHECK(!main_job_is_blocked_);
 
+  if (is_preconnect_) {
+    // Restrict the number of streams to 1 since the proxy server supports spdy.
+    job->RestrictNumStreamsToOne();
+  }

[Ryan Hamilton, 2017/01/24 19:36:59]

So this restricts the number of streams on the main job, right? But if we're
doing a preconnect for the main job, wouldn't we have started it by calling
Preconnect() which sets num_streams to 1?

+
   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, alternative_proxy_server,
-      job->net_log().net_log()));
-  AttachJob(alternative_job_.get());
+      this, is_preconnect_ ? PRECONNECT : ALTERNATIVE, session_, request_info,
+      priority, server_ssl_config, proxy_ssl_config, destination, origin_url,
+      alternative_proxy_server, job->net_log().net_log()));
+
+  if (!is_preconnect_) {
+    // Preconnect jobs do not have an associated request. So, the preconnect
+    // job can't be attached.
+    AttachJob(alternative_job_.get());
+  }
 
   can_start_alternative_proxy_job_ = false;
   main_job_is_blocked_ = true;

[Ryan Hamilton, 2017/01/24 19:36:59]

I believe this all means that the main job will now pause and give the alt-job a
head start. (Which is desirable). Can you make sure we have a test of this case?
(Where we don't send any traffic to the HTTP/2 proxy if the QUIC proxy can do
0-RTT.)

@@ -486,10 +500,28 @@ void HttpStreamFactoryImpl::JobController::OnNewSpdySessionReady(
 }
 
 void HttpStreamFactoryImpl::JobController::OnPreconnectsComplete(Job* job) {
-  DCHECK_EQ(main_job_.get(), job);
-  main_job_.reset();
-  factory_->OnPreconnectsCompleteInternal();
-  MaybeNotifyFactoryOfCompletion();
+  DCHECK(job);
+  DCHECK(main_job_.get() == job || alternative_job_.get() == job);
+  DCHECK_EQ(PRECONNECT, job->job_type());
+  if (is_preconnect_ && main_job_ && alternative_job_) {
+    UMA_HISTOGRAM_EXACT_LINEAR(
+        "Net.QuicAlternativeProxy.PreconnectRace.FirstJobFinished", 1, 2);

[Ryan Hamilton, 2017/01/24 19:36:59]

I'm not sure "FirstJobFinished" adds any value to the name of the histograms.
I'd probably remove it.

Also, you might consider making the histogram's value be true if the main job
finished first, and false otherwise (or use an enum). This will help us
understand what's going on better.

+  }
+
+  // Note that completion of a preconnect job does not indicate that
+  // preconnection was actually successful since the socket layer currently does
+  // not provide the success or failure status of the (possibly) multiple
+  // preconnect sockets.
+  if (job == main_job_.get())
+    main_job_.reset();
+
+  if (job == alternative_job_.get())
+    alternative_job_.reset();
+
+  if (!main_job_ && !alternative_job_) {
+    factory_->OnPreconnectsCompleteInternal();
+    MaybeNotifyFactoryOfCompletion();
+  }
 }
 
 void HttpStreamFactoryImpl::JobController::OnOrphanedJobComplete(
@@ -703,6 +735,7 @@ void HttpStreamFactoryImpl::JobController::CreateJobs(
 
 void HttpStreamFactoryImpl::JobController::AttachJob(Job* job) {
   DCHECK(job);
+  DCHECK(!is_preconnect_);
   factory_->request_map_[job] = request_;
 }
 
@@ -1037,12 +1070,6 @@ bool HttpStreamFactoryImpl::JobController::
     return false;
   }
 
-  if (is_preconnect_ || job->job_type() == PRECONNECT) {
-    // Preconnects should be fetched using only the main job to keep the
-    // resource utilization down.
-    return false;

[Ryan Hamilton, 2017/01/24 19:36:59]

you could probably add a DCHECK here checking the param which enable preconnects
to alt proxies.

-  }
-
   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.
@@ -1110,10 +1137,21 @@ void HttpStreamFactoryImpl::JobController::ReportAlternateProtocolUsage(
 }
 
 void HttpStreamFactoryImpl::JobController::StartAlternativeProxyServerJob() {
-  if (!alternative_job_ || !request_)
+  if (!alternative_job_)
     return;
   DCHECK(alternative_job_->alternative_proxy_server().is_valid());
-  alternative_job_->Start(request_->stream_type());
+
+  if (!is_preconnect_) {
+    if (!request_)
+      return;
+    DCHECK(alternative_job_->alternative_proxy_server().is_quic());
+    alternative_job_->Start(request_->stream_type());
+  } else {
+    // Restrict the number of streams to 1 since the alternative proxy server
+    // supports request priorities.
+    DCHECK(alternative_job_->alternative_proxy_server().is_quic());
+    alternative_job_->Preconnect(1 /* num_streams */);
+  }

[Ryan Hamilton, 2017/01/24 19:36:59]

nit: I'd structure this slightly differently:

if (is_preconnect) {
 ... Preconnect();
 return;
}

if (!request_)
  return;

...

This avoid a bit of nesting which is always nice.

 }
 
 bool HttpStreamFactoryImpl::JobController::IsJobOrphaned(Job* job) const {