From b3e2c306a1adcb117f4134281dfcf0cb6ac2467f Mon Sep 17 00:00:00 2001 From: cmumford Date: Mon, 9 Nov 2015 16:54:39 -0800 Subject: LeveldbValueStore: Deleting db when open fails due to corruption. Corrupted databases would never be deleted. This change will delete the leveldb database iff an open attempt fails as a result of corruption. This class is currently used by the Extensions component in Chrome. BUG=549177 Review URL: https://codereview.chromium.org/1428843002 Cr-Commit-Position: refs/heads/master@{#358722} --- .../browser/value_store/leveldb_value_store.cc | 61 ++++++++++++++-------- .../browser/value_store/leveldb_value_store.h | 3 +- 2 files changed, 42 insertions(+), 22 deletions(-) (limited to 'extensions/browser/value_store') diff --git a/extensions/browser/value_store/leveldb_value_store.cc b/extensions/browser/value_store/leveldb_value_store.cc index c11e655..4b38b7e 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( + "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 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 LeveldbValueStore::ToValueStoreError( diff --git a/extensions/browser/value_store/leveldb_value_store.h b/extensions/browser/value_store/leveldb_value_store.h index e763714..d486283 100644 --- a/extensions/browser/value_store/leveldb_value_store.h +++ b/extensions/browser/value_store/leveldb_value_store.h @@ -92,7 +92,7 @@ class LeveldbValueStore : public ValueStore, // Removes the on-disk database at |db_path_|. Any file system locks should // be released before calling this method. - void DeleteDbFile(); + bool DeleteDbFile(); // Returns whether the database is empty. bool IsEmpty(); @@ -103,6 +103,7 @@ class LeveldbValueStore : public ValueStore, // leveldb backend. scoped_ptr db_; base::HistogramBase* open_histogram_; + base::HistogramBase* restore_histogram_; DISALLOW_COPY_AND_ASSIGN(LeveldbValueStore); }; -- cgit v1.1