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/+/2531273
Reviewed-by: Christian Dullweber <[email protected]>
Reviewed-by: Dominic Battré <[email protected]>
Reviewed-by: Ryan Sturm <[email protected]>
Commit-Queue: Robert Ogden <[email protected]>
Cr-Commit-Position: refs/heads/master@{#827318}
diff --git a/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc b/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc
index 0af5e944..0d9e110f 100644
--- a/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc
+++ b/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 @@
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_);
diff --git a/chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_origin_decider.cc b/chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_origin_decider.cc
index 075ad15a..688617c 100644
--- a/chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_origin_decider.cc
+++ b/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
+
+// 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);
+
+ LoadFromPrefs();
+ if (ClearPastEntries()) {
+ SaveToPrefs();
+ }
+}
PrefetchProxyOriginDecider::~PrefetchProxyOriginDecider() = default;
-void PrefetchProxyOriginDecider::SetClockForTesting(const base::Clock* clock) {
- clock_ = clock;
-}
-
void PrefetchProxyOriginDecider::OnBrowsingDataCleared() {
origin_retry_afters_.clear();
+ SaveToPrefs();
}
bool PrefetchProxyOriginDecider::IsOriginOutsideRetryAfterWindow(
@@ -48,4 +78,62 @@
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;
}
diff --git a/chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_origin_decider.h b/chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_origin_decider.h
index 54e79eb..e554601 100644
--- a/chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_origin_decider.h
+++ b/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 @@
// 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.
diff --git a/chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_origin_decider_unittest.cc b/chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_origin_decider_unittest.cc
index a085d64..6ed9ca2 100644
--- a/chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_origin_decider_unittest.cc
+++ b/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 @@
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 @@
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")));
+ }
}
diff --git a/chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_service.cc b/chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_service.cc
index 8b3e0c8..557d997 100644
--- a/chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_service.cc
+++ b/chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_service.cc
@@ -22,7 +22,8 @@
: 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;
diff --git a/chrome/browser/prefs/browser_prefs.cc b/chrome/browser/prefs/browser_prefs.cc
index 7e31648..7f6e892 100644
--- a/chrome/browser/prefs/browser_prefs.cc
+++ b/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 @@
policy::DeveloperToolsPolicyHandler::RegisterProfilePrefs(registry);
policy::URLBlocklistManager::RegisterProfilePrefs(registry);
PrefProxyConfigTrackerImpl::RegisterProfilePrefs(registry);
+ PrefetchProxyOriginDecider::RegisterPrefs(registry);
PrefsTabHelper::RegisterProfilePrefs(registry, locale);
PreviewsHTTPSNotificationInfoBarDecider::RegisterProfilePrefs(registry);
Profile::RegisterProfilePrefs(registry);
