Add PrefService integration to PrefetchProxyOriginDecider

Persists origin retry-after data to disk. Serialization of a base::Time
into a DictionaryValue was tricky, suggestions welcome.

Bug: 1146210
Change-Id: I57e356c44d014076fcfe3895d82cd7bd0fbdbdcf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2531273Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Commit-Queue: Robert Ogden <robertogden@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827318}

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc

@@ -60,6 +60,9 @@
 #include "chrome/browser/permissions/adaptive_quiet_notification_permission_ui_enabler.h"
 #include "chrome/browser/permissions/permission_decision_auto_blocker_factory.h"
 #include "chrome/browser/prefetch/no_state_prefetch/prerender_manager_factory.h"
+#include "chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_origin_decider.h"
+#include "chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_service.h"
+#include "chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_service_factory.h"
 #include "chrome/browser/prefetch/search_prefetch/search_prefetch_service.h"
 #include "chrome/browser/prefetch/search_prefetch/search_prefetch_service_factory.h"
 #include "chrome/browser/previews/previews_service.h"
@@ -582,6 +585,12 @@ void ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData(
     if (lite_video_keyed_service)
       lite_video_keyed_service->ClearData(delete_begin_, delete_end_);
 
+    PrefetchProxyService* prefetch_proxy_service =
+        PrefetchProxyServiceFactory::GetForProfile(profile_);
+    if (prefetch_proxy_service) {
+      prefetch_proxy_service->origin_decider()->OnBrowsingDataCleared();
+    }
+
 #if defined(OS_ANDROID)
     OomInterventionDecider* oom_intervention_decider =
         OomInterventionDecider::GetForBrowserContext(profile_);

chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_origin_decider.cc

@@ -4,21 +4,51 @@
 
 #include "chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_origin_decider.h"
 
-#include "base/time/default_clock.h"
+#include <memory>
+#include <vector>
+
+#include "base/util/values/values_util.h"
 #include "chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_params.h"
 #include "chrome/browser/profiles/profile.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
 
-PrefetchProxyOriginDecider::PrefetchProxyOriginDecider()
-    : clock_(base::DefaultClock::GetInstance()) {}
+namespace {
+// This pref contains a dictionary value whose keys are string representations
+// of a url::Origin and values are a base::Time. The recorded base::Time is the
+// time at which prefetch requests to the corresponding origin can resume, (any
+// base::Time that is in the past can be removed). Entries to the dictionary are
+// created when a prefetch request gets a 503 response with Retry-After header.
+const char kRetryAfterPrefPath[] =
+    "chrome.prefetch_proxy.origin_decider.retry_after";
+}  // namespace
 
-PrefetchProxyOriginDecider::~PrefetchProxyOriginDecider() = default;
+// static
+void PrefetchProxyOriginDecider::RegisterPrefs(PrefRegistrySimple* registry) {
+  // Some loss in this pref (especially following a browser crash) is well
+  // tolerated and helps ensure the pref service isn't slammed.
+  registry->RegisterDictionaryPref(kRetryAfterPrefPath,
+                                   PrefRegistry::LOSSY_PREF);
+}
+
+PrefetchProxyOriginDecider::PrefetchProxyOriginDecider(
+    PrefService* pref_service,
+    base::Clock* clock)
+    : pref_service_(pref_service), clock_(clock) {
+  DCHECK(pref_service);
+  DCHECK(clock);
 
-void PrefetchProxyOriginDecider::SetClockForTesting(const base::Clock* clock) {
-  clock_ = clock;
+  LoadFromPrefs();
+  if (ClearPastEntries()) {
+    SaveToPrefs();
+  }
 }
 
+PrefetchProxyOriginDecider::~PrefetchProxyOriginDecider() = default;
+
 void PrefetchProxyOriginDecider::OnBrowsingDataCleared() {
   origin_retry_afters_.clear();
+  SaveToPrefs();
 }
 
 bool PrefetchProxyOriginDecider::IsOriginOutsideRetryAfterWindow(
@@ -48,4 +78,62 @@ void PrefetchProxyOriginDecider::ReportOriginRetryAfter(
 
   origin_retry_afters_.emplace(url::Origin::Create(url),
                                clock_->Now() + retry_after);
+  SaveToPrefs();
+}
+
+void PrefetchProxyOriginDecider::LoadFromPrefs() {
+  origin_retry_afters_.clear();
+
+  const base::DictionaryValue* dictionary =
+      pref_service_->GetDictionary(kRetryAfterPrefPath);
+  DCHECK(dictionary);
+
+  for (const auto& element : *dictionary) {
+    GURL url_origin(element.first);
+    if (!url_origin.is_valid()) {
+      // This may happen in the case of corrupted prefs, or otherwise. Handle
+      // gracefully.
+      NOTREACHED();
+      continue;
+    }
+
+    base::Optional<base::Time> retry_after = util::ValueToTime(*element.second);
+    if (!retry_after) {
+      // This may happen in the case of corrupted prefs, or otherwise. Handle
+      // gracefully.
+      NOTREACHED();
+      continue;
+    }
+
+    url::Origin origin = url::Origin::Create(url_origin);
+    origin_retry_afters_.emplace(origin, retry_after.value());
+  }
+}
+
+void PrefetchProxyOriginDecider::SaveToPrefs() const {
+  base::DictionaryValue dictionary;
+  for (const auto& element : origin_retry_afters_) {
+    std::string key = element.first.GetURL().spec();
+    base::Value value = util::TimeToValue(element.second);
+    dictionary.SetKey(std::move(key), std::move(value));
+  }
+  pref_service_->Set(kRetryAfterPrefPath, dictionary);
+}
+
+bool PrefetchProxyOriginDecider::ClearPastEntries() {
+  std::vector<url::Origin> to_remove;
+  for (const auto& entry : origin_retry_afters_) {
+    if (entry.second < clock_->Now()) {
+      to_remove.push_back(entry.first);
+    }
+  }
+
+  if (to_remove.empty()) {
+    return false;
+  }
+
+  for (const auto& rm : to_remove) {
+    origin_retry_afters_.erase(rm);
+  }
+  return true;
 }

chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_origin_decider.h

@@ -8,17 +8,27 @@
 #include <map>
 
 #include "base/time/clock.h"
+#include "base/time/default_clock.h"
 #include "base/time/time.h"
 #include "url/gurl.h"
 #include "url/origin.h"
 
+class PrefService;
+class PrefRegistrySimple;
+
 // A browser-scoped class that maintains persistent logic for when origins
 // should not be prefetched.
 class PrefetchProxyOriginDecider {
  public:
-  PrefetchProxyOriginDecider();
+  explicit PrefetchProxyOriginDecider(
+      PrefService* pref_service,
+      base::Clock* clock = base::DefaultClock::GetInstance());
   ~PrefetchProxyOriginDecider();
 
+  // Registers prefs. Should only be used for storing in a device-local registry
+  // (non-syncing).
+  static void RegisterPrefs(PrefRegistrySimple* registry);
+
   // This should be called anytime browsing data is cleared by the user so that
   // the persistent data store can be cleared as well.
   void OnBrowsingDataCleared();
@@ -32,9 +42,23 @@ class PrefetchProxyOriginDecider {
   // a maximum value provided by experiment params.
   void ReportOriginRetryAfter(const GURL& url, base::TimeDelta retry_after);
 
-  void SetClockForTesting(const base::Clock* clock);
+  PrefetchProxyOriginDecider(const PrefetchProxyOriginDecider&) = delete;
+  PrefetchProxyOriginDecider& operator=(const PrefetchProxyOriginDecider&) =
+      delete;
 
  private:
+  // These methods serialize and deserialize |origin_retry_afters_| to
+  // |pref_service_| in a dictionary value.
+  void LoadFromPrefs();
+  void SaveToPrefs() const;
+
+  // Erases any expired entries in |origin_retry_afters_|, returning true iff
+  // any entries were removed.
+  bool ClearPastEntries();
+
+  // Not owned.
+  PrefService* pref_service_;
+
   const base::Clock* clock_;
 
   // Maps origins to their last known retry_after time.

chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_origin_decider_unittest.cc

@@ -7,58 +7,76 @@
 #include "base/test/scoped_feature_list.h"
 #include "base/test/simple_test_clock.h"
 #include "chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_features.h"
+#include "components/prefs/testing_pref_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-TEST(PrefetchProxyOriginDeciderTest, DefaultEligible) {
-  PrefetchProxyOriginDecider decider;
-  EXPECT_TRUE(decider.IsOriginOutsideRetryAfterWindow(GURL("http://foo.com")));
+class PrefetchProxyOriginDeciderTest : public testing::Test {
+ public:
+  void SetUp() override {
+    PrefetchProxyOriginDecider::RegisterPrefs(pref_service_.registry());
+  }
+
+  std::unique_ptr<PrefetchProxyOriginDecider> NewDecider() {
+    return std::make_unique<PrefetchProxyOriginDecider>(&pref_service_,
+                                                        &clock_);
+  }
+
+  base::SimpleTestClock* clock() { return &clock_; }
+
+ private:
+  TestingPrefServiceSimple pref_service_;
+  base::SimpleTestClock clock_;
+};
+
+TEST_F(PrefetchProxyOriginDeciderTest, DefaultEligible) {
+  auto decider = NewDecider();
+  EXPECT_TRUE(decider->IsOriginOutsideRetryAfterWindow(GURL("http://foo.com")));
 }
 
-TEST(PrefetchProxyOriginDeciderTest, NegativeIgnored) {
+TEST_F(PrefetchProxyOriginDeciderTest, NegativeIgnored) {
   GURL url("http://foo.com");
-  PrefetchProxyOriginDecider decider;
-  decider.ReportOriginRetryAfter(url, base::TimeDelta::FromSeconds(-1));
-  EXPECT_TRUE(decider.IsOriginOutsideRetryAfterWindow(url));
+
+  auto decider = NewDecider();
+  decider->ReportOriginRetryAfter(url, base::TimeDelta::FromSeconds(-1));
+  EXPECT_TRUE(decider->IsOriginOutsideRetryAfterWindow(url));
 }
 
-TEST(PrefetchProxyOriginDeciderTest, MaxCap) {
+TEST_F(PrefetchProxyOriginDeciderTest, MaxCap) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndEnableFeatureWithParameters(
       features::kIsolatePrerenders, {{"max_retry_after_duration_secs", "10"}});
 
   GURL url("http://foo.com");
-  PrefetchProxyOriginDecider decider;
-  base::SimpleTestClock clock;
-  decider.SetClockForTesting(&clock);
 
-  decider.ReportOriginRetryAfter(url, base::TimeDelta::FromSeconds(15));
-  EXPECT_FALSE(decider.IsOriginOutsideRetryAfterWindow(url));
+  auto decider = NewDecider();
 
-  clock.Advance(base::TimeDelta::FromSeconds(11));
-  EXPECT_TRUE(decider.IsOriginOutsideRetryAfterWindow(url));
+  decider->ReportOriginRetryAfter(url, base::TimeDelta::FromSeconds(15));
+  EXPECT_FALSE(decider->IsOriginOutsideRetryAfterWindow(url));
+
+  clock()->Advance(base::TimeDelta::FromSeconds(11));
+  EXPECT_TRUE(decider->IsOriginOutsideRetryAfterWindow(url));
 }
 
-TEST(PrefetchProxyOriginDeciderTest, WaitsForDelta) {
+TEST_F(PrefetchProxyOriginDeciderTest, WaitsForDelta) {
   GURL url("http://foo.com");
-  PrefetchProxyOriginDecider decider;
-  base::SimpleTestClock clock;
-  decider.SetClockForTesting(&clock);
 
-  decider.ReportOriginRetryAfter(url, base::TimeDelta::FromSeconds(15));
+  auto decider = NewDecider();
+
+  decider->ReportOriginRetryAfter(url, base::TimeDelta::FromSeconds(15));
 
   for (size_t i = 0; i <= 15; i++) {
-    EXPECT_FALSE(decider.IsOriginOutsideRetryAfterWindow(url));
-    clock.Advance(base::TimeDelta::FromSeconds(1));
+    EXPECT_FALSE(decider->IsOriginOutsideRetryAfterWindow(url));
+    clock()->Advance(base::TimeDelta::FromSeconds(1));
   }
 
-  EXPECT_TRUE(decider.IsOriginOutsideRetryAfterWindow(url));
+  EXPECT_TRUE(decider->IsOriginOutsideRetryAfterWindow(url));
 }
 
-TEST(PrefetchProxyOriginDeciderTest, ByOrigin) {
-  PrefetchProxyOriginDecider decider;
+TEST_F(PrefetchProxyOriginDeciderTest, ByOrigin) {
+  auto decider = NewDecider();
 
-  decider.ReportOriginRetryAfter(GURL("http://foo.com"),
-                                 base::TimeDelta::FromSeconds(1));
+  decider->ReportOriginRetryAfter(GURL("http://foo.com"),
+                                  base::TimeDelta::FromSeconds(1));
 
   // Any url for the origin should be ineligible.
   for (const GURL& url : {
@@ -67,7 +85,7 @@ TEST(PrefetchProxyOriginDeciderTest, ByOrigin) {
            GURL("http://foo.com/?query=yes"),
        }) {
     SCOPED_TRACE(url);
-    EXPECT_FALSE(decider.IsOriginOutsideRetryAfterWindow(url));
+    EXPECT_FALSE(decider->IsOriginOutsideRetryAfterWindow(url));
   }
 
   // Other origins are eligible.
@@ -77,17 +95,40 @@ TEST(PrefetchProxyOriginDeciderTest, ByOrigin) {
            GURL("http://test.com/"),
        }) {
     SCOPED_TRACE(url);
-    EXPECT_TRUE(decider.IsOriginOutsideRetryAfterWindow(url));
+    EXPECT_TRUE(decider->IsOriginOutsideRetryAfterWindow(url));
   }
 }
 
-TEST(PrefetchProxyOriginDeciderTest, Clear) {
+TEST_F(PrefetchProxyOriginDeciderTest, Clear) {
   GURL url("http://foo.com");
-  PrefetchProxyOriginDecider decider;
 
-  decider.ReportOriginRetryAfter(url, base::TimeDelta::FromSeconds(1));
-  EXPECT_FALSE(decider.IsOriginOutsideRetryAfterWindow(url));
+  auto decider = NewDecider();
 
-  decider.OnBrowsingDataCleared();
-  EXPECT_TRUE(decider.IsOriginOutsideRetryAfterWindow(url));
+  decider->ReportOriginRetryAfter(url, base::TimeDelta::FromSeconds(1));
+  EXPECT_FALSE(decider->IsOriginOutsideRetryAfterWindow(url));
+
+  decider->OnBrowsingDataCleared();
+  EXPECT_TRUE(decider->IsOriginOutsideRetryAfterWindow(url));
+}
+
+TEST_F(PrefetchProxyOriginDeciderTest, PersistentPrefs) {
+  {
+    auto decider = NewDecider();
+
+    decider->ReportOriginRetryAfter(GURL("http://expired.com"),
+                                    base::TimeDelta::FromSeconds(1));
+    decider->ReportOriginRetryAfter(GURL("http://foo.com"),
+                                    base::TimeDelta::FromSeconds(3));
+  }
+
+  clock()->Advance(base::TimeDelta::FromSeconds(2));
+
+  {
+    auto decider = NewDecider();
+
+    EXPECT_TRUE(
+        decider->IsOriginOutsideRetryAfterWindow(GURL("http://expired.com")));
+    EXPECT_FALSE(
+        decider->IsOriginOutsideRetryAfterWindow(GURL("http://foo.com")));
+  }
 }

chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_service.cc

@@ -22,7 +22,8 @@ PrefetchProxyService::PrefetchProxyService(Profile* profile)
     : profile_(profile),
       proxy_configurator_(std::make_unique<PrefetchProxyProxyConfigurator>()),
       origin_prober_(std::make_unique<PrefetchProxyOriginProber>(profile)),
-      origin_decider_(std::make_unique<PrefetchProxyOriginDecider>()) {}
+      origin_decider_(
+          std::make_unique<PrefetchProxyOriginDecider>(profile->GetPrefs())) {}
 
 PrefetchProxyService::~PrefetchProxyService() = default;
 

chrome/browser/prefs/browser_prefs.cc

@@ -52,6 +52,7 @@
 #include "chrome/browser/permissions/quiet_notification_permission_ui_state.h"
 #include "chrome/browser/policy/developer_tools_policy_handler.h"
 #include "chrome/browser/policy/webusb_allow_devices_for_urls_policy_handler.h"
+#include "chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_origin_decider.h"
 #include "chrome/browser/prefs/chrome_pref_service_factory.h"
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
 #include "chrome/browser/prefs/origin_trial_prefs.h"
@@ -817,6 +818,7 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry,
   policy::DeveloperToolsPolicyHandler::RegisterProfilePrefs(registry);
   policy::URLBlocklistManager::RegisterProfilePrefs(registry);
   PrefProxyConfigTrackerImpl::RegisterProfilePrefs(registry);
+  PrefetchProxyOriginDecider::RegisterPrefs(registry);
   PrefsTabHelper::RegisterProfilePrefs(registry, locale);
   PreviewsHTTPSNotificationInfoBarDecider::RegisterProfilePrefs(registry);
   Profile::RegisterProfilePrefs(registry);