Wipe out offline page data on clearing cookie and site data

components/offline_pages/offline_page_metadata_store_impl.cc

 // Copyright 2015 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_impl.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/location.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/thread_task_runner_handle.h"
-#include "components/leveldb_proto/proto_database.h"
+#include "components/leveldb_proto/proto_database_impl.h"
 #include "components/offline_pages/offline_page_item.h"
 #include "components/offline_pages/proto/offline_pages.pb.h"
 #include "third_party/leveldatabase/env_chromium.h"
 #include "third_party/leveldatabase/src/include/leveldb/db.h"
 #include "url/gurl.h"
 
 using leveldb_proto::ProtoDatabase;
 
 namespace {
 // Statistics are logged to UMA with this string as part of histogram name. They
 // can all be found under LevelDB.*.OfflinePageMetadataStore. Changing this
 // needs to synchronize with histograms.xml, AND will also become incompatible
 // with older browsers still reporting the previous values.
 const char kDatabaseUMAClientName[] = "OfflinePageMetadataStore";
 }
 
 namespace offline_pages {
 namespace {
 
-typedef std::vector<OfflinePageEntry> OfflinePageEntryVector;
-
 void OfflinePageItemToEntry(const OfflinePageItem& item,
                             offline_pages::OfflinePageEntry* item_proto) {
   DCHECK(item_proto);
   item_proto->set_url(item.url.spec());
   item_proto->set_bookmark_id(item.bookmark_id);
   item_proto->set_version(item.version);
   std::string path_string;
 #if defined(OS_POSIX)
   path_string = item.file_path.value();
 #elif defined(OS_WIN)
@@ skipping 36 matching lines @@
   }
   if (item_proto.has_access_count()) {
     item->access_count = item_proto.access_count();
   }
   if (item_proto.has_flags()) {
     item->flags = static_cast<OfflinePageItem::Flags>(item_proto.flags());
   }
   return true;
 }
 
-void OnLoadDone(const OfflinePageMetadataStore::LoadCallback& callback,
-                const base::Callback<void()>& failure_callback,
-                bool success,
-                scoped_ptr<OfflinePageEntryVector> entries) {
-  UMA_HISTOGRAM_BOOLEAN("OfflinePages.LoadSuccess", success);
-  if (!success) {
-    DVLOG(1) << "Offline pages database load failed.";
-    failure_callback.Run();
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE, base::Bind(callback, false, std::vector<OfflinePageItem>()));
-    return;
-  }
-
-  std::vector<OfflinePageItem> result;
-  for (OfflinePageEntryVector::iterator it = entries->begin();
-       it != entries->end(); ++it) {
-    OfflinePageItem item;
-    if (OfflinePageItemFromEntry(*it, &item))
-      result.push_back(item);
-    else
-      DVLOG(1) << "Failed to create offline page item from proto.";
-  }
-  UMA_HISTOGRAM_COUNTS("OfflinePages.SavedPageCount", result.size());
-
-  base::MessageLoop::current()->PostTask(FROM_HERE,
-                                         base::Bind(callback, true, result));
-}
-
-void OnUpdateDone(const OfflinePageMetadataStore::UpdateCallback& callback,
-                  const base::Callback<void()>& failure_callback,
-                  bool success) {
-  if (!success) {
-    // TODO(fgorski): Add UMA for this case.
-    DVLOG(1) << "Offline pages database update failed.";
-    failure_callback.Run();
-  }
-
-  base::MessageLoop::current()->PostTask(FROM_HERE,
-                                         base::Bind(callback, success));
-}
-
 }  // namespace
 
 OfflinePageMetadataStoreImpl::OfflinePageMetadataStoreImpl(
-    scoped_ptr<ProtoDatabase<OfflinePageEntry>> database,
+    scoped_refptr<base::SequencedTaskRunner> background_task_runner,
     const base::FilePath& database_dir)
-    : database_(database.Pass()), weak_ptr_factory_(this) {
-  database_->Init(kDatabaseUMAClientName, database_dir,
-                  base::Bind(&OfflinePageMetadataStoreImpl::OnInitDone,
-                             weak_ptr_factory_.GetWeakPtr()));
+    : background_task_runner_(background_task_runner),
+      database_dir_(database_dir),
+      weak_ptr_factory_(this) {
 }
 
 OfflinePageMetadataStoreImpl::~OfflinePageMetadataStoreImpl() {
 }
 
-void OfflinePageMetadataStoreImpl::OnInitDone(bool success) {
-  if (!success) {
-    // TODO(fgorski): Add UMA for this case.
-    DVLOG(1) << "Offline pages database init failed.";
-    ResetDB();
-    return;
-  }
+void OfflinePageMetadataStoreImpl::Load(const LoadCallback& callback) {
+  // First initialize the database.
+  DCHECK(!database_);
+  database_.reset(new leveldb_proto::ProtoDatabaseImpl<OfflinePageEntry>(
+      background_task_runner_));
+  database_->Init(kDatabaseUMAClientName, database_dir_,
+                  base::Bind(&OfflinePageMetadataStoreImpl::LoadContinuation,
+                             weak_ptr_factory_.GetWeakPtr(),
+                             callback));
 }
 
-void OfflinePageMetadataStoreImpl::Load(const LoadCallback& callback) {
-  if (!database_.get()) {
-    // Failing fast here, because DB is not initialized, and there is nothing
-    // that can be done about it.
-    // Callback is invoked through message loop to avoid improper retry and
-    // simplify testing.
-    DVLOG(1) << "Offline pages database not available in Load.";
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE, base::Bind(callback, false, std::vector<OfflinePageItem>()));
+void OfflinePageMetadataStoreImpl::LoadContinuation(
+    const LoadCallback& callback,
+    bool success) {
+  if (!success) {
+    UMA_HISTOGRAM_BOOLEAN("OfflinePages.LoadSuccess", false);

[fgorski, 2015/10/23 20:55:03]

does it make sense to database_.reset() here? to not have it hang in memory?

Otherwise, if we expect the caller to properly clean up by calling Reset, that
should be clearly documented.

[jianli, 2015/10/26 21:42:43]

Done.

+    callback.Run(false, std::vector<OfflinePageItem>());
     return;
   }
 
-  database_->LoadEntries(base::Bind(
-      &OnLoadDone, callback, base::Bind(&OfflinePageMetadataStoreImpl::ResetDB,
-                                        weak_ptr_factory_.GetWeakPtr())));
+  // After initialization, start to load the data.
+  database_->LoadEntries(
+      base::Bind(&OfflinePageMetadataStoreImpl::LoadCompleted,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 callback));
+}
+
+void OfflinePageMetadataStoreImpl::LoadCompleted(
+    const LoadCallback& callback,
+    bool success,
+    scoped_ptr<std::vector<OfflinePageEntry>> entries) {
+  DCHECK(entries);
+
+  if (!success) {
+    DVLOG(1) << "Offline pages database load failed.";
+    UMA_HISTOGRAM_BOOLEAN("OfflinePages.LoadSuccess", false);

[fgorski, 2015/10/23 20:55:03]

It would likely make sense to distinguish this case from the previous one and
hence turn this UMA into an enum pointing to where it failed.

[jianli, 2015/10/26 21:42:43]

Done.

+    callback.Run(false, std::vector<OfflinePageItem>());
+    return;
+  }
+
+  std::vector<OfflinePageItem> result;
+  for (const auto& entry : *entries) {
+    OfflinePageItem item;
+    if (!OfflinePageItemFromEntry(entry, &item)) {

[fgorski, 2015/10/23 20:55:03]

Explain why you are changing this. This sounds too aggressive. This will make
the whole DB load fail if one of the entries is busted.

We could drop that entry instead of failing completely.

[jianli, 2015/10/26 21:42:43]

I don't think it is safe to allow partial load when certain data were corrupted
unless we add checksum to validate each entry. If some bad thing happens, we
should not trust the store data at all and we should rebuild it.

+      DVLOG(1) << "Failed to create offline page item from proto.";
+      UMA_HISTOGRAM_BOOLEAN("OfflinePages.LoadSuccess", false);
+      callback.Run(false, std::vector<OfflinePageItem>());
+      return;
+    }
+    result.push_back(item);
+  }
+  UMA_HISTOGRAM_COUNTS("OfflinePages.SavedPageCount", result.size());
+
+  callback.Run(true, result);
 }
 
 void OfflinePageMetadataStoreImpl::AddOrUpdateOfflinePage(
     const OfflinePageItem& offline_page_item,
     const UpdateCallback& callback) {
   scoped_ptr<ProtoDatabase<OfflinePageEntry>::KeyEntryVector> entries_to_save(
       new ProtoDatabase<OfflinePageEntry>::KeyEntryVector());
   scoped_ptr<std::vector<std::string>> keys_to_remove(
       new std::vector<std::string>());
 
@@ skipping 30 matching lines @@
     // Callback is invoked through message loop to avoid improper retry and
     // simplify testing.
     DVLOG(1) << "Offline pages database not available in UpdateEntries.";
     base::MessageLoop::current()->PostTask(FROM_HERE,
                                            base::Bind(callback, false));
     return;
   }
 
   database_->UpdateEntries(
       entries_to_save.Pass(), keys_to_remove.Pass(),
-      base::Bind(&OnUpdateDone, callback,
-                 base::Bind(&OfflinePageMetadataStoreImpl::ResetDB,
-                            weak_ptr_factory_.GetWeakPtr())));
+      base::Bind(&OfflinePageMetadataStoreImpl::UpdateCompleted,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 callback));
 }
 
-void OfflinePageMetadataStoreImpl::ResetDB() {
+void OfflinePageMetadataStoreImpl::UpdateCompleted(
+    const OfflinePageMetadataStore::UpdateCallback& callback,
+    bool success) {
+  if (!success) {
+    // TODO(fgorski): Add UMA for this case. Consider rebuilding the store.
+    DVLOG(1) << "Offline pages database update failed.";
+  }
+
+  callback.Run(success);
+}
+
+void OfflinePageMetadataStoreImpl::Reset(const ResetCallback& callback) {
+  database_->Destroy(
+      base::Bind(&OfflinePageMetadataStoreImpl::ResetCompleted,
+                weak_ptr_factory_.GetWeakPtr(),

[fgorski, 2015/10/23 20:55:03]

nit: git cl format is your friend.

[jianli, 2015/10/26 21:42:43]

Done.

+                callback));
+}
+
+void OfflinePageMetadataStoreImpl::ResetCompleted(
+    const ResetCallback& callback,
+    bool success) {
   database_.reset();
   weak_ptr_factory_.InvalidateWeakPtrs();
+  callback.Run(success);
 }
 
 }  // namespace offline_pages