[Merge 110][ZPS] Enable cross-session persistence for ZPS on NTP.

The current implementation of the in-memory ZPS cache is unable to
persist cached entries across multiple user sessions due to storing
entries entirely in memory. As such, when the user restarts their
browser and/or starts a new session from scratch, it's possible for them
to be presented with zero remote ZPS suggestions when focusing the
omnibox (e.g. on a slow internet connection).

In order to address this edge-case, this CL enhances the in-memory ZPS
caching service with the ability to persist cached entries by means of
the user preferences system which stores data on disk.

Furthermore, this CL introduces a dedicated cache entry for storing
ZPS responses received on the NTP (as opposed to simply caching them
alongside ZPS responses for SRP/Web). This change will ensure that
the NTP-related entry will no longer be evicted out of the cache due
to a large number of SRP/Web-related cache entries.

(cherry picked from commit bafe6e6)

Bug: 1351224
Change-Id: Ia3a697e2487c37f48b5ad4e8e3a55bd5d2147bde
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4148352
Reviewed-by: Robbie Gibson <rkgibson@google.com>
Reviewed-by: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Commit-Queue: Khalid Peer <khalidpeer@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1090655}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4156972
Cr-Commit-Position: refs/branch-heads/5481@{#251}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}

chrome/browser/autocomplete/zero_suggest_cache_service_factory.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/autocomplete/zero_suggest_cache_service_factory.h"
 
+#include "chrome/browser/profiles/profile.h"
 #include "components/omnibox/browser/omnibox_field_trial.h"
 
 // static
@@ -20,8 +21,9 @@ ZeroSuggestCacheServiceFactory* ZeroSuggestCacheServiceFactory::GetInstance() {
 
 KeyedService* ZeroSuggestCacheServiceFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
+  Profile* profile = Profile::FromBrowserContext(context);
   return new ZeroSuggestCacheService(
-      OmniboxFieldTrial::kZeroSuggestCacheMaxSize.Get());
+      profile->GetPrefs(), OmniboxFieldTrial::kZeroSuggestCacheMaxSize.Get());
 }
 
 ZeroSuggestCacheServiceFactory::ZeroSuggestCacheServiceFactory()

components/omnibox/browser/zero_suggest_cache_service.cc

@@ -8,27 +8,60 @@
 #include "base/trace_event/memory_usage_estimator.h"
 #include "components/omnibox/browser/autocomplete_input.h"
 #include "components/omnibox/browser/autocomplete_provider_client.h"
+#include "components/omnibox/browser/omnibox_prefs.h"
 #include "components/omnibox/browser/search_suggestion_parser.h"
+#include "components/prefs/pref_service.h"
 
 using CacheEntry = ZeroSuggestCacheService::CacheEntry;
 
-ZeroSuggestCacheService::ZeroSuggestCacheService(size_t cache_size)
-    : cache_(cache_size) {}
+ZeroSuggestCacheService::ZeroSuggestCacheService(PrefService* prefs,
+                                                 size_t cache_size)
+    : prefs_(prefs), cache_(cache_size) {
+  DCHECK_GT(cache_size, 0u);
 
-ZeroSuggestCacheService::~ZeroSuggestCacheService() = default;
+  if (prefs_) {
+    // Load cached ZPS response for NTP from prior session via prefs.
+    ntp_entry_.response_json =
+        omnibox::GetUserPreferenceForZeroSuggestCachedResponse(prefs_,
+                                                               /*page_url=*/"");
+  }
+}
+
+ZeroSuggestCacheService::~ZeroSuggestCacheService() {
+  if (prefs_) {
+    // Dump cached ZPS response for NTP to prefs.
+    omnibox::SetUserPreferenceForZeroSuggestCachedResponse(
+        prefs_, /*page_url=*/"", /*response=*/ntp_entry_.response_json);
+  }
+}
 
 CacheEntry ZeroSuggestCacheService::ReadZeroSuggestResponse(
     const std::string& page_url) const {
+  // Read cached ZPS response for NTP.
+  if (page_url.empty()) {
+    return ntp_entry_;
+  }
+
+  // Read cached ZPS response for SRP/Web.
   const auto it = cache_.Get(page_url);
   return it != cache_.end() ? it->second : CacheEntry();
 }
 
 void ZeroSuggestCacheService::StoreZeroSuggestResponse(
     const std::string& page_url,
     const CacheEntry& response) {
-  cache_.Put(page_url, response);
-  base::UmaHistogramCounts1M("Omnibox.ZeroSuggestProvider.CacheMemoryUsage",
-                             base::trace_event::EstimateMemoryUsage(cache_));
+  if (page_url.empty()) {
+    // Write ZPS response for NTP to cache.
+    ntp_entry_ = response;
+  } else {
+    // Write ZPS response for SRP/Web to cache.
+    cache_.Put(page_url, response);
+  }
+
+  base::UmaHistogramCounts1M(
+      "Omnibox.ZeroSuggestProvider.CacheMemoryUsage",
+      base::trace_event::EstimateMemoryUsage(cache_) +
+          base::trace_event::EstimateMemoryUsage(ntp_entry_));
 
   for (auto& observer : observers_) {
     observer.OnZeroSuggestResponseUpdated(page_url, response);

components/omnibox/browser/zero_suggest_cache_service.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_OMNIBOX_BROWSER_ZERO_SUGGEST_CACHE_SERVICE_H_
 #define COMPONENTS_OMNIBOX_BROWSER_ZERO_SUGGEST_CACHE_SERVICE_H_
 
+#include <memory>
 #include <string>
 
 #include "base/containers/lru_cache.h"
@@ -15,6 +16,7 @@
 #include "components/omnibox/browser/autocomplete_input.h"
 #include "components/omnibox/browser/autocomplete_provider_client.h"
 #include "components/omnibox/browser/search_suggestion_parser.h"
+#include "components/prefs/pref_service.h"
 
 class ZeroSuggestCacheService : public KeyedService {
  public:
@@ -50,7 +52,7 @@ class ZeroSuggestCacheService : public KeyedService {
                                               const CacheEntry& response) {}
   };
 
-  explicit ZeroSuggestCacheService(size_t cache_size);
+  ZeroSuggestCacheService(PrefService* prefs, size_t cache_size);
 
   ZeroSuggestCacheService(const ZeroSuggestCacheService&) = delete;
   ZeroSuggestCacheService& operator=(const ZeroSuggestCacheService&) = delete;
@@ -73,11 +75,16 @@ class ZeroSuggestCacheService : public KeyedService {
   void RemoveObserver(Observer* observer);
 
  private:
+  // Pref service used for in-memory cache data persistence. Not owned.
+  const raw_ptr<PrefService> prefs_;
   // Cache mapping each page URL to the corresponding zero suggest response
   // (serialized JSON). |mutable| is used here because reading from the cache,
   // while logically const, will actually modify the internal recency list of
   // the HashingLRUCache object.
   mutable base::HashingLRUCache<std::string, CacheEntry> cache_;
+  // Dedicated cache entry for "ZPS on NTP" data in order to minimize any
+  // negative impact due to cache eviction policy.
+  CacheEntry ntp_entry_;
   base::ObserverList<Observer> observers_;
 };
 

components/omnibox/browser/zero_suggest_cache_service_unittest.cc

@@ -8,6 +8,9 @@
 #include <string>
 
 #include "base/test/metrics/histogram_tester.h"
+#include "components/omnibox/browser/omnibox_prefs.h"
+#include "components/omnibox/browser/zero_suggest_provider.h"
+#include "components/prefs/testing_pref_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using CacheEntry = ZeroSuggestCacheService::CacheEntry;
@@ -37,21 +40,42 @@ struct TestCacheEntry {
   std::string response;
 };
 
-TEST(ZeroSuggestCacheServiceTest, CacheStartsEmpty) {
-  ZeroSuggestCacheService cache_svc(1);
+class ZeroSuggestCacheServiceTest : public testing::Test {
+ public:
+  ZeroSuggestCacheServiceTest() = default;
+
+  ZeroSuggestCacheServiceTest(const ZeroSuggestCacheServiceTest&) = delete;
+  ZeroSuggestCacheServiceTest& operator=(const ZeroSuggestCacheServiceTest&) =
+      delete;
+
+  void SetUp() override {
+    prefs_ = std::make_unique<TestingPrefServiceSimple>();
+    ZeroSuggestProvider::RegisterProfilePrefs(prefs_->registry());
+  }
+
+  void TearDown() override { prefs_.reset(); }
+
+  PrefService* GetPrefs() { return prefs_.get(); }
+
+ private:
+  std::unique_ptr<TestingPrefServiceSimple> prefs_;
+};
+
+TEST_F(ZeroSuggestCacheServiceTest, CacheStartsEmpty) {
+  ZeroSuggestCacheService cache_svc(GetPrefs(), 1);
   EXPECT_TRUE(cache_svc.IsCacheEmpty());
 }
 
-TEST(ZeroSuggestCacheServiceTest, StoreResponsePopulatesCache) {
-  ZeroSuggestCacheService cache_svc(1);
+TEST_F(ZeroSuggestCacheServiceTest, StoreResponsePopulatesCache) {
+  ZeroSuggestCacheService cache_svc(GetPrefs(), 1);
   cache_svc.StoreZeroSuggestResponse("https://www.google.com",
                                      CacheEntry("foo"));
   EXPECT_FALSE(cache_svc.IsCacheEmpty());
 }
 
-TEST(ZeroSuggestCacheServiceTest, StoreResponseRecordsMemoryUsageHistogram) {
+TEST_F(ZeroSuggestCacheServiceTest, StoreResponseRecordsMemoryUsageHistogram) {
   base::HistogramTester histogram_tester;
-  ZeroSuggestCacheService cache_svc(1);
+  ZeroSuggestCacheService cache_svc(GetPrefs(), 1);
 
   const std::string page_url = "https://www.google.com";
   const std::string response = "foo";
@@ -75,8 +99,8 @@ TEST(ZeroSuggestCacheServiceTest, StoreResponseRecordsMemoryUsageHistogram) {
   histogram_tester.ExpectTotalCount(histogram, 4);
 }
 
-TEST(ZeroSuggestCacheServiceTest, StoreResponseUpdatesExistingEntry) {
-  ZeroSuggestCacheService cache_svc(1);
+TEST_F(ZeroSuggestCacheServiceTest, StoreResponseUpdatesExistingEntry) {
+  ZeroSuggestCacheService cache_svc(GetPrefs(), 1);
 
   const std::string page_url = "https://www.google.com";
   const std::string old_response = "foo";
@@ -91,8 +115,8 @@ TEST(ZeroSuggestCacheServiceTest, StoreResponseUpdatesExistingEntry) {
             new_response);
 }
 
-TEST(ZeroSuggestCacheServiceTest, StoreResponseNotifiesObservers) {
-  ZeroSuggestCacheService cache_svc(2);
+TEST_F(ZeroSuggestCacheServiceTest, StoreResponseNotifiesObservers) {
+  ZeroSuggestCacheService cache_svc(GetPrefs(), 2);
 
   const std::string goog_url = "https://www.google.com";
   const std::string fb_url = "https://www.facebook.com";
@@ -142,8 +166,8 @@ TEST(ZeroSuggestCacheServiceTest, StoreResponseNotifiesObservers) {
   EXPECT_EQ(fb_observer.GetData().response_json, "bar");
 }
 
-TEST(ZeroSuggestCacheServiceTest, LeastRecentItemIsEvicted) {
-  ZeroSuggestCacheService cache_svc(2);
+TEST_F(ZeroSuggestCacheServiceTest, LeastRecentItemIsEvicted) {
+  ZeroSuggestCacheService cache_svc(GetPrefs(), 2);
 
   TestCacheEntry entry1 = {"https://www.facebook.com", "foo"};
   TestCacheEntry entry2 = {"https://www.google.com", "bar"};
@@ -168,8 +192,8 @@ TEST(ZeroSuggestCacheServiceTest, LeastRecentItemIsEvicted) {
             entry3.response);
 }
 
-TEST(ZeroSuggestCacheServiceTest, ReadResponseWillRetrieveMatchingData) {
-  ZeroSuggestCacheService cache_svc(1);
+TEST_F(ZeroSuggestCacheServiceTest, ReadResponseWillRetrieveMatchingData) {
+  ZeroSuggestCacheService cache_svc(GetPrefs(), 1);
 
   const std::string page_url = "https://www.google.com";
   const std::string response = "foo";
@@ -179,8 +203,8 @@ TEST(ZeroSuggestCacheServiceTest, ReadResponseWillRetrieveMatchingData) {
             response);
 }
 
-TEST(ZeroSuggestCacheServiceTest, ReadResponseUpdatesRecency) {
-  ZeroSuggestCacheService cache_svc(2);
+TEST_F(ZeroSuggestCacheServiceTest, ReadResponseUpdatesRecency) {
+  ZeroSuggestCacheService cache_svc(GetPrefs(), 2);
 
   TestCacheEntry entry1 = {"https://www.google.com", "foo"};
   TestCacheEntry entry2 = {"https://www.facebook.com", "bar"};
@@ -206,13 +230,54 @@ TEST(ZeroSuggestCacheServiceTest, ReadResponseUpdatesRecency) {
             entry3.response);
 }
 
-TEST(ZeroSuggestCacheServiceTest, ClearCacheResultsInEmptyCache) {
-  ZeroSuggestCacheService cache_svc(1);
+TEST_F(ZeroSuggestCacheServiceTest, ClearCacheResultsInEmptyCache) {
+  ZeroSuggestCacheService cache_svc(GetPrefs(), 1);
 
   cache_svc.StoreZeroSuggestResponse("https://www.google.com",
                                      CacheEntry("foo"));
 
+  EXPECT_FALSE(cache_svc.IsCacheEmpty());
+
   cache_svc.ClearCache();
 
   EXPECT_TRUE(cache_svc.IsCacheEmpty());
 }
+
+TEST_F(ZeroSuggestCacheServiceTest, CacheLoadsFromPrefsOnStartup) {
+  TestCacheEntry ntp_entry = {"", "foo"};
+
+  PrefService* prefs = GetPrefs();
+  omnibox::SetUserPreferenceForZeroSuggestCachedResponse(prefs, ntp_entry.url,
+                                                         ntp_entry.response);
+  ZeroSuggestCacheService cache_svc(prefs, 1);
+
+  EXPECT_EQ(cache_svc.ReadZeroSuggestResponse(ntp_entry.url).response_json,
+            ntp_entry.response);
+}
+
+TEST_F(ZeroSuggestCacheServiceTest, CacheDumpsToPrefsOnShutdown) {
+  TestCacheEntry ntp_entry = {"", "foo"};
+
+  PrefService* prefs = GetPrefs();
+  {
+    ZeroSuggestCacheService cache_svc(prefs, 1);
+    cache_svc.StoreZeroSuggestResponse(ntp_entry.url,
+                                       CacheEntry(ntp_entry.response));
+  }
+
+  EXPECT_EQ(omnibox::GetUserPreferenceForZeroSuggestCachedResponse(
+                prefs, ntp_entry.url),
+            ntp_entry.response);
+}
+
+TEST_F(ZeroSuggestCacheServiceTest, CacheWorksGivenNullPrefService) {
+  TestCacheEntry ntp_entry = {"", "foo"};
+
+  PrefService* prefs = nullptr;
+  ZeroSuggestCacheService cache_svc(prefs, 1);
+  cache_svc.StoreZeroSuggestResponse(ntp_entry.url,
+                                     CacheEntry(ntp_entry.response));
+
+  EXPECT_EQ(cache_svc.ReadZeroSuggestResponse(ntp_entry.url).response_json,
+            ntp_entry.response);
+}

components/omnibox/browser/zero_suggest_provider_unittest.cc

@@ -52,13 +52,10 @@ class FakeAutocompleteProviderClient : public MockAutocompleteProviderClient {
  public:
   FakeAutocompleteProviderClient()
       : template_url_service_(new TemplateURLService(nullptr, 0)),
-        pref_service_(new TestingPrefServiceSimple()),
-        zero_suggest_cache_service_(
-            std::make_unique<ZeroSuggestCacheService>(kCacheSize)) {
-    pref_service_->registry()->RegisterStringPref(
-        omnibox::kZeroSuggestCachedResults, std::string());
-    pref_service_->registry()->RegisterDictionaryPref(
-        omnibox::kZeroSuggestCachedResultsWithURL, base::Value::Dict());
+        pref_service_(new TestingPrefServiceSimple()) {
+    ZeroSuggestProvider::RegisterProfilePrefs(pref_service_->registry());
+    zero_suggest_cache_service_ = std::make_unique<ZeroSuggestCacheService>(
+        pref_service_.get(), kCacheSize);
   }
   FakeAutocompleteProviderClient(const FakeAutocompleteProviderClient&) =
       delete;

components/optimization_guide/content/browser/BUILD.gn

@@ -95,7 +95,10 @@ source_set("unit_tests") {
   deps = [
     ":browser",
     ":test_support",
+    "//components/history/core/test",
+    "//components/omnibox/browser:test_support",
     "//components/optimization_guide/core:test_support",
+    "//components/prefs:test_support",
     "//components/search_engines",
     "//components/ukm:test_support",
     "//content/test:test_support",

components/optimization_guide/content/browser/DEPS

@@ -3,9 +3,11 @@ include_rules = [
   "+components/continuous_search/common/public/mojom",
   "+components/google/core",
   "+components/history/core/browser",
+  "+components/history/core/test",
   "+components/keyed_service/core",
   "+components/no_state_prefetch/browser",
   "+components/omnibox/browser",
+  "+components/omnibox/common",
   "+components/optimization_guide/core",
   "+components/optimization_guide/proto",
   "+components/search_engines",

components/optimization_guide/content/browser/page_content_annotations_web_contents_observer_unittest.cc

@@ -10,11 +10,18 @@
 #include "base/test/scoped_feature_list.h"
 #include "components/google/core/common/google_switches.h"
 #include "components/history/core/browser/history_service.h"
+#include "components/history/core/browser/history_types.h"
+#include "components/history/core/test/test_history_database.h"
+#include "components/omnibox/browser/fake_autocomplete_provider_client.h"
+#include "components/omnibox/browser/zero_suggest_cache_service.h"
+#include "components/omnibox/browser/zero_suggest_provider.h"
+#include "components/omnibox/common/omnibox_features.h"
 #include "components/optimization_guide/content/browser/page_content_annotations_service.h"
 #include "components/optimization_guide/content/browser/test_optimization_guide_decider.h"
 #include "components/optimization_guide/core/optimization_guide_features.h"
 #include "components/optimization_guide/core/test_optimization_guide_model_provider.h"
 #include "components/optimization_guide/proto/page_entities_metadata.pb.h"
+#include "components/prefs/testing_pref_service.h"
 #include "components/search_engines/template_url_service.h"
 #include "components/ukm/test_ukm_recorder.h"
 #include "content/public/browser/navigation_handle.h"
@@ -194,6 +201,10 @@ class PageContentAnnotationsWebContentsObserverTest
     optimization_guide_model_provider_ =
         std::make_unique<TestOptimizationGuideModelProvider>();
     history_service_ = std::make_unique<history::HistoryService>();
+    pref_service_ = std::make_unique<TestingPrefServiceSimple>();
+    ZeroSuggestProvider::RegisterProfilePrefs(pref_service_->registry());
+    zero_suggest_cache_service_ = std::make_unique<ZeroSuggestCacheService>(
+        pref_service_.get(), /*cache_size=*/1);
     page_content_annotations_service_ =
         std::make_unique<FakePageContentAnnotationsService>(
             optimization_guide_model_provider_.get(), history_service_.get());
@@ -249,6 +260,9 @@ class PageContentAnnotationsWebContentsObserverTest
   std::unique_ptr<TestOptimizationGuideModelProvider>
       optimization_guide_model_provider_;
   std::unique_ptr<history::HistoryService> history_service_;
+  base::ScopedTempDir temp_dir_;
+  std::unique_ptr<TestingPrefServiceSimple> pref_service_;
+  std::unique_ptr<ZeroSuggestCacheService> zero_suggest_cache_service_;
   std::unique_ptr<FakePageContentAnnotationsService>
       page_content_annotations_service_;
   std::unique_ptr<TemplateURLService> template_url_service_;

ios/chrome/browser/autocomplete/zero_suggest_cache_service_factory.cc

@@ -35,7 +35,10 @@ ZeroSuggestCacheServiceFactory::~ZeroSuggestCacheServiceFactory() = default;
 std::unique_ptr<KeyedService>
 ZeroSuggestCacheServiceFactory::BuildServiceInstanceFor(
     web::BrowserState* context) const {
+  ChromeBrowserState* browser_state =
+      ChromeBrowserState::FromBrowserState(context);
   return std::make_unique<ZeroSuggestCacheService>(
+      browser_state->GetPrefs(),
       OmniboxFieldTrial::kZeroSuggestCacheMaxSize.Get());
 }