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 02d7054cd494fce85e20023c5b1c24063c59c9c1..c0aedb2e5e332d388c7eb581017632da2c597ddb 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"
@@ -96,6 +97,15 @@ class HttpStreamFactoryImpl::Job {
                                   const ProxyInfo& used_proxy_info,
                                   HttpAuthController* auth_controller) = 0;
 
+    // Invoked when |job| has completed proxy resolution.

[Zhongyi Shi, 2016/08/19 23:08:42]

nit: maybe mention it's default proxy resolution.

[tbansal1, 2016/08/20 01:53:39]

I do not understand this comment. You mean it is called only by the main job?
What is "default" proxy resolution?

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

Yeah, just mention that this method is trying to set a alt proxy jov with a
alternative proxy server if it's a HTTP request. 

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

Done.  I did not mention the specifics ("a HTTP request.") because that
seems like an internal detail.

+    virtual void OnResolveProxyComplete(
+        Job* job,
+        const HttpRequestInfo& request_info,
+        RequestPriority priority,
+        const SSLConfig& server_ssl_config,

[RyanSturm, 2016/08/19 19:03:23]

nit: I wouldn't complain if you forward declared SSLConfig, it's getting pulled
in via ssl_config_service.h

[tbansal1, 2016/08/20 01:53:39]

Done.

+        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 +166,12 @@ 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
+  // |delegate| is valid for the lifetime of the Job.
+  // 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.

[Zhongyi Shi, 2016/08/19 23:08:42]

The comments are a little confusing. If I understand correctly, QUIC proxy
connection will create a alternative job with alternative proxy if there's only
main job created, right?

Thus the alternative job might be:
1. a job running with alternative service
2. a job running with alternative proxy

And I think what you meant to say is if the alternative_service is provided, it
will use the alternative_service. Otherwise, if the alternative_proxy_server is
provided, it will use the the alternative_proxy_server.

[tbansal1, 2016/08/20 01:53:39]

Done.

   Job(Delegate* delegate,
       JobType job_type,
       HttpNetworkSession* session,
@@ -169,6 +182,7 @@ class HttpStreamFactoryImpl::Job {
       HostPortPair destination,
       GURL origin_url,
       AlternativeService alternative_service,
+      const ProxyServer& alternative_proxy_server,
       NetLog* net_log);
   virtual ~Job();
 
@@ -364,6 +378,9 @@ class HttpStreamFactoryImpl::Job {
 
   void MaybeMarkAlternativeServiceBroken();
 
+  // May notify proxy delegate that the alternative proxy server is broken.

[RyanSturm, 2016/08/19 19:03:23]

The "May" here is somewhat confusing. It should be clear about when it notifies
the proxy delegate. Can you change the to something more descriptive about when
it informs the proxy delegate?

[tbansal1, 2016/08/20 01:53:39]

Done.

+  void MaybeNotifyAlternativeProxyServerBroken() const;
+
   ClientSocketPoolManager::SocketGroupType GetSocketGroup() const;
 
   void MaybeCopyConnectionAttemptsFromSocketOrHandle();
@@ -412,6 +429,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 +515,7 @@ class HttpStreamFactoryImpl::JobFactory {
       HostPortPair destination,
       GURL origin_url,
       AlternativeService alternative_service,
+      const ProxyServer& proxy_server,

[RyanSturm, 2016/08/19 19:03:23]

s/proxy_server/alternative_proxy_server

[tbansal1, 2016/08/20 01:53:39]

Done.

       NetLog* net_log) = 0;
 
   // Creates a non-alternative Job.