LeveldbValueStore: Deleting db when open fails due to corruption.

extensions/browser/value_store/leveldb_value_store.cc

 // Copyright 2014 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 "extensions/browser/value_store/leveldb_value_store.h"
 
 #include "base/files/file_util.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/sys_string_conversions.h"
 #include "content/public/browser/browser_thread.h"
 #include "extensions/browser/value_store/value_store_util.h"
 #include "third_party/leveldatabase/env_chromium.h"
 #include "third_party/leveldatabase/src/include/leveldb/iterator.h"
 #include "third_party/leveldatabase/src/include/leveldb/write_batch.h"
 
 namespace util = value_store_util;
 using content::BrowserThread;
 
 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_CORRUPTION_RECOVERY_DELETE_SUCCESS = 0,
+  LEVELDB_CORRUPTION_RECOVERY_DELETE_FAILURE,
+  LEVELDB_CORRUPTION_RECOVERY_MAX
+};
+
 // Scoped leveldb snapshot which releases the snapshot on destruction.
 class ScopedSnapshot {
  public:
   explicit ScopedSnapshot(leveldb::DB* db)
       : db_(db), snapshot_(db->GetSnapshot()) {}
 
   ~ScopedSnapshot() {
     db_->ReleaseSnapshot(snapshot_);
   }
 
   const leveldb::Snapshot* get() {
     return snapshot_;
   }
 
  private:
   leveldb::DB* db_;
   const leveldb::Snapshot* snapshot_;
 
   DISALLOW_COPY_AND_ASSIGN(ScopedSnapshot);
 };
 
 }  // namespace
 
 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),
+      corruption_recovery_histogram_(nullptr) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
   // Used in lieu of UMA_HISTOGRAM_ENUMERATION because the histogram name is
   // not a constant.
   open_histogram_ = base::LinearHistogram::FactoryGet(
       "Extensions.Database.Open." + uma_client_name, 1,
       leveldb_env::LEVELDB_STATUS_MAX, leveldb_env::LEVELDB_STATUS_MAX + 1,
       base::Histogram::kUmaTargetedHistogramFlag);
+  corruption_recovery_histogram_ = base::LinearHistogram::FactoryGet(
+      "Extensions.Database.CorruptionRecovery." + uma_client_name, 1,
+      LEVELDB_CORRUPTION_RECOVERY_MAX, LEVELDB_CORRUPTION_RECOVERY_MAX + 1,
+      base::Histogram::kUmaTargetedHistogramFlag);
 }
 
 LeveldbValueStore::~LeveldbValueStore() {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
   // Delete the database from disk if it's empty (but only if we managed to
   // open it!). This is safe on destruction, assuming that we have exclusive
   // access to the database.
   if (db_ && IsEmpty())
     DeleteDbFile();
@@ skipping 262 matching lines @@
   leveldb::Options options;
   options.max_open_files = 0;  // Use minimum.
   options.create_if_missing = true;
   options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue;
 
   leveldb::DB* db = NULL;
   leveldb::Status status =
       leveldb::DB::Open(options, db_path_.AsUTF8Unsafe(), &db);
   if (open_histogram_)
     open_histogram_->Add(leveldb_env::GetLevelDBStatusUMAValue(status));
+  if (status.IsCorruption()) {
+    // We do not currently recover from corrupted databases, so completely

[Devlin, 2015/10/29 17:57:26]

We do try to recover.  See ValueStore::Restore().  This sounds like it could
blow away the whole database when maybe it could be saved?

[cmumford, 2015/10/29 22:34:22]

Yes, it appears as though ValueStoreFrontend::Backend::Get (which is the first
caller to ValueStore::Get() logs the failure, but (unless I'm mistaken) does not
pass that error back to the callback, so it get's dropped on the floor, and
ValueStore::Restore() is never called.

So my worry is that there's a lot of plumbing in between the actual error, and
the function that handles it, with plenty of opportunity for a mistake.

I think we should still keep the existing functions as-is, because corruption
can happen post open, but whenever it does happen we have three options:

  1) Blow it away (either delete or leveldb::DestroyDB).
  2) leveldb::RepairDB (which could still fail).
  3) Do nothing - but it *will* fail on next open attempt

So my vote would be to:

  1) keep ValueStore::Restore() as-is, but to delete (possibly repair) the
database upon open when corruption is detected.
  2) Modify ValueStoreFrontend::Backend::Get to pass back the
ValueStore::ReadResult to the callback (or have a second callback).

Does this sound reasonable. I'm still new to this code so am likely overlooking
something...

P.S. I'll respond to the question of RepairDB() on the bug.

[Devlin, 2015/10/30 20:13:17]

We try to restore when a call fails - this is in storage_api.cc.  E.g., we'll
try to perform a Get() (StorageStorageAreaGetFunction::RunWithStorage), and
then, if that fails, we'll try to handle the error by restore a particular key
or the entire database (ExtensionFunction::ResponseValue
SettingsFunction::HandleError), and then re-run.

Of course, "Restore" is really just "Delete carefully", starting with the single
key and advancing to the whole DB.

+    // delete it to avoid future open failures.
+    if (base::DeleteFile(db_path_, true /* recursive */))
+      corruption_recovery_histogram_->Add(
+          LEVELDB_CORRUPTION_RECOVERY_DELETE_SUCCESS);
+    else
+      corruption_recovery_histogram_->Add(
+          LEVELDB_CORRUPTION_RECOVERY_DELETE_FAILURE);
+  }
   if (!status.ok())
     return ToValueStoreError(status, util::NoKey());
 
   CHECK(db);
   db_.reset(db);
   return util::NoError();
 }
 
 scoped_ptr<ValueStore::Error> LeveldbValueStore::ReadFromDb(
     leveldb::ReadOptions options,
@@ skipping 89 matching lines @@
   CHECK(!status.IsNotFound());  // not an error
 
   std::string message = status.ToString();
   // The message may contain |db_path_|, which may be considered sensitive
   // data, and those strings are passed to the extension, so strip it out.
   base::ReplaceSubstringsAfterOffset(
       &message, 0u, db_path_.AsUTF8Unsafe(), "...");
 
   return Error::Create(CORRUPTION, message, key.Pass());
 }