Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(230)

Unified Diff: net/http/http_stream_factory_impl_job.cc

Issue 2814633003: Extract Proxy Resolution out of HttpStreamFactoryImpl::Job (Closed)
Patch Set: fix alt proxy tests Created 3 years, 7 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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..2077418dceb9af979711f9ac8bf53cf6e75da594 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(
- 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,
+ const 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,
+ const 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),
+ should_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;
+ }
+ 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.
+ 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 (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());
- }
-
- 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;
+ should_reconsider_proxy_ = true;
+ return error;
}
int HttpStreamFactoryImpl::Job::HandleCertificateError(int error) {
@@ -1564,4 +1447,69 @@ void HttpStreamFactoryImpl::Job::
delegate_->AddConnectionAttemptsToRequest(this, socket_attempts);
}
+HttpStreamFactoryImpl::JobFactory::JobFactory() {}
+
+HttpStreamFactoryImpl::JobFactory::~JobFactory() {}
+
+HttpStreamFactoryImpl::Job* HttpStreamFactoryImpl::JobFactory::CreateMainJob(
+ 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::CreateAltJob(
+ 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::CreateAltProxyJob(
+ 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

Powered by Google App Engine
This is Rietveld 408576698