Add a mechanism to disable caching of canary check result

In data reduction proxy, add a mechanism to disable the caching of
the canary check result. The goal is to determine if the caching
of the result is the root cause for the drop in page load count when
data saver probing experiment is enabled.

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

components/data_reduction_proxy/core/browser/network_properties_manager.cc

@@ -14,6 +14,7 @@
 #include "base/strings/string_split.h"
 #include "base/time/clock.h"
 #include "base/values.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/prefs/scoped_user_pref_update.h"
 
@@ -236,6 +237,8 @@ void NetworkPropertiesManager::OnChangeInNetworkID(
   if (it != network_properties_container_.end()) {
     network_properties_ = it->second;
     cached_entry_found = true;
+    if (params::ShouldDiscardCanaryCheckResult())
+      network_properties_.set_secure_proxy_disallowed_by_carrier(false);
 
   } else {
     // Reset to default state.
@@ -286,6 +289,8 @@ NetworkPropertiesManager::ConvertDictionaryValueToParsedPrefs(
         GetParsedNetworkProperty(it.second);
     if (!network_properties)
       continue;
+    if (params::ShouldDiscardCanaryCheckResult())
+      network_properties->set_secure_proxy_disallowed_by_carrier(false);
 
     read_prefs.emplace(std::make_pair(it.first, network_properties.value()));
   }

components/data_reduction_proxy/core/browser/network_properties_manager_unittest.cc

@@ -4,14 +4,19 @@
 
 #include "components/data_reduction_proxy/core/browser/network_properties_manager.h"
 
+#include <map>
+
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
-#include "base/test/metrics/histogram_tester.h"
+#include "base/test/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/simple_test_clock.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/default_clock.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/prefs/pref_registry_simple.h"
 #include "components/prefs/testing_pref_service.h"
@@ -35,6 +40,14 @@ class TestNetworkPropertiesManager : public NetworkPropertiesManager {
       scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner)
       : NetworkPropertiesManager(clock, pref_service, ui_task_runner) {}
   ~TestNetworkPropertiesManager() override {}
+
+  static void InitDiscardCanaryCheckResultExperiment(
+      base::test::ScopedFeatureList* scoped_feature_list) {
+    std::map<std::string, std::string> params;
+    params[params::GetDiscardCanaryCheckResultParam()] = "true";
+    scoped_feature_list->InitAndEnableFeatureWithParameters(
+        features::kDataReductionProxyRobustConnection, params);
+  }
 };
 
 TEST(NetworkPropertyTest, TestSetterGetterCaptivePortal) {
@@ -491,6 +504,49 @@ TEST(NetworkPropertyTest, TestDeleteOldValues) {
   }
 }
 
+TEST(NetworkPropertyTest,
+     TestSetterGetterDisallowedByCarrierDiscardingEnabled) {
+  base::test::ScopedFeatureList scoped_feature_list;
+
+  for (bool discard : {false, true}) {
+    if (discard) {
+      TestNetworkPropertiesManager::InitDiscardCanaryCheckResultExperiment(
+          &scoped_feature_list);
+    }
+
+    TestingPrefServiceSimple test_prefs;
+    test_prefs.registry()->RegisterDictionaryPref(prefs::kNetworkProperties);
+    base::MessageLoopForIO loop;
+    TestNetworkPropertiesManager network_properties_manager(
+        &test_prefs, base::ThreadTaskRunnerHandle::Get());
+
+    // First network ID has a captive portal and the canary check failed.
+    std::string first_network_id("test1");
+    network_properties_manager.OnChangeInNetworkID(first_network_id);
+    EXPECT_FALSE(network_properties_manager.IsCaptivePortal());
+    network_properties_manager.SetIsSecureProxyDisallowedByCarrier(true);
+    network_properties_manager.SetIsCaptivePortal(true);
+    EXPECT_TRUE(network_properties_manager.IsSecureProxyDisallowedByCarrier());
+    EXPECT_TRUE(network_properties_manager.IsCaptivePortal());
+    base::RunLoop().RunUntilIdle();
+
+    // Change to a different network. State should be reset when there is a
+    // change in the network ID.
+    std::string second_network_id("test2");
+    network_properties_manager.OnChangeInNetworkID(second_network_id);
+    EXPECT_FALSE(network_properties_manager.IsCaptivePortal());
+    EXPECT_FALSE(network_properties_manager.IsSecureProxyDisallowedByCarrier());
+    base::RunLoop().RunUntilIdle();
+
+    // Change back to |first_network_id|. Captive portal state should be
+    // persisted but the canary check state should not be.
+    network_properties_manager.OnChangeInNetworkID(first_network_id);
+    EXPECT_NE(discard,
+              network_properties_manager.IsSecureProxyDisallowedByCarrier());
+    EXPECT_TRUE(network_properties_manager.IsCaptivePortal());
+  }
+}
+
 }  // namespace
 
 }  // namespace data_reduction_proxy

components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc

@@ -58,6 +58,7 @@ const char kLitePageBlackListVersion[] = "lite-page-blacklist-version";
 
 const char kWarmupFetchCallbackEnabledParam[] = "warmup_fetch_callback_enabled";
 const char kMissingViaBypassDisabledParam[] = "bypass_missing_via_disabled";
+const char kDiscardCanaryCheckResultParam[] = "store_canary_check_result";
 
 bool IsIncludedInFieldTrial(const std::string& name) {
   return base::StartsWith(base::FieldTrialList::FindFullName(name), kEnabled,
@@ -129,6 +130,16 @@ const char* GetMissingViaBypassParamName() {
   return kMissingViaBypassDisabledParam;
 }
 
+const char* GetDiscardCanaryCheckResultParam() {
+  return kDiscardCanaryCheckResultParam;
+}
+
+bool ShouldDiscardCanaryCheckResult() {
+  return GetFieldTrialParamByFeatureAsBool(
+      features::kDataReductionProxyRobustConnection,
+      GetDiscardCanaryCheckResultParam(), false);
+}
+
 bool IsIncludedInServerExperimentsFieldTrial() {
   return !base::CommandLine::ForCurrentProcess()->HasSwitch(
              data_reduction_proxy::switches::

components/data_reduction_proxy/core/common/data_reduction_proxy_params.h

@@ -147,6 +147,14 @@ const char* GetWarmupCallbackParamName();
 // Returns the experiment parameter name to disable missing via header bypasses.
 const char* GetMissingViaBypassParamName();
 
+// Returns the experiment parameter name to discard the cached result for canary
+// check probe.
+const char* GetDiscardCanaryCheckResultParam();
+
+// Returns true if canary check result should not be cached or reused across
+// network changes.
+bool ShouldDiscardCanaryCheckResult();
+
 }  // namespace params
 
 // Provides initialization parameters. Proxy origins, and the secure proxy