Add logic for DRP holdback experiment, and make a new Feature

based study to holdback data reduction proxy.

Change-Id: Ib3c900ea4bd3441272051b53911a1f46e216a020
Bug: 954958
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1578127
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: rajendrant <rajendrant@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Reviewed-by: Robert Ogden <robertogden@chromium.org>
Cr-Commit-Position: refs/heads/master@{#653493}

chrome/browser/data_reduction_proxy/data_reduction_proxy_browsertest.cc

@@ -462,6 +462,55 @@ IN_PROC_BROWSER_TEST_F(DataReductionProxyBrowsertest, UMAMetricsRecorded) {
       1);
 }
 
+// Test that enabling the holdback disables the proxy.
+class DataReductionProxyWithHoldbackBrowsertest
+    : public ::testing::WithParamInterface<bool>,
+      public DataReductionProxyBrowsertest {
+ public:
+  void SetUp() override {
+    if (GetParam()) {
+      scoped_feature_list_.InitWithFeatures(
+          {features::kDataReductionProxyEnabledWithNetworkService,
+           data_reduction_proxy::features::kDataReductionProxyHoldback},
+          {});
+    } else {
+      scoped_feature_list_.InitWithFeatures(
+          {features::kDataReductionProxyEnabledWithNetworkService}, {});
+    }
+
+    InProcessBrowserTest::SetUp();
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_P(DataReductionProxyWithHoldbackBrowsertest,
+                       UpdateConfig) {
+  net::EmbeddedTestServer proxy_server;
+  proxy_server.RegisterRequestHandler(
+      base::BindRepeating(&BasicResponse, kPrimaryResponse));
+  ASSERT_TRUE(proxy_server.Start());
+
+  SetConfig(CreateConfigForServer(proxy_server));
+  // A network change forces the config to be fetched.
+  SimulateNetworkChange(network::mojom::ConnectionType::CONNECTION_3G);
+  WaitForConfig();
+
+  ui_test_utils::NavigateToURL(browser(), GURL("http://does.not.resolve/foo"));
+
+  if (GetParam()) {
+    EXPECT_NE(GetBody(), kPrimaryResponse);
+  } else {
+    EXPECT_EQ(GetBody(), kPrimaryResponse);
+  }
+}
+
+// Parameter is true if the data reduction proxy holdback should be enabled.
+INSTANTIATE_TEST_SUITE_P(,
+                         DataReductionProxyWithHoldbackBrowsertest,
+                         ::testing::Values(false, true));
+
 class DataReductionProxyBrowsertestWithNetworkService
     : public DataReductionProxyBrowsertest {
  public:
@@ -851,7 +900,9 @@ IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
 }
 
 IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
-                       DISABLED_ProxyBlockedOnAuthError) {
+                       DISABLE_ON_WIN_MAC_CHROMEOS(ProxyBlockedOnAuthError)) {
+  if (!IsNetworkServiceEnabled())
+    return;
   base::HistogramTester histogram_tester;
   net::EmbeddedTestServer test_server;
   test_server.RegisterRequestHandler(
@@ -870,7 +921,9 @@ IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
 // Tests that if using data reduction proxy results in redirect loop, then
 // the proxy is bypassed, and the request is fetched directly.
 IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
-                       DISABLED_RedirectCycle) {
+                       DISABLE_ON_WIN_MAC_CHROMEOS(RedirectCycle)) {
+  if (!IsNetworkServiceEnabled())
+    return;
   base::HistogramTester histogram_tester;
   net::EmbeddedTestServer test_server;
   test_server.RegisterRequestHandler(

components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator.cc

@@ -30,6 +30,8 @@ void DataReductionProxyConfigurator::Enable(
     const NetworkPropertiesManager& network_properties_manager,
     const std::vector<DataReductionProxyServer>& proxies_for_http) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!params::IsIncludedInHoldbackFieldTrial() || proxies_for_http.empty());
+
   net::ProxyConfig config =
       CreateProxyConfig(false /* probe_url_config */,
                         network_properties_manager, proxies_for_http);
@@ -43,6 +45,7 @@ net::ProxyConfig DataReductionProxyConfigurator::CreateProxyConfig(
     const NetworkPropertiesManager& network_properties_manager,
     const std::vector<DataReductionProxyServer>& proxies_for_http) const {
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!params::IsIncludedInHoldbackFieldTrial() || proxies_for_http.empty());
 
   net::ProxyConfig config;
   DCHECK(config.proxy_rules().proxies_for_http.IsEmpty());

components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc

@@ -382,11 +382,21 @@ DataReductionProxyIOData::CreateCustomProxyConfig(
     bool is_warmup_url,
     const std::vector<DataReductionProxyServer>& proxies_for_http) const {
   auto config = network::mojom::CustomProxyConfig::New();
-  config->rules = configurator_
-                      ->CreateProxyConfig(
-                          is_warmup_url, config_->GetNetworkPropertiesManager(),
-                          proxies_for_http)
-                      .proxy_rules();
+  if (params::IsIncludedInHoldbackFieldTrial()) {
+    config->rules =
+        configurator_
+            ->CreateProxyConfig(is_warmup_url,
+                                config_->GetNetworkPropertiesManager(),
+                                std::vector<DataReductionProxyServer>())
+            .proxy_rules();
+  } else {
+    config->rules =
+        configurator_
+            ->CreateProxyConfig(is_warmup_url,
+                                config_->GetNetworkPropertiesManager(),
+                                proxies_for_http)
+            .proxy_rules();
+  }
 
   net::EffectiveConnectionType type = GetEffectiveConnectionType();
   if (type > net::EFFECTIVE_CONNECTION_TYPE_OFFLINE) {

components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc

@@ -660,12 +660,14 @@ bool DataReductionProxyNetworkDelegate::WasEligibleWithoutHoldback(
                                            request.method())) {
     return false;
   }
-  net::ProxyConfig proxy_config =
-      data_reduction_proxy_config_->ProxyConfigIgnoringHoldback();
-  net::ProxyInfo data_reduction_proxy_info;
-  return util::ApplyProxyConfigToProxyInfo(proxy_config, proxy_retry_info,
-                                           request.url(),
-                                           &data_reduction_proxy_info);
+  // Deprecated non network-service code. If in proxy holdback group, assume
+  // that the URL was eligible for proxying if and only if it has scheme "http".
+  // This code should be removed once network service is enabled by default, and
+  // is necessary only to prevent DCHECKs from triggering in tests.
+  DCHECK(params::IsIncludedInHoldbackFieldTrial());
+  return data_reduction_proxy_config_->enabled_by_user_and_reachable() &&
+         request.url().SchemeIsHTTPOrHTTPS() &&
+         !request.url().SchemeIsCryptographic();
 }
 
 void DataReductionProxyNetworkDelegate::MaybeAddChromeProxyECTHeader(

components/data_reduction_proxy/core/common/data_reduction_proxy_features.cc

@@ -31,6 +31,12 @@ const base::Feature kDataReductionProxyLowMemoryDevicePromo{
 const base::Feature kDogfood{"DataReductionProxyDogfood",
                              base::FEATURE_DISABLED_BY_DEFAULT};
 
+// If enabled, the usage of data reduction proxy is disabled for HTTP URLs.
+// Does not affect the state of save-data header or other
+// features that may depend on data saver being enabled.
+const base::Feature kDataReductionProxyHoldback{
+    "DataReductionProxyHoldback", base::FEATURE_DISABLED_BY_DEFAULT};
+
 // Enables data reduction proxy when network service is enabled.
 const base::Feature kDataReductionProxyEnabledWithNetworkService{
     "DataReductionProxyEnabledWithNetworkService",

components/data_reduction_proxy/core/common/data_reduction_proxy_features.h

@@ -13,6 +13,7 @@ namespace features {
 extern const base::Feature kDataReductionProxyDecidesTransform;
 extern const base::Feature kDataReductionProxyLowMemoryDevicePromo;
 extern const base::Feature kDogfood;
+extern const base::Feature kDataReductionProxyHoldback;
 extern const base::Feature kDataReductionProxyEnabledWithNetworkService;
 extern const base::Feature kDataSaverUseOnDeviceSafeBrowsing;
 extern const base::Feature kDataReductionProxyBlockOnBadGatewayResponse;

components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc

@@ -104,7 +104,12 @@ bool IsIncludedInFREPromoFieldTrial() {
 }
 
 bool IsIncludedInHoldbackFieldTrial() {
-  return IsIncludedInFieldTrial("DataCompressionProxyHoldback");
+  // For now, DRP can be disabled using either the field trial or the feature.
+  // New server configs should use the feature capability.
+  // TODO(tbansal): Remove the field trial code.
+  return base::FeatureList::IsEnabled(
+             data_reduction_proxy::features::kDataReductionProxyHoldback) ||
+         IsIncludedInFieldTrial("DataCompressionProxyHoldback");
 }
 
 bool IsIncludedInSecureProxyHoldbackFieldTrial() {