Probe all data saver proxies instead of returning early

Currently, Chrome stops probing data saver proxies if a probe
is fetched via a non-data saver proxy (or over DIRECT).

This CL changes the Chrome behavior to not return early if the
previous probe was not fetched via data saver proxy. This
ensures that all data saver proxies are probed.

Change-Id: I13a5a3d52a414ca4eb7481a7ac43f2504b5cc123
Bug: 849387
Reviewed-on: https://chromium-review.googlesource.com/1083830
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564597}

components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc

@@ -488,29 +488,24 @@ void DataReductionProxyConfig::HandleWarmupFetcherResponse(
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(IsFetchInFlight());
 
-  // If the probe times out, then proxy server information may not have been
-  // set by the URL fetcher.
-  bool timed_out_with_no_proxy_data =
-      (success_response == WarmupURLFetcher::FetchResult::kTimedOut &&
-       !proxy_server.is_valid());
-
   base::Optional<DataReductionProxyTypeInfo> proxy_type_info =
       FindConfiguredDataReductionProxy(proxy_server);
 
   // Check the proxy server used.
-  if (!timed_out_with_no_proxy_data && !proxy_type_info) {
-    // No need to do anything here since the warmup fetch did not go through
-    // the data saver proxy.
+  if (!proxy_type_info && proxy_server.is_valid() &&
+      !proxy_server.is_direct()) {
+    // No need to do anything here since the warmup fetch went through
+    // a non-datasaver proxy.
     return;
   }
 
   bool is_secure_proxy = false;
   bool is_core_proxy = false;
 
-  if (!timed_out_with_no_proxy_data) {
+  if (proxy_type_info) {
+    DCHECK(proxy_server.is_valid());
+    DCHECK(!proxy_server.is_direct());
     is_secure_proxy = proxy_server.is_https() || proxy_server.is_quic();
-
-    DCHECK(proxy_type_info);
     is_core_proxy = proxy_type_info->proxy_servers[proxy_type_info->proxy_index]
                         .IsCoreProxy();
 
@@ -520,8 +515,10 @@ void DataReductionProxyConfig::HandleWarmupFetcherResponse(
     DCHECK_EQ(is_secure_proxy, GetInFlightWarmupProxyDetails()->first);
     DCHECK_EQ(is_core_proxy, GetInFlightWarmupProxyDetails()->second);
   } else {
-    // When the probe times out, the proxy information may not be set. Fill-in
-    // the missing data using the proxy that was being probed.
+    DCHECK(!proxy_server.is_valid() || proxy_server.is_direct());
+    // When the probe times out or if the warmup URL was fetched via DIRECT
+    // proxy, the data reduction proxy information may not be set. Fill-in the
+    // missing data using the proxy that was being probed.
     is_secure_proxy = warmup_url_fetch_in_flight_secure_proxy_;
     is_core_proxy = warmup_url_fetch_in_flight_core_proxy_;
   }

components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc

@@ -962,14 +962,6 @@ TEST_F(DataReductionProxyConfigTest, HandleWarmupFetcherResponse) {
   EXPECT_EQ(std::vector<net::ProxyServer>({kHttpsProxy, kHttpProxy}),
             GetConfiguredProxiesForHttp());
 
-  // Report failed warmup for a non-DataSaver proxy, and verify that it does not
-  // change the list of data saver proxies.
-  test_config()->HandleWarmupFetcherResponse(
-      net::ProxyServer(),
-      WarmupURLFetcher::FetchResult::kFailed /* success_response */);
-  EXPECT_EQ(std::vector<net::ProxyServer>({kHttpsProxy, kHttpProxy}),
-            GetConfiguredProxiesForHttp());
-
   // Set the details of the proxy to which the warmup URL probe is in-flight to
   // avoid triggering the DCHECKs in HandleWarmupFetcherResponse method.
   test_config()->SetInFlightWarmupProxyDetails(
@@ -1099,6 +1091,62 @@ TEST_F(DataReductionProxyConfigTest, HandleWarmupFetcherResponse) {
             GetConfiguredProxiesForHttp());
 }
 
+// Tests that the proxy server last used for fetching the warmup URL is marked
+// as failed when the warmup fetched callback returns an invalid proxy.
+TEST_F(DataReductionProxyConfigTest,
+       HandleWarmupFetcherResponse_InvalidProxyServer) {
+  const net::URLRequestStatus kSuccess(net::URLRequestStatus::SUCCESS, net::OK);
+  const net::ProxyServer kHttpsProxy = net::ProxyServer::FromURI(
+      "https://origin.net:443", net::ProxyServer::SCHEME_HTTP);
+  const net::ProxyServer kHttpProxy = net::ProxyServer::FromURI(
+      "fallback.net:80", net::ProxyServer::SCHEME_HTTP);
+
+  SetProxiesForHttpOnCommandLine({kHttpsProxy, kHttpProxy});
+  ResetSettings();
+
+  // The proxy is enabled.
+  test_config()->UpdateConfigForTesting(true, true, true);
+  test_config()->OnNewClientConfigFetched();
+  EXPECT_EQ(std::vector<net::ProxyServer>({kHttpsProxy, kHttpProxy}),
+            GetConfiguredProxiesForHttp());
+
+  // Report failed warmup for a non-DataSaver proxy, and verify that it
+  // changes the list of data saver proxies.
+  test_config()->HandleWarmupFetcherResponse(
+      net::ProxyServer(),
+      WarmupURLFetcher::FetchResult::kFailed /* success_response */);
+  EXPECT_EQ(std::vector<net::ProxyServer>({kHttpProxy}),
+            GetConfiguredProxiesForHttp());
+}
+
+// Tests that the proxy server last used for fetching the warmup URL is marked
+// as failed when the warmup fetched callback returns a direct proxy.
+TEST_F(DataReductionProxyConfigTest,
+       HandleWarmupFetcherResponse_DirectProxyServer) {
+  const net::URLRequestStatus kSuccess(net::URLRequestStatus::SUCCESS, net::OK);
+  const net::ProxyServer kHttpsProxy = net::ProxyServer::FromURI(
+      "https://origin.net:443", net::ProxyServer::SCHEME_HTTP);
+  const net::ProxyServer kHttpProxy = net::ProxyServer::FromURI(
+      "fallback.net:80", net::ProxyServer::SCHEME_HTTP);
+
+  SetProxiesForHttpOnCommandLine({kHttpsProxy, kHttpProxy});
+  ResetSettings();
+
+  // The proxy is enabled.
+  test_config()->UpdateConfigForTesting(true, true, true);
+  test_config()->OnNewClientConfigFetched();
+  EXPECT_EQ(std::vector<net::ProxyServer>({kHttpsProxy, kHttpProxy}),
+            GetConfiguredProxiesForHttp());
+
+  // Report failed warmup for a non-DataSaver proxy, and verify that it
+  // changes the list of data saver proxies.
+  test_config()->HandleWarmupFetcherResponse(
+      net::ProxyServer::Direct(),
+      WarmupURLFetcher::FetchResult::kFailed /* success_response */);
+  EXPECT_EQ(std::vector<net::ProxyServer>({kHttpProxy}),
+            GetConfiguredProxiesForHttp());
+}
+
 TEST_F(DataReductionProxyConfigTest, HandleWarmupFetcherRetry) {
   constexpr size_t kMaxWarmupURLFetchAttempts = 3;
 

components/data_reduction_proxy/core/browser/warmup_url_fetcher.cc

@@ -179,8 +179,7 @@ void WarmupURLFetcher::OnURLFetchComplete(const net::URLFetcher* source) {
   if (!source->GetStatus().is_success() &&
       source->GetStatus().error() == net::ERR_INTERNET_DISCONNECTED) {
     // Fetching failed due to Internet unavailability, and not due to some
-    // error. Set the proxy server to unknown.
-    callback_.Run(net::ProxyServer(), FetchResult::kSuccessful);
+    // error. No need to run the callback.
     CleanupAfterFetch();
     return;
   }