Race TCP connection to proxies with QUIC connections

net/http/http_stream_factory_impl_job.h

Index: net/http/http_stream_factory_impl_job.h
diff --git a/net/http/http_stream_factory_impl_job.h b/net/http/http_stream_factory_impl_job.h
index 142407928cc1658e8910d932bdb2a73c7934974f..9a29bdda8293e8dd2b5e4762214e07c56699d5a8 100644
--- a/net/http/http_stream_factory_impl_job.h
+++ b/net/http/http_stream_factory_impl_job.h
@@ -20,6 +20,7 @@
 #include "net/http/http_request_info.h"
 #include "net/http/http_stream_factory_impl.h"
 #include "net/log/net_log.h"
+#include "net/proxy/proxy_server.h"
 #include "net/proxy/proxy_service.h"
 #include "net/quic/chromium/quic_stream_factory.h"
 #include "net/socket/client_socket_handle.h"
@@ -35,6 +36,7 @@ class HttpAuthController;
 class HttpNetworkSession;
 class HttpStream;
 class SpdySessionPool;
+struct SSLConfig;
 class QuicHttpStream;
 
 // An HttpStreamRequestImpl exists for each stream which is in progress of being
@@ -96,6 +98,15 @@ class HttpStreamFactoryImpl::Job {
                                   const ProxyInfo& used_proxy_info,
                                   HttpAuthController* auth_controller) = 0;
 
+    // Invoked when |job| has completed proxy resolution.
+    virtual void OnResolveProxyComplete(
+        Job* job,
+        const HttpRequestInfo& request_info,
+        RequestPriority priority,
+        const SSLConfig& server_ssl_config,
+        const SSLConfig& proxy_ssl_config,
+        HttpStreamRequest::StreamType stream_type) = 0;
+
     // Invoked to notify the Request and Factory of the readiness of new
     // SPDY session.
     virtual void OnNewSpdySessionReady(
@@ -156,9 +167,14 @@ class HttpStreamFactoryImpl::Job {
       GURL origin_url,
       NetLog* net_log);
 
-  // Constructor for alternative Job.
-  // Job is owned by |delegate|, hence |delegate| is valid for the
-  // lifetime of the Job.
+  // Constructor for alternative Job. The Job is owned by |delegate|, hence

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

Reading the comments. I think it might be better to provide another constructor
for alternative proxy job so that we prevent the case where both
|alternative_service| and |alternative_proxy_server| are valid. It also helps us
to ready the code whether we are creating a alt job with alternative service or
alternative proxy server. 

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

I thought about it, and I had initially written code exactly like that. But that
meant that the Job class had 4 constructors:
(i) Main job
(ii) Alternative service job that takes alternative service as an argument
(iii) alternative proxy server job that takes alternative proxy server job as an
argument
(iv) A private constructor that is called by the other 3 constructos, and does
all the heavy lifting.

I am okay with this approach, but 4 constructors seemed like too much
repetition? wdyt?

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

The old code is not well written. You could possibly have only one generic
constructor for HttpStreamFactoryImpl::Job (we currently have two) but have 3
different APIs for HttpStreamFactoryImpl::JobFactory::CreateJob(we currently
have two). Does that sounds plausible?

https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job.h?...

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

Done.

+  // |delegate| is valid for the lifetime of the Job. If |alternative_service|
+  // is initialized, then the Job will use the alternative service. On the
+  // other hand, if |alternative_proxy_server| is a valid proxy server, then the
+  // job will use that instead of using ProxyService for proxy resolution.
+  // Further, if |alternative_proxy_server| is valid but bad proxy, then
+  // fallback proxies are not used. It is illegal to call this with an
+  // initialized |alternative_service|, and a valid |alternative_proxy_server|.
   Job(Delegate* delegate,
       JobType job_type,
       HttpNetworkSession* session,
@@ -169,6 +185,7 @@ class HttpStreamFactoryImpl::Job {
       HostPortPair destination,
       GURL origin_url,
       AlternativeService alternative_service,
+      const ProxyServer& alternative_proxy_server,
       NetLog* net_log);
   virtual ~Job();
 
@@ -364,6 +381,11 @@ class HttpStreamFactoryImpl::Job {
 
   void MaybeMarkAlternativeServiceBroken();
 
+  // May notify proxy delegate that the alternative proxy server is broken. The
+  // alternative proxy server is considered as broken if the alternative proxy
+  // server job failed, but the main job was successful.
+  void MaybeNotifyAlternativeProxyServerBroken() const;
+
   ClientSocketPoolManager::SocketGroupType GetSocketGroup() const;
 
   void MaybeCopyConnectionAttemptsFromSocketOrHandle();
@@ -412,6 +434,13 @@ class HttpStreamFactoryImpl::Job {
   // Unowned. |this| job is owned by |delegate_|.
   Delegate* delegate_;
 
+  // Alternative proxy server that should be used by |this| to fetch the
+  // request.
+  const ProxyServer alternative_proxy_server_;
+
+  // Alternative proxy server for the other job.
+  ProxyServer other_job_alternative_proxy_server_;
+
   JobType job_type_;
 
   // True if handling a HTTPS request, or using SPDY with SSL
@@ -491,6 +520,7 @@ class HttpStreamFactoryImpl::JobFactory {
       HostPortPair destination,
       GURL origin_url,
       AlternativeService alternative_service,
+      const ProxyServer& alternative_proxy_server,
       NetLog* net_log) = 0;
 
   // Creates a non-alternative Job.