Reland "Disable DRP proxies that fail warmup fetch in config"

This is a reland of 0ee0c8f9
after disabling the test that failed when network service is off

Original change's description:
> Disable DRP proxies that fail warmup fetch in config
>
> Warmup url fetch happens on startup and depending on whether it succeeds
> the http/https proxy should be enabled or disabled in the custom proxy
> config in network service. However the proxies are not getting disabled.
> This CL fixes that and also adds a feature flag to turn off this
> behavior.
>
> Bug: 954958
> Change-Id: I22f0dc4cfb0ef58b4922ecfe9ba57009744aba8b
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1579026
> Reviewed-by: Tarun Bansal <tbansal@chromium.org>
> Commit-Queue: rajendrant <rajendrant@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#653378}

Bug: 954958
Change-Id: I77d71c4c42f38dc2c6ba8b8bc55e4b0971c410d6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1580760Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: rajendrant <rajendrant@chromium.org>
Cr-Commit-Position: refs/heads/master@{#653465}

chrome/browser/data_reduction_proxy/data_reduction_proxy_browsertest.cc

@@ -13,6 +13,7 @@
 #include "base/test/bind_test_util.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
+#include "build/build_config.h"
 #include "chrome/browser/data_reduction_proxy/data_reduction_proxy_chrome_settings.h"
 #include "chrome/browser/data_reduction_proxy/data_reduction_proxy_chrome_settings_factory.h"
 #include "chrome/browser/metrics/subprocess_metrics_provider.h"
@@ -121,6 +122,12 @@ ClientConfig CreateConfigForServer(const net::EmbeddedTestServer& server) {
 
 }  // namespace
 
+#if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_CHROMEOS)
+#define DISABLE_ON_WIN_MAC_CHROMEOS(x) DISABLED_##x
+#else
+#define DISABLE_ON_WIN_MAC_CHROMEOS(x) x
+#endif
+
 class DataReductionProxyBrowsertestBase : public InProcessBrowserTest {
  public:
   void SetUpCommandLine(base::CommandLine* command_line) override {
@@ -165,6 +172,10 @@ class DataReductionProxyBrowsertestBase : public InProcessBrowserTest {
     WaitForConfig();
   }
 
+  bool IsNetworkServiceEnabled() const {
+    return base::FeatureList::IsEnabled(network::features::kNetworkService);
+  }
+
  protected:
   void EnableDataSaver(bool enabled) {
     data_reduction_proxy::DataReductionProxySettings::
@@ -977,7 +988,7 @@ IN_PROC_BROWSER_TEST_F(DataReductionProxyResourceTypeBrowsertest,
 
 class DataReductionProxyWarmupURLBrowsertest
     : public ::testing::WithParamInterface<
-          std::tuple<ProxyServer_ProxyScheme, bool>>,
+          std::tuple<ProxyServer_ProxyScheme, bool, bool>>,
       public DataReductionProxyBrowsertestBase {
  public:
   DataReductionProxyWarmupURLBrowsertest()
@@ -987,6 +998,10 @@ class DataReductionProxyWarmupURLBrowsertest
         secondary_server_(GetTestServerType()) {}
 
   void SetUpOnMainThread() override {
+    if (!std::get<2>(GetParam())) {
+      scoped_feature_list_.InitAndDisableFeature(
+          features::kDataReductionProxyDisableProxyFailedWarmup);
+    }
     primary_server_loop_ = std::make_unique<base::RunLoop>();
     primary_server_.RegisterRequestHandler(base::BindRepeating(
         &DataReductionProxyWarmupURLBrowsertest::WaitForWarmupRequest,
@@ -1054,30 +1069,66 @@ class DataReductionProxyWarmupURLBrowsertest
   std::unique_ptr<net::test_server::HttpResponse> WaitForWarmupRequest(
       base::RunLoop* run_loop,
       const net::test_server::HttpRequest& request) {
+    auto response = std::make_unique<net::test_server::BasicHttpResponse>();
     if (base::StartsWith(request.relative_url, "/e2e_probe",
                          base::CompareCase::SENSITIVE)) {
       run_loop->Quit();
+      response->set_content("content");
+      response->AddCustomHeader("via", via_header_);
+    } else if (base::StartsWith(request.relative_url, "/echoheader",
+                                base::CompareCase::SENSITIVE)) {
+      const auto chrome_proxy_header = request.headers.find("chrome-proxy");
+      if (chrome_proxy_header != request.headers.end()) {
+        response->set_content(chrome_proxy_header->second);
+        response->AddCustomHeader("chrome-proxy", "ofcl=1000");
+      }
     }
-    auto response = std::make_unique<net::test_server::BasicHttpResponse>();
-    response->set_content("content");
-    response->AddCustomHeader("via", via_header_);
     return response;
   }
 
   const std::string via_header_;
   net::EmbeddedTestServer primary_server_;
   net::EmbeddedTestServer secondary_server_;
+  base::test::ScopedFeatureList scoped_feature_list_;
 };
 
-IN_PROC_BROWSER_TEST_P(DataReductionProxyWarmupURLBrowsertest,
-                       WarmupURLsFetchedForEachProxy) {
+IN_PROC_BROWSER_TEST_P(
+    DataReductionProxyWarmupURLBrowsertest,
+    DISABLE_ON_WIN_MAC_CHROMEOS(WarmupURLsFetchedForEachProxy)) {
+  if (!IsNetworkServiceEnabled())
+    return;
+  net::EmbeddedTestServer test_server;
+  test_server.RegisterRequestHandler(
+      base::BindRepeating(&BasicResponse, kDummyBody));
+  ASSERT_TRUE(test_server.Start());
+
+  bool is_warmup_fetch_successful = std::get<1>(GetParam());
+  bool disallow_proxy_failed_warmup_feature_enabled = std::get<2>(GetParam());
   primary_server_loop_->Run();
 
   SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
   RetryForHistogramUntilCountReached(&histogram_tester_, GetHistogramName(), 1);
 
   histogram_tester_.ExpectUniqueSample(GetHistogramName(),
-                                       std::get<1>(GetParam()), 1);
+                                       is_warmup_fetch_successful, 1);
+
+  base::RunLoop().RunUntilIdle();
+
+  // Navigate to some URL to see if the proxy is only used when warmup URL fetch
+  // was successful.
+  ui_test_utils::NavigateToURL(
+      browser(), GetURLWithMockHost(test_server, "/echoheader?Chrome-Proxy"));
+  std::string body = GetBody();
+  if (is_warmup_fetch_successful) {
+    EXPECT_THAT(body, HasSubstr(kSessionKey));
+  } else {
+    if (disallow_proxy_failed_warmup_feature_enabled) {
+      EXPECT_THAT(body, kDummyBody);
+    } else {
+      // When the feature is disabled, the proxy is still being used
+      EXPECT_THAT(body, HasSubstr(kSessionKey));
+    }
+  }
 }
 
 // First parameter indicate proxy scheme for proxies that are being tested.
@@ -1086,9 +1137,13 @@ IN_PROC_BROWSER_TEST_P(DataReductionProxyWarmupURLBrowsertest,
 INSTANTIATE_TEST_SUITE_P(
     ,
     DataReductionProxyWarmupURLBrowsertest,
-    ::testing::Combine(testing::Values(ProxyServer_ProxyScheme_HTTP,
-                                       ProxyServer_ProxyScheme_HTTPS),
-                       ::testing::Bool()));
+    ::testing::Combine(
+        testing::Values(ProxyServer_ProxyScheme_HTTP,
+                        ProxyServer_ProxyScheme_HTTPS),
+        ::testing::Bool(),  // is_warmup_fetch_successful
+        ::testing::Bool()   // kDataReductionProxyDisallowProxyFailedWarmup
+                            // active
+        ));
 
 // Threadsafe log for recording a sequence of events as newline separated text.
 class EventLog {

components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc

@@ -25,6 +25,7 @@
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h"
 #include "components/data_reduction_proxy/core/browser/network_properties_manager.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_bypass_protocol.h"
+#include "components/data_reduction_proxy/core/common/data_reduction_proxy_features.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h"
@@ -161,7 +162,7 @@ void DataReductionProxyIOData::InitializeOnIOThread() {
   config_->InitializeOnIOThread(
       url_loader_factory,
       base::BindRepeating(&DataReductionProxyIOData::CreateCustomProxyConfig,
-                          base::Unretained(this)),
+                          base::Unretained(this), true),
       network_properties_manager_.get());
   bypass_stats_->InitializeOnIOThread();
   proxy_delegate_->InitializeOnIOThread(this);
@@ -378,14 +379,14 @@ void DataReductionProxyIOData::OnProxyConfigUpdated() {
 
 network::mojom::CustomProxyConfigPtr
 DataReductionProxyIOData::CreateCustomProxyConfig(
+    bool is_warmup_url,
     const std::vector<DataReductionProxyServer>& proxies_for_http) const {
   auto config = network::mojom::CustomProxyConfig::New();
-  config->rules =
-      configurator_
-          ->CreateProxyConfig(true /* probe_url_config */,
-                              config_->GetNetworkPropertiesManager(),
-                              proxies_for_http)
-          .proxy_rules();
+  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) {
@@ -408,8 +409,10 @@ void DataReductionProxyIOData::UpdateCustomProxyConfig() {
   if (!proxy_config_client_)
     return;
 
-  proxy_config_client_->OnCustomProxyConfigUpdated(
-      CreateCustomProxyConfig(config_->GetProxiesForHttp()));
+  proxy_config_client_->OnCustomProxyConfigUpdated(CreateCustomProxyConfig(
+      !base::FeatureList::IsEnabled(
+          features::kDataReductionProxyDisableProxyFailedWarmup),
+      config_->GetProxiesForHttp()));
 }
 
 void DataReductionProxyIOData::UpdateThrottleConfig() {

components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h

@@ -257,6 +257,7 @@ class DataReductionProxyIOData : public mojom::DataReductionProxy {
   // Creates a config using |proxies_for_http| that can be sent to the
   // NetworkContext.
   network::mojom::CustomProxyConfigPtr CreateCustomProxyConfig(
+      bool is_warmup_url,
       const std::vector<DataReductionProxyServer>& proxies_for_http) const;
 
   // Called when the list of proxies changes.

components/data_reduction_proxy/core/common/data_reduction_proxy_features.cc

@@ -49,5 +49,11 @@ const base::Feature kDataReductionProxyPopulatePreviewsPageIDToPingback{
     "DataReductionProxyPopulatePreviewsPageIDToPingback",
     base::FEATURE_ENABLED_BY_DEFAULT};
 
+// Enables not allowing proxies that fail warmup url fetch, to custom proxy
+// config updates when network service is enabled.
+const base::Feature kDataReductionProxyDisableProxyFailedWarmup{
+    "DataReductionProxyDisableProxyFailedWarmup",
+    base::FEATURE_ENABLED_BY_DEFAULT};
+
 }  // namespace features
 }  // namespace data_reduction_proxy

components/data_reduction_proxy/core/common/data_reduction_proxy_features.h

@@ -17,6 +17,7 @@ extern const base::Feature kDataReductionProxyEnabledWithNetworkService;
 extern const base::Feature kDataSaverUseOnDeviceSafeBrowsing;
 extern const base::Feature kDataReductionProxyBlockOnBadGatewayResponse;
 extern const base::Feature kDataReductionProxyPopulatePreviewsPageIDToPingback;
+extern const base::Feature kDataReductionProxyDisableProxyFailedWarmup;
 
 }  // namespace features
 }  // namespace data_reduction_proxy