ValueStore will now automatically repair corrupted db's.

Previously the owner of the ValueStore was responsible for handling
corruption errors, and calling RestoreKey() or Restore() accordingly.
This change shifts that policy decision to ValueStore. ValueStore::Status
has been enhanced to report the database restore status for the
caller to deal as deemed appropriate.

Review URL: https://codereview.chromium.org/1463463004

Cr-Commit-Position: refs/heads/master@{#364848}

7 files changed, 292 insertions, 200 deletions

diff --git a/extensions/browser/value_store/leveldb_value_store.cc b/extensions/browser/value_store/leveldb_value_store.cc
index 29e9c33..1c42ca5 100644
--- a/extensions/browser/value_store/leveldb_value_store.cc
+++ b/extensions/browser/value_store/leveldb_value_store.cc
@@ -26,6 +26,7 @@ namespace {
 
 const char kInvalidJson[] = "Invalid JSON";
 const char kCannotSerialize[] = "Cannot serialize value to JSON";
+const char kRestoredDuringOpen[] = "Database corruption repaired during open";
 
 // UMA values used when recovering from a corrupted leveldb.
 // Do not change/delete these values as you will break reporting for older
@@ -58,13 +59,30 @@ class ScopedSnapshot {
   DISALLOW_COPY_AND_ASSIGN(ScopedSnapshot);
 };
 
+ValueStore::StatusCode LevelDbToValueStoreStatus(
+    const leveldb::Status& status) {
+  if (status.ok())
+    return ValueStore::OK;
+  if (status.IsCorruption())
+    return ValueStore::CORRUPTION;
+  return ValueStore::OTHER_ERROR;
+}
+
 }  // namespace
 
 LeveldbValueStore::LeveldbValueStore(const std::string& uma_client_name,
                                      const base::FilePath& db_path)
-    : db_path_(db_path), open_histogram_(nullptr), restore_histogram_(nullptr) {
+    : db_path_(db_path),
+      db_unrecoverable_(false),
+      open_histogram_(nullptr),
+      restore_histogram_(nullptr) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
+  open_options_.max_open_files = 0;  // Use minimum.
+  open_options_.create_if_missing = true;
+  open_options_.paranoid_checks = true;
+  open_options_.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue;
+
   // Used in lieu of UMA_HISTOGRAM_ENUMERATION because the histogram name is
   // not a constant.
   open_histogram_ = base::LinearHistogram::FactoryGet(
@@ -112,28 +130,28 @@ size_t LeveldbValueStore::GetBytesInUse() {
 ValueStore::ReadResult LeveldbValueStore::Get(const std::string& key) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
-  Status open_error = EnsureDbIsOpen();
-  if (!open_error.ok())
-    return MakeReadResult(open_error);
+  Status status = EnsureDbIsOpen();
+  if (!status.ok())
+    return MakeReadResult(status);
 
   scoped_ptr<base::Value> setting;
-  Status error = ReadFromDb(leveldb::ReadOptions(), key, &setting);
-  if (!error.ok())
-    return MakeReadResult(error);
+  status.Merge(ReadFromDb(leveldb::ReadOptions(), key, &setting));
+  if (!status.ok())
+    return MakeReadResult(status);
 
   base::DictionaryValue* settings = new base::DictionaryValue();
   if (setting)
     settings->SetWithoutPathExpansion(key, setting.release());
-  return MakeReadResult(make_scoped_ptr(settings));
+  return MakeReadResult(make_scoped_ptr(settings), status);
 }
 
 ValueStore::ReadResult LeveldbValueStore::Get(
     const std::vector<std::string>& keys) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
-  Status open_error = EnsureDbIsOpen();
-  if (!open_error.ok())
-    return MakeReadResult(open_error);
+  Status status = EnsureDbIsOpen();
+  if (!status.ok())
+    return MakeReadResult(status);
 
   leveldb::ReadOptions options;
   scoped_ptr<base::DictionaryValue> settings(new base::DictionaryValue());
@@ -145,22 +163,22 @@ ValueStore::ReadResult LeveldbValueStore::Get(
   for (std::vector<std::string>::const_iterator it = keys.begin();
       it != keys.end(); ++it) {
     scoped_ptr<base::Value> setting;
-    Status error = ReadFromDb(options, *it, &setting);
-    if (!error.ok())
-      return MakeReadResult(error);
+    status.Merge(ReadFromDb(options, *it, &setting));
+    if (!status.ok())
+      return MakeReadResult(status);
     if (setting)
       settings->SetWithoutPathExpansion(*it, setting.release());
   }
 
-  return MakeReadResult(settings.Pass());
+  return MakeReadResult(settings.Pass(), status);
 }
 
 ValueStore::ReadResult LeveldbValueStore::Get() {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
-  Status open_error = EnsureDbIsOpen();
-  if (!open_error.ok())
-    return MakeReadResult(open_error);
+  Status status = EnsureDbIsOpen();
+  if (!status.ok())
+    return MakeReadResult(status);
 
   base::JSONReader json_reader;
   leveldb::ReadOptions options = leveldb::ReadOptions();
@@ -175,63 +193,69 @@ ValueStore::ReadResult LeveldbValueStore::Get() {
     std::string key = it->key().ToString();
     scoped_ptr<base::Value> value =
         json_reader.ReadToValue(it->value().ToString());
-    if (!value)
-      return MakeReadResult(Status(CORRUPTION, kInvalidJson));
+    if (!value) {
+      return MakeReadResult(Status(
+          CORRUPTION,
+          Delete(key).ok() ? RESTORE_REPAIR_SUCCESS : RESTORE_DELETE_FAILURE,
+          kInvalidJson));
+    }
     settings->SetWithoutPathExpansion(key, value.Pass());
   }
 
   if (it->status().IsNotFound()) {
     NOTREACHED() << "IsNotFound() but iterating over all keys?!";
-    return MakeReadResult(settings.Pass());
+    return MakeReadResult(settings.Pass(), status);
   }
 
-  if (!it->status().ok())
-    return MakeReadResult(ToValueStoreError(it->status()));
+  if (!it->status().ok()) {
+    status.Merge(ToValueStoreError(it->status()));
+    return MakeReadResult(status);
+  }
 
-  return MakeReadResult(settings.Pass());
+  return MakeReadResult(settings.Pass(), status);
 }
 
 ValueStore::WriteResult LeveldbValueStore::Set(
     WriteOptions options, const std::string& key, const base::Value& value) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
-  Status open_error = EnsureDbIsOpen();
-  if (!open_error.ok())
-    return MakeWriteResult(open_error);
+  Status status = EnsureDbIsOpen();
+  if (!status.ok())
+    return MakeWriteResult(status);
 
   leveldb::WriteBatch batch;
   scoped_ptr<ValueStoreChangeList> changes(new ValueStoreChangeList());
-  Status batch_error = AddToBatch(options, key, value, &batch, changes.get());
-  if (!batch_error.ok())
-    return MakeWriteResult(batch_error);
+  status.Merge(AddToBatch(options, key, value, &batch, changes.get()));
+  if (!status.ok())
+    return MakeWriteResult(status);
 
-  Status write_error = WriteToDb(&batch);
-  return write_error.ok() ? MakeWriteResult(changes.Pass())
-                          : MakeWriteResult(write_error);
+  status.Merge(WriteToDb(&batch));
+  return status.ok() ? MakeWriteResult(changes.Pass(), status)
+                     : MakeWriteResult(status);
 }
 
 ValueStore::WriteResult LeveldbValueStore::Set(
     WriteOptions options, const base::DictionaryValue& settings) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
-  Status open_error = EnsureDbIsOpen();
-  if (!open_error.ok())
-    return MakeWriteResult(open_error);
+  Status status = EnsureDbIsOpen();
+  if (!status.ok())
+    return MakeWriteResult(status);
 
   leveldb::WriteBatch batch;
   scoped_ptr<ValueStoreChangeList> changes(new ValueStoreChangeList());
 
   for (base::DictionaryValue::Iterator it(settings);
        !it.IsAtEnd(); it.Advance()) {
-    Status batch_error =
-        AddToBatch(options, it.key(), it.value(), &batch, changes.get());
-    if (!batch_error.ok())
-      return MakeWriteResult(batch_error);
+    status.Merge(
+        AddToBatch(options, it.key(), it.value(), &batch, changes.get()));
+    if (!status.ok())
+      return MakeWriteResult(status);
   }
 
-  Status write_error = WriteToDb(&batch);
-  return write_error.ok() ? MakeWriteResult(changes.Pass())
-                          : MakeWriteResult(write_error);
+  status.Merge(WriteToDb(&batch));
+  return status.ok() ? MakeWriteResult(changes.Pass(), status)
+                     : MakeWriteResult(status);
 }
 
 ValueStore::WriteResult LeveldbValueStore::Remove(const std::string& key) {
@@ -243,9 +267,9 @@ ValueStore::WriteResult LeveldbValueStore::Remove(
     const std::vector<std::string>& keys) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
-  Status open_error = EnsureDbIsOpen();
-  if (!open_error.ok())
-    return MakeWriteResult(open_error);
+  Status status = EnsureDbIsOpen();
+  if (!status.ok())
+    return MakeWriteResult(status);
 
   leveldb::WriteBatch batch;
   scoped_ptr<ValueStoreChangeList> changes(new ValueStoreChangeList());
@@ -253,9 +277,9 @@ ValueStore::WriteResult LeveldbValueStore::Remove(
   for (std::vector<std::string>::const_iterator it = keys.begin();
       it != keys.end(); ++it) {
     scoped_ptr<base::Value> old_value;
-    Status read_error = ReadFromDb(leveldb::ReadOptions(), *it, &old_value);
-    if (!read_error.ok())
-      return MakeWriteResult(read_error);
+    status.Merge(ReadFromDb(leveldb::ReadOptions(), *it, &old_value));
+    if (!status.ok())
+      return MakeWriteResult(status);
 
     if (old_value) {
       changes->push_back(ValueStoreChange(*it, old_value.release(), NULL));
@@ -263,10 +287,12 @@ ValueStore::WriteResult LeveldbValueStore::Remove(
     }
   }
 
-  leveldb::Status status = db_->Write(leveldb::WriteOptions(), &batch);
-  if (!status.ok() && !status.IsNotFound())
-    return MakeWriteResult(ToValueStoreError(status));
-  return MakeWriteResult(changes.Pass());
+  leveldb::Status ldb_status = db_->Write(leveldb::WriteOptions(), &batch);
+  if (!ldb_status.ok() && !ldb_status.IsNotFound()) {
+    status.Merge(ToValueStoreError(ldb_status));
+    return MakeWriteResult(status);
+  }
+  return MakeWriteResult(changes.Pass(), status);
 }
 
 ValueStore::WriteResult LeveldbValueStore::Clear() {
@@ -283,56 +309,11 @@ ValueStore::WriteResult LeveldbValueStore::Clear() {
     std::string next_key = base::DictionaryValue::Iterator(whole_db).key();
     scoped_ptr<base::Value> next_value;
     whole_db.RemoveWithoutPathExpansion(next_key, &next_value);
-    changes->push_back(
-        ValueStoreChange(next_key, next_value.release(), NULL));
+    changes->push_back(ValueStoreChange(next_key, next_value.release(), NULL));
   }
 
   DeleteDbFile();
-  return MakeWriteResult(changes.Pass());
-}
-
-bool LeveldbValueStore::Restore() {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-
-  // Possible to have a corrupted open database, so first close it.
-  db_.reset();
-
-  leveldb::Options options;
-  options.create_if_missing = true;
-  options.paranoid_checks = 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;
-  }
-
-  if (DeleteDbFile()) {
-    restore_histogram_->Add(LEVELDB_RESTORE_DELETE_SUCCESS);
-    return true;
-  }
-
-  restore_histogram_->Add(LEVELDB_RESTORE_DELETE_FAILURE);
-  return false;
-}
-
-bool LeveldbValueStore::RestoreKey(const std::string& key) {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-
-  ReadResult result = Get(key);
-  if (result->status().IsCorrupted()) {
-    leveldb::WriteBatch batch;
-    batch.Delete(key);
-    ValueStore::Status error = WriteToDb(&batch);
-    // If we can't delete the key, the restore failed.
-    if (!error.ok())
-      return false;
-    result = Get(key);
-  }
-
-  // The restore succeeded if there is no corruption error.
-  return !result->status().IsCorrupted();
+  return MakeWriteResult(changes.Pass(), read_result->status());
 }
 
 bool LeveldbValueStore::WriteToDbForTest(leveldb::WriteBatch* batch) {
@@ -371,34 +352,120 @@ bool LeveldbValueStore::OnMemoryDump(
   return true;
 }
 
+ValueStore::BackingStoreRestoreStatus LeveldbValueStore::LogRestoreStatus(
+    BackingStoreRestoreStatus restore_status) {
+  switch (restore_status) {
+    case RESTORE_NONE:
+      NOTREACHED();
+      break;
+    case RESTORE_DELETE_SUCCESS:
+      restore_histogram_->Add(LEVELDB_RESTORE_DELETE_SUCCESS);
+      break;
+    case RESTORE_DELETE_FAILURE:
+      restore_histogram_->Add(LEVELDB_RESTORE_DELETE_FAILURE);
+      break;
+    case RESTORE_REPAIR_SUCCESS:
+      restore_histogram_->Add(LEVELDB_RESTORE_REPAIR_SUCCESS);
+      break;
+  }
+  return restore_status;
+}
+
+ValueStore::BackingStoreRestoreStatus LeveldbValueStore::FixCorruption(
+    const std::string* key) {
+  leveldb::Status s;
+  if (key && db_) {
+    s = Delete(*key);
+    // Deleting involves writing to the log, so it's possible to have a
+    // perfectly OK database but still have a delete fail.
+    if (s.ok())
+      return LogRestoreStatus(RESTORE_REPAIR_SUCCESS);
+    else if (s.IsIOError())
+      return LogRestoreStatus(RESTORE_DELETE_FAILURE);
+    // Any other kind of failure triggers a db repair.
+  }
+
+  // Make sure database is closed.
+  db_.reset();
+
+  // First try the less lossy repair.
+  BackingStoreRestoreStatus restore_status = RESTORE_NONE;
+
+  leveldb::Options repair_options;
+  repair_options.create_if_missing = true;
+  repair_options.paranoid_checks = true;
+
+  // RepairDB can drop an unbounded number of leveldb tables (key/value sets).
+  s = leveldb::RepairDB(db_path_.AsUTF8Unsafe(), repair_options);
+
+  leveldb::DB* db = nullptr;
+  if (s.ok()) {
+    restore_status = RESTORE_REPAIR_SUCCESS;
+    s = leveldb::DB::Open(open_options_, db_path_.AsUTF8Unsafe(), &db);
+  }
+
+  if (!s.ok()) {
+    if (DeleteDbFile()) {
+      restore_status = RESTORE_DELETE_SUCCESS;
+      s = leveldb::DB::Open(open_options_, db_path_.AsUTF8Unsafe(), &db);
+    } else {
+      restore_status = RESTORE_DELETE_FAILURE;
+    }
+  }
+
+  if (s.ok())
+    db_.reset(db);
+  else
+    db_unrecoverable_ = true;
+
+  if (s.ok() && key) {
+    s = Delete(*key);
+    if (s.ok()) {
+      restore_status = RESTORE_REPAIR_SUCCESS;
+    } else if (s.IsIOError()) {
+      restore_status = RESTORE_DELETE_FAILURE;
+    } else {
+      db_.reset(db);
+      if (!DeleteDbFile())
+        db_unrecoverable_ = true;
+      restore_status = RESTORE_DELETE_FAILURE;
+    }
+  }
+
+  // Only log for the final and most extreme form of database restoration.
+  LogRestoreStatus(restore_status);
+
+  return restore_status;
+}
+
 ValueStore::Status LeveldbValueStore::EnsureDbIsOpen() {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
   if (db_)
     return Status();
 
-  leveldb::Options options;
-  options.max_open_files = 0;  // Use minimum.
-  options.create_if_missing = true;
-  options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue;
+  if (db_unrecoverable_) {
+    return ValueStore::Status(ValueStore::CORRUPTION,
+                              ValueStore::RESTORE_DELETE_FAILURE,
+                              "Database corrupted");
+  }
 
   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()) {
-    // 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();
+  leveldb::Status ldb_status =
+      leveldb::DB::Open(open_options_, db_path_.AsUTF8Unsafe(), &db);
+  open_histogram_->Add(leveldb_env::GetLevelDBStatusUMAValue(ldb_status));
+  Status status = ToValueStoreError(ldb_status);
+  if (ldb_status.ok()) {
+    db_.reset(db);
+  } else if (ldb_status.IsCorruption()) {
+    status.restore_status = FixCorruption(nullptr);
+    if (status.restore_status != RESTORE_DELETE_FAILURE) {
+      status.code = OK;
+      status.message = kRestoredDuringOpen;
+    }
   }
-  if (!status.ok())
-    return ToValueStoreError(status);
 
-  CHECK(db);
-  db_.reset(db);
-  return Status();
+  return status;
 }
 
 ValueStore::Status LeveldbValueStore::ReadFromDb(
@@ -422,7 +489,7 @@ ValueStore::Status LeveldbValueStore::ReadFromDb(
 
   scoped_ptr<base::Value> value = base::JSONReader().ReadToValue(value_as_json);
   if (!value)
-    return Status(CORRUPTION, kInvalidJson);
+    return Status(CORRUPTION, FixCorruption(&key), kInvalidJson);
 
   *setting = value.Pass();
   return Status();
@@ -438,11 +505,9 @@ ValueStore::Status LeveldbValueStore::AddToBatch(
 
   if (!(options & NO_GENERATE_CHANGES)) {
     scoped_ptr<base::Value> old_value;
-    Status read_error = ReadFromDb(leveldb::ReadOptions(), key, &old_value);
-    if (read_error.IsCorrupted())
-      batch->Delete(key);
-    else if (!read_error.ok())
-      return read_error;
+    Status status = ReadFromDb(leveldb::ReadOptions(), key, &old_value);
+    if (!status.ok())
+      return status;
     if (!old_value || !old_value->Equals(&value)) {
       changes->push_back(
           ValueStoreChange(key, old_value.release(), value.DeepCopy()));
@@ -463,7 +528,7 @@ ValueStore::Status LeveldbValueStore::AddToBatch(
 
 ValueStore::Status LeveldbValueStore::WriteToDb(leveldb::WriteBatch* batch) {
   leveldb::Status status = db_->Write(leveldb::WriteOptions(), batch);
-  return status.ok() ? Status() : ToValueStoreError(status);
+  return ToValueStoreError(status);
 }
 
 bool LeveldbValueStore::IsEmpty() {
@@ -479,6 +544,16 @@ bool LeveldbValueStore::IsEmpty() {
   return is_empty;
 }
 
+leveldb::Status LeveldbValueStore::Delete(const std::string& key) {
+  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  DCHECK(db_.get());
+
+  leveldb::WriteBatch batch;
+  batch.Delete(key);
+
+  return db_->Write(leveldb::WriteOptions(), &batch);
+}
+
 bool LeveldbValueStore::DeleteDbFile() {
   db_.reset();  // release any lock on the directory
   if (!base::DeleteFile(db_path_, true /* recursive */)) {
@@ -491,7 +566,6 @@ bool LeveldbValueStore::DeleteDbFile() {
 
 ValueStore::Status LeveldbValueStore::ToValueStoreError(
     const leveldb::Status& status) {
-  CHECK(!status.ok());
   CHECK(!status.IsNotFound());  // not an error
 
   std::string message = status.ToString();
@@ -500,5 +574,5 @@ ValueStore::Status LeveldbValueStore::ToValueStoreError(
   base::ReplaceSubstringsAfterOffset(
       &message, 0u, db_path_.AsUTF8Unsafe(), "...");
 
-  return Status(CORRUPTION, message);
+  return Status(LevelDbToValueStoreStatus(status), message);
 }
diff --git a/extensions/browser/value_store/leveldb_value_store.h b/extensions/browser/value_store/leveldb_value_store.h
index 81956fb..6b34e8a 100644
--- a/extensions/browser/value_store/leveldb_value_store.h
+++ b/extensions/browser/value_store/leveldb_value_store.h
@@ -51,8 +51,6 @@ class LeveldbValueStore : public ValueStore,
   WriteResult Remove(const std::string& key) override;
   WriteResult Remove(const std::vector<std::string>& keys) override;
   WriteResult Clear() override;
-  bool Restore() override;
-  bool RestoreKey(const std::string& key) override;
 
   // Write directly to the backing levelDB. Only used for testing to cause
   // corruption in the database.
@@ -63,6 +61,15 @@ class LeveldbValueStore : public ValueStore,
                     base::trace_event::ProcessMemoryDump* pmd) override;
 
  private:
+  // Fix the |key| or database. If |key| is not null and the database is open
+  // then the key will be deleted. Otherwise the database will be repaired, and
+  // failing that will be deleted.
+  BackingStoreRestoreStatus FixCorruption(const std::string* key);
+
+  // Log, to UMA, the status of an attempt to restore a database.
+  BackingStoreRestoreStatus LogRestoreStatus(
+      BackingStoreRestoreStatus restore_status);
+
   // Tries to open the database if it hasn't been opened already.
   ValueStore::Status EnsureDbIsOpen();
 
@@ -83,9 +90,12 @@ class LeveldbValueStore : public ValueStore,
   // Commits the changes in |batch| to the database.
   ValueStore::Status WriteToDb(leveldb::WriteBatch* batch);
 
-  // Converts an error leveldb::Status to a ValueStore::Status.
+  // Converts an error leveldb::Status to a ValueStore::Error.
   ValueStore::Status ToValueStoreError(const leveldb::Status& status);
 
+  // Delete a value (identified by |key|) from the value store.
+  leveldb::Status Delete(const std::string& key);
+
   // Removes the on-disk database at |db_path_|. Any file system locks should
   // be released before calling this method.
   bool DeleteDbFile();
@@ -95,9 +105,12 @@ class LeveldbValueStore : public ValueStore,
 
   // The location of the leveldb backend.
   const base::FilePath db_path_;
+  leveldb::Options open_options_;
 
   // leveldb backend.
   scoped_ptr<leveldb::DB> db_;
+  // Database is corrupt - restoration failed.
+  bool db_unrecoverable_;
   base::HistogramBase* open_histogram_;
   base::HistogramBase* restore_histogram_;
 
diff --git a/extensions/browser/value_store/leveldb_value_store_unittest.cc b/extensions/browser/value_store/leveldb_value_store_unittest.cc
index d5a28ac..bf83706 100644
--- a/extensions/browser/value_store/leveldb_value_store_unittest.cc
+++ b/extensions/browser/value_store/leveldb_value_store_unittest.cc
@@ -39,18 +39,20 @@ class LeveldbValueStoreUnitTest : public testing::Test {
  protected:
   void SetUp() override {
     ASSERT_TRUE(database_dir_.CreateUniqueTempDir());
-    OpenStore();
+    CreateStore();
     ASSERT_TRUE(store_->Get()->status().ok());
   }
 
   void TearDown() override {
+    if (!store_)
+      return;
     store_->Clear();
-    store_.reset();
+    CloseStore();
   }
 
   void CloseStore() { store_.reset(); }
 
-  void OpenStore() {
+  void CreateStore() {
     store_.reset(
         new LeveldbValueStore(kDatabaseUMAClientName, database_path()));
   }
@@ -82,15 +84,15 @@ TEST_F(LeveldbValueStoreUnitTest, RestoreKeyTest) {
   batch.Put(kCorruptKey, "[{(.*+\"\'\\");
   ASSERT_TRUE(store()->WriteToDbForTest(&batch));
 
-  // Verify corruption.
+  // Verify corruption (the first Get will return corruption).
   ValueStore::ReadResult result = store()->Get(kCorruptKey);
   ASSERT_FALSE(result->status().ok());
   ASSERT_EQ(ValueStore::CORRUPTION, result->status().code);
 
-  // Restore and verify.
-  ASSERT_TRUE(store()->RestoreKey(kCorruptKey));
+  // Verify restored (was deleted in the first Get).
   result = store()->Get(kCorruptKey);
-  EXPECT_TRUE(result->status().ok());
+  EXPECT_TRUE(result->status().ok()) << "Get result not OK: "
+                                     << result->status().message;
   EXPECT_TRUE(result->settings().empty());
 
   // Verify that the valid pair is still present.
@@ -126,18 +128,19 @@ TEST_F(LeveldbValueStoreUnitTest, RestoreDoesMinimumNecessary) {
   batch.Put(kCorruptKey2, kCorruptValue);
   ASSERT_TRUE(store()->WriteToDbForTest(&batch));
 
-  // Verify that we broke it, and then fix it.
+  // Verify that we broke it and that it was repaired by the value store.
   ValueStore::ReadResult result = store()->Get();
   ASSERT_FALSE(result->status().ok());
   ASSERT_EQ(ValueStore::CORRUPTION, result->status().code);
-
-  ASSERT_TRUE(store()->Restore());
+  ASSERT_EQ(ValueStore::RESTORE_REPAIR_SUCCESS,
+            result->status().restore_status);
 
   // We should still have all valid pairs present in the database.
   std::string value_string;
   for (size_t i = 0; i < kNotCorruptKeysSize; ++i) {
     result = store()->Get(kNotCorruptKeys[i]);
     EXPECT_TRUE(result->status().ok());
+    ASSERT_EQ(ValueStore::RESTORE_NONE, result->status().restore_status);
     EXPECT_TRUE(result->settings().HasKey(kNotCorruptKeys[i]));
     EXPECT_TRUE(
         result->settings().GetString(kNotCorruptKeys[i], &value_string));
@@ -172,19 +175,14 @@ TEST_F(LeveldbValueStoreUnitTest, RestoreFullDatabase) {
   for (base::FilePath file = enumerator.Next(); !file.empty();
        file = enumerator.Next()) {
     // WriteFile() failure is a result of -1.
-    ASSERT_NE(base::WriteFile(file, kLolCats.c_str(), kLolCats.length()),
-              -1);
+    ASSERT_NE(base::WriteFile(file, kLolCats.c_str(), kLolCats.length()), -1);
   }
-  OpenStore();
+  CreateStore();
 
-  // We should definitely have an error.
+  // We couldn't recover anything, but we should be in a sane state again.
   ValueStore::ReadResult result = store()->Get();
-  ASSERT_FALSE(result->status().ok());
-  ASSERT_EQ(ValueStore::CORRUPTION, result->status().code);
-
-  ASSERT_TRUE(store()->Restore());
-  result = store()->Get();
+  ASSERT_EQ(ValueStore::RESTORE_REPAIR_SUCCESS,
+            result->status().restore_status);
   EXPECT_TRUE(result->status().ok());
-  // We couldn't recover anything, but we should be in a sane state again.
   EXPECT_EQ(0u, result->settings().size());
 }
diff --git a/extensions/browser/value_store/testing_value_store.cc b/extensions/browser/value_store/testing_value_store.cc
index 73536f3..500e6e8 100644
--- a/extensions/browser/value_store/testing_value_store.cc
+++ b/extensions/browser/value_store/testing_value_store.cc
@@ -57,14 +57,14 @@ ValueStore::ReadResult TestingValueStore::Get(
       settings->SetWithoutPathExpansion(*it, value->DeepCopy());
     }
   }
-  return MakeReadResult(make_scoped_ptr(settings));
+  return MakeReadResult(make_scoped_ptr(settings), status_);
 }
 
 ValueStore::ReadResult TestingValueStore::Get() {
   read_count_++;
   if (!status_.ok())
     return MakeReadResult(status_);
-  return MakeReadResult(make_scoped_ptr(storage_.DeepCopy()));
+  return MakeReadResult(make_scoped_ptr(storage_.DeepCopy()), status_);
 }
 
 ValueStore::WriteResult TestingValueStore::Set(
@@ -94,7 +94,7 @@ ValueStore::WriteResult TestingValueStore::Set(
       storage_.SetWithoutPathExpansion(it.key(), it.value().DeepCopy());
     }
   }
-  return MakeWriteResult(changes.Pass());
+  return MakeWriteResult(changes.Pass(), status_);
 }
 
 ValueStore::WriteResult TestingValueStore::Remove(const std::string& key) {
@@ -115,7 +115,7 @@ ValueStore::WriteResult TestingValueStore::Remove(
       changes->push_back(ValueStoreChange(*it, old_value.release(), NULL));
     }
   }
-  return MakeWriteResult(changes.Pass());
+  return MakeWriteResult(changes.Pass(), status_);
 }
 
 ValueStore::WriteResult TestingValueStore::Clear() {
@@ -126,11 +126,3 @@ ValueStore::WriteResult TestingValueStore::Clear() {
   }
   return Remove(keys);
 }
-
-bool TestingValueStore::Restore() {
-  return true;
-}
-
-bool TestingValueStore::RestoreKey(const std::string& key) {
-  return true;
-}
diff --git a/extensions/browser/value_store/testing_value_store.h b/extensions/browser/value_store/testing_value_store.h
index 9559649..f439694 100644
--- a/extensions/browser/value_store/testing_value_store.h
+++ b/extensions/browser/value_store/testing_value_store.h
@@ -45,10 +45,6 @@ class TestingValueStore : public ValueStore {
   WriteResult Remove(const std::string& key) override;
   WriteResult Remove(const std::vector<std::string>& keys) override;
   WriteResult Clear() override;
-  // TestingValueStores can't get corrupted (they're all in-memory), so these
-  // just return true.
-  bool Restore() override;
-  bool RestoreKey(const std::string& key) override;
 
  private:
   base::DictionaryValue storage_;
diff --git a/extensions/browser/value_store/value_store.cc b/extensions/browser/value_store/value_store.cc
index 218956c..88ebe2a 100644
--- a/extensions/browser/value_store/value_store.cc
+++ b/extensions/browser/value_store/value_store.cc
@@ -8,17 +8,33 @@
 
 // Implementation of Status.
 
-ValueStore::Status::Status() : code(OK) {}
+ValueStore::Status::Status() : code(OK), restore_status(RESTORE_NONE) {}
 
 ValueStore::Status::Status(StatusCode code, const std::string& message)
-    : code(code), message(message) {}
+    : Status(code, RESTORE_NONE, message) {}
+
+ValueStore::Status::Status(StatusCode code,
+                           BackingStoreRestoreStatus restore_status,
+                           const std::string& message)
+    : code(code), restore_status(restore_status), message(message) {}
 
 ValueStore::Status::~Status() {}
 
+void ValueStore::Status::Merge(const Status& status) {
+  if (code == OK)
+    code = status.code;
+  if (message.empty() && !status.message.empty())
+    message = status.message;
+  if (restore_status == RESTORE_NONE)
+    restore_status = status.restore_status;
+}
+
 // Implementation of ReadResultType.
 
 ValueStore::ReadResultType::ReadResultType(
-    scoped_ptr<base::DictionaryValue> settings) : settings_(settings.Pass()) {
+    scoped_ptr<base::DictionaryValue> settings,
+    const Status& status)
+    : settings_(settings.Pass()), status_(status) {
   CHECK(settings_);
 }
 
@@ -30,8 +46,9 @@ ValueStore::ReadResultType::~ReadResultType() {}
 // Implementation of WriteResultType.
 
 ValueStore::WriteResultType::WriteResultType(
-    scoped_ptr<ValueStoreChangeList> changes)
-    : changes_(changes.Pass()) {
+    scoped_ptr<ValueStoreChangeList> changes,
+    const Status& status)
+    : changes_(changes.Pass()), status_(status) {
   CHECK(changes_);
 }
 
diff --git a/extensions/browser/value_store/value_store.h b/extensions/browser/value_store/value_store.h
index 9ac652a..ca9d48c 100644
--- a/extensions/browser/value_store/value_store.h
+++ b/extensions/browser/value_store/value_store.h
@@ -39,9 +39,23 @@ class ValueStore {
     OTHER_ERROR,
   };
 
+  enum BackingStoreRestoreStatus {
+    // No restore attempted.
+    RESTORE_NONE,
+    // Corrupted backing store successfully deleted.
+    RESTORE_DELETE_SUCCESS,
+    // Corrupted backing store cannot be deleted.
+    RESTORE_DELETE_FAILURE,
+    // Corrupted backing store successfully repaired.
+    RESTORE_REPAIR_SUCCESS,
+  };
+
   // The status (result) of an operation on a ValueStore.
   struct Status {
     Status();
+    Status(StatusCode code,
+           BackingStoreRestoreStatus restore_status,
+           const std::string& message);
     Status(StatusCode code, const std::string& message);
     ~Status();
 
@@ -49,9 +63,16 @@ class ValueStore {
 
     bool IsCorrupted() const { return code == CORRUPTION; }
 
+    // Merge |status| into this object. Any members (either |code|,
+    // |restore_status|, or |message| in |status| will be used, but only if this
+    // object's members are at their default value.
+    void Merge(const Status& status);
+
     // The status code.
     StatusCode code;
 
+    BackingStoreRestoreStatus restore_status;
+
     // Message associated with the status (error) if there is one.
     std::string message;
   };
@@ -59,7 +80,8 @@ class ValueStore {
   // The result of a read operation (Get).
   class ReadResultType {
    public:
-    explicit ReadResultType(scoped_ptr<base::DictionaryValue> settings);
+    ReadResultType(scoped_ptr<base::DictionaryValue> settings,
+                   const Status& status);
     explicit ReadResultType(const Status& status);
     ~ReadResultType();
 
@@ -86,7 +108,8 @@ class ValueStore {
   // The result of a write operation (Set/Remove/Clear).
   class WriteResultType {
    public:
-    explicit WriteResultType(scoped_ptr<ValueStoreChangeList> changes);
+    WriteResultType(scoped_ptr<ValueStoreChangeList> changes,
+                    const Status& status);
     explicit WriteResultType(const Status& status);
     ~WriteResultType();
 
@@ -122,17 +145,17 @@ class ValueStore {
   virtual ~ValueStore() {}
 
   // Helpers for making a Read/WriteResult.
-  template<typename T>
-  static ReadResult MakeReadResult(scoped_ptr<T> arg) {
-    return ReadResult(new ReadResultType(arg.Pass()));
+  template <typename T>
+  static ReadResult MakeReadResult(scoped_ptr<T> arg, const Status& status) {
+    return ReadResult(new ReadResultType(arg.Pass(), status));
   }
   static ReadResult MakeReadResult(const Status& status) {
     return ReadResult(new ReadResultType(status));
   }
 
-  template<typename T>
-  static WriteResult MakeWriteResult(scoped_ptr<T> arg) {
-    return WriteResult(new WriteResultType(arg.Pass()));
+  template <typename T>
+  static WriteResult MakeWriteResult(scoped_ptr<T> arg, const Status& status) {
+    return WriteResult(new WriteResultType(arg.Pass(), status));
   }
   static WriteResult MakeWriteResult(const Status& status) {
     return WriteResult(new WriteResultType(status));
@@ -177,27 +200,6 @@ class ValueStore {
 
   // Clears the storage.
   virtual WriteResult Clear() = 0;
-
-  // In the event of corruption, the ValueStore should be able to restore
-  // itself. This means deleting local corrupted files. If only a few keys are
-  // corrupted, then some of the database may be saved. If the full database is
-  // corrupted, this will erase it in its entirety.
-  // Returns true on success, false on failure. The only way this will fail is
-  // if we also cannot delete the database file.
-  // Note: This method may be expensive; some implementations may need to read
-  // the entire database to restore. Use sparingly.
-  // Note: This method (and the following RestoreKey()) are rude, and do not
-  // make any logs, track changes, or other generally polite things. Please do
-  // not use these as substitutes for Clear() and Remove().
-  virtual bool Restore() = 0;
-
-  // Similar to Restore(), but for only a particular key. If the key is corrupt,
-  // this will forcefully remove the key. It does not look at the database on
-  // the whole, which makes it faster, but does not guarantee there is no
-  // additional corruption.
-  // Returns true on success, and false on failure. If false, the next step is
-  // probably to Restore() the whole database.
-  virtual bool RestoreKey(const std::string& key) = 0;
 };
 
 #endif  // EXTENSIONS_BROWSER_VALUE_STORE_VALUE_STORE_H_