diff options
Diffstat (limited to 'extensions/browser')
13 files changed, 398 insertions, 356 deletions
diff --git a/extensions/browser/api/storage/settings_storage_quota_enforcer.cc b/extensions/browser/api/storage/settings_storage_quota_enforcer.cc index 82a0d60..98c299b 100644 --- a/extensions/browser/api/storage/settings_storage_quota_enforcer.cc +++ b/extensions/browser/api/storage/settings_storage_quota_enforcer.cc @@ -43,15 +43,6 @@ void Allocate( (*used_per_setting)[key] = new_size; } -// Frees the allocation of a setting in a record of total and per-setting usage. -void Free( - size_t* used_total, - std::map<std::string, size_t>* used_per_setting, - const std::string& key) { - *used_total -= (*used_per_setting)[key]; - used_per_setting->erase(key); -} - ValueStore::Status QuotaExceededError(Resource resource) { const char* name = NULL; // TODO(kalman): These hisograms are both silly and untracked. Fix. @@ -79,15 +70,17 @@ ValueStore::Status QuotaExceededError(Resource resource) { } // namespace -SettingsStorageQuotaEnforcer::SettingsStorageQuotaEnforcer( - const Limits& limits, ValueStore* delegate) - : limits_(limits), delegate_(delegate), used_total_(0) { - CalculateUsage(); -} +SettingsStorageQuotaEnforcer::SettingsStorageQuotaEnforcer(const Limits& limits, + ValueStore* delegate) + : limits_(limits), + delegate_(delegate), + used_total_(0), + usage_calculated_(false) {} SettingsStorageQuotaEnforcer::~SettingsStorageQuotaEnforcer() {} size_t SettingsStorageQuotaEnforcer::GetBytesInUse(const std::string& key) { + LazyCalculateUsage(); std::map<std::string, size_t>::iterator maybe_used = used_per_setting_.find(key); return maybe_used == used_per_setting_.end() ? 0u : maybe_used->second; @@ -96,35 +89,35 @@ size_t SettingsStorageQuotaEnforcer::GetBytesInUse(const std::string& key) { size_t SettingsStorageQuotaEnforcer::GetBytesInUse( const std::vector<std::string>& keys) { size_t used = 0; - for (std::vector<std::string>::const_iterator it = keys.begin(); - it != keys.end(); ++it) { - used += GetBytesInUse(*it); - } + for (const std::string& key : keys) + used += GetBytesInUse(key); return used; } size_t SettingsStorageQuotaEnforcer::GetBytesInUse() { // All ValueStore implementations rely on GetBytesInUse being // implemented here. + LazyCalculateUsage(); return used_total_; } ValueStore::ReadResult SettingsStorageQuotaEnforcer::Get( const std::string& key) { - return delegate_->Get(key); + return HandleResult(delegate_->Get(key)); } ValueStore::ReadResult SettingsStorageQuotaEnforcer::Get( const std::vector<std::string>& keys) { - return delegate_->Get(keys); + return HandleResult(delegate_->Get(keys)); } ValueStore::ReadResult SettingsStorageQuotaEnforcer::Get() { - return delegate_->Get(); + return HandleResult(delegate_->Get()); } ValueStore::WriteResult SettingsStorageQuotaEnforcer::Set( WriteOptions options, const std::string& key, const base::Value& value) { + LazyCalculateUsage(); size_t new_used_total = used_total_; std::map<std::string, size_t> new_used_per_setting = used_per_setting_; Allocate(key, value, &new_used_total, &new_used_per_setting); @@ -138,17 +131,20 @@ ValueStore::WriteResult SettingsStorageQuotaEnforcer::Set( return MakeWriteResult(QuotaExceededError(MAX_ITEMS)); } - WriteResult result = delegate_->Set(options, key, value); + WriteResult result = HandleResult(delegate_->Set(options, key, value)); if (!result->status().ok()) return result.Pass(); - used_total_ = new_used_total; - used_per_setting_.swap(new_used_per_setting); + if (usage_calculated_) { + used_total_ = new_used_total; + used_per_setting_.swap(new_used_per_setting); + } return result.Pass(); } ValueStore::WriteResult SettingsStorageQuotaEnforcer::Set( WriteOptions options, const base::DictionaryValue& values) { + LazyCalculateUsage(); size_t new_used_total = used_total_; std::map<std::string, size_t> new_used_per_setting = used_per_setting_; for (base::DictionaryValue::Iterator it(values); !it.IsAtEnd(); @@ -168,83 +164,78 @@ ValueStore::WriteResult SettingsStorageQuotaEnforcer::Set( return MakeWriteResult(QuotaExceededError(MAX_ITEMS)); } - WriteResult result = delegate_->Set(options, values); + WriteResult result = HandleResult(delegate_->Set(options, values)); if (!result->status().ok()) return result.Pass(); - used_total_ = new_used_total; - used_per_setting_ = new_used_per_setting; + if (usage_calculated_) { + used_total_ = new_used_total; + used_per_setting_ = new_used_per_setting; + } + return result.Pass(); } ValueStore::WriteResult SettingsStorageQuotaEnforcer::Remove( const std::string& key) { - WriteResult result = delegate_->Remove(key); + LazyCalculateUsage(); + WriteResult result = HandleResult(delegate_->Remove(key)); if (!result->status().ok()) return result.Pass(); - Free(&used_total_, &used_per_setting_, key); + Free(key); + return result.Pass(); } ValueStore::WriteResult SettingsStorageQuotaEnforcer::Remove( const std::vector<std::string>& keys) { - WriteResult result = delegate_->Remove(keys); + WriteResult result = HandleResult(delegate_->Remove(keys)); if (!result->status().ok()) return result.Pass(); - for (std::vector<std::string>::const_iterator it = keys.begin(); - it != keys.end(); ++it) { - Free(&used_total_, &used_per_setting_, *it); - } + for (const std::string& key : keys) + Free(key); + return result.Pass(); } ValueStore::WriteResult SettingsStorageQuotaEnforcer::Clear() { - WriteResult result = delegate_->Clear(); + LazyCalculateUsage(); + WriteResult result = HandleResult(delegate_->Clear()); if (!result->status().ok()) return result.Pass(); used_per_setting_.clear(); - used_total_ = 0; + used_total_ = 0u; + return result.Pass(); } -bool SettingsStorageQuotaEnforcer::Restore() { - if (!delegate_->Restore()) { - // If we failed, we can't calculate the usage - that's okay, though, because - // next time we Restore() (if it succeeds) we will recalculate usage anyway. - // So reset storage counters now to free up resources. +template <class T> +T SettingsStorageQuotaEnforcer::HandleResult(T result) { + if (result->status().restore_status != RESTORE_NONE) { + // Restoration means that an unknown amount, possibly all, of the data was + // lost from the database. Reset our counters - they will be lazily + // recalculated if/when needed. used_per_setting_.clear(); used_total_ = 0u; - return false; + usage_calculated_ = false; } - CalculateUsage(); - return true; + return result; } -bool SettingsStorageQuotaEnforcer::RestoreKey(const std::string& key) { - if (!delegate_->RestoreKey(key)) - return false; +void SettingsStorageQuotaEnforcer::LazyCalculateUsage() { + if (usage_calculated_) + return; - ReadResult result = Get(key); - // If the key was deleted as a result of the Restore() call, free it. - if (!result->settings().HasKey(key) && ContainsKey(used_per_setting_, key)) - Free(&used_total_, &used_per_setting_, key); - return true; -} + DCHECK_EQ(0u, used_total_); + DCHECK(used_per_setting_.empty()); -void SettingsStorageQuotaEnforcer::CalculateUsage() { - ReadResult maybe_settings = delegate_->Get(); + ReadResult maybe_settings = HandleResult(delegate_->Get()); if (!maybe_settings->status().ok()) { - // Try to restore the database if it's corrupt. - if (maybe_settings->status().IsCorrupted() && delegate_->Restore()) - maybe_settings = delegate_->Get(); - - if (!maybe_settings->status().ok()) { - LOG(WARNING) << "Failed to get settings for quota:" - << maybe_settings->status().message; - return; - } + LOG(WARNING) << "Failed to get settings for quota:" + << maybe_settings->status().message; + return; } for (base::DictionaryValue::Iterator it(maybe_settings->settings()); @@ -252,6 +243,19 @@ void SettingsStorageQuotaEnforcer::CalculateUsage() { it.Advance()) { Allocate(it.key(), it.value(), &used_total_, &used_per_setting_); } + + usage_calculated_ = true; +} + +void SettingsStorageQuotaEnforcer::Free(const std::string& key) { + if (!usage_calculated_) + return; + auto it = used_per_setting_.find(key); + if (it == used_per_setting_.end()) + return; + DCHECK_GE(used_total_, it->second); + used_total_ -= it->second; + used_per_setting_.erase(it); } } // namespace extensions diff --git a/extensions/browser/api/storage/settings_storage_quota_enforcer.h b/extensions/browser/api/storage/settings_storage_quota_enforcer.h index e3e5363..2b6d08b 100644 --- a/extensions/browser/api/storage/settings_storage_quota_enforcer.h +++ b/extensions/browser/api/storage/settings_storage_quota_enforcer.h @@ -49,14 +49,19 @@ class SettingsStorageQuotaEnforcer : 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; ValueStore* get_delegate_for_test() { return delegate_.get(); } private: - // Calculate the current usage for the database. - void CalculateUsage(); + template <class T> + T HandleResult(T result); + + // Calculate the current usage for the database if not already calculated. + void LazyCalculateUsage(); + + // Frees the allocation of a setting in a record of total and per-setting + // usage. + void Free(const std::string& key); // Limits configuration. const Limits limits_; @@ -68,6 +73,9 @@ class SettingsStorageQuotaEnforcer : public ValueStore { // JSON-encoded values. size_t used_total_; + // Have the total bytes used been calculated? + bool usage_calculated_; + // Map of item key to its size, including the key itself. std::map<std::string, size_t> used_per_setting_; diff --git a/extensions/browser/api/storage/storage_api.cc b/extensions/browser/api/storage/storage_api.cc index 6f754ad..2db23621 100644 --- a/extensions/browser/api/storage/storage_api.cc +++ b/extensions/browser/api/storage/storage_api.cc @@ -22,8 +22,7 @@ using content::BrowserThread; // SettingsFunction SettingsFunction::SettingsFunction() - : settings_namespace_(settings_namespace::INVALID), - tried_restoring_storage_(false) {} + : settings_namespace_(settings_namespace::INVALID) {} SettingsFunction::~SettingsFunction() {} @@ -71,11 +70,9 @@ void SettingsFunction::AsyncRunWithStorage(ValueStore* storage) { } ExtensionFunction::ResponseValue SettingsFunction::UseReadResult( - ValueStore::ReadResult result, - const std::string* key, - ValueStore* storage) { + ValueStore::ReadResult result) { if (!result->status().ok()) - return HandleError(result->status(), key, storage); + return Error(result->status().message); base::DictionaryValue* dict = new base::DictionaryValue(); dict->Swap(&result->settings()); @@ -83,11 +80,9 @@ ExtensionFunction::ResponseValue SettingsFunction::UseReadResult( } ExtensionFunction::ResponseValue SettingsFunction::UseWriteResult( - ValueStore::WriteResult result, - const std::string* key, - ValueStore* storage) { + ValueStore::WriteResult result) { if (!result->status().ok()) - return HandleError(result->status(), key, storage); + return Error(result->status().message); if (!result->changes().empty()) { observers_->Notify(FROM_HERE, &SettingsObserver::OnSettingsChanged, @@ -98,30 +93,6 @@ ExtensionFunction::ResponseValue SettingsFunction::UseWriteResult( return NoArguments(); } -ExtensionFunction::ResponseValue SettingsFunction::HandleError( - const ValueStore::Status& status, - const std::string* key, - ValueStore* storage) { - // If the method failed due to corruption, and we haven't tried to fix it, we - // can try to restore the storage and re-run it. Otherwise, the method has - // failed. - if (status.IsCorrupted() && !tried_restoring_storage_) { - tried_restoring_storage_ = true; - - // If the corruption is on a particular key, try to restore that key and - // re-run. - if (key && storage->RestoreKey(*key)) - return RunWithStorage(storage); - - // If the full database is corrupted, try to restore the whole thing and - // re-run. - if (storage->Restore()) - return RunWithStorage(storage); - } - - return Error(status.message); -} - // Concrete settings functions namespace { @@ -175,19 +146,19 @@ ExtensionFunction::ResponseValue StorageStorageAreaGetFunction::RunWithStorage( switch (input->GetType()) { case base::Value::TYPE_NULL: - return UseReadResult(storage->Get(), nullptr, storage); + return UseReadResult(storage->Get()); case base::Value::TYPE_STRING: { std::string as_string; input->GetAsString(&as_string); - return UseReadResult(storage->Get(as_string), &as_string, storage); + return UseReadResult(storage->Get(as_string)); } case base::Value::TYPE_LIST: { std::vector<std::string> as_string_list; AddAllStringValues(*static_cast<base::ListValue*>(input), &as_string_list); - return UseReadResult(storage->Get(as_string_list), nullptr, storage); + return UseReadResult(storage->Get(as_string_list)); } case base::Value::TYPE_DICTIONARY: { @@ -195,14 +166,13 @@ ExtensionFunction::ResponseValue StorageStorageAreaGetFunction::RunWithStorage( static_cast<base::DictionaryValue*>(input); ValueStore::ReadResult result = storage->Get(GetKeys(*as_dict)); if (!result->status().ok()) { - return UseReadResult(result.Pass(), nullptr, storage); + return UseReadResult(result.Pass()); } base::DictionaryValue* with_default_values = as_dict->DeepCopy(); with_default_values->MergeDictionary(&result->settings()); - return UseReadResult( - ValueStore::MakeReadResult(make_scoped_ptr(with_default_values)), - nullptr, storage); + return UseReadResult(ValueStore::MakeReadResult( + make_scoped_ptr(with_default_values), result->status())); } default: @@ -251,8 +221,7 @@ ExtensionFunction::ResponseValue StorageStorageAreaSetFunction::RunWithStorage( base::DictionaryValue* input = NULL; if (!args_->GetDictionary(0, &input)) return BadMessage(); - return UseWriteResult(storage->Set(ValueStore::DEFAULTS, *input), nullptr, - storage); + return UseWriteResult(storage->Set(ValueStore::DEFAULTS, *input)); } void StorageStorageAreaSetFunction::GetQuotaLimitHeuristics( @@ -270,14 +239,14 @@ StorageStorageAreaRemoveFunction::RunWithStorage(ValueStore* storage) { case base::Value::TYPE_STRING: { std::string as_string; input->GetAsString(&as_string); - return UseWriteResult(storage->Remove(as_string), &as_string, storage); + return UseWriteResult(storage->Remove(as_string)); } case base::Value::TYPE_LIST: { std::vector<std::string> as_string_list; AddAllStringValues(*static_cast<base::ListValue*>(input), &as_string_list); - return UseWriteResult(storage->Remove(as_string_list), nullptr, storage); + return UseWriteResult(storage->Remove(as_string_list)); } default: @@ -292,7 +261,7 @@ void StorageStorageAreaRemoveFunction::GetQuotaLimitHeuristics( ExtensionFunction::ResponseValue StorageStorageAreaClearFunction::RunWithStorage(ValueStore* storage) { - return UseWriteResult(storage->Clear(), nullptr, storage); + return UseWriteResult(storage->Clear()); } void StorageStorageAreaClearFunction::GetQuotaLimitHeuristics( diff --git a/extensions/browser/api/storage/storage_api.h b/extensions/browser/api/storage/storage_api.h index 0f56fe3..e6b532c 100644 --- a/extensions/browser/api/storage/storage_api.h +++ b/extensions/browser/api/storage/storage_api.h @@ -30,52 +30,26 @@ class SettingsFunction : public UIThreadExtensionFunction { // The StorageFrontend makes sure this is posted to the appropriate thread. virtual ResponseValue RunWithStorage(ValueStore* storage) = 0; - // Handles the |result| of a read function. - // - If the result succeeded, this will set |result_| and return. - // - If |result| failed with a ValueStore::CORRUPTION error, this will call - // RestoreStorageAndRetry(), and return that result. - // - If the |result| failed with a different error, this will set |error_| - // and return. - // - |key| will be non-null only when reading a single value. - ResponseValue UseReadResult(ValueStore::ReadResult result, - const std::string* key, - ValueStore* storage); + // Convert the |result| of a read function to the appropriate response value. + // - If the |result| succeeded this will return a response object argument. + // - If the |result| failed will return an error object. + ResponseValue UseReadResult(ValueStore::ReadResult result); // Handles the |result| of a write function. - // - If the result succeeded, this will set |result_| and return. - // - If |result| failed with a ValueStore::CORRUPTION error, this will call - // RestoreStorageAndRetry(), and return that result. - // - If the |result| failed with a different error, this will set |error_| - // and return. - // - |key| will be non-null only when writing a single value. - // This will also send out a change notification, if appropriate. - ResponseValue UseWriteResult(ValueStore::WriteResult result, - const std::string* key, - ValueStore* storage); + // - If the |result| succeeded this will send out change notification(s), if + // appropriate, and return no arguments. + // - If the |result| failed will return an error object. + ResponseValue UseWriteResult(ValueStore::WriteResult result); private: // Called via PostTask from Run. Calls RunWithStorage and then // SendResponse with its success value. void AsyncRunWithStorage(ValueStore* storage); - // Called if we encounter a ValueStore error. If the error is due to - // corruption, tries to restore the ValueStore and re-run the API function. - // If the storage cannot be restored or was due to some other error, then sets - // error and returns. This also sets the |tried_restoring_storage_| flag to - // ensure we don't enter a loop. - // - |key| will be non-null only when reading/writing a single value. - ResponseValue HandleError(const ValueStore::Status& error, - const std::string* key, - ValueStore* storage); - // The settings namespace the call was for. For example, SYNC if the API // call was chrome.settings.experimental.sync..., LOCAL if .local, etc. settings_namespace::Namespace settings_namespace_; - // A flag indicating whether or not we have tried to restore storage. We - // should only ever try once (per API call) in order to avoid entering a loop. - bool tried_restoring_storage_; - // Observers, cached so that it's only grabbed from the UI thread. scoped_refptr<SettingsObserverList> observers_; }; diff --git a/extensions/browser/api/storage/weak_unlimited_settings_storage.cc b/extensions/browser/api/storage/weak_unlimited_settings_storage.cc index 1089f23..3de934b 100644 --- a/extensions/browser/api/storage/weak_unlimited_settings_storage.cc +++ b/extensions/browser/api/storage/weak_unlimited_settings_storage.cc @@ -64,10 +64,4 @@ ValueStore::WriteResult WeakUnlimitedSettingsStorage::Clear() { return delegate_->Clear(); } -bool WeakUnlimitedSettingsStorage::Restore() { return delegate_->Restore(); } - -bool WeakUnlimitedSettingsStorage::RestoreKey(const std::string& key) { - return delegate_->RestoreKey(key); -} - } // namespace extensions diff --git a/extensions/browser/api/storage/weak_unlimited_settings_storage.h b/extensions/browser/api/storage/weak_unlimited_settings_storage.h index 9421287..a92c8a1 100644 --- a/extensions/browser/api/storage/weak_unlimited_settings_storage.h +++ b/extensions/browser/api/storage/weak_unlimited_settings_storage.h @@ -5,6 +5,9 @@ #ifndef EXTENSIONS_BROWSER_API_STORAGE_WEAK_UNLIMITED_SETTINGS_STORAGE_H_ #define EXTENSIONS_BROWSER_API_STORAGE_WEAK_UNLIMITED_SETTINGS_STORAGE_H_ +#include <string> +#include <vector> + #include "base/compiler_specific.h" #include "extensions/browser/value_store/value_store.h" @@ -35,8 +38,6 @@ class WeakUnlimitedSettingsStorage : 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; private: // The delegate storage area, NOT OWNED. 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_ |