diff options
Diffstat (limited to 'extensions/browser/value_store')
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_ |