[Paint Preview] Expire old previews

This CL expires old captures after 72 hours in order to allow for
previews to be reasonably current.

Bug: TODO
Change-Id: I0c86c928dc73564e3721d4d8f79b8b7693931619
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2386223Reviewed-by: Mehran Mahmoudi <mahmoudi@chromium.org>
Commit-Queue: Calder Kitagawa <ckitagawa@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803946}

chrome/browser/paint_preview/services/paint_preview_tab_service.cc

@@ -30,6 +30,8 @@ namespace {
 
 constexpr size_t kMaxPerCaptureSizeBytes = 5 * 1000L * 1000L;    // 5 MB.
 constexpr size_t kMaximumTotalCaptureSize = 25 * 1000L * 1000L;  // 25 MB.
+// The time horizon after which unused paint previews will be deleted.
+constexpr int kExpiryHorizonHrs = 72;
 
 #if defined(OS_ANDROID)
 void JavaBooleanCallbackAdapter(base::OnceCallback<void(bool)> callback,
@@ -153,6 +155,18 @@ void PaintPreviewTabService::AuditArtifacts(
                      weak_ptr_factory_.GetWeakPtr(), active_tab_ids));
 }
 
+void PaintPreviewTabService::GetCapturedPaintPreviewProto(
+    const DirectoryKey& key,
+    base::Optional<base::TimeDelta> expiry_horizon,
+    PaintPreviewBaseService::OnReadProtoCallback on_read_proto_callback) {
+  PaintPreviewBaseService::GetCapturedPaintPreviewProto(
+      key,
+      expiry_horizon.has_value()
+          ? expiry_horizon.value()
+          : base::TimeDelta::FromHours(kExpiryHorizonHrs),
+      std::move(on_read_proto_callback));
+}
+
 #if defined(OS_ANDROID)
 void PaintPreviewTabService::CaptureTabAndroid(
     JNIEnv* env,
@@ -273,7 +287,8 @@ void PaintPreviewTabService::OnFinished(int tab_id,
   GetTaskRunner()->PostTaskAndReplyWithResult(
       FROM_HERE,
       base::BindOnce(&FileManager::GetOldestArtifactsForCleanup, file_manager,
-                     kMaximumTotalCaptureSize),
+                     kMaximumTotalCaptureSize,
+                     base::TimeDelta::FromHours(kExpiryHorizonHrs)),
       base::BindOnce(&PaintPreviewTabService::CleanupOldestFiles,
                      weak_ptr_factory_.GetWeakPtr(), tab_id));
 }

chrome/browser/paint_preview/services/paint_preview_tab_service.h

@@ -78,6 +78,14 @@ class PaintPreviewTabService : public PaintPreviewBaseService {
   // occurred.
   void AuditArtifacts(const std::vector<int>& active_tab_ids);
 
+  // Override for GetCapturedPaintPreviewProto. Defaults expiry horizon to 72
+  // hrs if not specified.
+  void GetCapturedPaintPreviewProto(
+      const DirectoryKey& key,
+      base::Optional<base::TimeDelta> expiry_horizon,
+      PaintPreviewBaseService::OnReadProtoCallback on_read_proto_callback)
+      override;
+
 #if defined(OS_ANDROID)
   // JNI wrapped versions of the above methods
   void CaptureTabAndroid(

components/paint_preview/browser/file_manager.cc

@@ -218,13 +218,22 @@ bool FileManager::SerializePaintPreviewProto(const DirectoryKey& key,
   return result;
 }
 
-std::unique_ptr<PaintPreviewProto> FileManager::DeserializePaintPreviewProto(
-    const DirectoryKey& key) const {
+std::pair<FileManager::ProtoReadStatus, std::unique_ptr<PaintPreviewProto>>
+FileManager::DeserializePaintPreviewProto(const DirectoryKey& key) const {
   DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
   auto path = CreateOrGetDirectory(key, false);
   if (!path.has_value())
-    return nullptr;
-  return ReadProtoFromFile(path->AppendASCII(kProtoName));
+    return std::make_pair(ProtoReadStatus::kNoProto, nullptr);
+
+  auto proto_path = path->AppendASCII(kProtoName);
+  if (!base::PathExists(proto_path))
+    return std::make_pair(ProtoReadStatus::kNoProto, nullptr);
+
+  auto proto = ReadProtoFromFile(path->AppendASCII(kProtoName));
+  if (proto == nullptr)
+    return std::make_pair(ProtoReadStatus::kDeserializationError, nullptr);
+
+  return std::make_pair(ProtoReadStatus::kOk, std::move(proto));
 }
 
 base::flat_set<DirectoryKey> FileManager::ListUsedKeys() const {
@@ -242,13 +251,8 @@ base::flat_set<DirectoryKey> FileManager::ListUsedKeys() const {
 }
 
 std::vector<DirectoryKey> FileManager::GetOldestArtifactsForCleanup(
-    size_t max_size) {
-  // The rest of this function is expensive so cleanup should exit early if not
-  // required.
-  size_t size = base::ComputeDirectorySize(root_directory_);
-  if (size <= max_size)
-    return std::vector<DirectoryKey>();
-
+    size_t max_size,
+    base::TimeDelta expiry_horizon) {
   base::FileEnumerator file_enum(
       root_directory_, false,
       base::FileEnumerator::FILES | base::FileEnumerator::DIRECTORIES);
@@ -260,22 +264,29 @@ std::vector<DirectoryKey> FileManager::GetOldestArtifactsForCleanup(
 
   std::sort(file_infos.begin(), file_infos.end(), CompareByLastModified);
 
+  // If the oldest file doesn't need to expire attempt to early exit.
+  base::Time expiry_threshold =
+      base::Time::NowFromSystemTime() - expiry_horizon;
+
+  size_t size = base::ComputeDirectorySize(root_directory_);
   std::vector<DirectoryKey> keys_to_remove;
   for (const auto& file_info : file_infos) {
-    base::FilePath full_path = root_directory_.Append(file_info.GetName());
+    // Stop when both the max size and expiry threshold requirements are met.
+    if (size <= max_size && file_info.GetLastModifiedTime() > expiry_threshold)
+      break;
 
     size_t size_delta = file_info.GetSize();
     // Computing a directory size is expensive. Most files should hopefully be
     // compressed already.
-    if (file_info.IsDirectory())
+    if (file_info.IsDirectory()) {
+      base::FilePath full_path = root_directory_.Append(file_info.GetName());
       size_delta = base::ComputeDirectorySize(full_path);
+    }
 
     // Directory names should always be ASCII.
     keys_to_remove.emplace_back(
         file_info.GetName().RemoveExtension().MaybeAsASCII());
     size -= size_delta;
-    if (size <= max_size)
-      break;
   }
   return keys_to_remove;
 }

components/paint_preview/browser/file_manager.h

@@ -24,6 +24,12 @@ class PaintPreviewTabService;
 // This class is refcounted so scheduled tasks may continue during shutdown.
 class FileManager : public base::RefCountedThreadSafe<FileManager> {
  public:
+  enum class ProtoReadStatus : int {
+    kOk = 0,
+    kNoProto,
+    kDeserializationError,
+  };
+
   // Create a file manager for |root_directory|. Top level items in
   // |root_directoy| should be exclusively managed by this class. Items within
   // the subdirectories it creates can be freely modified. All methods will be
@@ -83,10 +89,10 @@ class FileManager : public base::RefCountedThreadSafe<FileManager> {
                                   const PaintPreviewProto& proto,
                                   bool compress) const;
 
-  // Deserializes PaintPreviewProto stored in |key|. Returns nullptr if not
-  // found or the proto cannot be parsed.
-  std::unique_ptr<PaintPreviewProto> DeserializePaintPreviewProto(
-      const DirectoryKey& key) const;
+  // Deserializes PaintPreviewProto stored in |key|. Returns a status and the
+  // proto. The proto will be nullptr if the ProtoReadStatus != kOk.
+  std::pair<ProtoReadStatus, std::unique_ptr<PaintPreviewProto>>
+  DeserializePaintPreviewProto(const DirectoryKey& key) const;
 
   // Lists the current set of in-use DirectoryKeys.
   base::flat_set<DirectoryKey> ListUsedKeys() const;
@@ -94,7 +100,9 @@ class FileManager : public base::RefCountedThreadSafe<FileManager> {
   // Returns a list of the least recently modified artifact sets until which
   // when deleted would result in a total capture size on disk that is less than
   // |max_size|.
-  std::vector<DirectoryKey> GetOldestArtifactsForCleanup(size_t max_size);
+  std::vector<DirectoryKey> GetOldestArtifactsForCleanup(
+      size_t max_size,
+      base::TimeDelta expiry_horizon);
 
  private:
   friend class base::RefCountedThreadSafe<FileManager>;

components/paint_preview/browser/file_manager_unittest.cc

@@ -252,8 +252,9 @@ TEST_F(FileManagerTest, HandleProto) {
   EXPECT_TRUE(manager->SerializePaintPreviewProto(key, original_proto, false));
   EXPECT_TRUE(base::PathExists(path.AppendASCII("proto.pb")));
   auto out_proto = manager->DeserializePaintPreviewProto(key);
-  EXPECT_NE(out_proto, nullptr);
-  EXPECT_THAT(*out_proto, EqualsProto(original_proto));
+  EXPECT_EQ(out_proto.first, FileManager::ProtoReadStatus::kOk);
+  EXPECT_NE(out_proto.second, nullptr);
+  EXPECT_THAT(*(out_proto.second), EqualsProto(original_proto));
 }
 
 TEST_F(FileManagerTest, HandleProtoCompressed) {
@@ -280,8 +281,9 @@ TEST_F(FileManagerTest, HandleProtoCompressed) {
 
   EXPECT_TRUE(base::PathExists(path.AddExtensionASCII(".zip")));
   auto out_proto = manager->DeserializePaintPreviewProto(key);
-  EXPECT_NE(out_proto, nullptr);
-  EXPECT_THAT(*out_proto, EqualsProto(original_proto));
+  EXPECT_EQ(out_proto.first, FileManager::ProtoReadStatus::kOk);
+  EXPECT_NE(out_proto.second, nullptr);
+  EXPECT_THAT(*(out_proto.second), EqualsProto(original_proto));
 
   EXPECT_TRUE(manager->CaptureExists(key));
 }
@@ -297,14 +299,19 @@ TEST_F(FileManagerTest, OldestFilesForCleanup) {
   auto key_0 = manager->CreateKey(0U);
   base::FilePath path_0 =
       manager->CreateOrGetDirectory(key_0, true).value_or(base::FilePath());
-  base::WriteFile(path_0.AppendASCII("0.txt"), data.data(), data.size());
+  auto path_0_file = path_0.AppendASCII("0.txt");
+  base::WriteFile(path_0_file, data.data(), data.size());
+  base::Time modified_time = base::Time::NowFromSystemTime();
+  base::TouchFile(path_0_file, modified_time, modified_time);
   {
-    auto to_delete = manager->GetOldestArtifactsForCleanup(0U);
+    auto to_delete = manager->GetOldestArtifactsForCleanup(
+        0U, base::TimeDelta::FromMinutes(20));
     ASSERT_EQ(to_delete.size(), 1U);
     EXPECT_EQ(to_delete[0], key_0);
   }
   {
-    auto to_delete = manager->GetOldestArtifactsForCleanup(50U);
+    auto to_delete = manager->GetOldestArtifactsForCleanup(
+        50U, base::TimeDelta::FromMinutes(20));
     EXPECT_EQ(to_delete.size(), 0U);
   }
 
@@ -313,14 +320,15 @@ TEST_F(FileManagerTest, OldestFilesForCleanup) {
       manager->CreateOrGetDirectory(key_1, true).value_or(base::FilePath());
   base::WriteFile(path_1.AppendASCII("1.txt"), data.data(), data.size());
   manager->CompressDirectory(key_1);
-  base::Time modified_time = base::Time::Now();
+  modified_time = base::Time::NowFromSystemTime();
   auto path_1_zip = path_1.AddExtensionASCII(".zip");
   base::TouchFile(path_0, modified_time - base::TimeDelta::FromSeconds(10),
                   modified_time - base::TimeDelta::FromSeconds(10));
   base::TouchFile(path_1_zip, modified_time, modified_time);
 
   {
-    auto to_delete = manager->GetOldestArtifactsForCleanup(0U);
+    auto to_delete = manager->GetOldestArtifactsForCleanup(
+        0U, base::TimeDelta::FromMinutes(20));
     ASSERT_EQ(to_delete.size(), 2U);
     // Elements should be ordered in oldest to newest.
     EXPECT_EQ(to_delete[0], key_0);
@@ -328,12 +336,14 @@ TEST_F(FileManagerTest, OldestFilesForCleanup) {
   }
   {
     // Zip is ~116 bytes.
-    auto to_delete = manager->GetOldestArtifactsForCleanup(120U);
+    auto to_delete = manager->GetOldestArtifactsForCleanup(
+        120U, base::TimeDelta::FromMinutes(20));
     ASSERT_EQ(to_delete.size(), 1U);
     EXPECT_EQ(to_delete[0], key_0);
   }
   {
-    auto to_delete = manager->GetOldestArtifactsForCleanup(150U);
+    auto to_delete = manager->GetOldestArtifactsForCleanup(
+        150U, base::TimeDelta::FromMinutes(20));
     EXPECT_EQ(to_delete.size(), 0U);
   }
 }

components/paint_preview/browser/paint_preview_base_service.cc

@@ -50,12 +50,51 @@ PaintPreviewBaseService::~PaintPreviewBaseService() = default;
 
 void PaintPreviewBaseService::GetCapturedPaintPreviewProto(
     const DirectoryKey& key,
+    base::Optional<base::TimeDelta> expiry_horizon,
     OnReadProtoCallback on_read_proto_callback) {
   task_runner_->PostTaskAndReplyWithResult(
       FROM_HERE,
-      base::BindOnce(&FileManager::DeserializePaintPreviewProto, file_manager_,
-                     key),
-      std::move(on_read_proto_callback));
+      base::BindOnce(
+          [](scoped_refptr<FileManager> file_manager, const DirectoryKey& key,
+             base::Optional<base::TimeDelta> expiry_horizon)
+              -> std::pair<PaintPreviewBaseService::ProtoReadStatus,
+                           std::unique_ptr<PaintPreviewProto>> {
+            if (expiry_horizon.has_value()) {
+              auto file_info = file_manager->GetInfo(key);
+              if (!file_info.has_value())
+                return std::make_pair(ProtoReadStatus::kNoProto, nullptr);
+
+              if (file_info->last_modified + expiry_horizon.value() <
+                  base::Time::NowFromSystemTime()) {
+                return std::make_pair(ProtoReadStatus::kExpired, nullptr);
+              }
+            }
+            auto result = file_manager->DeserializePaintPreviewProto(key);
+            PaintPreviewBaseService::ProtoReadStatus status =
+                ProtoReadStatus::kNoProto;
+            switch (result.first) {
+              case FileManager::ProtoReadStatus::kOk:
+                status = ProtoReadStatus::kOk;
+                break;
+              case FileManager::ProtoReadStatus::kNoProto:
+                status = ProtoReadStatus::kNoProto;
+                break;
+              case FileManager::ProtoReadStatus::kDeserializationError:
+                status = ProtoReadStatus::kDeserializationError;
+                break;
+              default:
+                NOTREACHED();
+            }
+            return std::make_pair(status, std::move(result.second));
+          },
+          file_manager_, key, expiry_horizon),
+      base::BindOnce(
+          [](OnReadProtoCallback callback,
+             std::pair<PaintPreviewBaseService::ProtoReadStatus,
+                       std::unique_ptr<PaintPreviewProto>> result) {
+            std::move(callback).Run(result.first, std::move(result.second));
+          },
+          std::move(on_read_proto_callback)));
 }
 
 void PaintPreviewBaseService::CapturePaintPreview(
@@ -80,14 +119,14 @@ void PaintPreviewBaseService::CapturePaintPreview(
     OnCapturedCallback callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (policy_ && !policy_->SupportedForContents(web_contents)) {
-    std::move(callback).Run(kContentUnsupported, {});
+    std::move(callback).Run(CaptureStatus::kContentUnsupported, {});
     return;
   }
 
   PaintPreviewClient::CreateForWebContents(web_contents);  // Is a singleton.
   auto* client = PaintPreviewClient::FromWebContents(web_contents);
   if (!client) {
-    std::move(callback).Run(kClientCreationFailed, {});
+    std::move(callback).Run(CaptureStatus::kClientCreationFailed, {});
     return;
   }
 
@@ -131,12 +170,12 @@ void PaintPreviewBaseService::OnCaptured(
       !result->capture_success) {
     DVLOG(1) << "ERROR: Paint Preview failed to capture for document "
              << guid.ToString() << " with error " << status;
-    std::move(callback).Run(kCaptureFailed, {});
+    std::move(callback).Run(CaptureStatus::kCaptureFailed, {});
     return;
   }
   base::UmaHistogramTimes("Browser.PaintPreview.Capture.TotalCaptureDuration",
                           base::TimeTicks::Now() - start_time);
-  std::move(callback).Run(kOk, std::move(result));
+  std::move(callback).Run(CaptureStatus::kOk, std::move(result));
 }
 
 }  // namespace paint_preview

components/paint_preview/browser/paint_preview_base_service.h

@@ -41,18 +41,26 @@ namespace paint_preview {
 // - SimpleKeyedServiceFactory
 class PaintPreviewBaseService : public KeyedService {
  public:
-  enum CaptureStatus {
+  enum class CaptureStatus : int {
     kOk = 0,
     kContentUnsupported,
     kClientCreationFailed,
     kCaptureFailed,
   };
 
+  enum class ProtoReadStatus : int {
+    kOk = 0,
+    kNoProto,
+    kDeserializationError,
+    kExpired,
+  };
+
   using OnCapturedCallback =
       base::OnceCallback<void(CaptureStatus, std::unique_ptr<CaptureResult>)>;
 
   using OnReadProtoCallback =
-      base::OnceCallback<void(std::unique_ptr<PaintPreviewProto>)>;
+      base::OnceCallback<void(ProtoReadStatus,
+                              std::unique_ptr<PaintPreviewProto>)>;
 
   // Creates a service instance for a feature. Artifacts produced will live in
   // |profile_dir|/paint_preview/|ascii_feature_name|. Implementers of the
@@ -79,10 +87,15 @@ class PaintPreviewBaseService : public KeyedService {
 
   // Acquires the PaintPreviewProto that is associated with |key| and sends it
   // to |on_read_proto_callback|. The default implementation attempts to invoke
-  // GetFileManager()->DeserializePaintPreviewProto(). Derived classes may
-  // override this function; for example, the proto is cached in memory.
+  // GetFileManager()->DeserializePaintPreviewProto(). If |expiry_horizon| is
+  // provided a proto that was last modified earlier than |now - expiry_horizon|
+  // will return the kExpired status.
+  //
+  // Derived classes may override this function; for example, the proto is
+  // cached in memory.
   virtual void GetCapturedPaintPreviewProto(
       const DirectoryKey& key,
+      base::Optional<base::TimeDelta> expiry_horizon,
       OnReadProtoCallback on_read_proto_callback);
 
   // Captures need to run on the Browser UI thread! Captures may involve child

components/paint_preview/player/compositor_status.h

@@ -23,6 +23,8 @@ enum class CompositorStatus : int {
   INVALID_REQUEST,
   OLD_VERSION,
   UNEXPECTED_VERSION,
+  CAPTURE_EXPIRED,
+  NO_CAPTURE,
   COUNT,
 };
 

components/paint_preview/player/player_compositor_delegate.cc

@@ -112,8 +112,9 @@ PlayerCompositorDelegate::PlayerCompositorDelegate(
                                        base::OnTaskRunnerDeleter(nullptr)) {
   if (skip_service_launch) {
     paint_preview_service_->GetCapturedPaintPreviewProto(
-        key, base::BindOnce(&PlayerCompositorDelegate::OnProtoAvailable,
-                            weak_factory_.GetWeakPtr(), expected_url));
+        key, base::nullopt,
+        base::BindOnce(&PlayerCompositorDelegate::OnProtoAvailable,
+                       weak_factory_.GetWeakPtr(), expected_url));
     return;
   }
   TRACE_EVENT_NESTABLE_ASYNC_BEGIN0("paint_preview",
@@ -179,15 +180,29 @@ void PlayerCompositorDelegate::OnCompositorClientCreated(
                                   "PlayerCompositorDelegate CreateCompositor",
                                   TRACE_ID_LOCAL(this));
   paint_preview_service_->GetCapturedPaintPreviewProto(
-      key, base::BindOnce(&PlayerCompositorDelegate::OnProtoAvailable,
-                          weak_factory_.GetWeakPtr(), expected_url));
+      key, base::nullopt,
+      base::BindOnce(&PlayerCompositorDelegate::OnProtoAvailable,
+                     weak_factory_.GetWeakPtr(), expected_url));
 }
 
 void PlayerCompositorDelegate::OnProtoAvailable(
     const GURL& expected_url,
+    PaintPreviewBaseService::ProtoReadStatus proto_status,
     std::unique_ptr<PaintPreviewProto> proto) {
-  if (!proto || !proto->IsInitialized()) {
-    // TODO(crbug.com/1021590): Handle initialization errors.
+  // TODO(crbug.com/1021590): Handle initialization errors.
+  if (proto_status == PaintPreviewBaseService::ProtoReadStatus::kExpired) {
+    OnCompositorReady(CompositorStatus::CAPTURE_EXPIRED, nullptr);
+    return;
+  }
+
+  if (proto_status == PaintPreviewBaseService::ProtoReadStatus::kNoProto) {
+    OnCompositorReady(CompositorStatus::NO_CAPTURE, nullptr);
+    return;
+  }
+
+  if (proto_status ==
+          PaintPreviewBaseService::ProtoReadStatus::kDeserializationError ||
+      !proto || !proto->IsInitialized()) {
     OnCompositorReady(CompositorStatus::PROTOBUF_DESERIALIZATION_ERROR,
                       nullptr);
     return;

components/paint_preview/player/player_compositor_delegate.h

@@ -76,6 +76,7 @@ class PlayerCompositorDelegate {
   void OnCompositorClientDisconnected();
 
   void OnProtoAvailable(const GURL& expected_url,
+                        PaintPreviewBaseService::ProtoReadStatus proto_status,
                         std::unique_ptr<PaintPreviewProto> proto);
   void SendCompositeRequest(
       mojom::PaintPreviewBeginCompositeRequestPtr begin_composite_request);

tools/metrics/histograms/enums.xml

@@ -69572,6 +69572,8 @@ would be helpful to identify which type is being sent.
   <int value="7" label="Invalid Request"/>
   <int value="8" label="Old Version"/>
   <int value="9" label="Unexpected Version"/>
+  <int value="10" label="Capture Expired"/>
+  <int value="11" label="No Capture"/>
 </enum>
 
 <enum name="TabbedPaintPreviewExitCause">