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/+/1579026Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: rajendrant <rajendrant@chromium.org>
Cr-Commit-Position: refs/heads/master@{#653378}

chrome/browser/data_reduction_proxy/data_reduction_proxy_browsertest.cc

@@ -977,7 +977,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 +987,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 +1058,63 @@ 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) {
+  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 +1123,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