Image Fetcher: Implement eviction for CacheStrategy::HOLD_UNTIL_EXPIRE.

This CL implements the eviction logic for HOLD_UNTIL_EXPIRE.

1. The GetEstimatedSize() is based on cache strategy now.

2. Each RunEviction will also check entries for
CacheStrategy::HOLD_UNTIL_EXPIRE.

TBR=wylieb@chromium.org

Bug: 1067049,1058534
Change-Id: I38007b8eeac8cac19a28f10906a711211382838a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2144741
Commit-Queue: Xing Liu <xingliu@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759153}

components/image_fetcher/core/cache/image_cache.cc

@@ -224,7 +224,8 @@ void ImageCache::RunEvictionOnStartup() {
 
 void ImageCache::RunEvictionWhenFull() {
   // Storage is within limits, bail out.
-  if (metadata_store_->GetEstimatedSize() < kCacheMaxSize) {
+  if (metadata_store_->GetEstimatedSize(CacheStrategy::BEST_EFFORT) <
+      kCacheMaxSize) {
     return;
   }
 

components/image_fetcher/core/cache/image_cache_unittest.cc

@@ -34,6 +34,7 @@ constexpr char kPrefLastStartupEviction[] =
     "cached_image_fetcher_last_startup_eviction_time";
 constexpr char kImageFetcherEventHistogramName[] = "ImageFetcher.Events";
 constexpr char kImageUrl[] = "http://gstatic.img.com/foo.jpg";
+constexpr char kOtherImageUrl[] = "http://gstatic.img.com/bar.jpg";
 constexpr char kImageUrlHashed[] = "3H7UODDH3WKDWK6FQ3IZT3LQMVBPYJ4M";
 constexpr char kImageData[] = "data";
 const int kOverMaxCacheSize = 65 * 1024 * 1024;
@@ -110,6 +111,17 @@ class CachedImageFetcherImageCacheTest : public testing::Test {
     RunUntilIdle();
   }
 
+  // Loads the image and verify the data callback.
+  void LoadImage(const std::string& url, const std::string& expected_data) {
+    EXPECT_CALL(*this, DataCallback(false, expected_data));
+    image_cache()->LoadImage(
+        false, url,
+        base::BindOnce(&CachedImageFetcherImageCacheTest::DataCallback,
+                       base::Unretained(this)));
+    db()->LoadCallback(true);
+    RunUntilIdle();
+  }
+
   bool IsMetadataPresent(const std::string& key) {
     return db_store_.find(key) != db_store_.end();
   }
@@ -193,14 +205,7 @@ TEST_F(CachedImageFetcherImageCacheTest, SanityTest) {
                            /* expiration_interval */ base::nullopt);
   RunUntilIdle();
 
-  EXPECT_CALL(*this, DataCallback(false, kImageData));
-  image_cache()->LoadImage(
-      false, kImageUrl,
-      base::BindOnce(&CachedImageFetcherImageCacheTest::DataCallback,
-                     base::Unretained(this)));
-  db()->LoadCallback(true);
-  RunUntilIdle();
-
+  LoadImage(kImageUrl, kImageData);
   image_cache()->DeleteImage(kImageUrl);
   RunUntilIdle();
 
@@ -233,14 +238,7 @@ TEST_F(CachedImageFetcherImageCacheTest, Save) {
   image_cache()->SaveImage(kImageUrl, kImageData,
                            /* needs_transcoding */ false,
                            /* expiration_interval */ base::nullopt);
-
-  EXPECT_CALL(*this, DataCallback(false, kImageData));
-  image_cache()->LoadImage(
-      false, kImageUrl,
-      base::BindOnce(&CachedImageFetcherImageCacheTest::DataCallback,
-                     base::Unretained(this)));
-  db()->LoadCallback(true);
-  RunUntilIdle();
+  LoadImage(kImageUrl, kImageData);
 }
 
 TEST_F(CachedImageFetcherImageCacheTest, Load) {
@@ -248,13 +246,7 @@ TEST_F(CachedImageFetcherImageCacheTest, Load) {
   auto metadata_before = GetMetadata(kImageUrlHashed);
 
   clock()->SetNow(clock()->Now() + base::TimeDelta::FromHours(1));
-  EXPECT_CALL(*this, DataCallback(false, kImageData));
-  image_cache()->LoadImage(
-      false, kImageUrl,
-      base::BindOnce(&CachedImageFetcherImageCacheTest::DataCallback,
-                     base::Unretained(this)));
-  db()->LoadCallback(true);
-  RunUntilIdle();
+  LoadImage(kImageUrl, kImageData);
   db()->LoadCallback(true);
   db()->UpdateCallback(true);
   RunUntilIdle();
@@ -268,13 +260,7 @@ TEST_F(CachedImageFetcherImageCacheTest, LoadReadOnly) {
   auto metadata_before = GetMetadata(kImageUrlHashed);
 
   clock()->SetNow(clock()->Now() + base::TimeDelta::FromHours(1));
-  EXPECT_CALL(*this, DataCallback(false, kImageData));
-  image_cache()->LoadImage(
-      true, kImageUrl,
-      base::BindOnce(&CachedImageFetcherImageCacheTest::DataCallback,
-                     base::Unretained(this)));
-  db()->LoadCallback(true);
-  RunUntilIdle();
+  LoadImage(kImageUrl, kImageData);
 
   auto metadata_after = GetMetadata(kImageUrlHashed);
   ASSERT_TRUE(IsMetadataEqual(metadata_before, metadata_after));
@@ -283,24 +269,11 @@ TEST_F(CachedImageFetcherImageCacheTest, LoadReadOnly) {
 TEST_F(CachedImageFetcherImageCacheTest, Delete) {
   PrepareImageCache(false);
 
-  EXPECT_CALL(*this, DataCallback(false, kImageData));
-  image_cache()->LoadImage(
-      false, kImageUrl,
-      base::BindOnce(&CachedImageFetcherImageCacheTest::DataCallback,
-                     base::Unretained(this)));
-  db()->LoadCallback(true);
-  RunUntilIdle();
-
+  LoadImage(kImageUrl, kImageData);
   image_cache()->DeleteImage(kImageUrl);
   RunUntilIdle();
 
-  EXPECT_CALL(*this, DataCallback(false, std::string()));
-  image_cache()->LoadImage(
-      false, kImageUrl,
-      base::BindOnce(&CachedImageFetcherImageCacheTest::DataCallback,
-                     base::Unretained(this)));
-  db()->LoadCallback(true);
-  RunUntilIdle();
+  LoadImage(kImageUrl, "");
 }
 
 TEST_F(CachedImageFetcherImageCacheTest, Eviction) {
@@ -310,14 +283,7 @@ TEST_F(CachedImageFetcherImageCacheTest, Eviction) {
   RunEvictionOnStartup(/* success */ true);
   ASSERT_EQ(clock()->Now(), prefs()->GetTime(kPrefLastStartupEviction));
 
-  EXPECT_CALL(*this, DataCallback(false, std::string()));
-  image_cache()->LoadImage(
-      false, kImageUrl,
-      base::BindOnce(&CachedImageFetcherImageCacheTest::DataCallback,
-                     base::Unretained(this)));
-  db()->LoadCallback(true);
-  RunUntilIdle();
-
+  LoadImage(kImageUrl, "");
   histogram_tester().ExpectBucketCount(
       kImageFetcherEventHistogramName,
       ImageFetcherEvent::kCacheStartupEvictionStarted, 1);
@@ -326,6 +292,25 @@ TEST_F(CachedImageFetcherImageCacheTest, Eviction) {
       ImageFetcherEvent::kCacheStartupEvictionFinished, 1);
 }
 
+// Verifies eviction for CacheStrategy::HOLD_UNTIL_EXPIRED.
+TEST_F(CachedImageFetcherImageCacheTest, EvictionHoldUtilExpires) {
+  PrepareImageCache(false);
+  clock()->SetNow(clock()->Now() + base::TimeDelta::FromDays(2));
+
+  image_cache()->SaveImage(kImageUrl, "image_data", false,
+                           base::TimeDelta::FromDays(10));
+
+  image_cache()->SaveImage(kOtherImageUrl, "other_image_data", false,
+                           base::TimeDelta::FromHours(1));
+  RunUntilIdle();
+
+  // Forward the clock to make image with |kOtherImageUrl| expired.
+  clock()->SetNow(clock()->Now() + base::TimeDelta::FromHours(3));
+  RunEvictionOnStartup(/* success */ true);
+  LoadImage(kImageUrl, "image_data");
+  LoadImage(kOtherImageUrl, "");
+}
+
 TEST_F(CachedImageFetcherImageCacheTest, EvictionWhenFull) {
   PrepareImageCache(false);
   InjectMetadata(kImageUrl, kOverMaxCacheSize, /* needs_transcoding */ false);
@@ -333,13 +318,7 @@ TEST_F(CachedImageFetcherImageCacheTest, EvictionWhenFull) {
   RunEvictionWhenFull(/* success */ true);
 
   // The data should be removed because it's over the allowed limit.
-  EXPECT_CALL(*this, DataCallback(false, ""));
-  image_cache()->LoadImage(
-      false, kImageUrl,
-      base::BindOnce(&CachedImageFetcherImageCacheTest::DataCallback,
-                     base::Unretained(this)));
-  db()->LoadCallback(true);
-  RunUntilIdle();
+  LoadImage(kImageUrl, "");
 }
 
 TEST_F(CachedImageFetcherImageCacheTest, EvictionTooSoon) {
@@ -348,13 +327,7 @@ TEST_F(CachedImageFetcherImageCacheTest, EvictionTooSoon) {
   clock()->SetNow(clock()->Now() + base::TimeDelta::FromDays(6));
   RunEvictionOnStartup(/* success */ true);
 
-  EXPECT_CALL(*this, DataCallback(false, kImageData));
-  image_cache()->LoadImage(
-      false, kImageUrl,
-      base::BindOnce(&CachedImageFetcherImageCacheTest::DataCallback,
-                     base::Unretained(this)));
-  db()->LoadCallback(true);
-  RunUntilIdle();
+  LoadImage(kImageUrl, kImageData);
 }
 
 TEST_F(CachedImageFetcherImageCacheTest, EvictionWhenEvictionAlreadyPerformed) {
@@ -364,14 +337,7 @@ TEST_F(CachedImageFetcherImageCacheTest, EvictionWhenEvictionAlreadyPerformed) {
                    clock()->Now());
   clock()->SetNow(clock()->Now() + base::TimeDelta::FromHours(23));
   RunEvictionOnStartup(/* success */ false);
-
-  EXPECT_CALL(*this, DataCallback(false, kImageData));
-  image_cache()->LoadImage(
-      false, kImageUrl,
-      base::BindOnce(&CachedImageFetcherImageCacheTest::DataCallback,
-                     base::Unretained(this)));
-  db()->LoadCallback(true);
-  RunUntilIdle();
+  LoadImage(kImageUrl, kImageData);
 }
 
 TEST_F(CachedImageFetcherImageCacheTest, Reconciliation) {
@@ -384,13 +350,7 @@ TEST_F(CachedImageFetcherImageCacheTest, Reconciliation) {
   RunReconciliation();
 
   // Data should be gone.
-  EXPECT_CALL(*this, DataCallback(false, std::string()));
-  image_cache()->LoadImage(
-      false, "foo",
-      base::BindOnce(&CachedImageFetcherImageCacheTest::DataCallback,
-                     base::Unretained(this)));
-  db()->LoadCallback(true);
-  RunUntilIdle();
+  LoadImage("foo", "");
 
   // Metadata should be gone.
   ASSERT_FALSE(IsMetadataPresent("bar"));
@@ -407,13 +367,7 @@ TEST_F(CachedImageFetcherImageCacheTest, ReconciliationMismatchData) {
   RunReconciliation();
 
   // Data should be gone.
-  EXPECT_CALL(*this, DataCallback(false, std::string()));
-  image_cache()->LoadImage(
-      false, "bar",
-      base::BindOnce(&CachedImageFetcherImageCacheTest::DataCallback,
-                     base::Unretained(this)));
-  db()->LoadCallback(true);
-  RunUntilIdle();
+  LoadImage("bar", "");
 }
 
 TEST_F(CachedImageFetcherImageCacheTest, ReconciliationMismatchMetadata) {

components/image_fetcher/core/cache/image_metadata_store.h

@@ -48,8 +48,9 @@ class ImageMetadataStore {
   // Returns all the keys this store has.
   virtual void GetAllKeys(KeysCallback callback) = 0;
 
-  // Returns the total size of what's in metadata, possibly incorrect.
-  virtual int GetEstimatedSize() = 0;
+  // Returns the total size of what's in metadata for a given cache strategy,
+  // possibly incorrect.
+  virtual int64_t GetEstimatedSize(CacheStrategy cache_strategy) = 0;
 
   // Deletes all metadata that's been cached before the boundary given as
   // |expiration_time|.

components/image_fetcher/core/cache/image_metadata_store_leveldb.cc

@@ -45,9 +45,9 @@ bool KeyMatcherFilter(std::string key, const std::string& other_key) {
   return key.compare(other_key) == 0;
 }
 
-bool SortByLastUsedTime(const CachedImageMetadataProto& a,
-                        const CachedImageMetadataProto& b) {
-  return a.last_used_time() < b.last_used_time();
+bool SortByLastUsedTime(const CachedImageMetadataProto* a,
+                        const CachedImageMetadataProto* b) {
+  return a->last_used_time() < b->last_used_time();
 }
 
 }  // namespace
@@ -71,8 +71,7 @@ ImageMetadataStoreLevelDB::ImageMetadataStoreLevelDB(
     std::unique_ptr<leveldb_proto::ProtoDatabase<CachedImageMetadataProto>>
         database,
     base::Clock* clock)
-    : estimated_size_(0),
-      initialization_status_(InitializationStatus::UNINITIALIZED),
+    : initialization_status_(InitializationStatus::UNINITIALIZED),
       database_(std::move(database)),
       clock_(clock) {}
 
@@ -114,7 +113,6 @@ void ImageMetadataStoreLevelDB::SaveImageMetadata(
   if (!IsInitialized()) {
     return;
   }
-  estimated_size_ += data_size;
 
   int64_t current_time = ToDatabaseTime(clock_->Now());
   CachedImageMetadataProto metadata_proto;
@@ -127,6 +125,9 @@ void ImageMetadataStoreLevelDB::SaveImageMetadata(
     metadata_proto.set_cache_strategy(CacheStrategy::HOLD_UNTIL_EXPIRED);
     metadata_proto.set_expiration_interval(
         expiration_interval->InMicroseconds());
+    estimated_size_[CacheStrategy::HOLD_UNTIL_EXPIRED] += data_size;
+  } else {
+    estimated_size_[CacheStrategy::BEST_EFFORT] += data_size;
   }
 
   auto entries_to_save = std::make_unique<MetadataKeyEntryVector>();
@@ -176,8 +177,9 @@ void ImageMetadataStoreLevelDB::GetAllKeys(KeysCallback callback) {
                                      std::move(callback)));
 }
 
-int ImageMetadataStoreLevelDB::GetEstimatedSize() {
-  return estimated_size_;
+int64_t ImageMetadataStoreLevelDB::GetEstimatedSize(
+    CacheStrategy cache_strategy) {
+  return estimated_size_[cache_strategy];
 }
 
 void ImageMetadataStoreLevelDB::EvictImageMetadata(base::Time expiration_time,
@@ -266,32 +268,18 @@ void ImageMetadataStoreLevelDB::EvictImageMetadataImpl(
     return;
   }
 
-  size_t total_bytes_stored = 0;
-  int64_t expiration_database_time = ToDatabaseTime(expiration_time);
   std::vector<std::string> keys_to_remove;
+  std::map<CacheStrategy, std::vector<const CachedImageMetadataProto*>>
+      entries_map;
   for (const CachedImageMetadataProto& entry : *entries) {
-    if (entry.creation_time() <= expiration_database_time) {
-      keys_to_remove.emplace_back(entry.key());
-    } else {
-      total_bytes_stored += entry.data_size();
-    }
+    entries_map[entry.cache_strategy()].push_back(&entry);
   }
 
-  // Only sort and remove more if the byte limit isn't satisfied.
-  if (total_bytes_stored > bytes_left) {
-    std::sort(entries->begin(), entries->end(), SortByLastUsedTime);
-    for (const CachedImageMetadataProto& entry : *entries) {
-      if (total_bytes_stored <= bytes_left) {
-        break;
-      }
-
-      keys_to_remove.emplace_back(entry.key());
-      total_bytes_stored -= entry.data_size();
-    }
+  for (auto& cache_strategy : entries_map) {
+    GetMetadataToRemove(cache_strategy.first, std::move(cache_strategy.second),
+                        expiration_time, bytes_left, &keys_to_remove);
   }
 
-  estimated_size_ = total_bytes_stored;
-
   if (keys_to_remove.empty()) {
     std::move(callback).Run({});
     return;
@@ -305,6 +293,59 @@ void ImageMetadataStoreLevelDB::EvictImageMetadataImpl(
                      keys_to_remove));
 }
 
+void ImageMetadataStoreLevelDB::GetMetadataToRemove(
+    CacheStrategy cache_strategy,
+    std::vector<const CachedImageMetadataProto*> entries,
+    base::Time expiration_time,
+    const size_t bytes_left,
+    std::vector<std::string>* keys_to_remove) {
+  DCHECK(keys_to_remove);
+  size_t total_bytes_stored = 0;
+  int64_t expiration_database_time = ToDatabaseTime(expiration_time);
+
+  switch (cache_strategy) {
+    case CacheStrategy::BEST_EFFORT:
+      // Removes expired entries.
+      for (const CachedImageMetadataProto* entry : entries) {
+        DCHECK_EQ(entry->cache_strategy(), CacheStrategy::BEST_EFFORT);
+        if (entry->creation_time() <= expiration_database_time) {
+          keys_to_remove->emplace_back(entry->key());
+        } else {
+          total_bytes_stored += entry->data_size();
+        }
+      }
+
+      // Only sort and remove more if the byte limit isn't satisfied.
+      if (total_bytes_stored > bytes_left) {
+        std::sort(entries.begin(), entries.end(), SortByLastUsedTime);
+        for (const CachedImageMetadataProto* entry : entries) {
+          if (total_bytes_stored <= bytes_left) {
+            break;
+          }
+
+          keys_to_remove->emplace_back(entry->key());
+          total_bytes_stored -= entry->data_size();
+        }
+      }
+      estimated_size_[cache_strategy] = total_bytes_stored;
+      break;
+    case CacheStrategy::HOLD_UNTIL_EXPIRED:
+      int64_t now = ToDatabaseTime(clock_->Now());
+      int64_t total_size = 0;
+      for (const auto* entry : entries) {
+        DCHECK_EQ(entry->cache_strategy(), CacheStrategy::HOLD_UNTIL_EXPIRED);
+        DCHECK(entry->has_expiration_interval());
+        if (entry->last_used_time() + entry->expiration_interval() < now) {
+          keys_to_remove->push_back(entry->key());
+        } else {
+          total_size += entry->data_size();
+        }
+      }
+      estimated_size_[cache_strategy] = total_size;
+      break;
+  }
+}
+
 void ImageMetadataStoreLevelDB::OnEvictImageMetadataDone(
     KeysCallback callback,
     std::vector<std::string> deleted_keys,

components/image_fetcher/core/cache/image_metadata_store_leveldb.h

@@ -64,7 +64,7 @@ class ImageMetadataStoreLevelDB : public ImageMetadataStore {
   // which means it hasn't been calculated yet, or it's an over-estimate. In
   // the case of an over estimate, it will be recified if you call into an
   // eviction routine.
-  int GetEstimatedSize() override;
+  int64_t GetEstimatedSize(CacheStrategy cache_strategy) override;
   void EvictImageMetadata(base::Time expiration_time,
                           const size_t bytes_left,
                           KeysCallback callback) override;
@@ -89,11 +89,16 @@ class ImageMetadataStoreLevelDB : public ImageMetadataStore {
       KeysCallback callback,
       bool success,
       std::unique_ptr<std::vector<CachedImageMetadataProto>> entries);
+  void GetMetadataToRemove(CacheStrategy cache_strategy,
+                           std::vector<const CachedImageMetadataProto*> entries,
+                           base::Time expiration_time,
+                           const size_t bytes_left,
+                           std::vector<std::string>* keys_to_remove);
   void OnEvictImageMetadataDone(KeysCallback callback,
                                 std::vector<std::string> deleted_keys,
                                 bool success);
 
-  int estimated_size_;
+  std::map<CacheStrategy, int64_t> estimated_size_;
   InitializationStatus initialization_status_;
   std::unique_ptr<leveldb_proto::ProtoDatabase<CachedImageMetadataProto>>
       database_;

components/image_fetcher/core/cache/image_metadata_store_leveldb_unittest.cc

@@ -328,8 +328,14 @@ TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest, GetAllKeysLoadFailed) {
 
 TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest, GetEstimatedSize) {
   PrepareDatabase(true);
+  EXPECT_EQ(5, metadata_store()->GetEstimatedSize(CacheStrategy::BEST_EFFORT));
 
-  EXPECT_EQ(5, metadata_store()->GetEstimatedSize());
+  metadata_store()->SaveImageMetadata(kOtherImageKey, 15,
+                                      /* needs_transcoding */ false,
+                                      base::TimeDelta::FromDays(7));
+  EXPECT_EQ(5, metadata_store()->GetEstimatedSize(CacheStrategy::BEST_EFFORT));
+  EXPECT_EQ(15, metadata_store()->GetEstimatedSize(
+                    CacheStrategy::HOLD_UNTIL_EXPIRED));
 }
 
 TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest,
@@ -351,8 +357,20 @@ TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest,
 TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest, GarbageCollect) {
   PrepareDatabase(true);
 
+  metadata_store()->SaveImageMetadata(kOtherImageKey, 100,
+                                      /* needs_transcoding */ false,
+                                      base::TimeDelta::FromSeconds(1));
+  db()->UpdateCallback(true);
+  EXPECT_EQ(metadata_store()->GetEstimatedSize(CacheStrategy::BEST_EFFORT),
+            kImageDataLength);
+  EXPECT_EQ(
+      metadata_store()->GetEstimatedSize(CacheStrategy::HOLD_UNTIL_EXPIRED),
+      100);
+
   // Calling GC with something to be collected.
-  EXPECT_CALL(*this, OnKeysReturned(std::vector<std::string>({kImageKey})));
+  EXPECT_CALL(
+      *this,
+      OnKeysReturned(std::vector<std::string>({kImageKey, kOtherImageKey})));
   RunGarbageCollection(
       base::TimeDelta::FromHours(1), base::TimeDelta::FromHours(1),
       base::BindOnce(
@@ -362,6 +380,10 @@ TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest, GarbageCollect) {
   db()->UpdateCallback(true);
 
   ASSERT_FALSE(IsDataPresent(kImageKey));
+  ASSERT_FALSE(IsDataPresent(kOtherImageKey));
+  EXPECT_EQ(metadata_store()->GetEstimatedSize(CacheStrategy::BEST_EFFORT), 0);
+  EXPECT_EQ(
+      metadata_store()->GetEstimatedSize(CacheStrategy::HOLD_UNTIL_EXPIRED), 0);
 }
 
 TEST_F(CachedImageFetcherImageMetadataStoreLevelDBTest, GarbageCollectNoHits) {