weblayer: adds expiring of favicons

This makes it so icons there were downloaded more than 30 days ago
are removed. This is done to prevent the database from indefinitely
growing.

BUG=1076463
TEST=covered by tests

Change-Id: I9df9407124dd9364517bd31949ecf718fd7a9bc8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2356050
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798345}

weblayer/browser/favicon/favicon_backend_wrapper.cc

@@ -12,6 +12,16 @@
 
 namespace weblayer {
 
+// Removing out of date entries can be costly. To avoid blocking the thread
+// this code runs on, the work is potentially throttled. Specifically at
+// most |kMaxNumberOfEntriesToRemoveAtATime| are removed during a single call.
+// If |kMaxNumberOfEntriesToRemoveAtATime| are removed, then there may be more
+// entries that can be removed, so the timer is restarted with a shorter time
+// out (|kTimeDeltaForRunningExpireWithRemainingWork|).
+constexpr base::TimeDelta kTimeDeltaForRunningExpireNoRemainingWork =
+    base::TimeDelta::FromHours(1);
+constexpr int kMaxNumberOfEntriesToRemoveAtATime = 100;
+
 FaviconBackendWrapper::FaviconBackendWrapper(
     scoped_refptr<base::SequencedTaskRunner> task_runner)
     : base::RefCountedDeleteOnSequence<FaviconBackendWrapper>(task_runner),
@@ -34,12 +44,16 @@ void FaviconBackendWrapper::Init(const base::FilePath& db_path) {
       return;
     }
   }
+
+  expire_timer_.Start(FROM_HERE, kTimeDeltaForRunningExpireWithRemainingWork,
+                      this, &FaviconBackendWrapper::OnExpireTimerFired);
 }
 
 void FaviconBackendWrapper::Shutdown() {
   // Ensures there isn't a reference to this in the task runner (by way of the
   // task the timer posts).
   commit_timer_.Stop();
+  expire_timer_.Stop();
 }
 
 void FaviconBackendWrapper::DeleteAndRecreateDatabase() {
@@ -145,4 +159,28 @@ void FaviconBackendWrapper::Commit() {
     favicon_backend_->Commit();
 }
 
+void FaviconBackendWrapper::OnExpireTimerFired() {
+  if (!favicon_backend_)
+    return;
+
+  // See comments above |kTimeDeltaForRunningExpireNoRemainingWork| for a
+  // description of this logic.
+  favicon::FaviconDatabase* db = favicon_backend_->db();
+  auto icon_ids = db->GetFaviconsLastUpdatedBefore(
+      base::Time::Now() - kTimeDeltaWhenEntriesAreRemoved,
+      kMaxNumberOfEntriesToRemoveAtATime);
+  for (favicon_base::FaviconID icon_id : icon_ids) {
+    db->DeleteFavicon(icon_id);
+    db->DeleteIconMappingsForFaviconId(icon_id);
+  }
+  if (!icon_ids.empty())
+    Commit();
+  const base::TimeDelta delta =
+      icon_ids.size() == kMaxNumberOfEntriesToRemoveAtATime
+          ? kTimeDeltaForRunningExpireWithRemainingWork
+          : kTimeDeltaForRunningExpireNoRemainingWork;
+  expire_timer_.Start(FROM_HERE, delta, this,
+                      &FaviconBackendWrapper::OnExpireTimerFired);
+}
+
 }  // namespace weblayer

weblayer/browser/favicon/favicon_backend_wrapper.h

@@ -29,8 +29,7 @@ class FaviconBackend;
 namespace weblayer {
 
 // FaviconBackendWrapper runs on a background task-runner and owns the database
-// side of favicons. This class largely delegates to favicon::FaviconBackend
-// and has very little logic.
+// side of favicons. This class largely delegates to favicon::FaviconBackend.
 class FaviconBackendWrapper
     : public base::RefCountedDeleteOnSequence<FaviconBackendWrapper>,
       public favicon::FaviconBackendDelegate {
@@ -82,10 +81,15 @@ class FaviconBackendWrapper
  private:
   friend class base::RefCountedDeleteOnSequence<FaviconBackendWrapper>;
   friend class base::DeleteHelper<FaviconBackendWrapper>;
+  friend class FaviconBackendWrapperTest;
+
   ~FaviconBackendWrapper() override;
 
   void Commit();
 
+  // Called to expire (remove) out of date icons and restart the timer.
+  void OnExpireTimerFired();
+
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
 
   // Timer used to delay commits for a short amount of time. This done to
@@ -96,9 +100,24 @@ class FaviconBackendWrapper
   // the database this will be null.
   std::unique_ptr<favicon::FaviconBackend> favicon_backend_;
 
+  // Timer used to remove items from the database that are likely no longer
+  // needed.
+  base::OneShotTimer expire_timer_;
+
   base::FilePath db_path_;
 };
 
+// These values are here only for tests.
+
+// Amount of time before favicons are removed. That is, any favicons downloaded
+// before this amount of time are removed.
+constexpr base::TimeDelta kTimeDeltaWhenEntriesAreRemoved =
+    base::TimeDelta::FromDays(30);
+
+// See comment near kMaxNumberOfEntriesToRemoveAtATime for details on this.
+constexpr base::TimeDelta kTimeDeltaForRunningExpireWithRemainingWork =
+    base::TimeDelta::FromMinutes(2);
+
 }  // namespace weblayer
 
 #endif  // WEBLAYER_BROWSER_FAVICON_FAVICON_BACKEND_WRAPPER_H_

weblayer/browser/favicon/favicon_backend_wrapper_unittest.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 "weblayer/browser/favicon/favicon_backend_wrapper.h"
+
+#include <vector>
+
+#include "base/files/file_path.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/memory/ref_counted_memory.h"
+#include "base/test/task_environment.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "components/favicon/core/favicon_backend.h"
+#include "components/favicon/core/favicon_database.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace weblayer {
+namespace {
+
+// Blobs for adding favicons.
+const unsigned char kBlob1[] =
+    "12346102356120394751634516591348710478123649165419234519234512349134";
+
+}  // namespace
+
+class FaviconBackendWrapperTest : public testing::Test {
+ protected:
+  favicon::FaviconBackend* backend() {
+    return wrapper_->favicon_backend_.get();
+  }
+
+  // testing::Test:
+  void SetUp() override {
+    // Get a temporary directory for the test DB files.
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+
+    db_path_ = temp_dir_.GetPath().AppendASCII("test_db");
+  }
+
+  void TearDown() override {
+    wrapper_ = nullptr;
+    testing::Test::TearDown();
+  }
+
+  base::test::SingleThreadTaskEnvironment task_environment_{
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
+  base::ScopedTempDir temp_dir_;
+  base::FilePath db_path_;
+  scoped_refptr<FaviconBackendWrapper> wrapper_;
+};
+
+TEST_F(FaviconBackendWrapperTest, BasicExpire) {
+  wrapper_ = base::MakeRefCounted<FaviconBackendWrapper>(
+      base::ThreadTaskRunnerHandle::Get());
+  wrapper_->Init(db_path_);
+  ASSERT_TRUE(backend());
+  auto* db = backend()->db();
+
+  std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1));
+  scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data));
+  GURL url("http://google.com");
+  const base::Time time1 = base::Time::Now();
+  favicon_base::FaviconID favicon_id1 =
+      db->AddFavicon(url, favicon_base::IconType::kTouchIcon, favicon,
+                     favicon::FaviconBitmapType::ON_VISIT, time1, gfx::Size());
+  ASSERT_NE(0, favicon_id1);
+  favicon::IconMappingID icon_mapping_id1 =
+      db->AddIconMapping(url, favicon_id1);
+  ASSERT_NE(0, icon_mapping_id1);
+
+  // Fast forward past first expire running.
+  task_environment_.FastForwardBy(kTimeDeltaForRunningExpireWithRemainingWork *
+                                  2);
+  // The icon should still be there.
+  EXPECT_TRUE(db->GetFaviconHeader(favicon_id1, nullptr, nullptr));
+  EXPECT_TRUE(db->HasMappingFor(favicon_id1));
+
+  // Fast forward such that the icon is removed.
+  task_environment_.FastForwardBy(kTimeDeltaWhenEntriesAreRemoved);
+  EXPECT_FALSE(db->GetFaviconHeader(favicon_id1, nullptr, nullptr));
+  EXPECT_FALSE(db->HasMappingFor(favicon_id1));
+}
+
+TEST_F(FaviconBackendWrapperTest, ExpireWithOneRemaining) {
+  wrapper_ = base::MakeRefCounted<FaviconBackendWrapper>(
+      base::ThreadTaskRunnerHandle::Get());
+  wrapper_->Init(db_path_);
+  ASSERT_TRUE(backend());
+  auto* db = backend()->db();
+
+  // Add two entries. The second is more recent then the first.
+  std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1));
+  scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data));
+  GURL url("http://google.com");
+  const base::Time time1 = base::Time::Now();
+  favicon_base::FaviconID favicon_id1 =
+      db->AddFavicon(url, favicon_base::IconType::kTouchIcon, favicon,
+                     favicon::FaviconBitmapType::ON_VISIT, time1, gfx::Size());
+  ASSERT_NE(0, favicon_id1);
+  favicon::IconMappingID icon_mapping_id1 =
+      db->AddIconMapping(url, favicon_id1);
+  ASSERT_NE(0, icon_mapping_id1);
+  const base::Time time2 = time1 + kTimeDeltaWhenEntriesAreRemoved / 2;
+  favicon_base::FaviconID favicon_id2 =
+      db->AddFavicon(url, favicon_base::IconType::kTouchIcon, favicon,
+                     favicon::FaviconBitmapType::ON_VISIT, time2, gfx::Size());
+  ASSERT_NE(0, favicon_id2);
+  favicon::IconMappingID icon_mapping_id2 =
+      db->AddIconMapping(url, favicon_id2);
+  ASSERT_NE(0, icon_mapping_id2);
+
+  // Fast forward such the first entry is expired and should be removed, but
+  // not the second.
+  task_environment_.FastForwardBy(kTimeDeltaWhenEntriesAreRemoved +
+                                  base::TimeDelta::FromDays(1));
+  EXPECT_FALSE(db->GetFaviconHeader(favicon_id1, nullptr, nullptr));
+  EXPECT_FALSE(db->HasMappingFor(favicon_id1));
+  EXPECT_TRUE(db->GetFaviconHeader(favicon_id2, nullptr, nullptr));
+  EXPECT_TRUE(db->HasMappingFor(favicon_id2));
+
+  // Fast forward enough such that second is removed.
+  task_environment_.FastForwardBy(kTimeDeltaWhenEntriesAreRemoved +
+                                  base::TimeDelta::FromDays(1));
+  EXPECT_FALSE(db->GetFaviconHeader(favicon_id2, nullptr, nullptr));
+  EXPECT_FALSE(db->HasMappingFor(favicon_id2));
+}
+
+}  // namespace weblayer

weblayer/test/BUILD.gn

@@ -244,12 +244,14 @@ source_set("run_all_unittests") {
 test("weblayer_unittests") {
   deps = [
     ":run_all_unittests",
+    "//components/favicon/core:database",
     "//components/site_isolation",
   ]
   if (is_android) {
     deps += [ ":weblayer_test_assets" ]
   }
   sources = [
+    "../browser/favicon/favicon_backend_wrapper_unittest.cc",
     "../browser/profile_disk_operations_unittests.cc",
     "../browser/site_isolation_policy_unittest.cc",
   ]