Garbage collect sync local cache GUIDs based on time

There's no point in remembering very old cache GUIDs since features
anyway filter for recent devices, so let's implement a
garbage-collection logic for the cache GUIDs stored in prefs.

Bug: 989340
Change-Id: I1b1f84e3026471f8f687caa54922e2ea43bafaba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1827435Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Auto-Submit: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702052}

chrome/browser/prefs/browser_prefs.cc

@@ -1193,4 +1193,7 @@ void MigrateObsoleteProfilePrefs(Profile* profile) {
   profile_prefs->ClearPref(kGoogleServicesUserAccountId);
   profile_prefs->ClearPref(
       kDataReductionProxySavingsClearedNegativeSystemClock);
+
+  // Added 10/2019.
+  syncer::DeviceInfoPrefs::MigrateRecentLocalCacheGuidsPref(profile_prefs);
 }

chrome/browser/sync/device_info_sync_service_factory.cc

@@ -11,6 +11,7 @@
 
 #include "base/bind.h"
 #include "base/memory/singleton.h"
+#include "base/time/default_clock.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
@@ -119,8 +120,8 @@ KeyedService* DeviceInfoSyncServiceFactory::BuildServiceInstanceFor(
       std::make_unique<syncer::LocalDeviceInfoProviderImpl>(
           chrome::GetChannel(), chrome::GetVersionString(),
           device_info_sync_client.get());
-  auto device_prefs =
-      std::make_unique<syncer::DeviceInfoPrefs>(profile->GetPrefs());
+  auto device_prefs = std::make_unique<syncer::DeviceInfoPrefs>(
+      profile->GetPrefs(), base::DefaultClock::GetInstance());
 
   return new syncer::DeviceInfoSyncServiceImpl(
       ModelTypeStoreServiceFactory::GetForProfile(profile)->GetStoreFactory(),

components/sync_device_info/BUILD.gn

@@ -82,6 +82,7 @@ source_set("unit_tests") {
   testonly = true
   sources = [
     "device_count_metrics_provider_unittest.cc",
+    "device_info_prefs_unittest.cc",
     "device_info_sync_bridge_unittest.cc",
     "device_info_util_unittest.cc",
     "local_device_info_provider_impl_unittest.cc",

components/sync_device_info/device_info_prefs.cc

@@ -5,8 +5,11 @@
 #include "components/sync_device_info/device_info_prefs.h"
 
 #include <algorithm>
+#include <utility>
 #include <vector>
 
+#include "base/time/clock.h"
+#include "base/time/default_clock.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
@@ -14,23 +17,69 @@
 namespace syncer {
 namespace {
 
-// The GUID device info will use to to remember the most recently used past
-// cache GUIDs in order starting with the current cache GUID.
-const char kDeviceInfoRecentGUIDs[] = "sync.local_device_guids";
-
-// The max number of local device most recent cached GUIDSs that will be stored.
-const int kMaxLocalCacheGuidsStored = 30;
+// Name of obsolete preference that stores most recently used past cache
+// GUIDs, most recent first.
+const char kObsoleteDeviceInfoRecentGUIDs[] = "sync.local_device_guids";
+
+// Preference name for storing recently used cache GUIDs and their timestamps
+// in days since Windows epoch. Most recent first.
+const char kDeviceInfoRecentGUIDsWithTimestamps[] =
+    "sync.local_device_guids_with_timestamp";
+
+// Keys used in the dictionaries stored in prefs.
+const char kCacheGuidKey[] = "cache_guid";
+const char kTimestampKey[] = "timestamp";
+
+// The max time a local device's cached GUIDs will be stored.
+constexpr base::TimeDelta kMaxTimeDeltaLocalCacheGuidsStored =
+    base::TimeDelta::FromDays(10);
+
+// The max number of local device most recent cached GUIDs that will be stored
+// in preferences.
+constexpr int kMaxLocalCacheGuidsStored = 30;
+
+// Returns true iff |dict| is a dictionary with a cache GUID that is equal to
+// |cache_guid|.
+bool MatchesGuidInDictionary(const base::Value& dict,
+                             const std::string& cache_guid) {
+  if (!dict.is_dict()) {
+    return false;
+  }
+  const std::string* v_cache_guid = dict.FindStringKey(kCacheGuidKey);
+  return v_cache_guid && *v_cache_guid == cache_guid;
+}
 
 }  // namespace
 
 // static
 void DeviceInfoPrefs::RegisterProfilePrefs(PrefRegistrySimple* registry) {
-  registry->RegisterListPref(kDeviceInfoRecentGUIDs);
+  registry->RegisterListPref(kDeviceInfoRecentGUIDsWithTimestamps);
+  registry->RegisterListPref(kObsoleteDeviceInfoRecentGUIDs);
+}
+
+// static
+void DeviceInfoPrefs::MigrateRecentLocalCacheGuidsPref(
+    PrefService* pref_service) {
+  base::span<const base::Value> obsolete_cache_guids =
+      pref_service->GetList(kObsoleteDeviceInfoRecentGUIDs)->GetList();
+  DeviceInfoPrefs prefs(pref_service, base::DefaultClock::GetInstance());
+
+  // Iterate in reverse order to maintain original order.
+  for (auto it = obsolete_cache_guids.rbegin();
+       it != obsolete_cache_guids.rend(); ++it) {
+    if (it->is_string()) {
+      prefs.AddLocalCacheGuid(it->GetString());
+    }
+  }
+
+  pref_service->ClearPref(kObsoleteDeviceInfoRecentGUIDs);
 }
 
-DeviceInfoPrefs::DeviceInfoPrefs(PrefService* pref_service)
-    : pref_service_(pref_service) {
-  DCHECK(pref_service);
+DeviceInfoPrefs::DeviceInfoPrefs(PrefService* pref_service,
+                                 const base::Clock* clock)
+    : pref_service_(pref_service), clock_(clock) {
+  DCHECK(pref_service_);
+  DCHECK(clock_);
 }
 
 DeviceInfoPrefs::~DeviceInfoPrefs() {}
@@ -38,30 +87,69 @@ DeviceInfoPrefs::~DeviceInfoPrefs() {}
 bool DeviceInfoPrefs::IsRecentLocalCacheGuid(
     const std::string& cache_guid) const {
   base::span<const base::Value> recent_local_cache_guids =
-      pref_service_->GetList(kDeviceInfoRecentGUIDs)->GetList();
+      pref_service_->GetList(kDeviceInfoRecentGUIDsWithTimestamps)->GetList();
 
-  return std::find(recent_local_cache_guids.begin(),
-                   recent_local_cache_guids.end(),
-                   base::Value(cache_guid)) != recent_local_cache_guids.end();
+  for (const auto& v : recent_local_cache_guids) {
+    if (MatchesGuidInDictionary(v, cache_guid)) {
+      return true;
+    }
+  }
+
+  return false;
 }
 
 void DeviceInfoPrefs::AddLocalCacheGuid(const std::string& cache_guid) {
-  ListPrefUpdate update(pref_service_, kDeviceInfoRecentGUIDs);
-  base::ListValue* pref_data = update.Get();
-  base::Value::ListStorage* recent_local_cache_guids = &pref_data->GetList();
-
-  if (std::find(recent_local_cache_guids->begin(),
-                recent_local_cache_guids->end(),
-                base::Value(cache_guid)) != recent_local_cache_guids->end()) {
-    // Local cache GUID already known.
-    return;
+  ListPrefUpdate update_cache_guids(pref_service_,
+                                    kDeviceInfoRecentGUIDsWithTimestamps);
+  base::Value::ListStorage* recent_local_cache_guids =
+      &(update_cache_guids.Get()->GetList());
+
+  for (auto it = recent_local_cache_guids->begin();
+       it != recent_local_cache_guids->end(); it++) {
+    if (MatchesGuidInDictionary(*it, cache_guid)) {
+      // Remove it from the list, to be reinserted below, in the first
+      // position.
+      recent_local_cache_guids->erase(it);
+      break;
+    }
   }
 
+  base::Value new_entry(base::Value::Type::DICTIONARY);
+  new_entry.SetKey(kCacheGuidKey, base::Value(cache_guid));
+  new_entry.SetKey(
+      kTimestampKey,
+      base::Value(clock_->Now().ToDeltaSinceWindowsEpoch().InDays()));
+
   recent_local_cache_guids->emplace(recent_local_cache_guids->begin(),
-                                    base::Value(cache_guid));
-  if (recent_local_cache_guids->size() > kMaxLocalCacheGuidsStored) {
+                                    std::move(new_entry));
+
+  while (recent_local_cache_guids->size() > kMaxLocalCacheGuidsStored) {
     recent_local_cache_guids->pop_back();
   }
 }
 
+void DeviceInfoPrefs::GarbageCollectExpiredCacheGuids() {
+  ListPrefUpdate update_cache_guids(pref_service_,
+                                    kDeviceInfoRecentGUIDsWithTimestamps);
+  base::Value::ListStorage* recent_local_cache_guids =
+      &(update_cache_guids.Get())->GetList();
+
+  for (auto it = recent_local_cache_guids->begin();
+       it != recent_local_cache_guids->end(); it++) {
+    base::Optional<int> days_since_epoch = it->FindIntKey(kTimestampKey);
+    const base::Time creation_time =
+        days_since_epoch ? base::Time::FromDeltaSinceWindowsEpoch(
+                               base::TimeDelta::FromDays(*days_since_epoch))
+                         : base::Time::Min();
+    if (creation_time < clock_->Now() - kMaxTimeDeltaLocalCacheGuidsStored) {
+      // Because the list of cache_guid prefs is maintained to be in reverse
+      // chronological order (from the newest cache guid to the oldest), it
+      // is safe to remove all of the entries after one that is found to be
+      // expired.
+      recent_local_cache_guids->erase(it, recent_local_cache_guids->end());
+      return;
+    }
+  }
+}
+
 }  // namespace syncer

components/sync_device_info/device_info_prefs.h

@@ -13,6 +13,10 @@
 class PrefService;
 class PrefRegistrySimple;
 
+namespace base {
+class Clock;
+}  // namespace base
+
 namespace syncer {
 
 // Use this for determining if a cache guid was recently used by this device.
@@ -20,14 +24,28 @@ class DeviceInfoPrefs {
  public:
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
 
-  explicit DeviceInfoPrefs(PrefService* pref_service);
+  static void MigrateRecentLocalCacheGuidsPref(PrefService* pref_service);
+
+  // |pref_service| and |clock| must outlive this class and be non null.
+  DeviceInfoPrefs(PrefService* pref_service, const base::Clock* clock);
   ~DeviceInfoPrefs();
 
+  // Returns if the given |cache_guid| is present in the saved pref. This is
+  // most reliable when dealing with recent devices only, due to garbage
+  // collection of local GUIDs, as per kMaxDaysLocalCacheGuidsStored.
   bool IsRecentLocalCacheGuid(const std::string& cache_guid) const;
+
+  // Adds the given |cache_guid| to the internal list stored in prefs and
+  // exposed via IsRecentLocalCacheGuid(). If the |cache_guid| already exists,
+  // this will reset the expiry date for that entry.
   void AddLocalCacheGuid(const std::string& cache_guid);
 
+  // Garbage-collects local cache GUIDs if too old.
+  void GarbageCollectExpiredCacheGuids();
+
  private:
   PrefService* const pref_service_;
+  const base::Clock* const clock_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceInfoPrefs);
 };

components/sync_device_info/device_info_prefs_unittest.cc

+// Copyright 2019 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 "components/sync_device_info/device_info_prefs.h"
+
+#include "base/strings/stringprintf.h"
+#include "base/test/simple_test_clock.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/scoped_user_pref_update.h"
+#include "components/prefs/testing_pref_service.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace syncer {
+namespace {
+
+class DeviceInfoPrefsTest : public testing::Test {
+ protected:
+  DeviceInfoPrefsTest() : device_info_prefs_(&pref_service_, &clock_) {
+    DeviceInfoPrefs::RegisterProfilePrefs(pref_service_.registry());
+  }
+  ~DeviceInfoPrefsTest() override = default;
+
+  DeviceInfoPrefs device_info_prefs_;
+  base::SimpleTestClock clock_;
+  TestingPrefServiceSimple pref_service_;
+};
+
+TEST_F(DeviceInfoPrefsTest, ShouldMigrateFromObsoletePref) {
+  const char kObsoleteDeviceInfoRecentGUIDs[] = "sync.local_device_guids";
+
+  ListPrefUpdate cache_guids_update(&pref_service_,
+                                    kObsoleteDeviceInfoRecentGUIDs);
+  base::Value::ListStorage* recent_local_cache_guids =
+      &cache_guids_update.Get()->GetList();
+
+  recent_local_cache_guids->emplace(recent_local_cache_guids->begin(),
+                                    base::Value("old_guid1"));
+  recent_local_cache_guids->emplace(recent_local_cache_guids->begin(),
+                                    base::Value("old_guid2"));
+
+  ASSERT_FALSE(device_info_prefs_.IsRecentLocalCacheGuid("old_guid1"));
+  ASSERT_FALSE(device_info_prefs_.IsRecentLocalCacheGuid("old_guid2"));
+
+  DeviceInfoPrefs::MigrateRecentLocalCacheGuidsPref(&pref_service_);
+
+  EXPECT_TRUE(device_info_prefs_.IsRecentLocalCacheGuid("old_guid1"));
+  EXPECT_TRUE(device_info_prefs_.IsRecentLocalCacheGuid("old_guid2"));
+}
+
+TEST_F(DeviceInfoPrefsTest, ShouldGarbageCollectExpiredCacheGuids) {
+  const base::TimeDelta kMaxDaysLocalCacheGuidsStored =
+      base::TimeDelta::FromDays(10);
+
+  device_info_prefs_.AddLocalCacheGuid("guid1");
+  clock_.Advance(kMaxDaysLocalCacheGuidsStored -
+                 base::TimeDelta::FromMinutes(1));
+  device_info_prefs_.AddLocalCacheGuid("guid2");
+
+  // First garbage collection immediately before taking effect, hence a no-op.
+  device_info_prefs_.GarbageCollectExpiredCacheGuids();
+  EXPECT_TRUE(device_info_prefs_.IsRecentLocalCacheGuid("guid1"));
+  EXPECT_TRUE(device_info_prefs_.IsRecentLocalCacheGuid("guid2"));
+
+  // Advancing one day causes the first GUID to be garbage-collected.
+  clock_.Advance(base::TimeDelta::FromDays(1));
+  device_info_prefs_.GarbageCollectExpiredCacheGuids();
+  EXPECT_FALSE(device_info_prefs_.IsRecentLocalCacheGuid("guid1"));
+  EXPECT_TRUE(device_info_prefs_.IsRecentLocalCacheGuid("guid2"));
+}
+
+TEST_F(DeviceInfoPrefsTest, ShouldTruncateAfterMaximumNumberOfGuids) {
+  const int kMaxLocalCacheGuidsStored = 30;
+
+  device_info_prefs_.AddLocalCacheGuid("orig_guid");
+
+  // Fill up exactly the maximum number, without triggering a truncation.
+  for (int i = 0; i < kMaxLocalCacheGuidsStored - 1; i++) {
+    device_info_prefs_.AddLocalCacheGuid(base::StringPrintf("guid%d", i));
+  }
+
+  EXPECT_TRUE(device_info_prefs_.IsRecentLocalCacheGuid("orig_guid"));
+
+  // Adding one more should truncate exactly one.
+  device_info_prefs_.AddLocalCacheGuid("newest_guid");
+  EXPECT_FALSE(device_info_prefs_.IsRecentLocalCacheGuid("orig_guid"));
+  EXPECT_TRUE(device_info_prefs_.IsRecentLocalCacheGuid("newest_guid"));
+  EXPECT_TRUE(device_info_prefs_.IsRecentLocalCacheGuid("guid1"));
+}
+
+}  // namespace
+
+}  // namespace syncer

components/sync_device_info/device_info_sync_bridge.cc

@@ -8,7 +8,7 @@
 
 #include <algorithm>
 #include <map>
-#include <set>
+#include <unordered_set>
 #include <utility>
 
 #include "base/bind.h"
@@ -187,6 +187,8 @@ void DeviceInfoSyncBridge::OnSyncStarting(
     const DataTypeActivationRequest& request) {
   // Store the cache GUID, mainly in case MergeSyncData() is executed later.
   local_cache_guid_ = request.cache_guid;
+  // Garbage-collect old local cache GUIDs, for privacy reasons.
+  device_info_prefs_->GarbageCollectExpiredCacheGuids();
   // Add the cache guid to the local prefs.
   device_info_prefs_->AddLocalCacheGuid(local_cache_guid_);
 }

components/sync_device_info/device_info_sync_bridge_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/bind.h"
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
+#include "base/test/simple_test_clock.h"
 #include "base/test/task_environment.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/sync/base/time.h"
@@ -296,7 +297,7 @@ class DeviceInfoSyncBridgeTest : public testing::Test,
         std::make_unique<TestLocalDeviceInfoProvider>(),
         ModelTypeStoreTestUtil::FactoryForForwardingStore(store_.get()),
         mock_processor_.CreateForwardingProcessor(),
-        std::make_unique<DeviceInfoPrefs>(&pref_service_));
+        std::make_unique<DeviceInfoPrefs>(&pref_service_, &clock_));
     bridge_->AddObserver(this);
   }
 
@@ -449,6 +450,8 @@ class DeviceInfoSyncBridgeTest : public testing::Test,
   }
 
  private:
+  base::SimpleTestClock clock_;
+
   int change_count_ = 0;
 
   // In memory model type store needs to be able to post tasks.

ios/chrome/browser/prefs/browser_prefs.mm

@@ -218,4 +218,7 @@ void MigrateObsoleteBrowserStatePrefs(PrefService* prefs) {
   // Added 09/2019
   prefs->ClearPref(kGoogleServicesUsername);
   prefs->ClearPref(kGoogleServicesUserAccountId);
+
+  // Added 10/2019.
+  syncer::DeviceInfoPrefs::MigrateRecentLocalCacheGuidsPref(prefs);
 }

ios/chrome/browser/sync/device_info_sync_service_factory.mm

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/memory/singleton.h"
+#include "base/time/default_clock.h"
 #include "components/keyed_service/core/service_access_type.h"
 #include "components/keyed_service/ios/browser_state_dependency_manager.h"
 #include "components/send_tab_to_self/features.h"
@@ -109,8 +110,8 @@ DeviceInfoSyncServiceFactory::BuildServiceInstanceFor(
   auto local_device_info_provider =
       std::make_unique<syncer::LocalDeviceInfoProviderImpl>(
           ::GetChannel(), ::GetVersionString(), device_info_sync_client.get());
-  auto device_prefs =
-      std::make_unique<syncer::DeviceInfoPrefs>(browser_state->GetPrefs());
+  auto device_prefs = std::make_unique<syncer::DeviceInfoPrefs>(
+      browser_state->GetPrefs(), base::DefaultClock::GetInstance());
 
   return std::make_unique<syncer::DeviceInfoSyncServiceImpl>(
       ModelTypeStoreServiceFactory::GetForBrowserState(browser_state)

ios/web_view/internal/sync/web_view_device_info_sync_service_factory.mm

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/memory/singleton.h"
+#include "base/time/default_clock.h"
 #include "components/keyed_service/ios/browser_state_dependency_manager.h"
 #include "components/signin/public/base/device_id_helper.h"
 #include "components/sync/model/model_type_store_service.h"
@@ -87,8 +88,8 @@ WebViewDeviceInfoSyncServiceFactory::BuildServiceInstanceFor(
       std::make_unique<syncer::LocalDeviceInfoProviderImpl>(
           version_info::Channel::STABLE, version_info::GetVersionNumber(),
           device_info_sync_client.get());
-  auto device_prefs =
-      std::make_unique<syncer::DeviceInfoPrefs>(browser_state->GetPrefs());
+  auto device_prefs = std::make_unique<syncer::DeviceInfoPrefs>(
+      browser_state->GetPrefs(), base::DefaultClock::GetInstance());
 
   return std::make_unique<syncer::DeviceInfoSyncServiceImpl>(
       WebViewModelTypeStoreServiceFactory::GetForBrowserState(browser_state)