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

components/offline_pages/offline_page_metadata_store_sql.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_metadata_store_sql.h"
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
@@ skipping 249 matching lines @@
                       const std::vector<OfflinePageItem>& result) {
   // TODO(fgorski): Switch to SQL specific UMA metrics.
   UMA_HISTOGRAM_ENUMERATION("OfflinePages.LoadStatus", status,
                             OfflinePageMetadataStore::LOAD_STATUS_COUNT);
   if (status == OfflinePageMetadataStore::LOAD_SUCCEEDED) {
     UMA_HISTOGRAM_COUNTS("OfflinePages.SavedPageCount",
                          static_cast<int32_t>(result.size()));
   } else {
     DVLOG(1) << "Offline pages database loading failed: " << status;
   }
-  runner->PostTask(FROM_HERE, base::Bind(callback, status, result));
+  runner->PostTask(FROM_HERE, base::Bind(callback, result));
 }
 
 void OpenConnectionSync(sql::Connection* db,
                         scoped_refptr<base::SingleThreadTaskRunner> runner,
                         const base::FilePath& path,
                         const base::Callback<void(StoreState)>& callback) {
   StoreState state =
       InitDatabase(db, path) ? StoreState::LOADED : StoreState::FAILED_LOADING;
   runner->PostTask(FROM_HERE, base::Bind(callback, state));
 }
@@ skipping 146 matching lines @@
   }
 
   runner->PostTask(FROM_HERE, base::Bind(callback, base::Passed(&result)));
 }
 
 void ResetSync(sql::Connection* db,
                const base::FilePath& db_file_path,
                scoped_refptr<base::SingleThreadTaskRunner> runner,
                const base::Callback<void(StoreState)>& callback) {
   // This method deletes the content of the whole store and reinitializes it.
-  bool success = db->Raze();
-  db->Close();
+  bool success = true;
+  if (db) {
+    success = db->Raze();
+    db->Close();
+  }
+  success = base::DeleteFile(db_file_path, true /*recursive*/) && success;
   StoreState state;
   if (success) {
     state = InitDatabase(db, db_file_path) ? StoreState::LOADED

[jianli, 2016/11/15 21:48:38]

I think we should first get back to main thread upon destroying database. From
there, we should recreate sql::Connection instance, instead of reusing an
existing one, and then do the initialization. I have concern that some fields in
sql::Connection instance are not reset. For example, commit_time_histogram_ and
etc which might lead to incorrect data.

[fgorski, 2016/11/15 23:14:12]

Done.

                                            : StoreState::FAILED_LOADING;
   } else {
     state = StoreState::FAILED_RESET;
   }
   runner->PostTask(FROM_HERE, base::Bind(callback, state));
 }
 
 }  // anonymous namespace
 
 OfflinePageMetadataStoreSQL::OfflinePageMetadataStoreSQL(
     scoped_refptr<base::SequencedTaskRunner> background_task_runner,
     const base::FilePath& path)
     : background_task_runner_(std::move(background_task_runner)),
       db_file_path_(path.AppendASCII("OfflinePages.db")),
       state_(StoreState::NOT_LOADED),
       weak_ptr_factory_(this) {
-  OpenConnection();
 }
 
 OfflinePageMetadataStoreSQL::~OfflinePageMetadataStoreSQL() {
   if (db_.get() &&
       !background_task_runner_->DeleteSoon(FROM_HERE, db_.release())) {
     DLOG(WARNING) << "SQL database will not be deleted.";
   }
 }
 
+void OfflinePageMetadataStoreSQL::Initialize(
+    const InitializeCallback& callback) {
+  DCHECK(!db_);
+  db_.reset(new sql::Connection());
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&OpenConnectionSync, db_.get(),
+                 base::ThreadTaskRunnerHandle::Get(), db_file_path_,
+                 base::Bind(&OfflinePageMetadataStoreSQL::OnOpenConnectionDone,
+                            weak_ptr_factory_.GetWeakPtr(), callback)));
+}
+
 void OfflinePageMetadataStoreSQL::GetOfflinePages(
     const LoadCallback& callback) {
-  if (!CheckDb(base::Bind(
-          callback, STORE_INIT_FAILED, std::vector<OfflinePageItem>()))) {
+  if (!CheckDb(base::Bind(callback, std::vector<OfflinePageItem>()))) {
     return;
   }
 
   background_task_runner_->PostTask(
       FROM_HERE, base::Bind(&GetOfflinePagesSync, db_.get(),
                             base::ThreadTaskRunnerHandle::Get(), callback));
 }
 
 void OfflinePageMetadataStoreSQL::AddOfflinePage(
     const OfflinePageItem& offline_page,
@@ skipping 34 matching lines @@
         base::ThreadTaskRunnerHandle::Get(), state(), offline_ids,
         ItemActionStatus::NOT_FOUND /* will be ignored */, callback);
     return;
   }
 
   background_task_runner_->PostTask(
       FROM_HERE, base::Bind(&RemoveOfflinePagesSync, offline_ids, db_.get(),
                             base::ThreadTaskRunnerHandle::Get(), callback));
 }
 
-void OfflinePageMetadataStoreSQL::Reset(const ResetCallback& callback) {
-  if (!CheckDb(base::Bind(callback, false)))
-    return;
-
+void OfflinePageMetadataStoreSQL::Reset(const InitializeCallback& callback) {
   background_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&ResetSync, db_.get(), db_file_path_,
                  base::ThreadTaskRunnerHandle::Get(),
-                 base::Bind(&OfflinePageMetadataStoreSQL::OnResetDone,
+                 base::Bind(&OfflinePageMetadataStoreSQL::OnOpenConnectionDone,
                             weak_ptr_factory_.GetWeakPtr(), callback)));
 }
 
 StoreState OfflinePageMetadataStoreSQL::state() const {
   return state_;
 }
 
 void OfflinePageMetadataStoreSQL::SetStateForTesting(StoreState state,
                                                      bool reset_db) {
   state_ = state;
   if (reset_db)
     db_.reset(nullptr);
 }
 
-void OfflinePageMetadataStoreSQL::OpenConnection() {
-  DCHECK(!db_);
-  db_.reset(new sql::Connection());
-  background_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&OpenConnectionSync, db_.get(),
-                 base::ThreadTaskRunnerHandle::Get(), db_file_path_,
-                 base::Bind(&OfflinePageMetadataStoreSQL::OnOpenConnectionDone,
-                            weak_ptr_factory_.GetWeakPtr())));
-}
-
-void OfflinePageMetadataStoreSQL::OnOpenConnectionDone(StoreState state) {
+void OfflinePageMetadataStoreSQL::OnOpenConnectionDone(
+    const InitializeCallback& callback,
+    StoreState state) {
   DCHECK(db_.get());
 
   state_ = state;
 
   // Unfortunately we were not able to open DB connection.
   if (state != StoreState::LOADED)
-    db_.reset();
+    db_.reset(nullptr);
 
-  // TODO(fgorski): This might be a place to start store recovery. Alternatively
-  // that can be attempted in the OfflinePageModel.
-}
-
-void OfflinePageMetadataStoreSQL::OnResetDone(const ResetCallback& callback,
-                                              StoreState state) {
-  OnOpenConnectionDone(state);
-  callback.Run(state == StoreState::LOADED);
+  callback.Run(state);
 }
 
 bool OfflinePageMetadataStoreSQL::CheckDb(const base::Closure& callback) {
   if (!db_.get() || state_ != StoreState::LOADED) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, callback);
     return false;
   }
   return true;
 }
 
 }  // namespace offline_pages