Extract Proxy Resolution out of HttpStreamFactoryImpl::Job

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 a060b353a5853eead544445ea5208812ba761209..94cdc09fecd1705a6b248940921bc3305887e936 100644
--- a/net/http/http_stream_factory_impl_job.cc
+++ b/net/http/http_stream_factory_impl_job.cc
@@ -148,23 +148,12 @@ std::unique_ptr<base::Value> NetLogHttpStreamProtoCallback(
   return std::move(dict);
 }
 
-// Returns parameters associated with the proxy resolution.
-std::unique_ptr<base::Value> NetLogHttpStreamJobProxyServerResolved(

[tbansal1, 2017/05/09 20:38:48]

Is it possible to keep this here? This adds the proxy server used by a given job
to the net log of this Job. When 2 proxy jobs are racing (e.g., for data saver),
this was useful for determining which job is the H2 proxy job, and which one is
QUIC proxy job. 

[xunjieli, 2017/05/10 00:26:23]

I moved this to http_stream_factory_impl_job_controller.cc. Will that be enough?

-    const ProxyServer& proxy_server,
-    NetLogCaptureMode /* capture_mode */) {
-  std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
-
-  dict->SetString("proxy_server", proxy_server.is_valid()
-                                      ? proxy_server.ToPacString()
-                                      : std::string());
-  return std::move(dict);
-}
-
 HttpStreamFactoryImpl::Job::Job(Delegate* delegate,
                                 JobType job_type,
                                 HttpNetworkSession* session,
                                 const HttpRequestInfo& request_info,
                                 RequestPriority priority,
+                                ProxyInfo proxy_info,
                                 const SSLConfig& server_ssl_config,
                                 const SSLConfig& proxy_ssl_config,
                                 HostPortPair destination,
@@ -176,6 +165,7 @@ HttpStreamFactoryImpl::Job::Job(Delegate* delegate,
           session,
           request_info,
           priority,
+          proxy_info,
           server_ssl_config,
           proxy_ssl_config,
           destination,
@@ -190,6 +180,7 @@ HttpStreamFactoryImpl::Job::Job(Delegate* delegate,
                                 HttpNetworkSession* session,
                                 const HttpRequestInfo& request_info,
                                 RequestPriority priority,
+                                ProxyInfo proxy_info,
                                 const SSLConfig& server_ssl_config,
                                 const SSLConfig& proxy_ssl_config,
                                 HostPortPair destination,
@@ -200,6 +191,7 @@ HttpStreamFactoryImpl::Job::Job(Delegate* delegate,
                                 NetLog* net_log)
     : request_info_(request_info),
       priority_(priority),
+      proxy_info_(proxy_info),
       server_ssl_config_(server_ssl_config),
       proxy_ssl_config_(proxy_ssl_config),
       net_log_(
@@ -209,7 +201,6 @@ HttpStreamFactoryImpl::Job::Job(Delegate* delegate,
       session_(session),
       state_(STATE_NONE),
       next_state_(STATE_NONE),
-      pac_request_(NULL),
       destination_(destination),
       origin_url_(origin_url),
       alternative_service_(alternative_service),
@@ -221,6 +212,7 @@ HttpStreamFactoryImpl::Job::Job(Delegate* delegate,
                  origin_url_.SchemeIs(url::kWssScheme)),
       using_spdy_(false),
       using_quic_(false),
+      reconsider_proxy_(false),
       quic_request_(session_->quic_stream_factory(),
                     session_->http_server_properties()),
       using_existing_quic_session_(false),
@@ -270,9 +262,6 @@ HttpStreamFactoryImpl::Job::~Job() {
     connection_.reset();
   }
 
-  if (pac_request_)
-    session_->proxy_service()->CancelPacRequest(pac_request_);
-
   // The stream could be in a partial state.  It is not reusable.
   if (stream_.get() && next_state_ != STATE_DONE)
     stream_->Close(true /* not reusable */);
@@ -317,8 +306,6 @@ int HttpStreamFactoryImpl::Job::RestartTunnelWithProxyAuth() {
 
 LoadState HttpStreamFactoryImpl::Job::GetLoadState() const {
   switch (next_state_) {
-    case STATE_RESOLVE_PROXY_COMPLETE:
-      return session_->proxy_service()->GetLoadState(pac_request_);
     case STATE_INIT_CONNECTION_COMPLETE:
     case STATE_CREATE_STREAM_COMPLETE:
       return using_quic_ ? LOAD_STATE_CONNECTING : connection_->GetLoadState();
@@ -687,13 +674,6 @@ int HttpStreamFactoryImpl::Job::DoLoop(int result) {
         DCHECK_EQ(OK, rv);
         rv = DoStart();
         break;
-      case STATE_RESOLVE_PROXY:
-        DCHECK_EQ(OK, rv);
-        rv = DoResolveProxy();
-        break;
-      case STATE_RESOLVE_PROXY_COMPLETE:
-        rv = DoResolveProxyComplete(rv);
-        break;
       case STATE_WAIT:
         DCHECK_EQ(OK, rv);
         rv = DoWait();
@@ -760,75 +740,7 @@ int HttpStreamFactoryImpl::Job::DoStart() {
     return ERR_UNSAFE_PORT;
   }
 
-  next_state_ = STATE_RESOLVE_PROXY;
-  return OK;
-}
-
-int HttpStreamFactoryImpl::Job::DoResolveProxy() {
-  DCHECK(!pac_request_);
-  DCHECK(session_);
-
-  next_state_ = STATE_RESOLVE_PROXY_COMPLETE;
-
-  if (request_info_.load_flags & LOAD_BYPASS_PROXY) {
-    proxy_info_.UseDirect();
-    return OK;
-  }
-
-  // If an alternative proxy server was provided, use that.
-  if (alternative_proxy_server_.is_valid()) {
-    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_);
-}
-
-int HttpStreamFactoryImpl::Job::DoResolveProxyComplete(int result) {
-  pac_request_ = NULL;
-
-  net_log_.AddEvent(
-      NetLogEventType::HTTP_STREAM_JOB_PROXY_SERVER_RESOLVED,
-      base::Bind(
-          &NetLogHttpStreamJobProxyServerResolved,
-          proxy_info_.is_empty() ? ProxyServer() : proxy_info_.proxy_server()));
-
-  if (result == OK) {
-    // Remove unsupported proxies from the list.
-    int supported_proxies =
-        ProxyServer::SCHEME_DIRECT | ProxyServer::SCHEME_HTTP |
-        ProxyServer::SCHEME_HTTPS | ProxyServer::SCHEME_SOCKS4 |
-        ProxyServer::SCHEME_SOCKS5;
-
-    if (session_->IsQuicEnabled())
-      supported_proxies |= ProxyServer::SCHEME_QUIC;
-
-    proxy_info_.RemoveProxiesWithoutScheme(supported_proxies);
-
-    if (proxy_info_.is_empty()) {
-      // No proxies/direct to choose from. This happens when we don't support
-      // any of the proxies in the returned list.
-      result = ERR_NO_SUPPORTED_PROXIES;
-    } else if (using_quic_ &&
-               (!proxy_info_.is_quic() && !proxy_info_.is_direct())) {
-      // QUIC can not be spoken to non-QUIC proxies.  This error should not be
-      // user visible, because the non-alternative Job should be resumed.
-      result = ERR_NO_SUPPORTED_PROXIES;
-    }
-  }
-
-  if (result != OK) {
-    return result;
-  }
-
   next_state_ = STATE_WAIT;
-
-  delegate_->OnResolveProxyComplete(this, request_info_, priority_,
-                                    server_ssl_config_, proxy_ssl_config_,
-                                    stream_type_);
-
   return OK;
 }
 
@@ -874,6 +786,18 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionImpl() {
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "462812 HttpStreamFactoryImpl::Job::DoInitConnection"));
   DCHECK(!connection_->is_initialized());
+
+  if (proxy_info_.is_empty()) {
+    // No proxies/direct to choose from. This happens when we don't support
+    // any of the proxies in the returned list.
+    return ERR_NO_SUPPORTED_PROXIES;
+  } else if (using_quic_ &&

[Ryan Hamilton, 2017/05/09 19:22:06]

nit: no else after return.

[xunjieli, 2017/05/10 00:26:23]

Done.

+             (!proxy_info_.is_quic() && !proxy_info_.is_direct())) {
+    // QUIC can not be spoken to non-QUIC proxies.  This error should not be
+    // user visible, because the non-alternative Job should be resumed.
+    return ERR_NO_SUPPORTED_PROXIES;
+  }
+
   DCHECK(proxy_info_.proxy_server().is_valid());
   next_state_ = STATE_INIT_CONNECTION_COMPLETE;
 
@@ -1146,6 +1070,7 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionComplete(int result) {
 
   if (result < 0 && !ssl_started)
     return ReconsiderProxyAfterError(result);
+
   establishing_tunnel_ = false;
 
   // Handle SSL errors below.
@@ -1412,19 +1337,7 @@ void HttpStreamFactoryImpl::Job::InitSSLConfig(SSLConfig* ssl_config,
     ssl_config->channel_id_enabled = false;
 }
 
-
 int HttpStreamFactoryImpl::Job::ReconsiderProxyAfterError(int error) {
-  DCHECK(!pac_request_);
-  DCHECK(session_);
-
-  // A failure to resolve the hostname or any error related to establishing a
-  // TCP connection could be grounds for trying a new proxy configuration.
-  //
-  // Why do this when a hostname cannot be resolved?  Some URLs only make sense
-  // to proxy servers.  The hostname in those URLs might fail to resolve if we
-  // are still using a non-proxy config.  We need to check if a proxy config
-  // now exists that corresponds to a proxy server that could load the URL.
-  //
   switch (error) {
     case ERR_PROXY_CONNECTION_FAILED:
     case ERR_NAME_NOT_RESOLVED:
@@ -1463,46 +1376,16 @@ int HttpStreamFactoryImpl::Job::ReconsiderProxyAfterError(int error) {
       return error;
   }
 
-  // Do not bypass non-QUIC proxy on ERR_MSG_TOO_BIG.
-  if (!proxy_info_.is_quic() && error == ERR_MSG_TOO_BIG)
-    return 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()) {
+  if (job_type() == ALTERNATIVE && alternative_proxy_server().is_valid()) {

[tbansal1, 2017/05/09 20:38:48]

why is the check for job_type needed?

[xunjieli, 2017/05/10 00:26:23]

Done. Removed.

     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());
-  }
-
-  int rv = session_->proxy_service()->ReconsiderProxyAfterError(
-      request_info_.url, request_info_.method, error, &proxy_info_,
-      io_callback_, &pac_request_, session_->params().proxy_delegate, net_log_);
-  if (rv == OK || rv == ERR_IO_PENDING) {
-    // If the error was during connection setup, there is no socket to
-    // disconnect.
-    if (connection_->socket())
-      connection_->socket()->Disconnect();
-    connection_->Reset();
-    delegate_->RemoveRequestFromSpdySessionRequestMapForJob(this);
-    next_state_ = STATE_RESOLVE_PROXY_COMPLETE;
-  } else {
-    // If ReconsiderProxyAfterError() failed synchronously, it means
-    // there was nothing left to fall-back to, so fail the transaction
-    // with the last connection error we got.
-    // TODO(eroman): This is a confusing contract, make it more obvious.
-    rv = error;
-  }
-
-  return rv;
+  reconsider_proxy_ = true;
+  return error;
 }
 
 int HttpStreamFactoryImpl::Job::HandleCertificateError(int error) {
@@ -1564,4 +1447,68 @@ void HttpStreamFactoryImpl::Job::
   delegate_->AddConnectionAttemptsToRequest(this, socket_attempts);
 }
 
+HttpStreamFactoryImpl::JobFactory::JobFactory() {}
+
+HttpStreamFactoryImpl::JobFactory::~JobFactory() {}
+
+HttpStreamFactoryImpl::Job* HttpStreamFactoryImpl::JobFactory::CreateJob(
+    HttpStreamFactoryImpl::Job::Delegate* delegate,
+    HttpStreamFactoryImpl::JobType job_type,
+    HttpNetworkSession* session,
+    const HttpRequestInfo& request_info,
+    RequestPriority priority,
+    ProxyInfo proxy_info,
+    const SSLConfig& server_ssl_config,
+    const SSLConfig& proxy_ssl_config,
+    HostPortPair destination,
+    GURL origin_url,
+    bool enable_ip_based_pooling,
+    NetLog* net_log) {
+  return new HttpStreamFactoryImpl::Job(
+      delegate, job_type, session, request_info, priority, proxy_info,
+      server_ssl_config, proxy_ssl_config, destination, origin_url,
+      enable_ip_based_pooling, net_log);
+}
+
+HttpStreamFactoryImpl::Job* HttpStreamFactoryImpl::JobFactory::CreateJob(
+    HttpStreamFactoryImpl::Job::Delegate* delegate,
+    HttpStreamFactoryImpl::JobType job_type,
+    HttpNetworkSession* session,
+    const HttpRequestInfo& request_info,
+    RequestPriority priority,
+    ProxyInfo proxy_info,
+    const SSLConfig& server_ssl_config,
+    const SSLConfig& proxy_ssl_config,
+    HostPortPair destination,
+    GURL origin_url,
+    AlternativeService alternative_service,
+    bool enable_ip_based_pooling,
+    NetLog* net_log) {
+  return new HttpStreamFactoryImpl::Job(
+      delegate, job_type, session, request_info, priority, proxy_info,
+      server_ssl_config, proxy_ssl_config, destination, origin_url,
+      alternative_service, ProxyServer(), enable_ip_based_pooling, net_log);
+}
+
+HttpStreamFactoryImpl::Job* HttpStreamFactoryImpl::JobFactory::CreateJob(
+    HttpStreamFactoryImpl::Job::Delegate* delegate,
+    HttpStreamFactoryImpl::JobType job_type,
+    HttpNetworkSession* session,
+    const HttpRequestInfo& request_info,
+    RequestPriority priority,
+    ProxyInfo proxy_info,
+    const SSLConfig& server_ssl_config,
+    const SSLConfig& proxy_ssl_config,
+    HostPortPair destination,
+    GURL origin_url,
+    const ProxyServer& alternative_proxy_server,
+    bool enable_ip_based_pooling,
+    NetLog* net_log) {
+  return new HttpStreamFactoryImpl::Job(
+      delegate, job_type, session, request_info, priority, proxy_info,
+      server_ssl_config, proxy_ssl_config, destination, origin_url,
+      AlternativeService(), alternative_proxy_server, enable_ip_based_pooling,
+      net_log);
+}
+
 }  // namespace net