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

components/offline_pages/offline_page_model_impl.cc

Index: components/offline_pages/offline_page_model_impl.cc
diff --git a/components/offline_pages/offline_page_model_impl.cc b/components/offline_pages/offline_page_model_impl.cc
index 9df2141e42d0a78c368c778df1e86e674993211d..79405e983c12f0073b4d58bdb9875775492803bf 100644
--- a/components/offline_pages/offline_page_model_impl.cc
+++ b/components/offline_pages/offline_page_model_impl.cc
@@ -647,7 +647,6 @@ void OfflinePageModelImpl::GetPagesByOnlineURLWhenLoadDone(
 }
 
 void OfflinePageModelImpl::CheckMetadataConsistency() {
-  DCHECK(is_loaded_);
   archive_manager_->GetAllArchives(
       base::Bind(&OfflinePageModelImpl::CheckMetadataConsistencyForArchivePaths,
                  weak_ptr_factory_.GetWeakPtr()));
@@ -792,7 +791,7 @@ void OfflinePageModelImpl::OnAddOfflinePageDone(
   if (result == SavePageResult::SUCCESS) {
     DeleteExistingPagesWithSameURL(offline_page);
   } else {
-    PostClearStorageIfNeededTask();
+    PostClearStorageIfNeededTask(false /* delayed */);
   }
 
   DeletePendingArchiver(archiver);
@@ -816,46 +815,83 @@ void OfflinePageModelImpl::OnEnsureArchivesDirCreatedDone(
   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();
 
-  // Run all the delayed tasks.
-  for (const auto& delayed_task : delayed_tasks_)
-    delayed_task.Run();
-  delayed_tasks_.clear();
+  // 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);
 
-  CheckMetadataConsistency();
+  // 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();
 
-  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,
@@ -914,7 +950,7 @@ void OfflinePageModelImpl::OnPagesFoundWithSameURL(
 void OfflinePageModelImpl::OnDeleteOldPagesWithSameURL(
     DeletePageResult result) {
   // TODO(romax) Add UMAs for failure cases.
-  PostClearStorageIfNeededTask();
+  PostClearStorageIfNeededTask(false /* delayed */);
 }
 
 void OfflinePageModelImpl::DeletePendingArchiver(
@@ -1053,6 +1089,11 @@ void OfflinePageModelImpl::CacheLoadedData(
 
 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);
 }
 
@@ -1067,12 +1108,15 @@ void OfflinePageModelImpl::OnStorageCleared(size_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(