[Offline Pages] New persistent page consistency check

Introduces a new consistency check strategy for persistent pages. Since
persistent pages should be living in an external directory and exposed
to third party file explorers, we should not deleting any files.

Some more details:
- For all persistent pages, try to see if it still has its archive file
 - If not and the page has been missing the file for a while, delete the
   entry from DB and notify the system download manager about deletion;
   If it's newly discovered missing, set the file missing time in DB.
 - If the file is there and the page has been marked as missing file,
   remove the file missing time.
- The new persistent page consistency check will run together with the
  ClearStorageTask, scheduled by actions(GetAllPages or SavePage) and
  limited by a time threshold.

Bug: 830102
Change-Id: Ie90d9fd988c642bdb6f598982f93e33642bd97ca
Reviewed-on: https://chromium-review.googlesource.com/1016015Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Peter Williamson <petewil@chromium.org>
Commit-Queue: Yafei Duan <romax@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552526}

components/offline_pages/core/BUILD.gn

@@ -44,6 +44,8 @@ static_library("core") {
     "model/offline_page_model_utils.h",
     "model/offline_page_upgrade_types.cc",
     "model/offline_page_upgrade_types.h",
+    "model/persistent_page_consistency_check_task.cc",
+    "model/persistent_page_consistency_check_task.h",
     "model/start_offline_page_upgrade_task.cc",
     "model/start_offline_page_upgrade_task.h",
     "model/startup_maintenance_task.cc",
@@ -167,6 +169,7 @@ source_set("unit_tests") {
     "model/mark_page_accessed_task_unittest.cc",
     "model/offline_page_model_taskified_unittest.cc",
     "model/offline_page_model_utils_unittest.cc",
+    "model/persistent_page_consistency_check_task_unittest.cc",
     "model/start_offline_page_upgrade_task_unittest.cc",
     "model/startup_maintenance_task_unittest.cc",
     "model/store_thumbnail_task_unittest.cc",

components/offline_pages/core/model/offline_page_item_generator.cc

@@ -32,6 +32,10 @@ OfflinePageItem OfflinePageItemGenerator::CreateItem() {
   item.last_access_time = last_access_time_;
   item.access_count = access_count_;
   item.digest = digest_;
+  item.file_missing_time = file_missing_time_;
+  if (use_offline_id_as_system_download_id_) {
+    item.system_download_id = item.offline_id;
+  }
   return item;
 }
 
@@ -95,4 +99,13 @@ void OfflinePageItemGenerator::SetDigest(const std::string& digest) {
   digest_ = digest;
 }
 
+void OfflinePageItemGenerator::SetFileMissingTime(
+    base::Time file_missing_time) {
+  file_missing_time_ = file_missing_time;
+}
+
+void OfflinePageItemGenerator::SetUseOfflineIdAsSystemDownloadId(bool enable) {
+  use_offline_id_as_system_download_id_ = enable;
+}
+
 }  // namespace offline_pages

components/offline_pages/core/model/offline_page_item_generator.h

@@ -36,6 +36,8 @@ class OfflinePageItemGenerator {
   void SetAccessCount(int access_count);
   void SetArchiveDirectory(const base::FilePath& archive_dir);
   void SetDigest(const std::string& digest);
+  void SetFileMissingTime(base::Time file_missing_time);
+  void SetUseOfflineIdAsSystemDownloadId(bool enable);
 
  private:
   std::string namespace_ = kDefaultNamespace;
@@ -49,6 +51,9 @@ class OfflinePageItemGenerator {
   int access_count_ = 0;
   base::FilePath archive_dir_;
   std::string digest_;
+  base::Time file_missing_time_;
+
+  bool use_offline_id_as_system_download_id_ = false;
 };
 }  // namespace offline_pages
 

components/offline_pages/core/model/offline_page_model_taskified.cc

@@ -28,6 +28,7 @@
 #include "components/offline_pages/core/model/get_thumbnail_task.h"
 #include "components/offline_pages/core/model/mark_page_accessed_task.h"
 #include "components/offline_pages/core/model/offline_page_model_utils.h"
+#include "components/offline_pages/core/model/persistent_page_consistency_check_task.h"
 #include "components/offline_pages/core/model/startup_maintenance_task.h"
 #include "components/offline_pages/core/model/store_thumbnail_task.h"
 #include "components/offline_pages/core/model/update_file_path_task.h"
@@ -715,6 +716,23 @@ void OfflinePageModelTaskified::RunMaintenanceTasks(const base::Time now,
       store_.get(), archive_manager_.get(), policy_controller_.get(), now,
       base::BindOnce(&OfflinePageModelTaskified::OnClearCachedPagesDone,
                      weak_ptr_factory_.GetWeakPtr())));
+
+  // TODO(https://crbug.com/834902) This might need a better execution plan.
+  task_queue_.AddTask(std::make_unique<PersistentPageConsistencyCheckTask>(
+      store_.get(), archive_manager_.get(), policy_controller_.get(), now,
+      base::BindOnce(
+          &OfflinePageModelTaskified::OnPersistentPageConsistencyCheckDone,
+          weak_ptr_factory_.GetWeakPtr())));
+}
+
+void OfflinePageModelTaskified::OnPersistentPageConsistencyCheckDone(
+    bool success,
+    const std::vector<int64_t>& pages_deleted) {
+  // If there's no persistent page expired, save some effort by exiting early.
+  // TODO(https://crbug.com/834909), use the temporary hidden bit in
+  // DownloadUIAdapter instead of calling remove directly.
+  if (pages_deleted.size() > 0)
+    download_manager_->Remove(pages_deleted);
 }
 
 void OfflinePageModelTaskified::OnClearCachedPagesDone(

components/offline_pages/core/model/offline_page_model_taskified.h

@@ -205,6 +205,9 @@ class OfflinePageModelTaskified : public OfflinePageModel,
   void RunMaintenanceTasks(const base::Time now, bool first_run);
   void OnClearCachedPagesDone(size_t deleted_page_count,
                               ClearStorageTask::ClearStorageResult result);
+  void OnPersistentPageConsistencyCheckDone(
+      bool success,
+      const std::vector<int64_t>& pages_deleted);
 
   // Method for upgrade to public storage.
   void PostSelectItemsMarkedForUpgrade();

components/offline_pages/core/model/offline_page_model_taskified_unittest.cc

@@ -23,6 +23,7 @@
 #include "components/offline_pages/core/model/offline_page_item_generator.h"
 #include "components/offline_pages/core/model/offline_page_model_utils.h"
 #include "components/offline_pages/core/model/offline_page_test_utils.h"
+#include "components/offline_pages/core/model/persistent_page_consistency_check_task.h"
 #include "components/offline_pages/core/offline_page_feature.h"
 #include "components/offline_pages/core/offline_page_item.h"
 #include "components/offline_pages/core/offline_page_metadata_store_sql.h"
@@ -829,7 +830,7 @@ TEST_F(OfflinePageModelTaskifiedTest, DeletePagesByOfflineId) {
   EXPECT_EQ(2LL, store_test_util()->GetPageCount());
 
   base::MockCallback<DeletePageCallback> callback;
-  EXPECT_CALL(callback, Run(testing::A<DeletePageResult>()));
+  EXPECT_CALL(callback, Run(A<DeletePageResult>()));
   CheckTaskQueueIdle();
 
   model()->DeletePagesByOfflineId({page1.offline_id}, callback.Get());
@@ -1399,11 +1400,12 @@ TEST_F(OfflinePageModelTaskifiedTest, GetAllPages) {
 // This test is affected by https://crbug.com/725685, which only affects windows
 // platform.
 #if defined(OS_WIN)
-#define MAYBE_ConsistencyCheckExecuted DISABLED_ConsistencyCheckExecuted
+#define MAYBE_StartupMaintenanceTaskExecuted \
+  DISABLED_StartupMaintenanceTaskExecuted
 #else
-#define MAYBE_ConsistencyCheckExecuted ConsistencyCheckExecuted
+#define MAYBE_StartupMaintenanceTaskExecuted StartupMaintenanceTaskExecuted
 #endif
-TEST_F(OfflinePageModelTaskifiedTest, MAYBE_ConsistencyCheckExecuted) {
+TEST_F(OfflinePageModelTaskifiedTest, MAYBE_StartupMaintenanceTaskExecuted) {
   // Insert temporary pages
   page_generator()->SetArchiveDirectory(temporary_dir_path());
   page_generator()->SetNamespace(kDefaultNamespace);
@@ -1420,12 +1422,11 @@ TEST_F(OfflinePageModelTaskifiedTest, MAYBE_ConsistencyCheckExecuted) {
 
   // Insert persistent pages.
   page_generator()->SetNamespace(kDownloadNamespace);
-  // Page missing archive file in pesistent directory.
+  // Page missing archive file in private directory.
   OfflinePageItem persistent_page1 = page_generator()->CreateItem();
   // Page missing metadata entry in database since it's not inserted into store.
   OfflinePageItem persistent_page2 = page_generator()->CreateItemWithTempFile();
-  // Page in persistent namespace saved in persistent directory to simulate
-  // pages saved in legacy directory.
+  // Page in persistent namespace saved in private directory.
   OfflinePageItem persistent_page3 = page_generator()->CreateItemWithTempFile();
   InsertPageIntoStore(persistent_page1);
   InsertPageIntoStore(persistent_page3);
@@ -1438,7 +1439,7 @@ TEST_F(OfflinePageModelTaskifiedTest, MAYBE_ConsistencyCheckExecuted) {
             test_utils::GetFileCountInDirectory(private_archive_dir_path()));
 
   // Execute GetAllPages and move the clock forward to cover the delay, in order
-  // to trigger consistency checks.
+  // to trigger StartupMaintenanceTask execution.
   base::MockCallback<MultipleOfflinePageItemCallback> callback;
   model()->GetAllPages(callback.Get());
   task_runner()->FastForwardBy(
@@ -1446,7 +1447,7 @@ TEST_F(OfflinePageModelTaskifiedTest, MAYBE_ConsistencyCheckExecuted) {
       base::TimeDelta::FromMilliseconds(1));
   PumpLoop();
 
-  EXPECT_EQ(1LL, store_test_util()->GetPageCount());
+  EXPECT_EQ(2LL, store_test_util()->GetPageCount());
   EXPECT_EQ(0UL, test_utils::GetFileCountInDirectory(temporary_dir_path()));
   EXPECT_EQ(1UL,
             test_utils::GetFileCountInDirectory(private_archive_dir_path()));
@@ -1525,6 +1526,115 @@ TEST_F(OfflinePageModelTaskifiedTest, ClearStorage) {
                                        1);
 }
 
+// This test is affected by https://crbug.com/725685, which only affects windows
+// platform.
+#if defined(OS_WIN)
+#define MAYBE_PersistentPageConsistencyCheckExecuted \
+  DISABLED_PersistentPageConsistencyCheckExecuted
+#else
+#define MAYBE_PersistentPageConsistencyCheckExecuted \
+  PersistentPageConsistencyCheckExecuted
+#endif
+TEST_F(OfflinePageModelTaskifiedTest, PersistentPageConsistencyCheckExecuted) {
+  // The PersistentPageConsistencyCheckTask should not be executed based on time
+  // delays after launch (aka the model being built).
+  task_runner()->FastForwardBy(base::TimeDelta::FromDays(1));
+  PumpLoop();
+  histogram_tester()->ExpectTotalCount(
+      "OfflinePages.ConsistencyCheck.Persistent.Result", 0);
+
+  // GetAllPages should schedule a delayed task that will eventually run
+  // PersistentPageConsistencyCheck.
+  base::MockCallback<MultipleOfflinePageItemCallback> callback;
+  model()->GetAllPages(callback.Get());
+  PumpLoop();
+  histogram_tester()->ExpectTotalCount(
+      "OfflinePages.ConsistencyCheck.Persistent.Result", 0);
+
+  // Add a persistent page with file.
+  page_generator()->SetNamespace(kDownloadNamespace);
+  page_generator()->SetArchiveDirectory(public_archive_dir_path());
+  OfflinePageItem page = page_generator()->CreateItemWithTempFile();
+  page.system_download_id = kDownloadId;
+  InsertPageIntoStore(page);
+  EXPECT_EQ(1UL,
+            test_utils::GetFileCountInDirectory(public_archive_dir_path()));
+  EXPECT_EQ(1LL, store_test_util()->GetPageCount());
+
+  // After the delay (plus 1 millisecond just in case), the consistency check
+  // should be enqueued and executed.
+  const base::TimeDelta run_delay =
+      OfflinePageModelTaskified::kMaintenanceTasksDelay +
+      base::TimeDelta::FromMilliseconds(1);
+  task_runner()->FastForwardBy(run_delay);
+  PumpLoop();
+  // But nothing should change.
+  EXPECT_EQ(1UL,
+            test_utils::GetFileCountInDirectory(public_archive_dir_path()));
+  EXPECT_EQ(1LL, store_test_util()->GetPageCount());
+  histogram_tester()->ExpectTotalCount(
+      "OfflinePages.ConsistencyCheck.Persistent.Result", 1);
+
+  // Delete the file associated with |page|, so the next time when the
+  // consistency check is executed, the page will be marked as hidden.
+  base::DeleteFile(page.file_path, false);
+
+  // Calling GetAllPages after only half of the enforced interval between
+  // consistency check runs should not schedule the task.
+  // Note: The previous elapsed delay is discounted from the clock advance here.
+  task_runner()->FastForwardBy(
+      OfflinePageModelTaskified::kClearStorageInterval / 2 - run_delay);
+  model()->GetAllPages(callback.Get());
+  // And advance the delay too.
+  task_runner()->FastForwardBy(run_delay);
+  PumpLoop();
+  // Confirm no persistent page consistency check is executed.
+  histogram_tester()->ExpectTotalCount(
+      "OfflinePages.ConsistencyCheck.Persistent.Result", 1);
+
+  // Forwarding by the full interval (plus 1 second just in case) should allow
+  // the task to be enqueued again and call GetAllPages again to enqueue the
+  // task.
+  task_runner()->FastForwardBy(
+      OfflinePageModelTaskified::kClearStorageInterval / 2 +
+      base::TimeDelta::FromSeconds(1));
+  model()->GetAllPages(callback.Get());
+  // And advance the delay too.
+  task_runner()->FastForwardBy(run_delay);
+  PumpLoop();
+  // Confirm persistent page consistency check is executed, and the page is
+  // marked as missing file.
+  EXPECT_EQ(0UL,
+            test_utils::GetFileCountInDirectory(public_archive_dir_path()));
+  EXPECT_EQ(1LL, store_test_util()->GetPageCount());
+  auto actual_page = store_test_util()->GetPageByOfflineId(page.offline_id);
+  ASSERT_TRUE(actual_page);
+  EXPECT_NE(base::Time(), actual_page->file_missing_time);
+  histogram_tester()->ExpectTotalCount(
+      "OfflinePages.ConsistencyCheck.Persistent.Result", 2);
+
+  // Forwarding by a long time that is enough for the page with missing file to
+  // get expired.
+  task_runner()->FastForwardBy(base::TimeDelta::FromDays(400));
+  // Saving a page should also immediately enqueue the consistency check task.
+  auto archiver = BuildArchiver(kTestUrl, ArchiverResult::SUCCESSFULLY_CREATED);
+  SavePageWithExpectedResult(kTestUrl, kTestClientId1, kTestUrl2,
+                             kEmptyRequestOrigin, std::move(archiver),
+                             SavePageResult::SUCCESS);
+  // Advance the delay to activate task execution.
+  task_runner()->FastForwardBy(run_delay);
+  PumpLoop();
+  // Confirm persistent page consistency check is executed, and the page is
+  // deleted from database, also notified system download manager.
+  EXPECT_EQ(0UL,
+            test_utils::GetFileCountInDirectory(public_archive_dir_path()));
+  EXPECT_EQ(1LL, store_test_util()->GetPageCount());
+  EXPECT_EQ(page.system_download_id,
+            download_manager_stub()->last_removed_id());
+  histogram_tester()->ExpectTotalCount(
+      "OfflinePages.ConsistencyCheck.Persistent.Result", 3);
+}
+
 TEST_F(OfflinePageModelTaskifiedTest, MaintenanceTasksAreDisabled) {
   // The maintenance tasks should not be executed when disabled by tests.
   model()->DoNotRunMaintenanceTasksForTesting();

components/offline_pages/core/model/persistent_page_consistency_check_task.cc

+// Copyright 2018 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/offline_pages/core/model/persistent_page_consistency_check_task.h"
+
+#include <string>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/files/file_util.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/numerics/safe_conversions.h"
+#include "components/offline_pages/core/archive_manager.h"
+#include "components/offline_pages/core/client_policy_controller.h"
+#include "components/offline_pages/core/offline_page_client_policy.h"
+#include "components/offline_pages/core/offline_page_metadata_store_sql.h"
+#include "components/offline_pages/core/offline_store_utils.h"
+#include "sql/connection.h"
+#include "sql/statement.h"
+#include "sql/transaction.h"
+
+namespace base {
+class Time;
+}  // namespace base
+
+namespace offline_pages {
+
+namespace {
+
+#define OFFLINE_PAGES_TABLE_NAME "offlinepages_v1"
+
+const base::TimeDelta kExpireThreshold = base::TimeDelta::FromDays(365);
+
+struct PageInfo {
+  int64_t offline_id;
+  base::FilePath file_path;
+  base::Time file_missing_time;
+  int64_t system_download_id;
+};
+
+std::vector<PageInfo> GetPageInfosByNamespaces(
+    const std::vector<std::string>& temp_namespaces,
+    sql::Connection* db) {
+  std::vector<PageInfo> result;
+
+  static const char kSql[] =
+      "SELECT offline_id, file_path, file_missing_time, system_download_id"
+      " FROM " OFFLINE_PAGES_TABLE_NAME " WHERE client_namespace = ?";
+
+  for (const auto& temp_namespace : temp_namespaces) {
+    sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+    statement.BindString(0, temp_namespace);
+    while (statement.Step()) {
+      result.push_back(
+          {statement.ColumnInt64(0),
+           store_utils::FromDatabaseFilePath(statement.ColumnString(1)),
+           store_utils::FromDatabaseTime(statement.ColumnInt64(2)),
+           statement.ColumnInt64(3)});
+    }
+  }
+
+  return result;
+}
+
+bool DeletePagesByOfflineIds(const std::vector<int64_t>& offline_ids,
+                             sql::Connection* db) {
+  static const char kSql[] =
+      "DELETE FROM " OFFLINE_PAGES_TABLE_NAME " WHERE offline_id = ?";
+
+  for (const auto& offline_id : offline_ids) {
+    sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+    statement.BindInt64(0, offline_id);
+    if (!statement.Run())
+      return false;
+  }
+  return true;
+}
+
+bool MarkPagesAsMissing(const std::vector<int64_t>& ids_of_missing_pages,
+                        base::Time missing_time,
+                        sql::Connection* db) {
+  static const char kSql[] = "UPDATE OR IGNORE " OFFLINE_PAGES_TABLE_NAME
+                             " SET file_missing_time = ?"
+                             " WHERE offline_id = ?";
+
+  for (auto offline_id : ids_of_missing_pages) {
+    sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+    statement.BindInt64(0, store_utils::ToDatabaseTime(missing_time));
+    statement.BindInt64(1, offline_id);
+    if (!statement.Run())
+      return false;
+  }
+  return true;
+}
+
+bool MarkPagesAsReappeared(const std::vector<int64_t>& ids_of_reappeared_pages,
+                           sql::Connection* db) {
+  static const char kSql[] = "UPDATE OR IGNORE " OFFLINE_PAGES_TABLE_NAME
+                             " SET file_missing_time = ?"
+                             " WHERE offline_id = ?";
+
+  base::Time invalid_time;
+  for (auto offline_id : ids_of_reappeared_pages) {
+    sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+    statement.BindInt64(0, store_utils::ToDatabaseTime(invalid_time));
+    statement.BindInt64(1, offline_id);
+    if (!statement.Run())
+      return false;
+  }
+  return true;
+}
+
+PersistentPageConsistencyCheckTask::CheckResult
+PersistentPageConsistencyCheckSync(
+    OfflinePageMetadataStoreSQL* store,
+    const base::FilePath& private_dir,
+    const base::FilePath& public_dir,
+    const std::vector<std::string>& persistent_namespaces,
+    base::Time check_time,
+    sql::Connection* db) {
+  std::vector<int64_t> download_ids_of_deleted_pages;
+  if (!db)
+    return {SyncOperationResult::INVALID_DB_CONNECTION,
+            download_ids_of_deleted_pages};
+
+  sql::Transaction transaction(db);
+  if (!transaction.Begin())
+    return {SyncOperationResult::TRANSACTION_BEGIN_ERROR,
+            download_ids_of_deleted_pages};
+
+  std::vector<PageInfo> persistent_page_infos =
+      GetPageInfosByNamespaces(persistent_namespaces, db);
+
+  std::vector<int64_t> pages_found_missing;
+  std::vector<int64_t> pages_reappeared;
+  std::vector<int64_t> page_ids_to_delete;
+  for (const auto& page_info : persistent_page_infos) {
+    if (base::PathExists(page_info.file_path)) {
+      if (page_info.file_missing_time != base::Time())
+        pages_reappeared.push_back(page_info.offline_id);
+    } else {
+      if (page_info.file_missing_time == base::Time()) {
+        pages_found_missing.push_back(page_info.offline_id);
+      } else {
+        if (check_time - page_info.file_missing_time > kExpireThreshold) {
+          page_ids_to_delete.push_back(page_info.offline_id);
+          download_ids_of_deleted_pages.push_back(page_info.system_download_id);
+        }
+      }
+    }
+  }
+
+  if (!DeletePagesByOfflineIds(page_ids_to_delete, db) ||
+      !MarkPagesAsMissing(pages_found_missing, check_time, db) ||
+      !MarkPagesAsReappeared(pages_reappeared, db)) {
+    return {SyncOperationResult::DB_OPERATION_ERROR,
+            download_ids_of_deleted_pages};
+  }
+
+  if (page_ids_to_delete.size() > 0) {
+    UMA_HISTOGRAM_COUNTS_1M(
+        "OfflinePages.ConsistencyCheck.Persistent.ExpiredEntryCount",
+        base::saturated_cast<int32_t>(page_ids_to_delete.size()));
+  }
+  if (pages_found_missing.size() > 0) {
+    UMA_HISTOGRAM_COUNTS_1M(
+        "OfflinePages.ConsistencyCheck.Persistent.MissingFileCount",
+        base::saturated_cast<int32_t>(pages_found_missing.size()));
+  }
+  if (pages_reappeared.size() > 0) {
+    UMA_HISTOGRAM_COUNTS_1M(
+        "OfflinePages.ConsistencyCheck.Persistent.ReappearedFileCount",
+        base::saturated_cast<int32_t>(pages_reappeared.size()));
+  }
+
+  if (!transaction.Commit())
+    return {SyncOperationResult::TRANSACTION_COMMIT_ERROR,
+            download_ids_of_deleted_pages};
+
+  return {SyncOperationResult::SUCCESS, download_ids_of_deleted_pages};
+}
+
+}  // namespace
+
+PersistentPageConsistencyCheckTask::CheckResult::CheckResult() = default;
+
+PersistentPageConsistencyCheckTask::CheckResult::CheckResult(
+    SyncOperationResult result,
+    const std::vector<int64_t>& system_download_ids)
+    : result(result), download_ids_of_deleted_pages(system_download_ids) {}
+
+PersistentPageConsistencyCheckTask::CheckResult::CheckResult(
+    const CheckResult& other) = default;
+
+PersistentPageConsistencyCheckTask::CheckResult&
+PersistentPageConsistencyCheckTask::CheckResult::operator=(
+    const CheckResult& other) = default;
+
+PersistentPageConsistencyCheckTask::CheckResult::~CheckResult() {}
+
+PersistentPageConsistencyCheckTask::PersistentPageConsistencyCheckTask(
+    OfflinePageMetadataStoreSQL* store,
+    ArchiveManager* archive_manager,
+    ClientPolicyController* policy_controller,
+    base::Time check_time,
+    PersistentPageConsistencyCheckCallback callback)
+    : store_(store),
+      archive_manager_(archive_manager),
+      policy_controller_(policy_controller),
+      check_time_(check_time),
+      callback_(std::move(callback)),
+      weak_ptr_factory_(this) {
+  DCHECK(store_);
+  DCHECK(archive_manager_);
+  DCHECK(policy_controller_);
+}
+
+PersistentPageConsistencyCheckTask::~PersistentPageConsistencyCheckTask() =
+    default;
+
+void PersistentPageConsistencyCheckTask::Run() {
+  std::vector<std::string> namespaces = policy_controller_->GetAllNamespaces();
+  std::vector<std::string> persistent_namespaces;
+  for (const auto& name_space : namespaces) {
+    if (!policy_controller_->IsRemovedOnCacheReset(name_space))
+      persistent_namespaces.push_back(name_space);
+  }
+
+  store_->Execute(base::BindOnce(&PersistentPageConsistencyCheckSync, store_,
+                                 archive_manager_->GetPrivateArchivesDir(),
+                                 archive_manager_->GetPublicArchivesDir(),
+                                 persistent_namespaces, check_time_),
+                  base::BindOnce(&PersistentPageConsistencyCheckTask::
+                                     OnPersistentPageConsistencyCheckDone,
+                                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void PersistentPageConsistencyCheckTask::OnPersistentPageConsistencyCheckDone(
+    CheckResult check_result) {
+  UMA_HISTOGRAM_ENUMERATION("OfflinePages.ConsistencyCheck.Persistent.Result",
+                            check_result.result,
+                            SyncOperationResult::RESULT_COUNT);
+  // If sync operation failed, invoke the callback with an empty list of
+  // download ids.
+  if (check_result.result != SyncOperationResult::SUCCESS) {
+    std::move(callback_).Run(false, {});
+  } else {
+    std::move(callback_).Run(true, check_result.download_ids_of_deleted_pages);
+  }
+  TaskComplete();
+}
+
+}  // namespace offline_pages

components/offline_pages/core/model/persistent_page_consistency_check_task.h

+// Copyright 2018 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.
+
+#ifndef COMPONENTS_OFFLINE_PAGES_CORE_MODEL_PERSISTENT_PAGE_CONSISTENCY_CHECK_TASK_H_
+#define COMPONENTS_OFFLINE_PAGES_CORE_MODEL_PERSISTENT_PAGE_CONSISTENCY_CHECK_TASK_H_
+
+#include <vector>
+
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "components/offline_pages/core/offline_store_types.h"
+#include "components/offline_pages/core/task.h"
+
+namespace offline_pages {
+
+class ArchiveManager;
+class ClientPolicyController;
+class OfflinePageMetadataStoreSQL;
+
+// This task is responsible for checking consistency of persistent pages, mark
+// the expired ones with the file missing time and recover the previously
+// missing entries back to normal.
+class PersistentPageConsistencyCheckTask : public Task {
+ public:
+  using PersistentPageConsistencyCheckCallback =
+      base::OnceCallback<void(bool success,
+                              const std::vector<int64_t>& pages_deleted)>;
+
+  struct CheckResult {
+    CheckResult();
+    CheckResult(SyncOperationResult result,
+                const std::vector<int64_t>& system_download_ids);
+    CheckResult(const CheckResult& other);
+    CheckResult& operator=(const CheckResult& other);
+    ~CheckResult();
+
+    SyncOperationResult result;
+    std::vector<int64_t> download_ids_of_deleted_pages;
+  };
+
+  PersistentPageConsistencyCheckTask(
+      OfflinePageMetadataStoreSQL* store,
+      ArchiveManager* archive_manager,
+      ClientPolicyController* policy_controller,
+      base::Time check_time,
+      PersistentPageConsistencyCheckCallback callback);
+  ~PersistentPageConsistencyCheckTask() override;
+
+  // Task implementation:
+  void Run() override;
+
+ private:
+  void OnPersistentPageConsistencyCheckDone(CheckResult result);
+
+  // The store containing the offline pages. Not owned.
+  OfflinePageMetadataStoreSQL* store_;
+  // The archive manager storing archive directories. Not owned.
+  ArchiveManager* archive_manager_;
+  // The policy controller which is used to acquire names of namespaces. Not
+  // owned.
+  ClientPolicyController* policy_controller_;
+  base::Time check_time_;
+  // The callback for the task.
+  PersistentPageConsistencyCheckCallback callback_;
+
+  base::WeakPtrFactory<PersistentPageConsistencyCheckTask> weak_ptr_factory_;
+  DISALLOW_COPY_AND_ASSIGN(PersistentPageConsistencyCheckTask);
+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_CORE_MODEL_PERSISTENT_PAGE_CONSISTENCY_CHECK_TASK_H_

components/offline_pages/core/model/persistent_page_consistency_check_task_unittest.cc

+// Copyright 2018 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/offline_pages/core/model/persistent_page_consistency_check_task.h"
+
+#include "base/bind.h"
+#include "base/test/histogram_tester.h"
+#include "base/test/mock_callback.h"
+#include "base/time/time.h"
+#include "build/build_config.h"
+#include "components/offline_pages/core/client_namespace_constants.h"
+#include "components/offline_pages/core/model/model_task_test_base.h"
+#include "components/offline_pages/core/model/offline_page_test_utils.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::A;
+using testing::Eq;
+using testing::UnorderedElementsAre;
+
+namespace offline_pages {
+
+using PersistentPageConsistencyCheckCallback =
+    PersistentPageConsistencyCheckTask::PersistentPageConsistencyCheckCallback;
+
+class PersistentPageConsistencyCheckTaskTest : public ModelTaskTestBase {
+ public:
+  PersistentPageConsistencyCheckTaskTest();
+  ~PersistentPageConsistencyCheckTaskTest() override;
+
+  void SetUp() override;
+  bool IsPageMissingFile(const OfflinePageItem& page);
+
+  base::HistogramTester* histogram_tester() { return histogram_tester_.get(); }
+
+ private:
+  std::unique_ptr<base::HistogramTester> histogram_tester_;
+};
+
+PersistentPageConsistencyCheckTaskTest::
+    PersistentPageConsistencyCheckTaskTest() {}
+
+PersistentPageConsistencyCheckTaskTest::
+    ~PersistentPageConsistencyCheckTaskTest() {}
+
+void PersistentPageConsistencyCheckTaskTest::SetUp() {
+  ModelTaskTestBase::SetUp();
+  histogram_tester_ = std::make_unique<base::HistogramTester>();
+}
+
+bool PersistentPageConsistencyCheckTaskTest::IsPageMissingFile(
+    const OfflinePageItem& page) {
+  auto actual_page = store_test_util()->GetPageByOfflineId(page.offline_id);
+  return (actual_page && actual_page->file_missing_time != base::Time());
+}
+
+// This test is affected by https://crbug.com/725685, which only affects windows
+// platform.
+#if defined(OS_WIN)
+#define MAYBE_ClearExpiredPersistentPages DISABLED_ClearExpiredPersistentPages
+#else
+#define MAYBE_ClearExpiredPersistentPages ClearExpiredPersistentPages
+#endif
+TEST_F(PersistentPageConsistencyCheckTaskTest,
+       MAYBE_ClearExpiredPersistentPages) {
+  base::Time expire_time = base::Time::Now() - base::TimeDelta::FromDays(400);
+
+  // |page{1,4}| will be marked as missing file.
+  // |page{2,5}| will be deleted from DB, since they've been expired for longer
+  // than threshold.
+  // |page{3,6}| will remove the file_missing_time from the entry, since
+  // they've been missing files but the files appeared again.
+  generator()->SetUseOfflineIdAsSystemDownloadId(true);
+  generator()->SetNamespace(kDownloadNamespace);
+  generator()->SetArchiveDirectory(PrivateDir());
+  OfflinePageItem page1 = AddPageWithoutFile();
+  generator()->SetFileMissingTime(expire_time);
+  OfflinePageItem page2 = AddPageWithoutFile();
+  OfflinePageItem page3 = AddPage();
+
+  generator()->SetArchiveDirectory(PublicDir());
+  generator()->SetFileMissingTime(base::Time());
+  OfflinePageItem page4 = AddPageWithoutFile();
+  generator()->SetFileMissingTime(expire_time);
+  OfflinePageItem page5 = AddPageWithoutFile();
+  OfflinePageItem page6 = AddPage();
+
+  EXPECT_EQ(6LL, store_test_util()->GetPageCount());
+  EXPECT_EQ(1UL, test_utils::GetFileCountInDirectory(PrivateDir()));
+  EXPECT_EQ(1UL, test_utils::GetFileCountInDirectory(PublicDir()));
+
+  base::MockCallback<PersistentPageConsistencyCheckCallback> callback;
+  EXPECT_CALL(callback,
+              Run(Eq(true), UnorderedElementsAre(page2.system_download_id,
+                                                 page5.system_download_id)));
+
+  auto task = std::make_unique<PersistentPageConsistencyCheckTask>(
+      store(), archive_manager(), policy_controller(), base::Time::Now(),
+      callback.Get());
+  RunTask(std::move(task));
+
+  EXPECT_EQ(4LL, store_test_util()->GetPageCount());
+  EXPECT_EQ(1UL, test_utils::GetFileCountInDirectory(PrivateDir()));
+  EXPECT_EQ(1UL, test_utils::GetFileCountInDirectory(PublicDir()));
+  EXPECT_TRUE(store_test_util()->GetPageByOfflineId(page1.offline_id));
+  EXPECT_TRUE(IsPageMissingFile(page1));
+  EXPECT_FALSE(store_test_util()->GetPageByOfflineId(page2.offline_id));
+  EXPECT_TRUE(store_test_util()->GetPageByOfflineId(page3.offline_id));
+  EXPECT_FALSE(IsPageMissingFile(page3));
+  EXPECT_TRUE(store_test_util()->GetPageByOfflineId(page4.offline_id));
+  EXPECT_TRUE(IsPageMissingFile(page4));
+  EXPECT_FALSE(store_test_util()->GetPageByOfflineId(page5.offline_id));
+  EXPECT_TRUE(store_test_util()->GetPageByOfflineId(page6.offline_id));
+  EXPECT_FALSE(IsPageMissingFile(page6));
+
+  histogram_tester()->ExpectUniqueSample(
+      "OfflinePages.ConsistencyCheck.Persistent.ExpiredEntryCount", 2, 1);
+  histogram_tester()->ExpectUniqueSample(
+      "OfflinePages.ConsistencyCheck.Persistent.MissingFileCount", 2, 1);
+  histogram_tester()->ExpectUniqueSample(
+      "OfflinePages.ConsistencyCheck.Persistent.ReappearedFileCount", 2, 1);
+  histogram_tester()->ExpectUniqueSample(
+      "OfflinePages.ConsistencyCheck.Persistent.Result",
+      static_cast<int>(SyncOperationResult::SUCCESS), 1);
+}
+
+}  // namespace offline_pages

components/offline_pages/core/model/startup_maintenance_task.cc

@@ -89,29 +89,51 @@ bool DeleteFiles(const std::vector<base::FilePath>& file_paths) {
   return result;
 }
 
-SyncOperationResult ClearLegacyTempPagesSync(
+// This method is clearing the private dir(the legacy dir).
+// - For all files associated with temporary pages:
+//   The strategy is if any temporary page
+//   is still left behind in the legacy dir, delete them.
+// - For all files associated with persistent pages:
+//   Leave them as-is, since they might be still in use.
+// - For all files without any associated DB entry:
+//   Delete the files, since they're 'headless' and has no way to be accessed.
+SyncOperationResult ClearLegacyPagesInPrivateDirSync(
     sql::Connection* db,
-    const std::vector<std::string>& namespaces,
-    const base::FilePath& legacy_archives_dir) {
+    const std::vector<std::string>& temporary_namespaces,
+    const std::vector<std::string>& persistent_namespaces,
+    const base::FilePath& private_dir) {
   // One large database transaction that will:
   // 1. Get temporary page infos from the database.
-  // 2. Decide which pages to delete (still in legacy archive directory).
-  // 3. Delete metadata entries from the database.
+  // 2. Get persistent page infos from the database, in case they're in private
+  //    dir.
+  // 3. Get all file paths in private dir as a set F.
+  // 4. For each temporary page info:
+  //    - If its file path is in F, record its offline id for deletion.
+  // 5. For each persistent page info:
+  //    - If its file path is in F, remove it from F.
+  // 6. Delete page entries by recorded offline ids, and delete the remaining
+  //    files in F.
   sql::Transaction transaction(db);
   if (!transaction.Begin())
     return SyncOperationResult::TRANSACTION_BEGIN_ERROR;
 
-  std::vector<PageInfo> temp_page_infos =
-      GetPageInfosByNamespaces(namespaces, db);
+  std::vector<PageInfo> temporary_page_infos =
+      GetPageInfosByNamespaces(temporary_namespaces, db);
+  std::vector<PageInfo> persistent_page_infos =
+      GetPageInfosByNamespaces(persistent_namespaces, db);
+  std::map<base::FilePath, PageInfo> path_to_page_info;
 
+  std::set<base::FilePath> archive_paths = GetAllArchives(private_dir);
   std::vector<int64_t> offline_ids_to_delete;
-  std::vector<base::FilePath> files_to_delete;
-  for (const auto& page_info : temp_page_infos) {
-    // Get pages whose archive files are still in the legacy archives directory.
-    if (legacy_archives_dir.IsParent(page_info.file_path)) {
+
+  for (const auto& page_info : temporary_page_infos) {
+    if (archive_paths.find(page_info.file_path) != archive_paths.end())
       offline_ids_to_delete.push_back(page_info.offline_id);
-      files_to_delete.push_back(page_info.file_path);
-    }
+  }
+  for (const auto& page_info : persistent_page_infos) {
+    auto iter = archive_paths.find(page_info.file_path);
+    if (iter != archive_paths.end())
+      archive_paths.erase(iter);
   }
 
   // Try to delete the pages by offline ids collected above.
@@ -124,17 +146,26 @@ SyncOperationResult ClearLegacyTempPagesSync(
   if (!transaction.Commit())
     return SyncOperationResult::TRANSACTION_COMMIT_ERROR;
 
+  std::vector<base::FilePath> files_to_delete(archive_paths.begin(),
+                                              archive_paths.end());
   if (!DeleteFiles(files_to_delete))
     return SyncOperationResult::FILE_OPERATION_ERROR;
 
+  size_t headless_file_count =
+      files_to_delete.size() - offline_ids_to_delete.size();
+  if (headless_file_count > 0) {
+    UMA_HISTOGRAM_COUNTS_1M(
+        "OfflinePages.ConsistencyCheck.Legacy.DeletedHeadlessFileCount",
+        headless_file_count);
+  }
+
   return SyncOperationResult::SUCCESS;
 }
 
-SyncOperationResult CheckConsistencySync(
+SyncOperationResult CheckTemporaryPageConsistencySync(
     sql::Connection* db,
     const std::vector<std::string>& namespaces,
-    const base::FilePath& archives_dir,
-    const std::string& prefix_for_uma) {
+    const base::FilePath& archives_dir) {
   // One large database transaction that will:
   // 1. Get page infos by |namespaces| from the database.
   // 2. Decide which pages to delete.
@@ -165,9 +196,8 @@ SyncOperationResult CheckConsistencySync(
     // committed.
     if (!DeletePagesByOfflineIds(offline_ids_to_delete, db))
       return SyncOperationResult::DB_OPERATION_ERROR;
-    base::UmaHistogramCounts1M(
-        "OfflinePages.ConsistencyCheck." + prefix_for_uma +
-            ".PagesMissingArchiveFileCount",
+    UMA_HISTOGRAM_COUNTS_1M(
+        "OfflinePages.ConsistencyCheck.Temporary.PagesMissingArchiveFileCount",
         base::saturated_cast<int32_t>(offline_ids_to_delete.size()));
   }
 
@@ -186,9 +216,9 @@ SyncOperationResult CheckConsistencySync(
   if (files_to_delete.size() > 0) {
     if (!DeleteFiles(files_to_delete))
       return SyncOperationResult::FILE_OPERATION_ERROR;
-    base::UmaHistogramCounts1M("OfflinePages.ConsistencyCheck." +
-                                   prefix_for_uma + ".PagesMissingDbEntryCount",
-                               static_cast<int32_t>(files_to_delete.size()));
+    UMA_HISTOGRAM_COUNTS_1M(
+        "OfflinePages.ConsistencyCheck.Temporary.PagesMissingDbEntryCount",
+        static_cast<int32_t>(files_to_delete.size()));
   }
 
   return SyncOperationResult::SUCCESS;
@@ -232,25 +262,16 @@ bool StartupMaintenanceSync(OfflinePageMetadataStoreSQL* store,
 
   // Clear temporary pages that are in legacy directory, which is also the
   // directory that serves as the 'private' directory.
-  SyncOperationResult result = ClearLegacyTempPagesSync(
-      db, temporary_namespaces, archive_manager->GetPrivateArchivesDir());
+  SyncOperationResult result = ClearLegacyPagesInPrivateDirSync(
+      db, temporary_namespaces, persistent_namespaces,
+      archive_manager->GetPrivateArchivesDir());
 
   // Clear temporary pages in cache directory.
-  result = CheckConsistencySync(db, temporary_namespaces,
-                                archive_manager->GetTemporaryArchivesDir(),
-                                "Temporary" /*prefix_for_uma*/);
+  result = CheckTemporaryPageConsistencySync(
+      db, temporary_namespaces, archive_manager->GetTemporaryArchivesDir());
   UMA_HISTOGRAM_ENUMERATION("OfflinePages.ConsistencyCheck.Temporary.Result",
                             result, SyncOperationResult::RESULT_COUNT);
 
-  // Clear persistent pages in private directory.
-  // TODO(romax): this can be merged with the legacy temporary pages clearing
-  // above.
-  result = CheckConsistencySync(db, persistent_namespaces,
-                                archive_manager->GetPrivateArchivesDir(),
-                                "Persistent" /*prefix_for_uma*/);
-  UMA_HISTOGRAM_ENUMERATION("OfflinePages.ConsistencyCheck.Persistent.Result",
-                            result, SyncOperationResult::RESULT_COUNT);
-
   // Report storage usage UMA.
   ReportStorageUsageSync(db, namespaces, archive_manager);
 

components/offline_pages/core/model/startup_maintenance_task.h

@@ -16,8 +16,8 @@ class ClientPolicyController;
 class OfflinePageMetadataStoreSQL;
 
 // This task is responsible for executing maintenance sub-tasks during Chrome
-// startup, including: consistency checks, legacy directory cleaning and report
-// storage usage UMA.
+// startup, including: temporary page consistency check, legacy directory
+// cleaning and report storage usage UMA.
 class StartupMaintenanceTask : public Task {
  public:
   StartupMaintenanceTask(OfflinePageMetadataStoreSQL* store,

tools/metrics/histograms/histograms.xml

@@ -58436,9 +58436,40 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="OfflinePages.ConsistencyCheck.Legacy.DeletedHeadlessFileCount"
+    units="files">
+  <owner>romax@chromium.org</owner>
+  <summary>
+    Number of files which are deleted during legacy dir clearing since they have
+    no associated DB entry and live in private directory.
+  </summary>
+</histogram>
+
+<histogram name="OfflinePages.ConsistencyCheck.Persistent.ExpiredEntryCount"
+    units="entries">
+  <owner>romax@chromium.org</owner>
+  <summary>
+    Number of DB entries (in persistent namespaces) that have been missing their
+    files for longer than 365 days, and deleted during maintenance task.
+  </summary>
+</histogram>
+
+<histogram name="OfflinePages.ConsistencyCheck.Persistent.MissingFileCount"
+    units="files">
+  <owner>romax@chromium.org</owner>
+  <summary>
+    Number of files that are found missing during maintenance task, which is
+    also the number of DB entries that are updated with valid file missing time.
+  </summary>
+</histogram>
+
 <histogram
     name="OfflinePages.ConsistencyCheck.Persistent.PagesMissingArchiveFileCount"
     units="pages">
+  <obsolete>
+    Deprecated 04/2018, since saving public offline pages to external download
+    directory needs a different consistency check strategy.
+  </obsolete>
   <owner>romax@chromium.org</owner>
   <summary>
     Number of persistent offline pages without archive file when checking
@@ -58449,6 +58480,10 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
 <histogram
     name="OfflinePages.ConsistencyCheck.Persistent.PagesMissingDbEntryCount"
     units="pages">
+  <obsolete>
+    Deprecated 04/2018, since saving public offline pages to external download
+    directory needs a different consistency check strategy.
+  </obsolete>
   <owner>romax@chromium.org</owner>
   <summary>
     Number of archives without database entry when checking persistent page
@@ -58456,6 +58491,15 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="OfflinePages.ConsistencyCheck.Persistent.ReappearedFileCount"
+    units="files">
+  <owner>romax@chromium.org</owner>
+  <summary>
+    Number of files that were marked as missing reappeared in the file system,
+    which is also the number of DB entries that removes file missing time.
+  </summary>
+</histogram>
+
 <histogram name="OfflinePages.ConsistencyCheck.Persistent.Result"
     enum="OfflinePagesSyncOperationResult">
   <owner>romax@chromium.org</owner>