Change default behavior to http cache reset to not reset.

In the case where the previous state of the finch trials was
unknown (when the previous status was not saved previously
due to this feature not being in the code), we had been
defaulting to resetting the cache.  This CL changes that
behavior and only resets when there is a known change to the
cache.  It also improves the testing to use PRE_ tests to
more accurately model reloading behavior rather than relying
on explicitly setting variables.

Also fixes flaky behavior introduced due to cross-process
testing.  Note the TODO regarding the bug to clean up the manner
in which we resolve the race condition.

Bug: 1041542,1041810
Change-Id: I9c40c9be9682b5372e1f3fb2851ef6531802f1bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2003249
Commit-Queue: Eric Robinson <ericrobinson@chromium.org>
Reviewed-by: Shivani Sharma <shivanisha@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#733822}

chrome/browser/net/profile_network_context_service.cc

@@ -349,9 +349,9 @@ void ProfileNetworkContextService::RegisterLocalStatePrefs(
       prefs::kAmbientAuthenticationInPrivateModesEnabled,
       static_cast<int>(net::AmbientAuthAllowedProfileTypes::REGULAR_ONLY));
 
-  // For information about whether to reset the HTTP Cache or not, store the
-  // groups for all the relevant experiments.  Initially unknown status for all.
-  registry->RegisterStringPref(kHttpCacheFinchExperimentGroups, "NoneNoneNone");
+  // For information about whether to reset the HTTP Cache or not, defaults
+  // to the empty string, which does not prompt a reset.
+  registry->RegisterStringPref(kHttpCacheFinchExperimentGroups, "");
 }
 
 void ProfileNetworkContextService::DisableQuicIfNotAllowed() {
@@ -606,11 +606,11 @@ bool GetHttpCacheBackendResetParam(PrefService* local_state) {
   base::FieldTrial* field_trial = base::FeatureList::GetFieldTrial(
       net::features::kSplitCacheByNetworkIsolationKey);
   std::string current_field_trial_status =
-      (field_trial ? field_trial->group_name() : "None");
+      (field_trial ? field_trial->group_name() : "None") + " ";
   field_trial = base::FeatureList::GetFieldTrial(
       net::features::kAppendFrameOriginToNetworkIsolationKey);
   current_field_trial_status +=
-      (field_trial ? field_trial->group_name() : "None");
+      (field_trial ? field_trial->group_name() : "None") + " ";
   field_trial = base::FeatureList::GetFieldTrial(
       net::features::kUseRegistrableDomainInNetworkIsolationKey);
   current_field_trial_status +=
@@ -621,7 +621,8 @@ bool GetHttpCacheBackendResetParam(PrefService* local_state) {
   local_state->SetString(kHttpCacheFinchExperimentGroups,
                          current_field_trial_status);
 
-  return current_field_trial_status != previous_field_trial_status;
+  return !previous_field_trial_status.empty() &&
+         current_field_trial_status != previous_field_trial_status;
 }
 
 network::mojom::NetworkContextParamsPtr

chrome/browser/net/profile_network_context_service_browsertest.cc

@@ -17,6 +17,7 @@
 #include "base/task/thread_pool/thread_pool_instance.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/threading/platform_thread.h"  // For |Sleep()|.
 #include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
@@ -156,6 +157,29 @@ IN_PROC_BROWSER_TEST_F(ProfileNetworkContextServiceBrowsertest, BrotliEnabled) {
   EXPECT_TRUE(base::Contains(encodings, "br"));
 }
 
+void CheckCacheResetStatus(base::HistogramTester* histograms, bool reset) {
+  // TODO(crbug/1041810): The failure case, here, is to time out.  Since Chrome
+  // doesn't synchronize cache loading, there's no guarantee that this is
+  // complete and it's merely available at earliest convenience.  If shutdown
+  // occurs prior to the cache being loaded, then nothing is reported.  This
+  // should probably be fixed to avoid the use of the sleep function, but that
+  // will require synchronizing in some meaningful way to guarantee the cache
+  // has been loaded prior to testing the histograms.
+  while (!histograms->GetBucketCount("HttpCache.HardReset", reset)) {
+    content::FetchHistogramsFromChildProcesses();
+    SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
+    base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(5));
+  }
+
+  if (reset) {
+    // Some tests load the cache multiple times, but should only be reset once.
+    EXPECT_EQ(histograms->GetBucketCount("HttpCache.HardReset", true), 1);
+  } else {
+    // Make sure it's never reset.
+    EXPECT_EQ(histograms->GetBucketCount("HttpCache.HardReset", true), 0);
+  }
+}
+
 class ProfileNetworkContextServiceCacheSameBrowsertest
     : public ProfileNetworkContextServiceBrowsertest {
  public:
@@ -170,40 +194,36 @@ class ProfileNetworkContextServiceCacheSameBrowsertest
     ProfileNetworkContextServiceBrowsertest::SetUp();
   }
 
-  void CheckCacheNotReset() {
-    content::FetchHistogramsFromChildProcesses();
-    SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
-    // Some tests load the cache multiple times, so compare to zero.
-    EXPECT_GT(histograms_.GetBucketCount("HttpCache.HardReset", false), 0);
-    // Make sure it's never reset.
-    EXPECT_EQ(histograms_.GetBucketCount("HttpCache.HardReset", true), 0);
-  }
+  base::HistogramTester histograms_;
 
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
-  base::HistogramTester histograms_;
 };
 
-// Flaky on Linux and Mac: https://crbug.com/1041810
-#if defined(OS_LINUX) || defined(OS_MACOSX)
-#define MAYBE_TestCacheResetParameter DISABLED_TestCacheResetParameter
-#else
-#define MAYBE_TestCacheResetParameter TestCacheResetParameter
-#endif
 IN_PROC_BROWSER_TEST_F(ProfileNetworkContextServiceCacheSameBrowsertest,
-                       MAYBE_TestCacheResetParameter) {
-  base::RunLoop().RunUntilIdle();
-  base::ThreadPoolInstance::Get()->FlushForTesting();
+                       PRE_TestCacheResetParameter) {
+  CheckCacheResetStatus(&histograms_, false);
 
-  // At this point, we have already called the initialization once on startup.
+  // At this point, we have already called the initialization.
   // Verify that we have the correct values in the local_state.
   PrefService* local_state = g_browser_process->local_state();
   DCHECK_EQ(
       local_state->GetString(
           "profile_network_context_service.http_cache_finch_experiment_groups"),
-      "NoneNoneNone");
+      "None None None");
+}
+
+IN_PROC_BROWSER_TEST_F(ProfileNetworkContextServiceCacheSameBrowsertest,
+                       TestCacheResetParameter) {
+  CheckCacheResetStatus(&histograms_, false);
 
-  CheckCacheNotReset();
+  // At this point, we have already called the initialization.
+  // Verify that we have the correct values in the local_state.
+  PrefService* local_state = g_browser_process->local_state();
+  DCHECK_EQ(
+      local_state->GetString(
+          "profile_network_context_service.http_cache_finch_experiment_groups"),
+      "None None None");
 }
 
 class ProfileNetworkContextServiceCacheChangeBrowsertest
@@ -217,33 +237,45 @@ class ProfileNetworkContextServiceCacheChangeBrowsertest
   }
   ~ProfileNetworkContextServiceCacheChangeBrowsertest() override = default;
 
-  void CheckCacheReset() {
-    content::FetchHistogramsFromChildProcesses();
-    SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
-    // Some tests load the cache multiple times, but should only be reset once.
-    EXPECT_EQ(histograms_.GetBucketCount("HttpCache.HardReset", true), 1);
-  }
+  base::HistogramTester histograms_;
 
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
-  base::HistogramTester histograms_;
 };
 
 // Flaky on Linux and Mac: https://crbug.com/1041810
+// The first time we load, even if we're in an experiment there's no reset
+// from the unknown state.
 IN_PROC_BROWSER_TEST_F(ProfileNetworkContextServiceCacheChangeBrowsertest,
-                       MAYBE_TestCacheResetParameter) {
-  base::RunLoop().RunUntilIdle();
-  base::ThreadPoolInstance::Get()->FlushForTesting();
+                       PRE_TestCacheResetParameter) {
+  CheckCacheResetStatus(&histograms_, false);
 
-  // At this point, we have already called the initialization once on startup.
+  // At this point, we have already called the initialization.
   // Verify that we have the correct values in the local_state.
   PrefService* local_state = g_browser_process->local_state();
   DCHECK_EQ(
       local_state->GetString(
           "profile_network_context_service.http_cache_finch_experiment_groups"),
-      "Nonescoped_feature_list_trial_groupNone");
+      "None scoped_feature_list_trial_group None");
+  // Set the local state for the next test.
+  local_state->SetString(
+      "profile_network_context_service.http_cache_finch_experiment_groups",
+      "None None None");
+}
 
-  CheckCacheReset();
+// The second time we load we know the state, which was "None None None" for the
+// previous test, so we should see a reset being in an experiment.
+IN_PROC_BROWSER_TEST_F(ProfileNetworkContextServiceCacheChangeBrowsertest,
+                       TestCacheResetParameter) {
+  CheckCacheResetStatus(&histograms_, true);
+
+  // At this point, we have already called the initialization once.
+  // Verify that we have the correct values in the local_state.
+  PrefService* local_state = g_browser_process->local_state();
+  DCHECK_EQ(
+      local_state->GetString(
+          "profile_network_context_service.http_cache_finch_experiment_groups"),
+      "None scoped_feature_list_trial_group None");
 }
 
 class AmbientAuthenticationTestWithPolicy