LeveldbValueStore: Deleting db when open fails due to corruption.

extensions/browser/value_store/leveldb_value_store.cc

Index: extensions/browser/value_store/leveldb_value_store.cc
diff --git a/extensions/browser/value_store/leveldb_value_store.cc b/extensions/browser/value_store/leveldb_value_store.cc
index c11e6551b280a99cbc983f1c3adec6d4670dbc01..4b38b7e0960b9baf2bb6da40a2ad55c1309c71ba 100644
--- a/extensions/browser/value_store/leveldb_value_store.cc
+++ b/extensions/browser/value_store/leveldb_value_store.cc
@@ -29,6 +29,16 @@ namespace {
 const char kInvalidJson[] = "Invalid JSON";
 const char kCannotSerialize[] = "Cannot serialize value to JSON";
 
+// UMA values used when recovering from a corrupted leveldb.
+// Do not change/delete these values as you will break reporting for older
+// copies of Chrome. Only add new values to the end.
+enum LevelDBCorruptionRecoveryValue {
+  LEVELDB_RESTORE_DELETE_SUCCESS = 0,
+  LEVELDB_RESTORE_DELETE_FAILURE,
+  LEVELDB_RESTORE_REPAIR_SUCCESS,
+  LEVELDB_RESTORE_MAX
+};
+
 // Scoped leveldb snapshot which releases the snapshot on destruction.
 class ScopedSnapshot {
  public:
@@ -54,7 +64,7 @@ class ScopedSnapshot {
 
 LeveldbValueStore::LeveldbValueStore(const std::string& uma_client_name,
                                      const base::FilePath& db_path)
-    : db_path_(db_path), open_histogram_(nullptr) {
+    : db_path_(db_path), open_histogram_(nullptr), restore_histogram_(nullptr) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
   // Used in lieu of UMA_HISTOGRAM_ENUMERATION because the histogram name is
@@ -63,6 +73,9 @@ LeveldbValueStore::LeveldbValueStore(const std::string& uma_client_name,
       "Extensions.Database.Open." + uma_client_name, 1,
       leveldb_env::LEVELDB_STATUS_MAX, leveldb_env::LEVELDB_STATUS_MAX + 1,
       base::Histogram::kUmaTargetedHistogramFlag);
+  restore_histogram_ = base::LinearHistogram::FactoryGet(

[Devlin, 2015/11/06 19:54:46]

This is histogram black magic to me, so I'll trust the histogram owner to verify
it. :)

[cmumford, 2015/11/09 23:04:21]

I manually hack-ed in a call to
"restore_histogram_->Add(LEVELDB_RESTORE_REPAIR_SUCCESS);" and verified it was
in about:histograms.

+      "Extensions.Database.Restore." + uma_client_name, 1, LEVELDB_RESTORE_MAX,
+      LEVELDB_RESTORE_MAX + 1, base::Histogram::kUmaTargetedHistogramFlag);
   base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
       this, "LeveldbValueStore", base::ThreadTaskRunnerHandle::Get());
 }
@@ -287,28 +300,26 @@ ValueStore::WriteResult LeveldbValueStore::Clear() {
 bool LeveldbValueStore::Restore() {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
-  ReadResult result = Get();
-  std::string previous_key;
-  while (result->IsCorrupted()) {
-    // If we don't have a specific corrupted key, or we've tried and failed to
-    // clear this specific key, or we fail to restore the key, then wipe the
-    // whole database.
-    if (!result->error().key.get() || *result->error().key == previous_key ||
-        !RestoreKey(*result->error().key)) {
-      DeleteDbFile();
-      result = Get();
-      break;
-    }
+  // Possible to have a corrupted open database, so first close it.
+  db_.reset();
+
+  leveldb::Options options;
+  options.create_if_missing = true;
+
+  // Repair can drop an unbounded number of leveldb tables (key/value sets)
+  leveldb::Status status = leveldb::RepairDB(db_path_.AsUTF8Unsafe(), options);
+  if (status.ok()) {
+    restore_histogram_->Add(LEVELDB_RESTORE_REPAIR_SUCCESS);
+    return true;
+  }
 
-    // Otherwise, re-Get() the database to check if there is still any
-    // corruption.
-    previous_key = *result->error().key;
-    result = Get();
+  if (DeleteDbFile()) {
+    restore_histogram_->Add(LEVELDB_RESTORE_DELETE_SUCCESS);
+    return true;
   }
 
-  // If we still have an error, it means we've tried deleting the database file,
-  // and failed. There's nothing more we can do.
-  return !result->IsCorrupted();
+  restore_histogram_->Add(LEVELDB_RESTORE_DELETE_FAILURE);
+  return false;
 }
 
 bool LeveldbValueStore::RestoreKey(const std::string& key) {
@@ -381,6 +392,12 @@ scoped_ptr<ValueStore::Error> LeveldbValueStore::EnsureDbIsOpen() {
       leveldb::DB::Open(options, db_path_.AsUTF8Unsafe(), &db);
   if (open_histogram_)
     open_histogram_->Add(leveldb_env::GetLevelDBStatusUMAValue(status));
+  if (status.IsCorruption()) {
+    // Returning a corruption error should result in Restore() being called.
+    // However, since once a leveldb becomes corrupt it's unusable without
+    // some kind of repair or delete, so do that right now.
+    Restore();
+  }
   if (!status.ok())
     return ToValueStoreError(status, util::NoKey());
 
@@ -468,12 +485,14 @@ bool LeveldbValueStore::IsEmpty() {
   return is_empty;
 }
 
-void LeveldbValueStore::DeleteDbFile() {
+bool LeveldbValueStore::DeleteDbFile() {
   db_.reset();  // release any lock on the directory
   if (!base::DeleteFile(db_path_, true /* recursive */)) {
     LOG(WARNING) << "Failed to delete LeveldbValueStore database at " <<
         db_path_.value();
+    return false;
   }
+  return true;
 }
 
 scoped_ptr<ValueStore::Error> LeveldbValueStore::ToValueStoreError(