Allow at most one preconnect to HTTP2 proxy servers

If Chromium is using a HTTP2 proxy server, then the preconnect jobs
are skipped if a preconnect request is already pending.

This prevents Chromium from unnecessarily establishing multiple
preconnections to an HTTP2 proxy. The new behavior is guarded
behind a field trial so that the performance benefits can be
evaluated.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=667471

Review-Url: https://codereview.chromium.org/2522703002
Cr-Commit-Position: refs/heads/master@{#438179}

net/http/http_stream_factory_impl.cc

@@ -8,6 +8,8 @@
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/field_trial.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "net/http/http_network_session.h"
@@ -16,10 +18,12 @@
 #include "net/http/http_stream_factory_impl_job_controller.h"
 #include "net/http/http_stream_factory_impl_request.h"
 #include "net/http/transport_security_state.h"
+#include "net/proxy/proxy_info.h"
 #include "net/quic/core/quic_server_id.h"
 #include "net/spdy/bidirectional_stream_spdy_impl.h"
 #include "net/spdy/spdy_http_stream.h"
 #include "url/gurl.h"
+#include "url/scheme_host_port.h"
 #include "url/url_constants.h"
 
 namespace net {
@@ -84,13 +88,24 @@ class DefaultJobFactory : public HttpStreamFactoryImpl::JobFactory {
         alternative_proxy_server, net_log);
   }
 };
+
+// Returns true if only one preconnect to proxy servers is allowed via field
+// trial.
+bool AllowOnlyOnePreconnectToProxyServers() {
+  return base::StartsWith(base::FieldTrialList::FindFullName(
+                              "NetAllowOnlyOnePreconnectToProxyServers"),
+                          "Enabled", base::CompareCase::INSENSITIVE_ASCII);
+}
+
 }  // anonymous namespace
 
 HttpStreamFactoryImpl::HttpStreamFactoryImpl(HttpNetworkSession* session,
                                              bool for_websockets)
     : session_(session),
       job_factory_(new DefaultJobFactory()),
-      for_websockets_(for_websockets) {}
+      for_websockets_(for_websockets),
+      allow_only_one_preconnect_to_proxy_servers_(
+          AllowOnlyOnePreconnectToProxyServers()) {}
 
 HttpStreamFactoryImpl::~HttpStreamFactoryImpl() {
   DCHECK(request_map_.empty());
@@ -239,4 +254,63 @@ void HttpStreamFactoryImpl::OnJobControllerComplete(JobController* controller) {
   NOTREACHED();
 }
 
+bool HttpStreamFactoryImpl::OnInitConnection(const JobController& controller,
+                                             const ProxyInfo& proxy_info) {
+  if (!controller.is_preconnect()) {
+    // Connection initialization can be skipped only for the preconnect jobs.
+    return false;
+  }
+
+  if (!allow_only_one_preconnect_to_proxy_servers_ ||
+      !ProxyServerSupportsPriorities(proxy_info)) {
+    return false;
+  }
+
+  if (base::ContainsKey(preconnecting_proxy_servers_,
+                        proxy_info.proxy_server())) {
+    UMA_HISTOGRAM_EXACT_LINEAR("Net.PreconnectSkippedToProxyServers", 1, 2);
+    // Skip preconnect to the proxy server since we are already preconnecting
+    // (probably via some other job).
+    return true;
+  }
+
+  // Add the proxy server to the set of preconnecting proxy servers.
+  // The maximum size of |preconnecting_proxy_servers_|.
+  static const size_t kMaxPreconnectingServerSize = 3;
+  if (preconnecting_proxy_servers_.size() >= kMaxPreconnectingServerSize) {
+    // Erase the first entry. A better approach (at the cost of higher memory
+    // overhead) may be to erase the least recently used entry.
+    preconnecting_proxy_servers_.erase(preconnecting_proxy_servers_.begin());
+  }
+
+  preconnecting_proxy_servers_.insert(proxy_info.proxy_server());
+  DCHECK_GE(kMaxPreconnectingServerSize, preconnecting_proxy_servers_.size());
+  // The first preconnect should be allowed.
+  return false;
+}
+
+void HttpStreamFactoryImpl::OnStreamReady(const ProxyInfo& proxy_info) {
+  if (proxy_info.is_empty())
+    return;
+  preconnecting_proxy_servers_.erase(proxy_info.proxy_server());
+}
+
+bool HttpStreamFactoryImpl::ProxyServerSupportsPriorities(
+    const ProxyInfo& proxy_info) const {
+  if (proxy_info.is_empty() || !proxy_info.proxy_server().is_valid())
+    return false;
+
+  if (!proxy_info.proxy_server().is_https())
+    return false;
+
+  HostPortPair host_port_pair = proxy_info.proxy_server().host_port_pair();
+  DCHECK(!host_port_pair.IsEmpty());
+
+  url::SchemeHostPort scheme_host_port("https", host_port_pair.host(),
+                                       host_port_pair.port());
+
+  return session_->http_server_properties()->SupportsRequestPriority(
+      scheme_host_port);
+}
+
 }  // namespace net

net/http/http_stream_factory_impl.h

@@ -25,6 +25,7 @@
 namespace net {
 
 class HttpNetworkSession;
+class ProxyInfo;
 class SpdySession;
 class NetLogWithSource;
 
@@ -129,6 +130,19 @@ class NET_EXPORT_PRIVATE HttpStreamFactoryImpl : public HttpStreamFactory {
   // from |job_controller_set_|.
   void OnJobControllerComplete(JobController* controller);
 
+  // Returns true if a connection to the proxy server contained in |proxy_info|
+  // can be skipped by a job controlled by |controller|.
+  bool OnInitConnection(const JobController& controller,
+                        const ProxyInfo& proxy_info);
+
+  // Notifies |this| that a stream to the proxy server contained in |proxy_info|
+  // is ready.
+  void OnStreamReady(const ProxyInfo& proxy_info);
+
+  // Returns true if |proxy_info| contains a proxy server that supports request
+  // priorities.
+  bool ProxyServerSupportsPriorities(const ProxyInfo& proxy_info) const;
+
   HttpNetworkSession* const session_;
 
   // All Requests are handed out to clients. By the time HttpStreamFactoryImpl
@@ -146,9 +160,18 @@ class NET_EXPORT_PRIVATE HttpStreamFactoryImpl : public HttpStreamFactory {
   // Factory used by job controllers for creating jobs.
   std::unique_ptr<JobFactory> job_factory_;
 
+  // Set of proxy servers that support request priorities to which subsequent
+  // preconnects should be skipped.
+  std::set<ProxyServer> preconnecting_proxy_servers_;
+
   SpdySessionRequestMap spdy_session_request_map_;
 
   const bool for_websockets_;
+
+  // True if only one preconnect is allowed to proxy servers that support
+  // request priorities.
+  const bool allow_only_one_preconnect_to_proxy_servers_;
+
   DISALLOW_COPY_AND_ASSIGN(HttpStreamFactoryImpl);
 };
 

net/http/http_stream_factory_impl_job.cc

@@ -476,6 +476,8 @@ void HttpStreamFactoryImpl::Job::OnHttpsProxyTunnelResponseCallback(
 }
 
 void HttpStreamFactoryImpl::Job::OnPreconnectsComplete() {
+  DCHECK(!new_spdy_session_);
+
   if (new_spdy_session_.get()) {
     delegate_->OnNewSpdySessionReady(this, new_spdy_session_,
                                      spdy_session_direct_);
@@ -805,6 +807,11 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionImpl() {
   DCHECK(proxy_info_.proxy_server().is_valid());
   next_state_ = STATE_INIT_CONNECTION_COMPLETE;
 
+  if (delegate_->OnInitConnection(proxy_info_)) {
+    // Return since the connection initialization can be skipped.
+    return OK;
+  }
+
   using_ssl_ = origin_url_.SchemeIs(url::kHttpsScheme) ||
                origin_url_.SchemeIs(url::kWssScheme);
   using_spdy_ = false;

net/http/http_stream_factory_impl_job.h

@@ -100,6 +100,10 @@ class HttpStreamFactoryImpl::Job {
                                   const ProxyInfo& used_proxy_info,
                                   HttpAuthController* auth_controller) = 0;
 
+    // Returns true if the connection initialization to the proxy server
+    // contained in |proxy_info| can be skipped.
+    virtual bool OnInitConnection(const ProxyInfo& proxy_info) = 0;
+
     // Invoked when |job| has completed proxy resolution. The delegate may
     // create an alternative proxy server job to fetch the request.
     virtual void OnResolveProxyComplete(

net/http/http_stream_factory_impl_job_controller.cc

@@ -174,6 +174,11 @@ void HttpStreamFactoryImpl::JobController::OnStreamReady(
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info) {
   DCHECK(job);
+  // TODO(tbansal): Remove |used_proxy_info| from the method arguments.
+  DCHECK(job->proxy_info().is_empty() == used_proxy_info.is_empty() ||
+         job->proxy_info().proxy_server() == used_proxy_info.proxy_server());
+
+  factory_->OnStreamReady(job->proxy_info());
 
   if (job_bound_ && bound_job_ != job) {
     // We have bound a job to the associated Request, |job| has been orphaned.
@@ -367,6 +372,11 @@ void HttpStreamFactoryImpl::JobController::OnNeedsProxyAuth(
                              auth_controller);
 }
 
+bool HttpStreamFactoryImpl::JobController::OnInitConnection(
+    const ProxyInfo& proxy_info) {
+  return factory_->OnInitConnection(*this, proxy_info);
+}
+
 void HttpStreamFactoryImpl::JobController::OnResolveProxyComplete(
     Job* job,
     const HttpRequestInfo& request_info,
@@ -413,6 +423,7 @@ void HttpStreamFactoryImpl::JobController::OnNewSpdySessionReady(
     bool direct) {
   DCHECK(job);
   DCHECK(job->using_spdy());
+  DCHECK(!is_preconnect_);
 
   bool is_job_orphaned = job_bound_ && bound_job_ != job;
 

net/http/http_stream_factory_impl_job_controller.h

@@ -117,6 +117,8 @@ class HttpStreamFactoryImpl::JobController
                         const ProxyInfo& used_proxy_info,
                         HttpAuthController* auth_controller) override;
 
+  bool OnInitConnection(const ProxyInfo& proxy_info) override;
+
   void OnResolveProxyComplete(
       Job* job,
       const HttpRequestInfo& request_info,
@@ -167,6 +169,8 @@ class HttpStreamFactoryImpl::JobController
   WebSocketHandshakeStreamBase::CreateHelper*
   websocket_handshake_stream_create_helper() override;
 
+  bool is_preconnect() const { return is_preconnect_; }
+
  private:
   friend class JobControllerPeer;
 

net/http/http_stream_factory_impl_job_controller_unittest.cc

@@ -936,7 +936,7 @@ TEST_F(HttpStreamFactoryImplJobControllerTest,
   EXPECT_CALL(request_delegate_, OnStreamReady(_, _, http_stream))
       .WillOnce(Invoke(DeleteHttpStreamPointer));
   job_controller_->OnStreamReady(job_factory_.main_job(), SSLConfig(),
-                                 ProxyInfo());
+                                 job_factory_.main_job()->proxy_info());
 
   // JobController shouldn't report the status of alternative server job as
   // request is already successfully served.