Integrate IsolatedPrerender with DRP config updates

Plumbs DRP prefetch proxy hosts to where they will be used in
IsolatedPrerender.

Bug: 1042829
Change-Id: Id531927075470b37f79af4d7cf42b0352fe785ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2090147Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: Robert Ogden <robertogden@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748749}

chrome/browser/prerender/isolated/isolated_prerender_features.cc

@@ -15,10 +15,6 @@ const base::Feature kIsolatePrerenders{"IsolatePrerenders",
 const base::Feature kIsolatePrerendersMustProbeOrigin{
     "IsolatePrerendersMustProbeOrigin", base::FEATURE_DISABLED_BY_DEFAULT};
 
-// Forces all isolated prerenders to be proxied through a CONNECT tunnel.
-const base::Feature kIsolatedPrerenderUsesProxy{
-    "IsolatedPrerenderUsesProxy", base::FEATURE_DISABLED_BY_DEFAULT};
-
 // Prefetches main frame HTML resources for results on Google SRPs.
 const base::Feature kPrefetchSRPNavigationPredictions_HTMLOnly{
     "PrefetchSRPNavigationPredictions_HTMLOnly",

chrome/browser/prerender/isolated/isolated_prerender_features.h

@@ -11,7 +11,6 @@ namespace features {
 
 extern const base::Feature kIsolatePrerenders;
 extern const base::Feature kIsolatePrerendersMustProbeOrigin;
-extern const base::Feature kIsolatedPrerenderUsesProxy;
 extern const base::Feature kPrefetchSRPNavigationPredictions_HTMLOnly;
 
 }  // namespace features

chrome/browser/prerender/isolated/isolated_prerender_params.cc

@@ -16,22 +16,10 @@ bool IsolatedPrerenderIsEnabled() {
   return base::FeatureList::IsEnabled(features::kIsolatePrerenders);
 }
 
-base::Optional<GURL> IsolatedPrerenderProxyServer() {
-  if (!base::FeatureList::IsEnabled(features::kIsolatedPrerenderUsesProxy))
-    return base::nullopt;
-
-  GURL url(base::GetFieldTrialParamValueByFeature(
-      features::kIsolatedPrerenderUsesProxy, "proxy_server_url"));
-  if (!url.is_valid() || !url.has_host() || !url.has_scheme())
-    return base::nullopt;
-  return url;
-}
-
 bool IsolatedPrerenderShouldReplaceDataReductionCustomProxy() {
   bool replace =
       data_reduction_proxy::params::IsIncludedInHoldbackFieldTrial() &&
-      IsolatedPrerenderIsEnabled() &&
-      IsolatedPrerenderProxyServer().has_value();
+      IsolatedPrerenderIsEnabled();
   // TODO(robertogden): Remove this once all pieces are landed.
   DCHECK(!replace);
   return replace;

chrome/browser/prerender/isolated/isolated_prerender_params.h

@@ -13,9 +13,6 @@
 // Returns true if the Isolated Prerender feature is enabled.
 bool IsolatedPrerenderIsEnabled();
 
-// Returns the URL of the proxy server to use in isolated prerenders, if any.
-base::Optional<GURL> IsolatedPrerenderProxyServer();
-
 // Returns true if the proxy for Isolated Prerenders should replace the DRP
 // custom proxy.
 bool IsolatedPrerenderShouldReplaceDataReductionCustomProxy();

chrome/browser/prerender/isolated/isolated_prerender_proxy_configurator.cc

@@ -22,6 +22,13 @@ void IsolatedPrerenderProxyConfigurator::UpdateTunnelHeaders(
   UpdateCustomProxyConfig();
 }
 
+void IsolatedPrerenderProxyConfigurator::UpdateProxyHosts(
+    const std::vector<GURL>& proxy_hosts) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  proxy_hosts_ = proxy_hosts;
+  UpdateCustomProxyConfig();
+}
+
 void IsolatedPrerenderProxyConfigurator::AddCustomProxyConfigClient(
     mojo::Remote<network::mojom::CustomProxyConfigClient> config_client) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -39,18 +46,28 @@ void IsolatedPrerenderProxyConfigurator::UpdateCustomProxyConfig() {
   if (!data_reduction_proxy::params::IsIncludedInHoldbackFieldTrial())
     return;
 
-  if (!IsolatedPrerenderProxyServer().has_value())
+  if (!IsolatedPrerenderIsEnabled())
+    return;
+
+  // If a proxy config has never been sent, and there's no hosts to send, don't
+  // bother.
+  if (proxy_hosts_.empty() && !sent_proxy_update_)
     return;
 
   network::mojom::CustomProxyConfigPtr config = CreateCustomProxyConfig();
   for (auto& client : proxy_config_clients_) {
     client->OnCustomProxyConfigUpdated(config->Clone());
   }
+
+  if (!proxy_hosts_.empty()) {
+    sent_proxy_update_ = true;
+  }
 }
 
 network::mojom::CustomProxyConfigPtr
 IsolatedPrerenderProxyConfigurator::CreateCustomProxyConfig() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   net::ProxyConfig::ProxyRules rules;
   DCHECK(rules.proxies_for_http.IsEmpty());
   DCHECK(rules.proxies_for_https.IsEmpty());
@@ -60,9 +77,11 @@ IsolatedPrerenderProxyConfigurator::CreateCustomProxyConfig() const {
       net::ProxyConfig::ProxyRules::Type::PROXY_LIST_PER_SCHEME;
   // DIRECT is intentionally not added here because we want the proxy to always
   // be used in order to mask the user's IP address during the prerender.
-  config->rules.proxies_for_https.AddProxyServer(net::ProxyServer(
-      net::ProxyServer::SCHEME_HTTPS,
-      net::HostPortPair::FromURL(IsolatedPrerenderProxyServer().value())));
+  for (const GURL& host : proxy_hosts_) {
+    DCHECK(host.is_valid());
+    config->rules.proxies_for_https.AddProxyServer(net::ProxyServer(
+        net::ProxyServer::SCHEME_HTTPS, net::HostPortPair::FromURL(host)));
+  }
   // This ensures that the user's set proxy is honored, although we also disable
   // the feature is such cases.
   config->should_override_existing_config = false;

chrome/browser/prerender/isolated/isolated_prerender_proxy_configurator.h

@@ -5,12 +5,15 @@
 #ifndef CHROME_BROWSER_PRERENDER_ISOLATED_ISOLATED_PRERENDER_PROXY_CONFIGURATOR_H_
 #define CHROME_BROWSER_PRERENDER_ISOLATED_ISOLATED_PRERENDER_PROXY_CONFIGURATOR_H_
 
+#include <vector>
+
 #include "base/sequence_checker.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "mojo/public/cpp/bindings/remote_set.h"
 #include "net/http/http_request_headers.h"
 #include "services/network/public/mojom/network_context.mojom.h"
+#include "url/gurl.h"
 
 // Configures the use of the IP-masking CONNECT tunnel proxy for Isolated
 // Prerenders.
@@ -19,9 +22,13 @@ class IsolatedPrerenderProxyConfigurator {
   IsolatedPrerenderProxyConfigurator();
   ~IsolatedPrerenderProxyConfigurator();
 
+  // Updates the headers needed to connect to the proxy.
   void UpdateTunnelHeaders(
       const net::HttpRequestHeaders& connect_tunnel_headers);
 
+  // Updates the hosts used to connect to the proxy.
+  void UpdateProxyHosts(const std::vector<GURL>& proxy_hosts);
+
   // Adds a config client that can be used to update Data Reduction Proxy
   // settings.
   void AddCustomProxyConfigClient(
@@ -38,6 +45,13 @@ class IsolatedPrerenderProxyConfigurator {
   // The headers used to setup the connect tunnel.
   net::HttpRequestHeaders connect_tunnel_headers_;
 
+  // The configured proxy hosts.
+  std::vector<GURL> proxy_hosts_;
+
+  // Set to true when a custom proxy config with a non-empty set of proxies is
+  // sent to the Network Service.
+  bool sent_proxy_update_ = false;
+
   // The set of clients that will get updates about changes to the proxy config.
   mojo::RemoteSet<network::mojom::CustomProxyConfigClient>
       proxy_config_clients_;

chrome/browser/prerender/isolated/isolated_prerender_proxy_configurator_unittest.cc

@@ -65,7 +65,8 @@ class IsolatedPrerenderProxyConfiguratorTest : public testing::Test {
   }
 
   void VerifyLatestProxyConfig(const GURL& proxy_url,
-                               const net::HttpRequestHeaders& headers) {
+                               const net::HttpRequestHeaders& headers,
+                               bool want_empty = false) {
     auto config = LatestProxyConfig();
     ASSERT_TRUE(config);
 
@@ -83,8 +84,12 @@ class IsolatedPrerenderProxyConfiguratorTest : public testing::Test {
     EXPECT_EQ(config->rules.proxies_for_http.size(), 0U);
     EXPECT_EQ(config->rules.proxies_for_ftp.size(), 0U);
 
-    ASSERT_EQ(config->rules.proxies_for_https.size(), 1U);
-    EXPECT_EQ(GURL(config->rules.proxies_for_https.Get().ToURI()), proxy_url);
+    if (want_empty) {
+      EXPECT_EQ(config->rules.proxies_for_https.size(), 0U);
+    } else {
+      ASSERT_EQ(config->rules.proxies_for_https.size(), 1U);
+      EXPECT_EQ(GURL(config->rules.proxies_for_https.Get().ToURI()), proxy_url);
+    }
   }
 
   IsolatedPrerenderProxyConfigurator* configurator() {
@@ -97,10 +102,10 @@ class IsolatedPrerenderProxyConfiguratorTest : public testing::Test {
   std::unique_ptr<TestCustomProxyConfigClient> config_client_;
 };
 
-TEST_F(IsolatedPrerenderProxyConfiguratorTest, BothFeaturesOff) {
+TEST_F(IsolatedPrerenderProxyConfiguratorTest, FeaturesOff) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitWithFeatures(
-      {}, {features::kIsolatedPrerenderUsesProxy,
+      {}, {features::kIsolatePrerenders,
            data_reduction_proxy::features::kDataReductionProxyHoldback});
 
   configurator()->UpdateCustomProxyConfig();
@@ -111,7 +116,7 @@ TEST_F(IsolatedPrerenderProxyConfiguratorTest, BothFeaturesOff) {
 TEST_F(IsolatedPrerenderProxyConfiguratorTest, DRPFeatureOff) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitWithFeatures(
-      {features::kIsolatedPrerenderUsesProxy},
+      {features::kIsolatePrerenders},
       {data_reduction_proxy::features::kDataReductionProxyHoldback});
 
   configurator()->UpdateCustomProxyConfig();
@@ -120,11 +125,11 @@ TEST_F(IsolatedPrerenderProxyConfiguratorTest, DRPFeatureOff) {
   EXPECT_FALSE(LatestProxyConfig());
 }
 
-TEST_F(IsolatedPrerenderProxyConfiguratorTest, ProxyFeatureOff) {
+TEST_F(IsolatedPrerenderProxyConfiguratorTest, PrefetchFeatureOff) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitWithFeatures(
       {data_reduction_proxy::features::kDataReductionProxyHoldback},
-      {features::kIsolatedPrerenderUsesProxy});
+      {features::kIsolatePrerenders});
 
   configurator()->UpdateCustomProxyConfig();
   base::RunLoop().RunUntilIdle();
@@ -132,64 +137,48 @@ TEST_F(IsolatedPrerenderProxyConfiguratorTest, ProxyFeatureOff) {
   EXPECT_FALSE(LatestProxyConfig());
 }
 
-TEST_F(IsolatedPrerenderProxyConfiguratorTest, NoProxyServer) {
-  base::test::ScopedFeatureList drp_scoped_feature_list;
-  drp_scoped_feature_list.InitAndEnableFeature(
-      data_reduction_proxy::features::kDataReductionProxyHoldback);
-
-  base::test::ScopedFeatureList proxy_scoped_feature_list;
-  proxy_scoped_feature_list.InitAndEnableFeature(
-      features::kIsolatedPrerenderUsesProxy);
+TEST_F(IsolatedPrerenderProxyConfiguratorTest, NoProxyServers) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitWithFeatures(
+      {features::kIsolatePrerenders,
+       data_reduction_proxy::features::kDataReductionProxyHoldback},
+      {});
 
-  configurator()->UpdateCustomProxyConfig();
+  configurator()->UpdateProxyHosts({});
   base::RunLoop().RunUntilIdle();
 
   EXPECT_FALSE(LatestProxyConfig());
 }
 
-TEST_F(IsolatedPrerenderProxyConfiguratorTest, InvalidProxyServerURL_NoScheme) {
-  base::test::ScopedFeatureList drp_scoped_feature_list;
-  drp_scoped_feature_list.InitAndEnableFeature(
-      data_reduction_proxy::features::kDataReductionProxyHoldback);
-
-  base::test::ScopedFeatureList proxy_scoped_feature_list;
-  proxy_scoped_feature_list.InitAndEnableFeatureWithParameters(
-      features::kIsolatedPrerenderUsesProxy, {{"proxy_server_url", "invalid"}});
+TEST_F(IsolatedPrerenderProxyConfiguratorTest, ClearProxyServers) {
+  GURL proxy_url("https://proxy.com");
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitWithFeatures(
+      {features::kIsolatePrerenders,
+       data_reduction_proxy::features::kDataReductionProxyHoldback},
+      {});
 
-  configurator()->UpdateCustomProxyConfig();
+  configurator()->UpdateProxyHosts({proxy_url});
   base::RunLoop().RunUntilIdle();
 
-  EXPECT_FALSE(LatestProxyConfig());
-}
-
-TEST_F(IsolatedPrerenderProxyConfiguratorTest, InvalidProxyServerURL_NoHost) {
-  base::test::ScopedFeatureList drp_scoped_feature_list;
-  drp_scoped_feature_list.InitAndEnableFeature(
-      data_reduction_proxy::features::kDataReductionProxyHoldback);
-
-  base::test::ScopedFeatureList proxy_scoped_feature_list;
-  proxy_scoped_feature_list.InitAndEnableFeatureWithParameters(
-      features::kIsolatedPrerenderUsesProxy,
-      {{"proxy_server_url", "https://"}});
+  net::HttpRequestHeaders headers;
+  VerifyLatestProxyConfig(proxy_url, headers);
 
-  configurator()->UpdateCustomProxyConfig();
+  configurator()->UpdateProxyHosts({});
   base::RunLoop().RunUntilIdle();
 
-  EXPECT_FALSE(LatestProxyConfig());
+  VerifyLatestProxyConfig(GURL(), headers, /*want_empty=*/true);
 }
 
 TEST_F(IsolatedPrerenderProxyConfiguratorTest, ValidProxyServerURL) {
   GURL proxy_url("https://proxy.com");
-  base::test::ScopedFeatureList drp_scoped_feature_list;
-  drp_scoped_feature_list.InitAndEnableFeature(
-      data_reduction_proxy::features::kDataReductionProxyHoldback);
-
-  base::test::ScopedFeatureList proxy_scoped_feature_list;
-  proxy_scoped_feature_list.InitAndEnableFeatureWithParameters(
-      features::kIsolatedPrerenderUsesProxy,
-      {{"proxy_server_url", proxy_url.spec()}});
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitWithFeatures(
+      {features::kIsolatePrerenders,
+       data_reduction_proxy::features::kDataReductionProxyHoldback},
+      {});
 
-  configurator()->UpdateCustomProxyConfig();
+  configurator()->UpdateProxyHosts({proxy_url});
   base::RunLoop().RunUntilIdle();
 
   net::HttpRequestHeaders headers;
@@ -198,18 +187,16 @@ TEST_F(IsolatedPrerenderProxyConfiguratorTest, ValidProxyServerURL) {
 
 TEST_F(IsolatedPrerenderProxyConfiguratorTest, ValidProxyServerURLWithHeaders) {
   GURL proxy_url("https://proxy.com");
-  base::test::ScopedFeatureList drp_scoped_feature_list;
-  drp_scoped_feature_list.InitAndEnableFeature(
-      data_reduction_proxy::features::kDataReductionProxyHoldback);
-
-  base::test::ScopedFeatureList proxy_scoped_feature_list;
-  proxy_scoped_feature_list.InitAndEnableFeatureWithParameters(
-      features::kIsolatedPrerenderUsesProxy,
-      {{"proxy_server_url", proxy_url.spec()}});
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitWithFeatures(
+      {features::kIsolatePrerenders,
+       data_reduction_proxy::features::kDataReductionProxyHoldback},
+      {});
 
   net::HttpRequestHeaders headers;
   headers.SetHeader("X-Testing", "Hello World");
   configurator()->UpdateTunnelHeaders(headers);
+  configurator()->UpdateProxyHosts({proxy_url});
 
   base::RunLoop().RunUntilIdle();
   VerifyLatestProxyConfig(proxy_url, headers);

chrome/browser/prerender/isolated/isolated_prerender_service.cc

@@ -37,5 +37,10 @@ void IsolatedPrerenderService::OnProxyRequestHeadersChanged(
   proxy_configurator_->UpdateTunnelHeaders(headers);
 }
 
+void IsolatedPrerenderService::OnPrefetchProxyHostsChanged(
+    const std::vector<GURL>& prefetch_proxies) {
+  proxy_configurator_->UpdateProxyHosts(prefetch_proxies);
+}
+
 void IsolatedPrerenderService::OnSettingsInitialized() {}
 void IsolatedPrerenderService::OnDataSaverEnabledChanged(bool enabled) {}

chrome/browser/prerender/isolated/isolated_prerender_service.h

@@ -6,9 +6,11 @@
 #define CHROME_BROWSER_PRERENDER_ISOLATED_ISOLATED_PRERENDER_SERVICE_H_
 
 #include <memory>
+#include <vector>
 
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "url/gurl.h"
 
 class Profile;
 class IsolatedPrerenderProxyConfigurator;
@@ -36,6 +38,8 @@ class IsolatedPrerenderService
       const net::HttpRequestHeaders& headers) override;
   void OnSettingsInitialized() override;
   void OnDataSaverEnabledChanged(bool enabled) override;
+  void OnPrefetchProxyHostsChanged(
+      const std::vector<GURL>& prefetch_proxies) override;
 
   // KeyedService:
   void Shutdown() override;