Adding a pref to preserve search prefetch redirects likely in disk cache

This allows session restore to use the disk cache for prefetched search
navigations (it also supports cross-session forward back). The pref is
lossy since it would not matter too much to miss these cases
occasionally, and the pref is updated somewhere around 1% of
navigations.

Bug: 1162121
Change-Id: I967a0cd784ad9b9768cac84878a9b683cbd698ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2613357
Commit-Queue: Ryan Sturm <ryansturm@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Robert Ogden <robertogden@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841269}

chrome/browser/BUILD.gn

@@ -1324,6 +1324,8 @@ static_library("browser") {
     "prefetch/no_state_prefetch/prerender_link_manager_factory.h",
     "prefetch/no_state_prefetch/prerender_manager_factory.cc",
     "prefetch/no_state_prefetch/prerender_manager_factory.h",
+    "prefetch/pref_names.cc",
+    "prefetch/pref_names.h",
     "prefetch/prefetch_proxy/prefetch_proxy_features.cc",
     "prefetch/prefetch_proxy/prefetch_proxy_features.h",
     "prefetch/prefetch_proxy/prefetch_proxy_from_string_url_loader.cc",

chrome/browser/prefetch/pref_names.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/prefetch/pref_names.h"
+
+namespace prefetch {
+namespace prefs {
+
+// This pref contains a dictionary value whose keys are string representations
+// of a URL. The values are a tuple (as a List Value) where the first value is a
+// string representation of a URL and a a base::Time. This pref is limited to 10
+// entries in the dictionary.
+// The two URLs are not the same URL.
+const char kCachePrefPath[] = "prefetch.search_prefetch.cache";
+
+// 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 prefs
+}  // namespace prefetch

chrome/browser/prefetch/pref_names.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_PREFETCH_PREF_NAMES_H_
+#define CHROME_BROWSER_PREFETCH_PREF_NAMES_H_
+
+namespace prefetch {
+namespace prefs {
+
+extern const char kCachePrefPath[];
+
+extern const char kRetryAfterPrefPath[];
+
+}  // namespace prefs
+}  // namespace prefetch
+
+#endif  // CHROME_BROWSER_PREFETCH_PREF_NAMES_H_

chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_origin_decider.cc

@@ -9,26 +9,17 @@
 
 #include "base/metrics/histogram_macros.h"
 #include "base/util/values/values_util.h"
+#include "chrome/browser/prefetch/pref_names.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"
 
-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,
+  registry->RegisterDictionaryPref(prefetch::prefs::kRetryAfterPrefPath,
                                    PrefRegistry::LOSSY_PREF);
 }
 
@@ -90,7 +81,7 @@ void PrefetchProxyOriginDecider::LoadFromPrefs() {
   origin_retry_afters_.clear();
 
   const base::DictionaryValue* dictionary =
-      pref_service_->GetDictionary(kRetryAfterPrefPath);
+      pref_service_->GetDictionary(prefetch::prefs::kRetryAfterPrefPath);
   DCHECK(dictionary);
 
   for (const auto& element : *dictionary) {
@@ -122,7 +113,7 @@ void PrefetchProxyOriginDecider::SaveToPrefs() const {
     base::Value value = util::TimeToValue(element.second);
     dictionary.SetKey(std::move(key), std::move(value));
   }
-  pref_service_->Set(kRetryAfterPrefPath, dictionary);
+  pref_service_->Set(prefetch::prefs::kRetryAfterPrefPath, dictionary);
 }
 
 bool PrefetchProxyOriginDecider::ClearPastEntries() {

chrome/browser/prefetch/search_prefetch/search_prefetch_service.cc

@@ -8,8 +8,11 @@
 #include "base/callback.h"
 #include "base/location.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/util/values/values_util.h"
+#include "base/values.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/net/prediction_options.h"
+#include "chrome/browser/prefetch/pref_names.h"
 #include "chrome/browser/prefetch/search_prefetch/back_forward_search_prefetch_url_loader.h"
 #include "chrome/browser/prefetch/search_prefetch/field_trial_settings.h"
 #include "chrome/browser/prefetch/search_prefetch/full_body_search_prefetch_request.h"
@@ -25,6 +28,7 @@
 #include "components/omnibox/browser/base_search_provider.h"
 #include "components/omnibox/browser/omnibox_event_global_tracker.h"
 #include "components/omnibox/browser/omnibox_log.h"
+#include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "components/search_engines/template_url_service.h"
 #include "url/origin.h"
@@ -75,9 +79,20 @@ void RecordFinalStatus(SearchPrefetchStatus status) {
 
 }  // namespace
 
+// static
+void SearchPrefetchService::RegisterProfilePrefs(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(prefetch::prefs::kCachePrefPath,
+                                   PrefRegistry::LOSSY_PREF);
+}
+
 SearchPrefetchService::SearchPrefetchService(Profile* profile)
     : profile_(profile) {
   DCHECK(!profile_->IsOffTheRecord());
+
+  if (LoadFromPrefs())
+    SaveToPrefs();
 }
 
 SearchPrefetchService::~SearchPrefetchService() = default;
@@ -328,6 +343,7 @@ void SearchPrefetchService::ClearPrefetches() {
   prefetches_.clear();
   prefetch_expiry_timers_.clear();
   prefetch_cache_.clear();
+  SaveToPrefs();
 }
 
 void SearchPrefetchService::DeletePrefetch(base::string16 search_terms) {
@@ -438,19 +454,28 @@ void SearchPrefetchService::OnTemplateURLServiceChanged() {
 }
 
 void SearchPrefetchService::ClearCacheEntry(const GURL& navigation_url) {
+  if (prefetch_cache_.find(navigation_url) == prefetch_cache_.end()) {
+    return;
+  }
+
   prefetch_cache_.erase(navigation_url);
+  SaveToPrefs();
 }
 
 void SearchPrefetchService::UpdateServeTime(const GURL& navigation_url) {
   if (prefetch_cache_.find(navigation_url) == prefetch_cache_.end())
     return;
+
   prefetch_cache_[navigation_url].second = base::Time::Now();
+  SaveToPrefs();
 }
 
 void SearchPrefetchService::AddCacheEntry(const GURL& navigation_url,
                                           const GURL& prefetch_url) {
-  // TODO(ryansturm): Add prefs support to handle session restore.
-  // https://crbug.com/1162121.
+  if (navigation_url == prefetch_url) {
+    return;
+  }
+
   prefetch_cache_.emplace(navigation_url,
                           std::make_pair(prefetch_url, base::Time::Now()));
 
@@ -468,4 +493,100 @@ void SearchPrefetchService::AddCacheEntry(const GURL& navigation_url,
     }
   }
   ClearCacheEntry(url_to_remove);
+  SaveToPrefs();
+}
+
+bool SearchPrefetchService::LoadFromPrefs() {
+  prefetch_cache_.clear();
+  const base::DictionaryValue* dictionary =
+      profile_->GetPrefs()->GetDictionary(prefetch::prefs::kCachePrefPath);
+  DCHECK(dictionary);
+
+  auto* template_url_service =
+      TemplateURLServiceFactory::GetForProfile(profile_);
+  if (!template_url_service ||
+      !template_url_service->GetDefaultSearchProvider()) {
+    return dictionary->size() > 0;
+  }
+
+  for (const auto& element : *dictionary) {
+    GURL navigation_url(element.first);
+    if (!navigation_url.is_valid()) {
+      continue;
+    }
+
+    if (!element.second) {
+      continue;
+    }
+
+    base::Value::ConstListView const prefetch_url_and_time =
+        base::Value::AsListValue(*element.second).GetList();
+
+    if (prefetch_url_and_time.size() != 2 ||
+        !prefetch_url_and_time[0].is_string() ||
+        !prefetch_url_and_time[1].is_string()) {
+      continue;
+    }
+
+    std::string prefetch_url;
+    if (!prefetch_url_and_time[0].GetAsString(&prefetch_url)) {
+      continue;
+    }
+
+    // Make sure we are only mapping same origin in case of corrupted prefs.
+    if (url::Origin::Create(navigation_url) !=
+        url::Origin::Create(GURL(prefetch_url))) {
+      continue;
+    }
+
+    // Don't redirect same URL.
+    if (navigation_url == prefetch_url) {
+      continue;
+    }
+
+    // Make sure the navigation URL is still a search URL.
+    base::string16 search_terms;
+    template_url_service->GetDefaultSearchProvider()->ExtractSearchTermsFromURL(
+        navigation_url, template_url_service->search_terms_data(),
+        &search_terms);
+
+    if (search_terms.size() == 0) {
+      continue;
+    }
+
+    base::Optional<base::Time> last_update =
+        util::ValueToTime(prefetch_url_and_time[1]);
+    if (!last_update) {
+      continue;
+    }
+
+    // This time isn't valid.
+    if (last_update.value() > base::Time::Now()) {
+      continue;
+    }
+
+    prefetch_cache_.emplace(
+        navigation_url,
+        std::make_pair(GURL(prefetch_url), last_update.value()));
+  }
+  return dictionary->size() > prefetch_cache_.size();
+}
+
+void SearchPrefetchService::SaveToPrefs() const {
+  base::DictionaryValue dictionary;
+  for (const auto& element : prefetch_cache_) {
+    std::string navigation_url = element.first.spec();
+    std::string prefetch_url = element.second.first.spec();
+    auto time =
+        std::make_unique<base::Value>(util::TimeToValue(element.second.second));
+    base::ListValue value;
+    value.AppendString(prefetch_url);
+    value.Append(std::move(time));
+    dictionary.SetKey(std::move(navigation_url), std::move(value));
+  }
+  profile_->GetPrefs()->Set(prefetch::prefs::kCachePrefPath, dictionary);
+}
+
+bool SearchPrefetchService::LoadFromPrefsForTesting() {
+  return LoadFromPrefs();
 }

chrome/browser/prefetch/search_prefetch/search_prefetch_service.h

@@ -22,6 +22,7 @@
 
 class AutocompleteController;
 struct OmniboxLog;
+class PrefRegistrySimple;
 class Profile;
 class SearchPrefetchURLLoader;
 
@@ -117,6 +118,11 @@ class SearchPrefetchService : public KeyedService,
   base::Optional<SearchPrefetchStatus> GetSearchPrefetchStatusForTesting(
       base::string16 search_terms);
 
+  // Calls |LoadFromPrefs()|.
+  bool LoadFromPrefsForTesting();
+
+  static void RegisterProfilePrefs(PrefRegistrySimple* registry);
+
  private:
   // Records a cache entry for a navigation that is being served.
   void AddCacheEntry(const GURL& navigation_url, const GURL& prefetch_url);
@@ -132,6 +138,14 @@ class SearchPrefetchService : public KeyedService,
   // closes.
   void OnURLOpenedFromOmnibox(OmniboxLog* log);
 
+  // These methods serialize and deserialize |prefetch_cache_| to
+  // |profile_| pref service in a dictionary value.
+  //
+  // Returns true iff loading the prefs removed at least one entry, so the pref
+  // should be saved.
+  bool LoadFromPrefs();
+  void SaveToPrefs() const;
+
   // Prefetches that are started are stored using search terms as a key. Only
   // one prefetch should be started for a given search term until the old
   // prefetch expires.

chrome/browser/prefetch/search_prefetch/search_prefetch_service_browsertest.cc

@@ -1260,6 +1260,72 @@ IN_PROC_BROWSER_TEST_P(SearchPrefetchServiceEnabledBrowserTest,
   EXPECT_TRUE(base::Contains(inner_html, "prefetch"));
 }
 
+IN_PROC_BROWSER_TEST_P(SearchPrefetchServiceEnabledBrowserTest,
+                       BackPrefetchServedAfterPrefs) {
+  // This test prefetches and serves two SRP responses. It then navigates back
+  // then forward, the back navigation should not be cached, due to cache limit
+  // size of 1, the second navigation should be cached.
+
+  base::HistogramTester histogram_tester;
+  auto* search_prefetch_service =
+      SearchPrefetchServiceFactory::GetForProfile(browser()->profile());
+  EXPECT_NE(nullptr, search_prefetch_service);
+
+  std::string search_terms = "prefetch_content";
+  GURL prefetch_url = GetSearchServerQueryURL(search_terms + "&pf=cs");
+  EXPECT_TRUE(search_prefetch_service->MaybePrefetchURL(prefetch_url));
+  WaitUntilStatusChangesTo(base::ASCIIToUTF16(search_terms),
+                           SearchPrefetchStatus::kComplete);
+  ui_test_utils::NavigateToURL(browser(),
+                               GetSearchServerQueryURL(search_terms));
+
+  // The prefetch should be served, and only 1 request should be issued.
+  EXPECT_EQ(1u, search_server_requests().size());
+  auto inner_html = GetDocumentInnerHTML();
+  EXPECT_FALSE(base::Contains(inner_html, "regular"));
+  EXPECT_TRUE(base::Contains(inner_html, "prefetch"));
+
+  search_terms = "prefetch_content_2";
+  prefetch_url = GetSearchServerQueryURL(search_terms + "&pf=cs");
+  EXPECT_TRUE(search_prefetch_service->MaybePrefetchURL(prefetch_url));
+  WaitUntilStatusChangesTo(base::ASCIIToUTF16(search_terms),
+                           SearchPrefetchStatus::kComplete);
+  ui_test_utils::NavigateToURL(browser(),
+                               GetSearchServerQueryURL(search_terms));
+
+  // The prefetch should be served, and only 1 request (now the second total
+  // request) should be issued.
+  EXPECT_EQ(2u, search_server_requests().size());
+  inner_html = GetDocumentInnerHTML();
+  EXPECT_FALSE(base::Contains(inner_html, "regular"));
+  EXPECT_TRUE(base::Contains(inner_html, "prefetch"));
+
+  content::TestNavigationObserver back_load_observer(GetWebContents());
+  GetWebContents()->GetController().GoBack();
+  back_load_observer.Wait();
+
+  // There should not be a cached prefetch request, so there should be a network
+  // request.
+  EXPECT_EQ(3u, search_server_requests().size());
+  inner_html = GetDocumentInnerHTML();
+  EXPECT_TRUE(base::Contains(inner_html, "regular"));
+  EXPECT_FALSE(base::Contains(inner_html, "prefetch"));
+
+  // Reload the map from prefs.
+  EXPECT_FALSE(search_prefetch_service->LoadFromPrefsForTesting());
+
+  content::TestNavigationObserver forward_load_observer(GetWebContents());
+  GetWebContents()->GetController().GoForward();
+  forward_load_observer.Wait();
+
+  // There should be a cached prefetch request, so there should not be a new
+  // network request.
+  EXPECT_EQ(3u, search_server_requests().size());
+  inner_html = GetDocumentInnerHTML();
+  EXPECT_FALSE(base::Contains(inner_html, "regular"));
+  EXPECT_TRUE(base::Contains(inner_html, "prefetch"));
+}
+
 IN_PROC_BROWSER_TEST_P(SearchPrefetchServiceEnabledBrowserTest,
                        EvictedCacheFallsback) {
   // This test prefetches and serves a SRP responses. It then navigates to a

chrome/browser/prefs/browser_prefs.cc

@@ -54,6 +54,7 @@
 #include "chrome/browser/permissions/quiet_notification_permission_ui_state.h"
 #include "chrome/browser/policy/developer_tools_policy_handler.h"
 #include "chrome/browser/prefetch/prefetch_proxy/prefetch_proxy_origin_decider.h"
+#include "chrome/browser/prefetch/search_prefetch/search_prefetch_service.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"
@@ -883,6 +884,7 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry,
   QuietNotificationPermissionUiState::RegisterProfilePrefs(registry);
   RegisterBrowserUserPrefs(registry);
   safe_browsing::RegisterProfilePrefs(registry);
+  SearchPrefetchService::RegisterProfilePrefs(registry);
   blocked_content::SafeBrowsingTriggeredPopupBlocker::RegisterProfilePrefs(
       registry);
   security_interstitials::InsecureFormBlockingPage::RegisterProfilePrefs(