[Offline Pages] Filtering expired pages and fix consistency check.

components/offline_pages/offline_page_model_impl.cc

 // Copyright 2016 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/offline_page_model_impl.h"
 
 #include <algorithm>
 #include <limits>
 
 #include "base/bind.h"
@@ skipping 288 matching lines @@
       archives_dir_, offline_id,
       base::Bind(&OfflinePageModelImpl::OnCreateArchiveDone,
                  weak_ptr_factory_.GetWeakPtr(), url, offline_id, client_id,
                  base::Time::Now(), callback));
   pending_archivers_.push_back(std::move(archiver));
 }
 
 void OfflinePageModelImpl::MarkPageAccessed(int64_t offline_id) {
   DCHECK(is_loaded_);
   auto iter = offline_pages_.find(offline_id);
-  if (iter == offline_pages_.end())
+  if (iter == offline_pages_.end() || iter->second.IsExpired())
     return;
 
   // Make a copy of the cached item and update it. The cached item should only
   // be updated upon the successful store operation.
   OfflinePageItem offline_page_item = iter->second;
 
   ReportPageHistogramsAfterAccess(offline_page_item);
 
   offline_page_item.last_access_time = base::Time::Now();
   offline_page_item.access_count++;
@@ skipping 87 matching lines @@
 }
 
 void OfflinePageModelImpl::HasPagesAfterLoadDone(
     const std::string& name_space,
     const HasPagesCallback& callback) const {
   DCHECK(is_loaded_);
 
   bool has_pages = false;
 
   for (const auto& id_page_pair : offline_pages_) {
-    if (id_page_pair.second.client_id.name_space == name_space) {
+    if (id_page_pair.second.client_id.name_space == name_space &&
+        !id_page_pair.second.IsExpired()) {
       has_pages = true;
       break;
     }
   }
 
   callback.Run(has_pages);
 }
 
 void OfflinePageModelImpl::CheckPagesExistOffline(
     const std::set<GURL>& urls,
     const CheckPagesExistOfflineCallback& callback) {
   RunWhenLoaded(
       base::Bind(&OfflinePageModelImpl::CheckPagesExistOfflineAfterLoadDone,
                  weak_ptr_factory_.GetWeakPtr(), urls, callback));
 }
 
 void OfflinePageModelImpl::CheckPagesExistOfflineAfterLoadDone(
     const std::set<GURL>& urls,
     const CheckPagesExistOfflineCallback& callback) {
   DCHECK(is_loaded_);
   CheckPagesExistOfflineResult result;
   for (const auto& id_page_pair : offline_pages_) {
+    if (id_page_pair.second.IsExpired())
+      continue;
     auto iter = urls.find(id_page_pair.second.url);
     if (iter != urls.end())
       result.insert(*iter);
   }
   callback.Run(result);
 }
 
 void OfflinePageModelImpl::GetAllPages(
     const MultipleOfflinePageItemCallback& callback) {
   RunWhenLoaded(base::Bind(&OfflinePageModelImpl::GetAllPagesAfterLoadDone,
                            weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void OfflinePageModelImpl::GetAllPagesAfterLoadDone(
     const MultipleOfflinePageItemCallback& callback) const {
   DCHECK(is_loaded_);
 
   MultipleOfflinePageItemResult offline_pages;
-  for (const auto& id_page_pair : offline_pages_)
-    offline_pages.push_back(id_page_pair.second);
+  for (const auto& id_page_pair : offline_pages_) {
+    if (!id_page_pair.second.IsExpired())
+      offline_pages.push_back(id_page_pair.second);
+  }
 
   callback.Run(offline_pages);
 }
 
 void OfflinePageModelImpl::GetOfflineIdsForClientId(
     const ClientId& client_id,
     const MultipleOfflineIdCallback& callback) {
   RunWhenLoaded(
       base::Bind(&OfflinePageModelImpl::GetOfflineIdsForClientIdWhenLoadDone,
                  weak_ptr_factory_.GetWeakPtr(), client_id, callback));
 }
 
 void OfflinePageModelImpl::GetOfflineIdsForClientIdWhenLoadDone(
     const ClientId& client_id,
     const MultipleOfflineIdCallback& callback) const {
   callback.Run(MaybeGetOfflineIdsForClientId(client_id));
 }
 
 const std::vector<int64_t> OfflinePageModelImpl::MaybeGetOfflineIdsForClientId(
     const ClientId& client_id) const {
   DCHECK(is_loaded_);
   std::vector<int64_t> results;
 
   // We want only all pages, including those marked for deletion.
   // TODO(bburns): actually use an index rather than linear scan.
   for (const auto& id_page_pair : offline_pages_) {
-    if (id_page_pair.second.client_id == client_id)
+    if (id_page_pair.second.client_id == client_id &&
+        !id_page_pair.second.IsExpired()) {
       results.push_back(id_page_pair.second.offline_id);
+    }
   }
   return results;
 }
 
 void OfflinePageModelImpl::GetPageByOfflineId(
     int64_t offline_id,
     const SingleOfflinePageItemCallback& callback) {
   RunWhenLoaded(
       base::Bind(&OfflinePageModelImpl::GetPageByOfflineIdWhenLoadDone,
                  weak_ptr_factory_.GetWeakPtr(), offline_id, callback));
 }
 
 void OfflinePageModelImpl::GetPageByOfflineIdWhenLoadDone(
     int64_t offline_id,
     const SingleOfflinePageItemCallback& callback) const {
   SingleOfflinePageItemResult result;
   const OfflinePageItem* match = MaybeGetPageByOfflineId(offline_id);
   if (match != nullptr)
     result = *match;
   callback.Run(result);
 }
 
 const OfflinePageItem* OfflinePageModelImpl::MaybeGetPageByOfflineId(
     int64_t offline_id) const {
   const auto iter = offline_pages_.find(offline_id);
-  return iter != offline_pages_.end() ? &(iter->second) : nullptr;
+  return iter != offline_pages_.end() && !iter->second.IsExpired()
+             ? &(iter->second)
+             : nullptr;
 }
 
 void OfflinePageModelImpl::GetPageByOfflineURL(
     const GURL& offline_url,
     const SingleOfflinePageItemCallback& callback) {
   RunWhenLoaded(
       base::Bind(&OfflinePageModelImpl::GetPageByOfflineURLWhenLoadDone,
                  weak_ptr_factory_.GetWeakPtr(), offline_url, callback));
 }
 
 void OfflinePageModelImpl::GetPageByOfflineURLWhenLoadDone(
     const GURL& offline_url,
     const SingleOfflinePageItemCallback& callback) const {
+  // Getting pages by offline URL does not exclude expired pages, as the caller
+  // already holds the offline URL and simply needs to look up a corresponding
+  // online URL.
   base::Optional<OfflinePageItem> result;
 
   for (const auto& id_page_pair : offline_pages_) {
     if (id_page_pair.second.GetOfflineURL() == offline_url) {
       callback.Run(base::make_optional(id_page_pair.second));
       return;
     }
   }
 
   callback.Run(base::nullopt);
 }
 
 const OfflinePageItem* OfflinePageModelImpl::MaybeGetPageByOfflineURL(
     const GURL& offline_url) const {
+  // Getting pages by offline URL does not exclude expired pages, as the caller
+  // already holds the offline URL and simply needs to look up a corresponding
+  // online URL.
   for (const auto& id_page_pair : offline_pages_) {
     if (id_page_pair.second.GetOfflineURL() == offline_url)
       return &(id_page_pair.second);
   }
   return nullptr;
 }
 
 void OfflinePageModelImpl::GetBestPageForOnlineURL(
     const GURL& online_url,
     const SingleOfflinePageItemCallback callback) {
@@ skipping 21 matching lines @@
       base::Bind(&OfflinePageModelImpl::GetPagesByOnlineURLWhenLoadDone,
                  weak_ptr_factory_.GetWeakPtr(), online_url, callback));
 }
 
 void OfflinePageModelImpl::GetPagesByOnlineURLWhenLoadDone(
     const GURL& online_url,
     const MultipleOfflinePageItemCallback& callback) const {
   std::vector<OfflinePageItem> result;
 
   for (const auto& id_page_pair : offline_pages_) {
-    if (id_page_pair.second.url == online_url)
+    if (id_page_pair.second.url == online_url &&
+        !id_page_pair.second.IsExpired()) {
       result.push_back(id_page_pair.second);
+    }
   }
 
   callback.Run(result);
 }
 
 const OfflinePageItem* OfflinePageModelImpl::MaybeGetBestPageForOnlineURL(
     const GURL& online_url) const {
   const OfflinePageItem* result = nullptr;
   for (const auto& id_page_pair : offline_pages_) {
-    if (id_page_pair.second.url == online_url) {
+    if (id_page_pair.second.url == online_url &&
+        !id_page_pair.second.IsExpired()) {
       if (!result || id_page_pair.second.creation_time > result->creation_time)
         result = &(id_page_pair.second);
     }
   }
   return result;
 }
 
-void OfflinePageModelImpl::CheckForExternalFileDeletion() {
+void OfflinePageModelImpl::CheckMetadataConsistency() {
   DCHECK(is_loaded_);
 
   archive_manager_->GetAllArchives(
-      base::Bind(&OfflinePageModelImpl::ScanForMissingArchiveFiles,
+      base::Bind(&OfflinePageModelImpl::DoCheckMetadataConsistency,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 void OfflinePageModelImpl::ExpirePages(
     const std::vector<int64_t>& offline_ids,
     const base::Time& expiration_time,
     const base::Callback<void(bool)>& callback) {
   std::vector<base::FilePath> paths_to_delete;
   for (int64_t offline_id : offline_ids) {
     auto iter = offline_pages_.find(offline_id);
@@ skipping 149 matching lines @@
   storage_manager_.reset(new OfflinePageStorageManager(
       this, GetPolicyController(), archive_manager_.get()));
 
   // Run all the delayed tasks.
   for (const auto& delayed_task : delayed_tasks_)
     delayed_task.Run();
   delayed_tasks_.clear();
 
   FOR_EACH_OBSERVER(Observer, observers_, OfflinePageModelLoaded(this));
 
-  CheckForExternalFileDeletion();
+  CheckMetadataConsistency();
 
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE, base::Bind(&OfflinePageModelImpl::ClearStorageIfNeeded,
                             weak_ptr_factory_.GetWeakPtr(),
                             base::Bind(&OfflinePageModelImpl::OnStorageCleared,
                                        weak_ptr_factory_.GetWeakPtr())),
       kStorageManagerStartingDelay);
 }
 
 void OfflinePageModelImpl::InformSavePageDone(const SavePageCallback& callback,
@@ skipping 60 matching lines @@
     DeletePageResult result) {
   UMA_HISTOGRAM_ENUMERATION("OfflinePages.DeletePageResult",
                             static_cast<int>(result),
                             static_cast<int>(DeletePageResult::RESULT_COUNT));
   archive_manager_->GetStorageStats(
       base::Bind(&ReportStorageHistogramsAfterDelete));
   if (!callback.is_null())
     callback.Run(result);
 }
 
-void OfflinePageModelImpl::ScanForMissingArchiveFiles(
+void OfflinePageModelImpl::DoCheckMetadataConsistency(
+    const std::set<base::FilePath>& archive_paths) {
+  ExpirePagesMissingArchiveFile(archive_paths);
+  DeleteHeadlessArchives(archive_paths);

[jianli, 2016/06/08 21:10:35]

I think "Orphaned" is better than "Headless".

[romax, 2016/06/08 22:18:59]

sure, i've changed to orphaned.

+}
+
+void OfflinePageModelImpl::ExpirePagesMissingArchiveFile(
     const std::set<base::FilePath>& archive_paths) {
   std::vector<int64_t> ids_of_pages_missing_archive_file;
-  std::vector<std::pair<int64_t, ClientId>> offline_client_id_pairs;
   for (const auto& id_page_pair : offline_pages_) {
-    if (archive_paths.count(id_page_pair.second.file_path) == 0UL) {
+    if (archive_paths.count(id_page_pair.second.file_path) == 0UL)
       ids_of_pages_missing_archive_file.push_back(id_page_pair.first);
-      offline_client_id_pairs.push_back(
-          std::make_pair(id_page_pair.first, id_page_pair.second.client_id));
-    }
+  }

[fgorski, 2016/06/08 20:27:07]

nit: could you put a space here for better readability?

[romax, 2016/06/08 22:18:59]

Done.

+  if (ids_of_pages_missing_archive_file.empty())
+    return;

[fgorski, 2016/06/08 20:27:07]

nit: same here (by space I mean empty line of course)

[romax, 2016/06/08 22:18:59]

Done.

+  ExpirePages(
+      ids_of_pages_missing_archive_file, base::Time::Now(),
+      base::Bind(&OfflinePageModelImpl::OnExpirePagesMissingArchiveFileDone,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 ids_of_pages_missing_archive_file));
+}
+
+void OfflinePageModelImpl::OnExpirePagesMissingArchiveFileDone(
+    const std::vector<int64_t>& offline_ids,
+    bool success) {
+  UMA_HISTOGRAM_COUNTS("OfflinePages.Consistency.PagesMissingArchiveFileCount",
+                       static_cast<int32_t>(offline_ids.size()));
+  UMA_HISTOGRAM_BOOLEAN(
+      "OfflinePages.Consistency.ExpirePagesMissingArchiveFileResult", success);
+}
+
+void OfflinePageModelImpl::DeleteHeadlessArchives(
+    const std::set<base::FilePath>& archive_paths) {
+  // Archives are considered unused unless they are pointed to by some pages.

[jianli, 2016/06/08 21:10:35]

unused => orphaned

[romax, 2016/06/08 22:18:59]

Done.

+  std::set<base::FilePath> unused_archives(archive_paths);
+  for (const auto& id_page_pair : offline_pages_) {

[fgorski, 2016/06/08 20:27:07]

nit: you don't need {} for one-liner, and in our team we agreed to not use it,
even thou it is optional.

[romax, 2016/06/08 22:18:59]

Done.

+    unused_archives.erase(id_page_pair.second.file_path);
   }
 
-  // No offline pages missing archive files, we can bail out.
-  if (ids_of_pages_missing_archive_file.empty())
+  if (unused_archives.empty())
     return;

[fgorski, 2016/06/08 20:27:07]

nit: extra line would be useful for readability.

[romax, 2016/06/08 22:18:59]

Done.

-
-  DeletePageCallback remove_pages_done_callback(base::Bind(
-      &OfflinePageModelImpl::OnRemoveOfflinePagesMissingArchiveFileDone,
-      weak_ptr_factory_.GetWeakPtr(), offline_client_id_pairs));
-
-  store_->RemoveOfflinePages(
-      ids_of_pages_missing_archive_file,
-      base::Bind(&OfflinePageModelImpl::OnRemoveOfflinePagesDone,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 ids_of_pages_missing_archive_file,
-                 remove_pages_done_callback));
+  std::vector<base::FilePath> headless_archives(unused_archives.begin(),
+                                                unused_archives.end());
+  archive_manager_->DeleteMultipleArchives(

[fgorski, 2016/06/08 20:27:07]

I had one more thought about this. What if an archive is being created as we are
running metadata consistency check and there is no entry for it yet. Can we only
proceed with deleting the file, if it is older than 10 second or something like
that?  Chances we hit it are slim, but still. Would be great to at least think
through that scenario. (This would have to be done inside the delete method, as
you need to stat the file. Perhaps an extra parameter is in order, but you work
it out)

[jianli, 2016/06/08 21:10:35]

I agree with Filip that we need to handle this case. We probably don't need to
delete a file that was created a couple hours ago. The 10 second is not enough,
considering potential DST change. We could do this in another patch.

[romax, 2016/06/08 22:18:59]

This seems not a trivial issue. Will have another patch for this.

+      headless_archives,
+      base::Bind(&OfflinePageModelImpl::OnDeleteHeadlessArchivesDone,
+                 weak_ptr_factory_.GetWeakPtr(), headless_archives));
 }
 
-void OfflinePageModelImpl::OnRemoveOfflinePagesMissingArchiveFileDone(
-    const std::vector<std::pair<int64_t, ClientId>>& offline_client_id_pairs,
-    DeletePageResult /* result */) {
-  for (const auto& id_pair : offline_client_id_pairs) {
-    FOR_EACH_OBSERVER(Observer, observers_,
-                      OfflinePageDeleted(id_pair.first, id_pair.second));
-  }
+void OfflinePageModelImpl::OnDeleteHeadlessArchivesDone(
+    const std::vector<base::FilePath>& archives,
+    bool success) {
+  UMA_HISTOGRAM_COUNTS("OfflinePages.Consistency.HeadlessArchivesCount",
+                       static_cast<int32_t>(archives.size()));
+  UMA_HISTOGRAM_BOOLEAN("OfflinePages.Consistency.DeleteHeadlessArchivesResult",
+                        success);
 }
 
 void OfflinePageModelImpl::OnRemoveAllFilesDoneForClearAll(
     const base::Closure& callback,
     DeletePageResult result) {
   store_->Reset(base::Bind(&OfflinePageModelImpl::OnResetStoreDoneForClearAll,
                            weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void OfflinePageModelImpl::OnResetStoreDoneForClearAll(
@@ skipping 52 matching lines @@
 void OfflinePageModelImpl::RunWhenLoaded(const base::Closure& task) {
   if (!is_loaded_) {
     delayed_tasks_.push_back(task);
     return;
   }
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, task);
 }
 
 }  // namespace offline_pages