Allow a preconnecting job to a HTTP2 proxy server based on privacy mode

tbansal · Commit bot · commit 37e33d58c076 · 2017-01-23T17:02:15.000Z
Currently, one preconnect job is allowed in-flight for one proxy server. This CL changes it to allow one in-flight preconnect job for one (proxy server, privacy mode) tuple. This is needed since requests that use different privacy modes cannot share the same connection. BUG=667471 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2648593002 Cr-Commit-Position: refs/heads/master@{#445392}
diff --git a/net/http/http_stream_factory_impl.cc b/net/http/http_stream_factory_impl.cc
@@ -5,6 +5,7 @@
 #include "net/http/http_stream_factory_impl.h"
 
 #include <string>
+#include <tuple>
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
@@ -247,8 +248,26 @@ void HttpStreamFactoryImpl::OnJobControllerComplete(JobController* controller) {
   NOTREACHED();
 }
 
+HttpStreamFactoryImpl::PreconnectingProxyServer::PreconnectingProxyServer(
+    ProxyServer proxy_server,
+    PrivacyMode privacy_mode)
+    : proxy_server(proxy_server), privacy_mode(privacy_mode) {}
+
+bool HttpStreamFactoryImpl::PreconnectingProxyServer::operator<(
+    const PreconnectingProxyServer& other) const {
+  return std::tie(proxy_server, privacy_mode) <
+         std::tie(other.proxy_server, other.privacy_mode);
+}
+
+bool HttpStreamFactoryImpl::PreconnectingProxyServer::operator==(
+    const PreconnectingProxyServer& other) const {
+  return proxy_server == other.proxy_server &&
+         privacy_mode == other.privacy_mode;
+}
+
 bool HttpStreamFactoryImpl::OnInitConnection(const JobController& controller,
-                                             const ProxyInfo& proxy_info) {
+                                             const ProxyInfo& proxy_info,
+                                             PrivacyMode privacy_mode) {
   if (!controller.is_preconnect()) {
     // Connection initialization can be skipped only for the preconnect jobs.
     return false;
@@ -259,8 +278,11 @@ bool HttpStreamFactoryImpl::OnInitConnection(const JobController& controller,
     return false;
   }
 
+  PreconnectingProxyServer preconnecting_proxy_server(proxy_info.proxy_server(),
+                                                      privacy_mode);
+
   if (base::ContainsKey(preconnecting_proxy_servers_,
-                        proxy_info.proxy_server())) {
+                        preconnecting_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).
@@ -276,16 +298,18 @@ bool HttpStreamFactoryImpl::OnInitConnection(const JobController& controller,
     preconnecting_proxy_servers_.erase(preconnecting_proxy_servers_.begin());
   }
 
-  preconnecting_proxy_servers_.insert(proxy_info.proxy_server());
+  preconnecting_proxy_servers_.insert(preconnecting_proxy_server);
   DCHECK_GE(kMaxPreconnectingServerSize, preconnecting_proxy_servers_.size());
   // The first preconnect should be allowed.
   return false;
 }
 
-void HttpStreamFactoryImpl::OnStreamReady(const ProxyInfo& proxy_info) {
+void HttpStreamFactoryImpl::OnStreamReady(const ProxyInfo& proxy_info,
+                                          PrivacyMode privacy_mode) {
   if (proxy_info.is_empty())
     return;
-  preconnecting_proxy_servers_.erase(proxy_info.proxy_server());
+  preconnecting_proxy_servers_.erase(
+      PreconnectingProxyServer(proxy_info.proxy_server(), privacy_mode));
 }
 
 bool HttpStreamFactoryImpl::ProxyServerSupportsPriorities(
diff --git a/net/http/http_stream_factory_impl.h b/net/http/http_stream_factory_impl.h
@@ -16,6 +16,7 @@
 #include "base/memory/ref_counted.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/net_export.h"
+#include "net/base/privacy_mode.h"
 #include "net/base/request_priority.h"
 #include "net/http/http_stream_factory.h"
 #include "net/proxy/proxy_server.h"
@@ -87,6 +88,20 @@ class NET_EXPORT_PRIVATE HttpStreamFactoryImpl : public HttpStreamFactory {
   typedef std::map<SpdySessionKey, RequestSet> SpdySessionRequestMap;
   typedef std::set<std::unique_ptr<JobController>> JobControllerSet;
 
+  // |PreconnectingProxyServer| holds information of a connection to a single
+  // proxy server.
+  struct PreconnectingProxyServer {
+    PreconnectingProxyServer(ProxyServer proxy_server,
+                             PrivacyMode privacy_mode);
+
+    // Needed to be an element of std::set.
+    bool operator<(const PreconnectingProxyServer& other) const;
+    bool operator==(const PreconnectingProxyServer& other) const;
+
+    const ProxyServer proxy_server;
+    const PrivacyMode privacy_mode;
+  };
+
   // Values must not be changed or reused.  Keep in sync with identically named
   // enum in histograms.xml.
   enum AlternativeServiceType {
@@ -133,13 +148,15 @@ class NET_EXPORT_PRIVATE HttpStreamFactoryImpl : public HttpStreamFactory {
   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|.
+  // that has privacy mode |privacy_mode| can be skipped by a job controlled by
+  // |controller|.
   bool OnInitConnection(const JobController& controller,
-                        const ProxyInfo& proxy_info);
+                        const ProxyInfo& proxy_info,
+                        PrivacyMode privacy_mode);
 
   // Notifies |this| that a stream to the proxy server contained in |proxy_info|
-  // is ready.
-  void OnStreamReady(const ProxyInfo& proxy_info);
+  // with privacy mode |privacy_mode| is ready.
+  void OnStreamReady(const ProxyInfo& proxy_info, PrivacyMode privacy_mode);
 
   // Returns true if |proxy_info| contains a proxy server that supports request
   // priorities.
@@ -164,7 +181,7 @@ class NET_EXPORT_PRIVATE HttpStreamFactoryImpl : public HttpStreamFactory {
 
   // Set of proxy servers that support request priorities to which subsequent
   // preconnects should be skipped.
-  std::set<ProxyServer> preconnecting_proxy_servers_;
+  std::set<PreconnectingProxyServer> preconnecting_proxy_servers_;
 
   SpdySessionRequestMap spdy_session_request_map_;
 
diff --git a/net/http/http_stream_factory_impl_job_controller.cc b/net/http/http_stream_factory_impl_job_controller.cc
@@ -52,6 +52,7 @@ HttpStreamFactoryImpl::JobController::JobController(
       main_job_is_blocked_(false),
       bound_job_(nullptr),
       can_start_alternative_proxy_job_(false),
+      privacy_mode_(PRIVACY_MODE_DISABLED),
       ptr_factory_(this) {
   DCHECK(factory);
 }
@@ -79,6 +80,8 @@ HttpStreamFactoryImpl::Request* HttpStreamFactoryImpl::JobController::Start(
   DCHECK(factory_);
   DCHECK(!request_);
 
+  privacy_mode_ = request_info.privacy_mode;
+
   request_ = new Request(request_info.url, this, delegate,
                          websocket_handshake_stream_create_helper, net_log,
                          stream_type);
@@ -97,6 +100,8 @@ void HttpStreamFactoryImpl::JobController::Preconnect(
   DCHECK(!main_job_);
   DCHECK(!alternative_job_);
 
+  privacy_mode_ = request_info.privacy_mode;
+
   is_preconnect_ = true;
   HostPortPair destination(HostPortPair::FromURL(request_info.url));
   GURL origin_url = ApplyHostMappingRules(request_info.url, &destination);
@@ -179,7 +184,7 @@ void HttpStreamFactoryImpl::JobController::OnStreamReady(
     const SSLConfig& used_ssl_config) {
   DCHECK(job);
 
-  factory_->OnStreamReady(job->proxy_info());
+  factory_->OnStreamReady(job->proxy_info(), privacy_mode_);
 
   if (IsJobOrphaned(job)) {
     // We have bound a job to the associated Request, |job| has been orphaned.
@@ -375,7 +380,7 @@ void HttpStreamFactoryImpl::JobController::OnNeedsProxyAuth(
 
 bool HttpStreamFactoryImpl::JobController::OnInitConnection(
     const ProxyInfo& proxy_info) {
-  return factory_->OnInitConnection(*this, proxy_info);
+  return factory_->OnInitConnection(*this, proxy_info, privacy_mode_);
 }
 
 void HttpStreamFactoryImpl::JobController::OnResolveProxyComplete(
diff --git a/net/http/http_stream_factory_impl_job_controller.h b/net/http/http_stream_factory_impl_job_controller.h
@@ -6,6 +6,7 @@
 #define NET_HTTP_HTTP_STREAM_FACTORY_IMPL_JOB_CONTROLLER_H_
 
 #include "net/base/host_port_pair.h"
+#include "net/base/privacy_mode.h"
 #include "net/http/http_stream_factory_impl_job.h"
 #include "net/http/http_stream_factory_impl_request.h"
 
@@ -311,6 +312,9 @@ class HttpStreamFactoryImpl::JobController
   // True if an alternative proxy server job can be started to fetch |request_|.
   bool can_start_alternative_proxy_job_;
 
+  // Privacy mode that should be used for fetching the resource.
+  PrivacyMode privacy_mode_;
+
   base::WeakPtrFactory<JobController> ptr_factory_;
 };
 
diff --git a/net/http/http_stream_factory_impl_unittest.cc b/net/http/http_stream_factory_impl_unittest.cc
@@ -17,6 +17,7 @@
 #include "base/run_loop.h"
 #include "base/test/histogram_tester.h"
 #include "net/base/port_util.h"
+#include "net/base/privacy_mode.h"
 #include "net/base/test_completion_callback.h"
 #include "net/base/test_proxy_delegate.h"
 #include "net/cert/ct_policy_enforcer.h"
@@ -388,6 +389,9 @@ class CapturePreconnectsSocketPool : public ParentPool {
     return last_num_streams_;
   }
 
+  // Resets |last_num_streams_| to its default value.
+  void reset_last_num_streams() { last_num_streams_ = -1; }
+
   int RequestSocket(const std::string& group_name,
                     const void* socket_params,
                     RequestPriority priority,
@@ -1309,6 +1313,87 @@ TEST_F(HttpStreamFactoryTest, OnlyOnePreconnectToProxyServer) {
   }
 }
 
+// Verify that preconnect to a HTTP2 proxy server with a privacy mode different
+// than that of the in-flight preconnect job succeeds.
+TEST_F(HttpStreamFactoryTest, ProxyServerPreconnectDifferentPrivacyModes) {
+  int num_streams = 1;
+  base::HistogramTester histogram_tester;
+  GURL url = GURL("http://www.google.com");
+  std::unique_ptr<ProxyService> proxy_service =
+      ProxyService::CreateFixedFromPacResult("HTTPS myproxy.org:443");
+
+  // Set up the proxy server as a server that supports request priorities.
+  HttpServerPropertiesImpl http_server_properties;
+
+  url::SchemeHostPort spdy_server("https", "myproxy.org", 443);
+  http_server_properties.SetSupportsSpdy(spdy_server, true);
+
+  SpdySessionDependencies session_deps;
+  HttpNetworkSession::Params params =
+      SpdySessionDependencies::CreateSessionParams(&session_deps);
+  params.enable_quic = true;
+  params.proxy_service = proxy_service.get();
+  params.http_server_properties = &http_server_properties;
+  ASSERT_TRUE(params.restrict_to_one_preconnect_for_proxies);
+
+  std::unique_ptr<HttpNetworkSession> session(new HttpNetworkSession(params));
+
+  HttpNetworkSessionPeer peer(session.get());
+  HostPortPair proxy_host("myproxy.org", 443);
+
+  CapturePreconnectsHttpProxySocketPool* http_proxy_pool =
+      new CapturePreconnectsHttpProxySocketPool(
+          session_deps.host_resolver.get(), session_deps.cert_verifier.get(),
+          session_deps.transport_security_state.get(),
+          session_deps.cert_transparency_verifier.get(),
+          session_deps.ct_policy_enforcer.get());
+  CapturePreconnectsSSLSocketPool* ssl_conn_pool =
+      new CapturePreconnectsSSLSocketPool(
+          session_deps.host_resolver.get(), session_deps.cert_verifier.get(),
+          session_deps.transport_security_state.get(),
+          session_deps.cert_transparency_verifier.get(),
+          session_deps.ct_policy_enforcer.get());
+
+  std::unique_ptr<MockClientSocketPoolManager> mock_pool_manager(
+      new MockClientSocketPoolManager);
+  mock_pool_manager->SetSocketPoolForHTTPProxy(
+      proxy_host, base::WrapUnique(http_proxy_pool));
+  mock_pool_manager->SetSocketPoolForSSLWithProxy(
+      proxy_host, base::WrapUnique(ssl_conn_pool));
+  peer.SetClientSocketPoolManager(std::move(mock_pool_manager));
+
+  HttpRequestInfo request_privacy_mode_disabled;
+  request_privacy_mode_disabled.method = "GET";
+  request_privacy_mode_disabled.url = url;
+  request_privacy_mode_disabled.load_flags = 0;
+
+  // First preconnect job should succeed.
+  session->http_stream_factory()->PreconnectStreams(
+      num_streams, request_privacy_mode_disabled);
+  EXPECT_EQ(-1, ssl_conn_pool->last_num_streams());
+  EXPECT_EQ(num_streams, http_proxy_pool->last_num_streams());
+  http_proxy_pool->reset_last_num_streams();
+
+  // Second preconnect job with same privacy mode should not succeed.
+  session->http_stream_factory()->PreconnectStreams(
+      num_streams + 1, request_privacy_mode_disabled);
+  EXPECT_EQ(-1, ssl_conn_pool->last_num_streams());
+  EXPECT_EQ(-1, http_proxy_pool->last_num_streams());
+
+  // Next request with a different privacy mode should succeed.
+  HttpRequestInfo request_privacy_mode_enabled;
+  request_privacy_mode_enabled.method = "GET";
+  request_privacy_mode_enabled.url = url;
+  request_privacy_mode_enabled.load_flags = 0;
+  request_privacy_mode_enabled.privacy_mode = PRIVACY_MODE_ENABLED;
+
+  // Request with a different privacy mode should succeed.
+  session->http_stream_factory()->PreconnectStreams(
+      num_streams, request_privacy_mode_enabled);
+  EXPECT_EQ(-1, ssl_conn_pool->last_num_streams());
+  EXPECT_EQ(num_streams, http_proxy_pool->last_num_streams());
+}
+
 namespace {
 
 TEST_F(HttpStreamFactoryTest, PrivacyModeDisablesChannelId) {
