[Offline pages] Resetting offline page metadata store to handle LOAD/INIT failures

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 629 matching lines @@
     if (online_url_without_fragment ==
         id_page_pair.second.url.ReplaceComponents(remove_params)) {
       result.push_back(id_page_pair.second);
     }
   }
 
   callback.Run(result);
 }
 
 void OfflinePageModelImpl::CheckMetadataConsistency() {
-  DCHECK(is_loaded_);
   archive_manager_->GetAllArchives(
       base::Bind(&OfflinePageModelImpl::CheckMetadataConsistencyForArchivePaths,
                  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;
@@ skipping 124 matching lines @@
   } else if (status == ItemActionStatus::ALREADY_EXISTS) {
     result = SavePageResult::ALREADY_EXISTS;
   } else {
     result = SavePageResult::STORE_FAILURE;
   }
   InformSavePageDone(callback, result, offline_page.client_id,
                      offline_page.offline_id);
   if (result == SavePageResult::SUCCESS) {
     DeleteExistingPagesWithSameURL(offline_page);
   } else {
-    PostClearStorageIfNeededTask();
+    PostClearStorageIfNeededTask(false /* delayed */);
   }
 
   DeletePendingArchiver(archiver);
   for (Observer& observer : observers_)
     observer.OfflinePageModelChanged(this);
 }
 
 void OfflinePageModelImpl::OnMarkPageAccesseDone(
     const OfflinePageItem& offline_page_item,
     std::unique_ptr<OfflinePagesUpdateResult> result) {
   // Update the item in the cache only upon success.
   if (result->updated_items.size() > 0)
     offline_pages_[offline_page_item.offline_id] = offline_page_item;
 
   // No need to fire OfflinePageModelChanged event since updating access info
   // should not have any impact to the UI.
 }
 
 void OfflinePageModelImpl::OnEnsureArchivesDirCreatedDone(
     const base::TimeTicks& start_time) {
   UMA_HISTOGRAM_TIMES("OfflinePages.Model.ArchiveDirCreationTime",
                       base::TimeTicks::Now() - start_time);
 
-  store_->GetOfflinePages(base::Bind(&OfflinePageModelImpl::OnLoadDone,
-                                     weak_ptr_factory_.GetWeakPtr(),
-                                     start_time));
+  int kResetAttemptsLeft = 1;

[jianli, 2016/11/16 01:34:50]

nit: add const

[fgorski, 2016/11/16 19:10:13]

Done.

+  store_->Initialize(base::Bind(&OfflinePageModelImpl::OnStoreInitialized,
+                                weak_ptr_factory_.GetWeakPtr(), start_time,
+                                kResetAttemptsLeft));
+}
+
+void OfflinePageModelImpl::OnStoreInitialized(const base::TimeTicks& start_time,
+                                              int reset_attempts_left,
+                                              bool success) {
+  if (success) {
+    DCHECK_EQ(store_->state(), StoreState::LOADED);
+    store_->GetOfflinePages(base::Bind(&OfflinePageModelImpl::OnLoadDone,
+                                       weak_ptr_factory_.GetWeakPtr(),
+                                       start_time));
+    return;
+  }
+
+  DCHECK_EQ(store_->state(), StoreState::FAILED_LOADING);
+  // If there are no more reset attempts left, stop here.
+  if (reset_attempts_left == 0) {
+    CompleteLoad();
+    return;
+  }
+
+  // Otherwise reduce the remaining attempts counter and reset store.
+  store_->Reset(base::Bind(&OfflinePageModelImpl::OnStoreResetDone,
+                           weak_ptr_factory_.GetWeakPtr(), start_time,
+                           reset_attempts_left - 1));
+}
+
+void OfflinePageModelImpl::OnStoreResetDone(const base::TimeTicks& start_time,
+                                            int reset_attempts_left,
+                                            bool success) {
+  if (success) {
+    DCHECK_EQ(store_->state(), StoreState::NOT_LOADED);
+    store_->Initialize(base::Bind(&OfflinePageModelImpl::OnStoreInitialized,
+                                  weak_ptr_factory_.GetWeakPtr(), start_time,
+                                  reset_attempts_left));
+    return;
+  }
+
+  DCHECK_EQ(store_->state(), StoreState::FAILED_RESET);
+  CompleteLoad();
 }
 
 void OfflinePageModelImpl::OnLoadDone(
     const base::TimeTicks& start_time,
-    OfflinePageMetadataStore::LoadStatus load_status,
     const std::vector<OfflinePageItem>& offline_pages) {
   DCHECK(!is_loaded_);
-  is_loaded_ = true;
-
-  // TODO(jianli): rebuild the store upon failure.
-
-  if (load_status == OfflinePageMetadataStore::LOAD_SUCCEEDED)
-    CacheLoadedData(offline_pages);
 
   UMA_HISTOGRAM_TIMES("OfflinePages.Model.ConstructionToLoadedEventTime",
                       base::TimeTicks::Now() - start_time);
 
-  // Create Storage Manager.
-  storage_manager_.reset(new OfflinePageStorageManager(
-      this, GetPolicyController(), archive_manager_.get()));
+  CacheLoadedData(offline_pages);
+  CompleteLoad();
+
+  // Ensure necessary cleanup operations are started.
+  CheckMetadataConsistency();
+}
+
+void OfflinePageModelImpl::CompleteLoad() {

[jianli, 2016/11/16 01:34:50]

It is a bit hard to understand the difference from the names OnLoadDone and
CompleteLoad.

Since CompleteLoad applies to both success and failure case, the name
CompleteLoad seems to only suggest the successful load (also given that no
success/result parameter is passed). How about renaming it to something like
FinalizeAfterLoadAttempt?

[fgorski, 2016/11/16 19:10:13]

Done.

Renamed:
OnLoadDone -> OnInitialGetOfflinePagesDone
CompleteLoad -> FinalizeModelLoad

+  is_loaded_ = true;
+
+  // All actions below are meant to be taken regardless of successful load of
+  // the store.
+
+  // Inform observers the load is done.
+  for (Observer& observer : observers_)
+    observer.OfflinePageModelLoaded(this);
 
   // Run all the delayed tasks.

[jianli, 2016/11/16 01:34:50]

Also mention that the delay task will check for the load state and return
earlier.

[fgorski, 2016/11/16 19:10:13]

I am not sure what you are referring to. Nothing about this code changed so I'd
preferably leave it the way it was.

   for (const auto& delayed_task : delayed_tasks_)
     delayed_task.Run();
   delayed_tasks_.clear();
 
-  for (Observer& observer : observers_)
-    observer.OfflinePageModelLoaded(this);
-
-  CheckMetadataConsistency();
-
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE, base::Bind(&OfflinePageModelImpl::ClearStorageIfNeeded,
-                            weak_ptr_factory_.GetWeakPtr(),
-                            base::Bind(&OfflinePageModelImpl::OnStorageCleared,
-                                       weak_ptr_factory_.GetWeakPtr())),
-      kStorageManagerStartingDelay);
+  // Clear storage.

[jianli, 2016/11/16 01:34:50]

Is it still needed when failed to load?

[fgorski, 2016/11/16 19:10:13]

Yes, if I fail to reset the store I would like to reclaim storage.
Since we are already in situation where old DB cannot be recovered, that would
happen anyway with the successful reset. This is actually calling that code
earlier. (Perhaps freeing some space for the user).

+  PostClearStorageIfNeededTask(true /* delayed */);
 }
 
 void OfflinePageModelImpl::InformSavePageDone(const SavePageCallback& callback,
                                               SavePageResult result,
                                               const ClientId& client_id,
                                               int64_t offline_id) {
   ReportSavePageResultHistogramAfterSave(client_id, result);
   archive_manager_->GetStorageStats(
       base::Bind(&ReportStorageHistogramsAfterSave));
   callback.Run(result, offline_id);
@@ skipping 38 matching lines @@
     page_ids_to_delete.push_back(item.offline_id);
   DeletePagesByOfflineId(
       page_ids_to_delete,
       base::Bind(&OfflinePageModelImpl::OnDeleteOldPagesWithSameURL,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 void OfflinePageModelImpl::OnDeleteOldPagesWithSameURL(
     DeletePageResult result) {
   // TODO(romax) Add UMAs for failure cases.
-  PostClearStorageIfNeededTask();
+  PostClearStorageIfNeededTask(false /* delayed */);
 }
 
 void OfflinePageModelImpl::DeletePendingArchiver(
     OfflinePageArchiver* archiver) {
   pending_archivers_.erase(std::find(pending_archivers_.begin(),
                                      pending_archivers_.end(), archiver));
 }
 
 void OfflinePageModelImpl::OnDeleteArchiveFilesDone(
     const std::vector<int64_t>& offline_ids,
@@ skipping 118 matching lines @@
 
 void OfflinePageModelImpl::CacheLoadedData(
     const std::vector<OfflinePageItem>& offline_pages) {
   offline_pages_.clear();
   for (const auto& offline_page : offline_pages)
     offline_pages_[offline_page.offline_id] = offline_page;
 }
 
 void OfflinePageModelImpl::ClearStorageIfNeeded(
     const ClearStorageCallback& callback) {
+  // Create Storage Manager if necessary.
+  if (!storage_manager_) {
+    storage_manager_.reset(new OfflinePageStorageManager(
+        this, GetPolicyController(), archive_manager_.get()));
+  }
   storage_manager_->ClearPagesIfNeeded(callback);
 }
 
 void OfflinePageModelImpl::OnStorageCleared(size_t expired_page_count,
                                             ClearStorageResult result) {
   UMA_HISTOGRAM_ENUMERATION("OfflinePages.ClearStorageResult",
                             static_cast<int>(result),
                             static_cast<int>(ClearStorageResult::RESULT_COUNT));
   if (expired_page_count > 0) {
     UMA_HISTOGRAM_COUNTS("OfflinePages.ExpirePage.BatchSize",
                          static_cast<int32_t>(expired_page_count));
   }
 }
 
-void OfflinePageModelImpl::PostClearStorageIfNeededTask() {
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
+void OfflinePageModelImpl::PostClearStorageIfNeededTask(bool delayed) {
+  base::TimeDelta delay =
+      delayed ? kStorageManagerStartingDelay : base::TimeDelta();
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(

[jianli, 2016/11/16 01:34:50]

Is is possible we can post multiple clear tasks, esp when we now call
PostClearStorageIfNeeded more frequently?

[fgorski, 2016/11/16 19:10:13]

We don't call anything more frequently. This is covering line 853 from the old
code (scheduling with a delay). And the rest is left intact.

       FROM_HERE, base::Bind(&OfflinePageModelImpl::ClearStorageIfNeeded,
                             weak_ptr_factory_.GetWeakPtr(),
                             base::Bind(&OfflinePageModelImpl::OnStorageCleared,
-                                       weak_ptr_factory_.GetWeakPtr())));
+                                       weak_ptr_factory_.GetWeakPtr())),
+      delay);
 }
 
 bool OfflinePageModelImpl::IsRemovedOnCacheReset(
     const OfflinePageItem& offline_page) const {
   return policy_controller_->IsRemovedOnCacheReset(
       offline_page.client_id.name_space);
 }
 
 void OfflinePageModelImpl::RunWhenLoaded(const base::Closure& task) {
   if (!is_loaded_) {
     delayed_tasks_.push_back(task);
     return;
   }
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, task);
 }
 
 base::Time OfflinePageModelImpl::GetCurrentTime() const {
   return testing_clock_ ? testing_clock_->Now() : base::Time::Now();
 }
 
 }  // namespace offline_pages