[Previews] Persist the last time a hints fetch was attempted.

This change stores the last time a hints fetch was attempted in the
pref service to persist between application restarts. This will limit
the HintsFetcher from sending requests too frequently to the remote
optimization guide if a failure or crash occurs.


Bug: 932707
Change-Id: Iea5fde783f51758ba7e4491d7cbda12991455c37
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1678227Reviewed-by: Doug Arnett <dougarnett@chromium.org>
Reviewed-by: Robert Ogden <robertogden@chromium.org>
Commit-Queue: Michael Crouse <mcrouse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#672715}

components/optimization_guide/optimization_guide_prefs.cc

@@ -14,6 +14,12 @@ namespace prefs {
 // loaded, broken down by hint source.
 const char kHintLoadedCounts[] = "optimization_guide.hint_loaded_counts";
 
+// A pref that stores the last time a hints fetch was attempted. This limits the
+// frequency that hints are fetched and prevents a crash loop that continually
+// fetches hints on startup.
+const char kHintsFetcherLastFetchAttempt[] =
+    "optimization_guide.hintsfetcher.last_fetch_attempt";
+
 // A dictionary pref that stores the set of hosts that cannot have hints fetched
 // for until visited again after DataSaver was enabled. If The hash of the host
 // is in the dictionary, then it is on the blacklist and should not be used, the
@@ -29,6 +35,10 @@ const char kHintsFetcherTopHostBlacklistState[] =
 
 void RegisterProfilePrefs(PrefRegistrySimple* registry) {
   registry->RegisterDictionaryPref(kHintLoadedCounts, PrefRegistry::LOSSY_PREF);
+  registry->RegisterInt64Pref(
+      kHintsFetcherLastFetchAttempt,
+      base::Time().ToDeltaSinceWindowsEpoch().InMicroseconds(),
+      PrefRegistry::LOSSY_PREF);
   registry->RegisterDictionaryPref(kHintsFetcherTopHostBlacklist,
                                    PrefRegistry::LOSSY_PREF);
   registry->RegisterIntegerPref(

components/optimization_guide/optimization_guide_prefs.h

@@ -13,6 +13,7 @@ namespace optimization_guide {
 namespace prefs {
 
 extern const char kHintLoadedCounts[];
+extern const char kHintsFetcherLastFetchAttempt[];
 extern const char kHintsFetcherTopHostBlacklist[];
 extern const char kHintsFetcherTopHostBlacklistState[];
 

components/previews/content/previews_optimization_guide.cc

@@ -15,6 +15,7 @@
 #include "base/time/default_clock.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h"
 #include "components/optimization_guide/hints_component_info.h"
+#include "components/optimization_guide/optimization_guide_prefs.h"
 #include "components/optimization_guide/optimization_guide_service.h"
 #include "components/optimization_guide/proto/hints.pb.h"
 #include "components/prefs/pref_service.h"
@@ -153,9 +154,6 @@ PreviewsOptimizationGuide::PreviewsOptimizationGuide(
       url_loader_factory_(url_loader_factory),
       ui_weak_ptr_factory_(this) {
   DCHECK(optimization_guide_service_);
-  // TODO(mcrouse): This needs to be a pref to persist the last fetch attempt
-  // time and prevent crash loops.
-  last_fetch_attempt_ = base::Time();
   hint_cache_->Initialize(
       ShouldPurgeHintCacheStoreOnStartup(),
       base::BindOnce(&PreviewsOptimizationGuide::OnHintCacheInitialized,
@@ -426,13 +424,28 @@ void PreviewsOptimizationGuide::OnHintsUpdated(
       ShouldOverrideFetchHintsTimer()) {
     // Skip the fetch scheduling logic and perform a hints fetch immediately
     // after initialization.
-    last_fetch_attempt_ = time_clock_->Now();
+    SetLastHintsFetchAttemptTime(time_clock_->Now());
     FetchHints();
   } else {
     ScheduleHintsFetch();
   }
 }
 
+void PreviewsOptimizationGuide::SetLastHintsFetchAttemptTime(
+    base::Time last_attempt_time) {
+  DCHECK(pref_service_);
+  pref_service_->SetInt64(
+      optimization_guide::prefs::kHintsFetcherLastFetchAttempt,
+      last_attempt_time.ToDeltaSinceWindowsEpoch().InMicroseconds());
+}
+
+base::Time PreviewsOptimizationGuide::GetLastHintsFetchAttemptTime() const {
+  DCHECK(pref_service_);
+  return base::Time::FromDeltaSinceWindowsEpoch(
+      base::TimeDelta::FromMicroseconds(pref_service_->GetInt64(
+          optimization_guide::prefs::kHintsFetcherLastFetchAttempt)));
+}
+
 void PreviewsOptimizationGuide::ScheduleHintsFetch() {
   DCHECK(!hints_fetch_timer_.IsRunning());
   DCHECK(pref_service_);
@@ -444,13 +457,13 @@ void PreviewsOptimizationGuide::ScheduleHintsFetch() {
   const base::TimeDelta time_until_update_time =
       hint_cache_->FetchedHintsUpdateTime() - time_clock_->Now();
   const base::TimeDelta time_until_retry =
-      last_fetch_attempt_ + kFetchRetryDelay - time_clock_->Now();
+      GetLastHintsFetchAttemptTime() + kFetchRetryDelay - time_clock_->Now();
   base::TimeDelta fetcher_delay;
   if (time_until_update_time <= base::TimeDelta() &&
       time_until_retry <= base::TimeDelta()) {
     // Fetched hints in the store should be updated and an attempt has not
     // been made in last |kFetchRetryDelay|.
-    last_fetch_attempt_ = time_clock_->Now();
+    SetLastHintsFetchAttemptTime(time_clock_->Now());
     hints_fetch_timer_.Start(FROM_HERE, RandomFetchDelay(), this,
                              &PreviewsOptimizationGuide::FetchHints);
   } else {

components/previews/content/previews_optimization_guide.h

@@ -170,6 +170,12 @@ class PreviewsOptimizationGuide
   // engagement scores using |hints_fetcher_|.
   void FetchHints();
 
+  // Return the time when a hints fetch request was last attempted.
+  base::Time GetLastHintsFetchAttemptTime() const;
+
+  // Set the time when a hints fetch was last attempted to |last_attempt_time|.
+  void SetLastHintsFetchAttemptTime(base::Time last_attempt_time);
+
   // The OptimizationGuideService that this guide is listening to. Not owned.
   optimization_guide::OptimizationGuideService* optimization_guide_service_;
 
@@ -205,8 +211,6 @@ class PreviewsOptimizationGuide
   // Clock used for scheduling the |hints_fetch_timer_|.
   const base::Clock* time_clock_;
 
-  base::Time last_fetch_attempt_;
-
   // A reference to the PrefService for this profile. Not owned.
   PrefService* pref_service_ = nullptr;
 

components/previews/content/previews_optimization_guide_unittest.cc

@@ -299,6 +299,10 @@ class PreviewsOptimizationGuideTest : public ProtoDatabaseProviderTestBase {
     }
   }
 
+  const base::Clock* GetMockClock() const {
+    return scoped_task_environment_.GetMockClock();
+  }
+
   void ResetGuide() {
     guide_.reset();
     RunUntilIdle();
@@ -1924,6 +1928,43 @@ TEST_F(PreviewsOptimizationGuideTest, HintsFetcherDisabled) {
   InitializeFixedCountResourceLoadingHints();
 }
 
+TEST_F(PreviewsOptimizationGuideTest, HintsFetcherLastFetchAtttempt) {
+  base::HistogramTester histogram_tester;
+  base::test::ScopedFeatureList scoped_list;
+  scoped_list.InitAndEnableFeature(features::kOptimizationHintsFetching);
+
+  // Set the last fetch attempt to 5 minutes ago, simulating a short duration
+  // since last execution (simulating browser crash/close-reopen).
+  base::TimeDelta minutes_since_attempt = base::TimeDelta::FromMinutes(5);
+  base::Time last_attempt_time = GetMockClock()->Now() - minutes_since_attempt;
+
+  pref_service()->SetInt64(
+      optimization_guide::prefs::kHintsFetcherLastFetchAttempt,
+      last_attempt_time.ToDeltaSinceWindowsEpoch().InMicroseconds());
+
+  guide()->SetHintsFetcherForTesting(
+      BuildTestHintsFetcher(HintsFetcherEndState::kFetchSuccessWithHints));
+
+  std::vector<std::string> hosts = {"example1.com", "example2.com"};
+  EXPECT_CALL(*top_host_provider(), GetTopHosts(testing::_))
+      .Times(1)
+      .WillRepeatedly(testing::Return(hosts));
+
+  // Load hints so that OnHintsUpdated is called. This will force FetchHints to
+  // be triggered if OptimizationHintsFetching is enabled.
+  InitializeFixedCountResourceLoadingHints();
+
+  // Move the clock forward a short duration to ensure that the hints fetch does
+  // not occur too quickly after the simulated short relaunch.
+  MoveClockForwardBy(base::TimeDelta::FromMinutes(5));
+  EXPECT_FALSE(hints_fetcher()->hints_fetched());
+
+  // Move clock forward by the maximum delay, |kTestFetchRetryDelaySeconds to
+  // trigger the hints fetch.
+  MoveClockForwardBy(base::TimeDelta::FromSeconds(kTestFetchRetryDelaySecs));
+  EXPECT_TRUE(hints_fetcher()->hints_fetched());
+}
+
 class PreviewsOptimizationGuideDataSaverOffTest
     : public PreviewsOptimizationGuideTest {
  public: