Aggressively fetch client config

1. Tune the backoff policy for client config fetch aggressively.
2. Allow fetching client config even when in background in Android.

Both these changes are under a feature flag and enabled by default.

The tests are changed to use the same backoff policy as the source code

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

components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc

@@ -80,7 +80,8 @@ const uint32_t kMaxBackgroundFetchIntervalSeconds = 6 * 60 * 60;  // 6 hours.
 #endif
 
 // This is the default backoff policy used to communicate with the Data
-// Reduction Proxy configuration service.
+// Reduction Proxy configuration service. The policy gets overwritten based on
+// kDataReductionProxyAggressiveConfigFetch.
 const net::BackoffEntry::Policy kDefaultBackoffPolicy = {
     0,                // num_errors_to_ignore
     10 * 1000,        // initial_delay_ms
@@ -134,8 +135,16 @@ void RecordAuthExpiredSessionKey(bool matches) {
 
 }  // namespace
 
-const net::BackoffEntry::Policy& GetBackoffPolicy() {
-  return kDefaultBackoffPolicy;
+net::BackoffEntry::Policy GetBackoffPolicy() {
+  net::BackoffEntry::Policy policy = kDefaultBackoffPolicy;
+  if (base::FeatureList::IsEnabled(
+          features::kDataReductionProxyAggressiveConfigFetch)) {
+    // Disabling always_use_initial_delay allows no backoffs until
+    // num_errors_to_ignore failures have occurred.
+    policy.num_errors_to_ignore = 2;
+    policy.always_use_initial_delay = false;
+  }
+  return policy;
 }
 
 DataReductionProxyConfigServiceClient::DataReductionProxyConfigServiceClient(
@@ -152,7 +161,8 @@ DataReductionProxyConfigServiceClient::DataReductionProxyConfigServiceClient(
       io_data_(io_data),
       network_connection_tracker_(network_connection_tracker),
       config_storer_(config_storer),
-      backoff_entry_(&backoff_policy),
+      backoff_policy_(backoff_policy),
+      backoff_entry_(&backoff_policy_),
       config_service_url_(util::AddApiKeyToUrl(params::GetConfigServiceURL())),
       enabled_(false),
       remote_config_applied_(false),
@@ -192,7 +202,10 @@ DataReductionProxyConfigServiceClient::CalculateNextConfigRefreshTime(
 
 #if defined(OS_ANDROID)
   foreground_fetch_pending_ = false;
-  if (!fetch_succeeded && IsApplicationStateBackground()) {
+  if (!fetch_succeeded &&
+      !base::FeatureList::IsEnabled(
+          features::kDataReductionProxyAggressiveConfigFetch) &&
+      IsApplicationStateBackground()) {
     // If Chromium is in background, then fetch the config when Chromium comes
     // to foreground or after max of |kMaxBackgroundFetchIntervalSeconds| and
     // |backoff_delay|.

components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.h

@@ -48,7 +48,7 @@ typedef base::Callback<void(const std::string&)> ConfigStorer;
 
 // Retrieves the default net::BackoffEntry::Policy for the Data Reduction Proxy
 // configuration service client.
-const net::BackoffEntry::Policy& GetBackoffPolicy();
+net::BackoffEntry::Policy GetBackoffPolicy();
 
 // Retrieves the Data Reduction Proxy configuration from a remote service. This
 // object lives on the IO thread.
@@ -210,6 +210,7 @@ class DataReductionProxyConfigServiceClient
   ConfigStorer config_storer_;
 
   // Used to calculate the backoff time on request failures.
+  net::BackoffEntry::Policy backoff_policy_;
   net::BackoffEntry backoff_entry_;
 
   // The URL for retrieving the Data Reduction Proxy configuration.

components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc

@@ -18,6 +18,7 @@
 #include "base/test/bind_test_util.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/mock_entropy_provider.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/time/default_clock.h"
 #include "base/time/time.h"
@@ -28,6 +29,7 @@
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h"
 #include "components/data_reduction_proxy/core/browser/network_properties_manager.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_params_test_utils.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.h"
@@ -83,10 +85,18 @@ namespace data_reduction_proxy {
 
 class DataReductionProxyConfigServiceClientTest : public testing::Test {
  protected:
-  DataReductionProxyConfigServiceClientTest()
+  DataReductionProxyConfigServiceClientTest(
+      bool enable_aggressive_config_fetch_feature = false)
       : test_shared_url_loader_factory_(
             base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
                 &test_url_loader_factory_)) {
+    if (enable_aggressive_config_fetch_feature) {
+      scoped_feature_list_.InitAndEnableFeature(
+          features::kDataReductionProxyAggressiveConfigFetch);
+    } else {
+      scoped_feature_list_.InitAndDisableFeature(
+          features::kDataReductionProxyAggressiveConfigFetch);
+    }
     // TODO(tonikitoo): Do a clean up pass to remove unneeded class members
     // once the URLFetcher->SimpleURLLoader switch stabalizes.
     // This includes |context_|, |context_storage_|, |mock_socket_factory_|,
@@ -424,6 +434,7 @@ class DataReductionProxyConfigServiceClientTest : public testing::Test {
   }
 
  private:
+  base::test::ScopedFeatureList scoped_feature_list_;
   base::test::ScopedTaskEnvironment task_environment_{
       base::test::ScopedTaskEnvironment::MainThreadType::IO};
   std::unique_ptr<net::TestURLRequestContext> context_;
@@ -508,7 +519,7 @@ TEST_F(DataReductionProxyConfigServiceClientTest, EnsureBackoff) {
   config_client()->RetrieveConfig();
   RunUntilIdle();
   EXPECT_EQ(std::vector<net::ProxyServer>(), GetConfiguredProxiesForHttp());
-  EXPECT_EQ(base::TimeDelta::FromSeconds(20), config_client()->GetDelay());
+  EXPECT_EQ(base::TimeDelta::FromSeconds(30), config_client()->GetDelay());
 
 #if defined(OS_ANDROID)
   EXPECT_FALSE(config_client()->foreground_fetch_pending());
@@ -518,7 +529,7 @@ TEST_F(DataReductionProxyConfigServiceClientTest, EnsureBackoff) {
   config_client()->RetrieveConfig();
   RunUntilIdle();
   EXPECT_EQ(std::vector<net::ProxyServer>(), GetConfiguredProxiesForHttp());
-  EXPECT_EQ(base::TimeDelta::FromSeconds(40), config_client()->GetDelay());
+  EXPECT_EQ(base::TimeDelta::FromSeconds(90), config_client()->GetDelay());
   EXPECT_TRUE(persisted_config().empty());
   EXPECT_TRUE(persisted_config_retrieval_time().is_null());
 
@@ -617,7 +628,7 @@ TEST_F(DataReductionProxyConfigServiceClientTest,
   config_client()->RetrieveConfig();
   RunUntilIdle();
   EXPECT_EQ(1, config_client()->failed_attempts_before_success());
-  EXPECT_EQ(base::TimeDelta::FromSeconds(20), config_client()->GetDelay());
+  EXPECT_EQ(base::TimeDelta::FromSeconds(30), config_client()->GetDelay());
   EXPECT_EQ(std::vector<net::ProxyServer>(), GetConfiguredProxiesForHttp());
   EXPECT_TRUE(request_options()->GetSecureSession().empty());
 
@@ -662,8 +673,8 @@ TEST_F(DataReductionProxyConfigServiceClientTest, OnIPAddressChange) {
     }
 
     // Verify that the backoff increased exponentially.
-    EXPECT_EQ(base::TimeDelta::FromSeconds(320),
-              config_client()->GetDelay());  // 320 = 20 * 2^(5-1)
+    EXPECT_EQ(base::TimeDelta::FromSeconds(2430),
+              config_client()->GetDelay());  // 2430 = 30 * 3^(5-1)
     EXPECT_EQ(kFailureCount, config_client()->GetBackoffErrorCount());
 
     // IP address change should reset.
@@ -705,8 +716,8 @@ TEST_F(DataReductionProxyConfigServiceClientTest,
   }
 
   // Verify that the backoff increased exponentially.
-  EXPECT_EQ(base::TimeDelta::FromSeconds(320),
-            config_client()->GetDelay());  // 320 = 20 * 2^(5-1)
+  EXPECT_EQ(base::TimeDelta::FromSeconds(2430),
+            config_client()->GetDelay());  // 2430 = 30 * 3^(5-1)
   EXPECT_EQ(kFailureCount, config_client()->GetBackoffErrorCount());
 
   // IP address change should reset.
@@ -1437,12 +1448,12 @@ TEST_F(DataReductionProxyConfigServiceClientTest, FetchConfigOnForeground) {
     EXPECT_FALSE(config_client()->foreground_fetch_pending());
     histogram_tester.ExpectTotalCount(
         "DataReductionProxy.ConfigService.FetchLatency", 0);
-    EXPECT_EQ(base::TimeDelta::FromSeconds(20), config_client()->GetDelay());
+    EXPECT_EQ(base::TimeDelta::FromSeconds(30), config_client()->GetDelay());
     config_client()->TriggerApplicationStatusToForeground();
     RunUntilIdle();
     histogram_tester.ExpectTotalCount(
         "DataReductionProxy.ConfigService.FetchLatency", 0);
-    EXPECT_EQ(base::TimeDelta::FromSeconds(20), config_client()->GetDelay());
+    EXPECT_EQ(base::TimeDelta::FromSeconds(30), config_client()->GetDelay());
   }
 
   {
@@ -1470,6 +1481,55 @@ TEST_F(DataReductionProxyConfigServiceClientTest, FetchConfigOnForeground) {
     VerifyRemoteSuccess(true);
   }
 }
+
+class DataReductionProxyAggressiveConfigServiceClientTest
+    : public DataReductionProxyConfigServiceClientTest {
+ public:
+  DataReductionProxyAggressiveConfigServiceClientTest()
+      : DataReductionProxyConfigServiceClientTest(true) {}
+};
+
+TEST_F(DataReductionProxyAggressiveConfigServiceClientTest,
+       AggressiveFetchConfigOnBackground) {
+  Init(true);
+  SetDataReductionProxyEnabled(true, true);
+
+  // Tests that config fetch failures while Chromium is in background, trigger
+  // refetches while still in background, and no refetch happens Chromium
+  // comes to foreground, when the aggressive client config fetch feature is
+  // enabled.
+  base::HistogramTester histogram_tester;
+  AddMockFailure();
+  AddMockFailure();
+  AddMockSuccess();
+  config_client()->set_application_state_background(true);
+  config_client()->RetrieveConfig();
+  RunUntilIdle();
+  // Three fetches are triggered in background without any backoff. First two
+  // fail, while the third succeeds.
+  EXPECT_EQ(base::TimeDelta::FromSeconds(0),
+            config_client()->GetBackoffTimeUntilRelease());
+  EXPECT_EQ(base::TimeDelta::FromSeconds(kConfigRefreshDurationSeconds),
+            config_client()->GetDelay());
+  EXPECT_FALSE(config_client()->foreground_fetch_pending());
+  histogram_tester.ExpectTotalCount(
+      "DataReductionProxy.ConfigService.FetchLatency", 1);
+  histogram_tester.ExpectBucketCount(
+      "DataReductionProxy.ConfigService.FetchFailedAttemptsBeforeSuccess", 2,
+      1);
+
+  // No new fetch should happen when Chromium comes to foreground.
+  config_client()->set_application_state_background(false);
+  config_client()->TriggerApplicationStatusToForeground();
+  RunUntilIdle();
+  EXPECT_FALSE(config_client()->foreground_fetch_pending());
+  histogram_tester.ExpectTotalCount(
+      "DataReductionProxy.ConfigService.FetchLatency", 1);
+  EXPECT_EQ(base::TimeDelta::FromSeconds(kConfigRefreshDurationSeconds),
+            config_client()->GetDelay());
+  VerifyRemoteSuccess(true);
+}
+
 #endif
 
 }  // namespace data_reduction_proxy

components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc

@@ -68,15 +68,14 @@ enum TestContextOptions {
 
 const char kTestKey[] = "test-key";
 
-const net::BackoffEntry::Policy kTestBackoffPolicy = {
-    0,               // num_errors_to_ignore
-    10 * 1000,       // initial_delay_ms
-    2,               // multiply_factor
-    0,               // jitter_factor
-    30 * 60 * 1000,  // maximum_backoff_ms
-    -1,              // entry_lifetime_ms
-    true,            // always_use_initial_delay
-};
+net::BackoffEntry::Policy kTestBackoffPolicy;
+
+const net::BackoffEntry::Policy& GetTestBackoffPolicy() {
+  kTestBackoffPolicy = data_reduction_proxy::GetBackoffPolicy();
+  // Remove jitter to bring certainty in the tests.
+  kTestBackoffPolicy.jitter_factor = 0;
+  return kTestBackoffPolicy;
+}
 
 }  // namespace
 
@@ -109,8 +108,9 @@ TestDataReductionProxyConfigServiceClient::
         DataReductionProxyConfig* config,
         DataReductionProxyIOData* io_data,
         network::NetworkConnectionTracker* network_connection_tracker,
-        ConfigStorer config_storer)
-    : DataReductionProxyConfigServiceClient(kTestBackoffPolicy,
+        ConfigStorer config_storer,
+        const net::BackoffEntry::Policy& backoff_policy)
+    : DataReductionProxyConfigServiceClient(backoff_policy,
                                             request_options,
                                             config_values,
                                             config,
@@ -121,7 +121,7 @@ TestDataReductionProxyConfigServiceClient::
       is_application_state_background_(false),
 #endif
       tick_clock_(base::Time::UnixEpoch()),
-      test_backoff_entry_(&kTestBackoffPolicy, &tick_clock_) {
+      test_backoff_entry_(&backoff_policy, &tick_clock_) {
 }
 
 TestDataReductionProxyConfigServiceClient::
@@ -141,6 +141,11 @@ base::TimeDelta TestDataReductionProxyConfigServiceClient::GetDelay() const {
   return config_refresh_timer_.GetCurrentDelay();
 }
 
+base::TimeDelta
+TestDataReductionProxyConfigServiceClient::GetBackoffTimeUntilRelease() const {
+  return test_backoff_entry_.GetTimeUntilRelease();
+}
+
 int TestDataReductionProxyConfigServiceClient::GetBackoffErrorCount() {
   return test_backoff_entry_.failure_count();
 }
@@ -515,7 +520,8 @@ DataReductionProxyTestContext::Builder::Build() {
         std::move(params), io_data->request_options(), raw_mutable_config,
         io_data->config(), io_data.get(), test_network_connection_tracker,
         base::BindRepeating(&TestConfigStorer::StoreSerializedConfig,
-                            base::Unretained(config_storer.get()))));
+                            base::Unretained(config_storer.get())),
+        GetTestBackoffPolicy()));
   } else if (use_config_client_) {
     config_client.reset(new DataReductionProxyConfigServiceClient(
         GetBackoffPolicy(), io_data->request_options(), raw_mutable_config,

components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h

@@ -98,7 +98,8 @@ class TestDataReductionProxyConfigServiceClient
       DataReductionProxyConfig* config,
       DataReductionProxyIOData* io_data,
       network::NetworkConnectionTracker* network_connection_tracker,
-      ConfigStorer config_storer);
+      ConfigStorer config_storer,
+      const net::BackoffEntry::Policy& backoff_policy);
 
   ~TestDataReductionProxyConfigServiceClient() override;
 
@@ -112,6 +113,8 @@ class TestDataReductionProxyConfigServiceClient
 
   int GetBackoffErrorCount();
 
+  base::TimeDelta GetBackoffTimeUntilRelease() const;
+
   void SetConfigServiceURL(const GURL& service_url);
 
   int32_t failed_attempts_before_success() const;

components/data_reduction_proxy/core/common/data_reduction_proxy_features.cc

@@ -67,5 +67,11 @@ const base::Feature kDataReductionProxyDisableProxyFailedWarmup{
 const base::Feature kDataReductionProxyServerExperiments{
     "DataReductionProxyServerExperiments", base::FEATURE_DISABLED_BY_DEFAULT};
 
+// Enables fetching the client config aggressively by tuning the backoff params
+// and by not deferring fetches while Chrome is in background.
+const base::Feature kDataReductionProxyAggressiveConfigFetch{
+    "DataReductionProxyAggressiveConfigFetch",
+    base::FEATURE_ENABLED_BY_DEFAULT};
+
 }  // namespace features
 }  // namespace data_reduction_proxy

components/data_reduction_proxy/core/common/data_reduction_proxy_features.h

@@ -20,6 +20,7 @@ extern const base::Feature kDataReductionProxyBlockOnBadGatewayResponse;
 extern const base::Feature kDataReductionProxyPopulatePreviewsPageIDToPingback;
 extern const base::Feature kDataReductionProxyDisableProxyFailedWarmup;
 extern const base::Feature kDataReductionProxyServerExperiments;
+extern const base::Feature kDataReductionProxyAggressiveConfigFetch;
 
 }  // namespace features
 }  // namespace data_reduction_proxy