From fa9df436883cbd43039e8fd5c20ed3cecf7c7d64 Mon Sep 17 00:00:00 2001 From: esmael Date: Mon, 6 May 2024 10:48:45 -0700 Subject: [PATCH] Simplify PersistentSettings. Building off the work of @briantting: https://github.com/youtube/cobalt/pull/3095 b/305057554 --- cobalt/browser/application.cc | 12 +- cobalt/browser/browser_module.cc | 2 +- cobalt/h5vcc/h5vcc_metrics.cc | 10 +- cobalt/h5vcc/h5vcc_settings.cc | 11 +- cobalt/h5vcc/h5vcc_storage.cc | 8 +- cobalt/network/network_module.cc | 6 +- cobalt/network/url_request_context.cc | 21 +- .../persistent_storage/persistent_settings.cc | 249 ++++++------- .../persistent_storage/persistent_settings.h | 90 ++--- .../persistent_settings_test.cc | 327 +++--------------- cobalt/watchdog/watchdog.cc | 30 +- cobalt/watchdog/watchdog_test.cc | 63 ++-- .../service_worker_persistent_settings.cc | 187 ++++------ .../service_worker_persistent_settings.h | 7 +- 14 files changed, 339 insertions(+), 684 deletions(-) diff --git a/cobalt/browser/application.cc b/cobalt/browser/application.cc index b94411d99085..61d157bc14f0 100644 --- a/cobalt/browser/application.cc +++ b/cobalt/browser/application.cc @@ -1513,10 +1513,14 @@ void Application::InitMetrics() { metrics::CobaltMetricsServicesManager::GetInstance(); // Before initializing metrics manager, set any persisted settings like if // it's enabled or upload interval. - bool is_metrics_enabled = persistent_settings_->GetPersistentSettingAsBool( - metrics::kMetricEnabledSettingName, false); - auto metric_event_interval = persistent_settings_->GetPersistentSettingAsInt( - metrics::kMetricEventIntervalSettingName, 300); + bool is_metrics_enabled = + persistent_settings_->Get(metrics::kMetricEnabledSettingName) + .GetIfBool() + .value_or(false); + auto metric_event_interval = + persistent_settings_->Get(metrics::kMetricEventIntervalSettingName) + .GetIfInt() + .value_or(300); metrics_services_manager_->SetEventDispatcher(&event_dispatcher_); metrics_services_manager_->SetUploadInterval(metric_event_interval); metrics_services_manager_->ToggleMetricsEnabled(is_metrics_enabled); diff --git a/cobalt/browser/browser_module.cc b/cobalt/browser/browser_module.cc index 5e5c7da3cdfc..64b0d53c74e7 100644 --- a/cobalt/browser/browser_module.cc +++ b/cobalt/browser/browser_module.cc @@ -793,7 +793,7 @@ void BrowserModule::OnLoad() { web_module_loaded_.Signal(); - options_.persistent_settings->ValidatePersistentSettings(); + options_.persistent_settings->Validate(); ValidateCacheBackendSettings(); } diff --git a/cobalt/h5vcc/h5vcc_metrics.cc b/cobalt/h5vcc/h5vcc_metrics.cc index 0b22b176983c..04846ca5cb12 100644 --- a/cobalt/h5vcc/h5vcc_metrics.cc +++ b/cobalt/h5vcc/h5vcc_metrics.cc @@ -119,9 +119,8 @@ script::HandlePromiseVoid H5vccMetrics::Disable( void H5vccMetrics::ToggleMetricsEnabled(bool is_enabled, base::OnceClosure done_callback) { - persistent_settings_->SetPersistentSetting( - browser::metrics::kMetricEnabledSettingName, - std::make_unique(is_enabled)); + persistent_settings_->Set(browser::metrics::kMetricEnabledSettingName, + base::Value(is_enabled)); browser::metrics::CobaltMetricsServicesManager::GetInstance() ->ToggleMetricsEnabled(is_enabled, std::move(done_callback)); } @@ -132,9 +131,8 @@ bool H5vccMetrics::IsEnabled() { } void H5vccMetrics::SetMetricEventInterval(uint32_t interval_seconds) { - persistent_settings_->SetPersistentSetting( - browser::metrics::kMetricEventIntervalSettingName, - std::make_unique(static_cast(interval_seconds))); + persistent_settings_->Set(browser::metrics::kMetricEventIntervalSettingName, + base::Value(static_cast(interval_seconds))); browser::metrics::CobaltMetricsServicesManager::GetInstance() ->SetUploadInterval(interval_seconds); } diff --git a/cobalt/h5vcc/h5vcc_settings.cc b/cobalt/h5vcc/h5vcc_settings.cc index 59d76c863024..38b87090214b 100644 --- a/cobalt/h5vcc/h5vcc_settings.cc +++ b/cobalt/h5vcc/h5vcc_settings.cc @@ -84,9 +84,8 @@ bool H5vccSettings::Set(const std::string& name, SetValueType value) const { if (!persistent_settings_) { return false; } else { - persistent_settings_->SetPersistentSetting( - network::kQuicEnabledPersistentSettingsKey, - std::make_unique(value.AsType() != 0)); + persistent_settings_->Set(network::kQuicEnabledPersistentSettingsKey, + base::Value(value.AsType() != 0)); // Tell NetworkModule (if exists) to re-query persistent settings. if (network_module_) { network_module_->SetEnableQuicFromPersistentSettings(); @@ -107,16 +106,14 @@ bool H5vccSettings::Set(const std::string& name, SetValueType value) const { void H5vccSettings::SetPersistentSettingAsInt(const std::string& key, int value) const { if (persistent_settings_) { - persistent_settings_->SetPersistentSetting( - key, std::make_unique(value)); + persistent_settings_->Set(key, base::Value(value)); } } int H5vccSettings::GetPersistentSettingAsInt(const std::string& key, int default_setting) const { if (persistent_settings_) { - return persistent_settings_->GetPersistentSettingAsInt(key, - default_setting); + return persistent_settings_->Get(key).GetIfInt().value_or(default_setting); } return default_setting; } diff --git a/cobalt/h5vcc/h5vcc_storage.cc b/cobalt/h5vcc/h5vcc_storage.cc index 9b88ae9dcc2f..fdd660e0ff67 100644 --- a/cobalt/h5vcc/h5vcc_storage.cc +++ b/cobalt/h5vcc/h5vcc_storage.cc @@ -372,9 +372,9 @@ H5vccStorageResourceTypeQuotaBytesDictionary H5vccStorage::GetQuota() { } void H5vccStorage::EnableCache() { - persistent_settings_->SetPersistentSetting( + persistent_settings_->Set( network::disk_cache::kCacheEnabledPersistentSettingsKey, - std::make_unique(true)); + base::Value(true)); network::disk_cache::settings::SetCacheEnabled(true); @@ -384,9 +384,9 @@ void H5vccStorage::EnableCache() { } void H5vccStorage::DisableCache() { - persistent_settings_->SetPersistentSetting( + persistent_settings_->Set( network::disk_cache::kCacheEnabledPersistentSettingsKey, - std::make_unique(false)); + base::Value(false)); network::disk_cache::settings::SetCacheEnabled(false); diff --git a/cobalt/network/network_module.cc b/cobalt/network/network_module.cc index de6b98cc9849..60ddccbc1b3c 100644 --- a/cobalt/network/network_module.cc +++ b/cobalt/network/network_module.cc @@ -109,8 +109,10 @@ void NetworkModule::SetProxy(const std::string& custom_proxy_rules) { void NetworkModule::SetEnableQuicFromPersistentSettings() { // Called on initialization and when the persistent setting is changed. if (options_.persistent_settings != nullptr) { - bool enable_quic = options_.persistent_settings->GetPersistentSettingAsBool( - kQuicEnabledPersistentSettingsKey, false); + bool enable_quic = + options_.persistent_settings->Get(kQuicEnabledPersistentSettingsKey) + .GetIfBool() + .value_or(false); task_runner()->PostTask( FROM_HERE, base::Bind(&URLRequestContext::SetEnableQuic, diff --git a/cobalt/network/url_request_context.cc b/cobalt/network/url_request_context.cc index ed047d13e58c..0672f4893547 100644 --- a/cobalt/network/url_request_context.cc +++ b/cobalt/network/url_request_context.cc @@ -72,8 +72,8 @@ void LoadDiskCacheQuotaSettings( std::string directory = disk_cache::defaults::GetSubdirectory(resource_type); uint32_t bucket_size = - static_cast(settings->GetPersistentSettingAsDouble( - directory, disk_cache::defaults::GetQuota(resource_type))); + static_cast(settings->Get(directory).GetIfDouble().value_or( + disk_cache::defaults::GetQuota(resource_type))); quotas[resource_type] = bucket_size; total_size += bucket_size; } @@ -93,9 +93,7 @@ void LoadDiskCacheQuotaSettings( disk_cache::settings::SetQuota(resource_type, default_quota); std::string directory = disk_cache::defaults::GetSubdirectory(resource_type); - settings->SetPersistentSetting( - directory, - std::make_unique(static_cast(default_quota))); + settings->Set(directory, base::Value(static_cast(default_quota))); } } @@ -290,8 +288,10 @@ URLRequestContext::URLRequestContext( http_cache->set_can_disable_by_mime_type(true); if (persistent_settings != nullptr) { auto cache_enabled = - persistent_settings->GetPersistentSettingAsBool( - disk_cache::kCacheEnabledPersistentSettingsKey, true); + persistent_settings + ->Get(disk_cache::kCacheEnabledPersistentSettingsKey) + .GetIfBool() + .value_or(true); disk_cache::settings::SetCacheEnabled(cache_enabled); if (!cache_enabled) { http_cache->set_mode(net::HttpCache::Mode::DISABLE); @@ -344,13 +344,12 @@ void URLRequestContext::OnQuicToggle(const std::string& message) { void URLRequestContext::UpdateCacheSizeSetting(disk_cache::ResourceType type, uint32_t bytes) { CHECK(cache_persistent_settings_); - cache_persistent_settings_->SetPersistentSetting( - disk_cache::defaults::GetSubdirectory(type), - std::make_unique(static_cast(bytes))); + cache_persistent_settings_->Set(disk_cache::defaults::GetSubdirectory(type), + base::Value(static_cast(bytes))); } void URLRequestContext::ValidateCachePersistentSettings() { - cache_persistent_settings_->ValidatePersistentSettings(); + cache_persistent_settings_->Validate(); } void URLRequestContext::AssociateKeyWithResourceType( diff --git a/cobalt/persistent_storage/persistent_settings.cc b/cobalt/persistent_storage/persistent_settings.cc index ada6424385bb..202598ab478a 100644 --- a/cobalt/persistent_storage/persistent_settings.cc +++ b/cobalt/persistent_storage/persistent_settings.cc @@ -20,7 +20,6 @@ #include "base/bind.h" #include "base/logging.h" #include "cobalt/base/task_runner_util.h" -#include "components/prefs/json_pref_store.h" #include "starboard/common/file.h" #include "starboard/common/log.h" #include "starboard/configuration_constants.h" @@ -30,7 +29,8 @@ namespace cobalt { namespace persistent_storage { void PersistentSettings::WillDestroyCurrentMessageLoop() { - // Clear all member variables allocated from the thread. + if (!pref_store_) return; + base::AutoLock auto_lock(pref_store_lock_); pref_store_.reset(); } @@ -42,15 +42,14 @@ PersistentSettings::PersistentSettings(const std::string& file_name) std::vector storage_dir(kSbFileMaxPath, 0); SbSystemGetPath(kSbSystemPathCacheDirectory, storage_dir.data(), kSbFileMaxPath); - persistent_settings_file_ = - std::string(storage_dir.data()) + kSbFileSepString + file_name; - LOG(INFO) << "Persistent settings file path: " << persistent_settings_file_; - - task_runner()->PostTask(FROM_HERE, - base::Bind(&PersistentSettings::InitializePrefStore, - base::Unretained(this))); - pref_store_initialized_.Wait(); - destruction_observer_added_.Wait(); + file_path_ = + base::FilePath(std::string(storage_dir.data())).Append(file_name); + DLOG(INFO) << "Persistent settings file path: " << file_path_; + + Run(FROM_HERE, + base::Bind(&PersistentSettings::InitializePrefStore, + base::Unretained(this)), + /*blocking=*/true); } PersistentSettings::~PersistentSettings() { @@ -58,7 +57,7 @@ PersistentSettings::~PersistentSettings() { DCHECK(thread_.IsRunning()); // Wait for all previously posted tasks to finish. - base::task_runner_util::WaitForFence(thread_.task_runner(), FROM_HERE); + Fence(FROM_HERE); thread_.Stop(); } @@ -70,171 +69,123 @@ void PersistentSettings::InitializePrefStore() { // then destroy the internal components before the task runner is reset. // No posted tasks will be executed once the thread is stopped. base::CurrentThread::Get()->AddDestructionObserver(this); + // Read preferences into memory. - { - base::AutoLock auto_lock(pref_store_lock_); - pref_store_ = base::MakeRefCounted( - base::FilePath(persistent_settings_file_)); - pref_store_->ReadPrefs(); - persistent_settings_ = pref_store_->GetValues(); - pref_store_initialized_.Signal(); - } + pref_store_ = base::MakeRefCounted(file_path_); + pref_store_->ReadPrefs(); + // Remove settings file and do not allow writes to the file until the // |ValidatePersistentSettings()| is called. - starboard::SbFileDeleteRecursive(persistent_settings_file_.c_str(), true); - destruction_observer_added_.Signal(); + DeleteFile(); } -void PersistentSettings::ValidatePersistentSettings(bool blocking) { - task_runner()->PostTask( - FROM_HERE, - base::BindOnce(&PersistentSettings::ValidatePersistentSettingsHelper, - base::Unretained(this), blocking)); +void PersistentSettings::DeleteFile() { + starboard::SbFileDeleteRecursive(file_path_.value().c_str(), true); } -void PersistentSettings::ValidatePersistentSettingsHelper(bool blocking) { - DCHECK_EQ(base::SequencedTaskRunner::GetCurrentDefault(), task_runner()); - if (!validated_initial_settings_) { - base::AutoLock auto_lock(pref_store_lock_); - validated_initial_settings_ = true; - CommitPendingWrite(blocking); +void PersistentSettings::Validate() { + if (validated_initial_settings_) return; + if (base::SequencedTaskRunner::GetCurrentDefault() != task_runner()) { + Run(FROM_HERE, + base::BindOnce(&PersistentSettings::Validate, base::Unretained(this))); + return; } + validated_initial_settings_ = true; + pref_store_->CommitPendingWrite(); } -bool PersistentSettings::GetPersistentSettingAsBool(const std::string& key, - bool default_setting) { - // Wait for all previously posted tasks to finish. - base::task_runner_util::WaitForFence(thread_.task_runner(), FROM_HERE); - base::AutoLock auto_lock(pref_store_lock_); - absl::optional result = persistent_settings_.FindBool(key); - return result.value_or(default_setting); -} - -int PersistentSettings::GetPersistentSettingAsInt(const std::string& key, - int default_setting) { - // Wait for all previously posted tasks to finish. - base::task_runner_util::WaitForFence(thread_.task_runner(), FROM_HERE); - base::AutoLock auto_lock(pref_store_lock_); - absl::optional result = persistent_settings_.FindInt(key); - return result.value_or(default_setting); -} - -double PersistentSettings::GetPersistentSettingAsDouble( - const std::string& key, double default_setting) { - // Wait for all previously posted tasks to finish. - base::task_runner_util::WaitForFence(thread_.task_runner(), FROM_HERE); +base::Value PersistentSettings::Get(const std::string& key) const { + if (!pref_store_) return base::Value(); base::AutoLock auto_lock(pref_store_lock_); - absl::optional result = persistent_settings_.FindDouble(key); - return result.value_or(default_setting); -} - -std::string PersistentSettings::GetPersistentSettingAsString( - const std::string& key, const std::string& default_setting) { - // Wait for all previously posted tasks to finish. - base::task_runner_util::WaitForFence(thread_.task_runner(), FROM_HERE); - base::AutoLock auto_lock(pref_store_lock_); - const std::string* result = persistent_settings_.FindString(key); - if (result) return *result; - return default_setting; -} - -std::vector PersistentSettings::GetPersistentSettingAsList( - const std::string& key) { - // Wait for all previously posted tasks to finish. - base::task_runner_util::WaitForFence(thread_.task_runner(), FROM_HERE); - base::AutoLock auto_lock(pref_store_lock_); - const base::Value::List* result = persistent_settings_.FindList(key); - std::vector values; - if (result) { - for (const auto& value : *result) { - values.emplace_back(value.Clone()); - } + base::Value value; + Run(FROM_HERE, + base::BindOnce( + [](base::WeakPtr pref_store, const std::string& key, + base::Value* result) { + const base::Value* value = nullptr; + pref_store->GetValue(key, &value); + *result = value ? value->Clone() : base::Value(); + }, + pref_store_->AsWeakPtr(), key, &value), + /*blocking=*/true); + return std::move(value); +} + +WriteablePrefStore::PrefWriteFlags PersistentSettings::GetPrefWriteFlags() + const { + return validated_initial_settings_ + ? WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS + : WriteablePrefStore::LOSSY_PREF_WRITE_FLAG; +} + + +void PersistentSettings::Set(const std::string& key, base::Value value) { + if (!pref_store_) return; + if (base::SequencedTaskRunner::GetCurrentDefault() != task_runner()) { + Run(FROM_HERE, + base::BindOnce(&PersistentSettings::Set, base::Unretained(this), key, + std::move(value))); + return; } - return values; -} - -base::flat_map> -PersistentSettings::GetPersistentSettingAsDictionary(const std::string& key) { - // Wait for all previously posted tasks to finish. - base::task_runner_util::WaitForFence(thread_.task_runner(), FROM_HERE); base::AutoLock auto_lock(pref_store_lock_); - base::Value::Dict* result = persistent_settings_.FindDict(key); - base::flat_map> dict; - if (result) { - for (base::Value::Dict::iterator it = result->begin(); it != result->end(); - ++it) { - dict.insert(std::make_pair( - it->first, base::Value::ToUniquePtrValue(std::move(it->second)))); - } - return dict; + pref_store_->SetValue(key, std::move(value), GetPrefWriteFlags()); + if (validated_initial_settings_) { + pref_store_->CommitPendingWrite(); } - return dict; -} - -void PersistentSettings::SetPersistentSetting( - const std::string& key, std::unique_ptr value, bool blocking) { - task_runner()->PostTask( - FROM_HERE, - base::BindOnce(&PersistentSettings::SetPersistentSettingHelper, - base::Unretained(this), key, std::move(value), blocking)); } -void PersistentSettings::SetPersistentSettingHelper( - const std::string& key, std::unique_ptr value, bool blocking) { - DCHECK_EQ(base::SequencedTaskRunner::GetCurrentDefault(), task_runner()); +void PersistentSettings::Remove(const std::string& key) { + if (!pref_store_) return; + if (base::SequencedTaskRunner::GetCurrentDefault() != task_runner()) { + Run(FROM_HERE, base::BindOnce(&PersistentSettings::Remove, + base::Unretained(this), key)); + return; + } base::AutoLock auto_lock(pref_store_lock_); - pref_store_->SetValue(key, base::Value::FromUniquePtrValue(std::move(value)), - WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); - persistent_settings_ = pref_store_->GetValues(); + pref_store_->RemoveValue(key, GetPrefWriteFlags()); if (validated_initial_settings_) { - CommitPendingWrite(blocking); + pref_store_->CommitPendingWrite(); } } -void PersistentSettings::RemovePersistentSetting(const std::string& key, - bool blocking) { - task_runner()->PostTask( - FROM_HERE, - base::BindOnce(&PersistentSettings::RemovePersistentSettingHelper, - base::Unretained(this), key, blocking)); -} - -void PersistentSettings::RemovePersistentSettingHelper(const std::string& key, - bool blocking) { - DCHECK_EQ(base::SequencedTaskRunner::GetCurrentDefault(), task_runner()); - base::AutoLock auto_lock(pref_store_lock_); - pref_store_->RemoveValue(key, WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); - persistent_settings_ = pref_store_->GetValues(); - if (validated_initial_settings_) { - CommitPendingWrite(blocking); +void PersistentSettings::RemoveAll() { + if (!pref_store_) return; + if (base::SequencedTaskRunner::GetCurrentDefault() != task_runner()) { + Run(FROM_HERE, + base::BindOnce(&PersistentSettings::RemoveAll, base::Unretained(this))); + return; } + base::AutoLock auto_lock(pref_store_lock_); + DeleteFile(); + pref_store_->ReadPrefs(); } -void PersistentSettings::DeletePersistentSettings() { - task_runner()->PostTask( - FROM_HERE, - base::BindOnce(&PersistentSettings::DeletePersistentSettingsHelper, - base::Unretained(this))); +void PersistentSettings::WaitForPendingFileWrite() { + if (!pref_store_) return; + base::WaitableEvent done; + Run(FROM_HERE, base::BindOnce(&JsonPrefStore::CommitPendingWrite, + pref_store_->AsWeakPtr(), base::OnceClosure(), + base::BindOnce(&base::WaitableEvent::Signal, + base::Unretained(&done)))); + done.Wait(); } -void PersistentSettings::DeletePersistentSettingsHelper() { - DCHECK_EQ(base::SequencedTaskRunner::GetCurrentDefault(), task_runner()); - base::AutoLock auto_lock(pref_store_lock_); - starboard::SbFileDeleteRecursive(persistent_settings_file_.c_str(), true); - pref_store_->ReadPrefs(); - persistent_settings_ = pref_store_->GetValues(); +void PersistentSettings::Fence(const base::Location& location) { + Run(location, base::OnceClosure(), /*blocking=*/true); } -void PersistentSettings::CommitPendingWrite(bool blocking) { - if (blocking) { - base::WaitableEvent written; - pref_store_->CommitPendingWrite( - base::OnceClosure(), - base::BindOnce(&base::WaitableEvent::Signal, Unretained(&written))); - written.Wait(); - } else { - pref_store_->CommitPendingWrite(); - } +void PersistentSettings::Run(const base::Location& location, + base::OnceClosure task, bool blocking) const { + std::unique_ptr done; + if (blocking) done.reset(new base::WaitableEvent()); + task_runner()->PostTask( + location, base::BindOnce( + [](base::OnceClosure task, base::WaitableEvent* done) { + if (task) std::move(task).Run(); + if (done) done->Signal(); + }, + std::move(task), done.get())); + if (done) done->Wait(); } } // namespace persistent_storage diff --git a/cobalt/persistent_storage/persistent_settings.h b/cobalt/persistent_storage/persistent_settings.h index 010254b3d31c..c0c1f503ba2c 100644 --- a/cobalt/persistent_storage/persistent_settings.h +++ b/cobalt/persistent_storage/persistent_settings.h @@ -22,6 +22,7 @@ #include "base/bind_helpers.h" #include "base/containers/flat_map.h" +#include "base/files/file_path.h" #include "base/functional/callback_forward.h" #include "base/synchronization/lock.h" #include "base/synchronization/waitable_event.h" @@ -29,9 +30,14 @@ #include "base/task/sequenced_task_runner.h" #include "base/threading/thread.h" #include "base/values.h" -#include "components/prefs/persistent_pref_store.h" +#include "components/prefs/json_pref_store.h" namespace cobalt { + +namespace watchdog { +class WatchdogTest; +} + namespace persistent_storage { // PersistentSettings manages Cobalt settings that persist from one application @@ -40,71 +46,56 @@ namespace persistent_storage { // PersistentSettings uses JsonPrefStore for most of its functionality. // JsonPrefStore maintains thread safety by requiring that all access occurs on // the Sequence that created it. -class PersistentSettings : public base::CurrentThread::DestructionObserver { +class PersistentSettings : public base::CurrentThread::DestructionObserver, + public base::SupportsWeakPtr { public: explicit PersistentSettings(const std::string& file_name); ~PersistentSettings(); - // The task runner this object is running on. - base::SequencedTaskRunner* task_runner() const { - return thread_.task_runner(); - } - - // From base::CurrentThread::DestructionObserver. + // base::CurrentThread::DestructionObserver void WillDestroyCurrentMessageLoop() override; // Validates persistent settings by restoring the file on successful start up. - void ValidatePersistentSettings(bool blocking = false); + void Validate(); - // Getters and Setters for persistent settings. - bool GetPersistentSettingAsBool(const std::string& key, bool default_setting); + base::Value Get(const std::string& key) const; - int GetPersistentSettingAsInt(const std::string& key, int default_setting); + void Set(const std::string& key, base::Value value); - double GetPersistentSettingAsDouble(const std::string& key, - double default_setting); + void Remove(const std::string& key); - std::string GetPersistentSettingAsString(const std::string& key, - const std::string& default_setting); + void RemoveAll(); - std::vector GetPersistentSettingAsList(const std::string& key); + private: + friend class PersistentSettingTest; + friend class cobalt::watchdog::WatchdogTest; - base::flat_map> - GetPersistentSettingAsDictionary(const std::string& key); + void InitializePrefStore(); - void SetPersistentSetting(const std::string& key, - std::unique_ptr value, - bool blocking = false); + WriteablePrefStore::PrefWriteFlags GetPrefWriteFlags() const; - void RemovePersistentSetting(const std::string& key, bool blocking = false); + void DeleteFile(); - void DeletePersistentSettings(); + void CommitPendingWrite(); - private: - // Called by the constructor to initialize pref_store_ from - // the dedicated thread_ as a JSONPrefStore. - void InitializePrefStore(); + void Run(const base::Location& location, base::OnceClosure closure, + bool blocking = false) const; - void ValidatePersistentSettingsHelper(bool blocking); + void Fence(const base::Location& location); - void SetPersistentSettingHelper(const std::string& key, - std::unique_ptr value, - bool blocking); - - void RemovePersistentSettingHelper(const std::string& key, bool blocking); - - void DeletePersistentSettingsHelper(); + // The task runner this object is running on. + base::SequencedTaskRunner* task_runner() const { + return thread_.task_runner(); + } - void CommitPendingWrite(bool blocking); + void WaitForPendingFileWrite(); // Persistent settings file path. - std::string persistent_settings_file_; + base::FilePath file_path_; // PrefStore used for persistent settings. - scoped_refptr pref_store_; - - // Persistent settings dictionary. - base::Value::Dict persistent_settings_; + scoped_refptr pref_store_; + mutable base::Lock pref_store_lock_; // The thread created and owned by PersistentSettings. All pref_store_ // methods must be called from this thread. @@ -113,21 +104,6 @@ class PersistentSettings : public base::CurrentThread::DestructionObserver { // Flag indicating whether or not initial persistent settings have been // validated. bool validated_initial_settings_; - - // This event is used to signal when Initialize has been called and - // pref_store_ mutations can now occur. - base::WaitableEvent pref_store_initialized_ = { - base::WaitableEvent::ResetPolicy::MANUAL, - base::WaitableEvent::InitialState::NOT_SIGNALED}; - - // This event is used to signal when the destruction observers have been - // added to the task runner. This is then used in Stop() to ensure that - // processing doesn't continue until the thread is cleanly shutdown. - base::WaitableEvent destruction_observer_added_ = { - base::WaitableEvent::ResetPolicy::MANUAL, - base::WaitableEvent::InitialState::NOT_SIGNALED}; - - base::Lock pref_store_lock_; }; } // namespace persistent_storage diff --git a/cobalt/persistent_storage/persistent_settings_test.cc b/cobalt/persistent_storage/persistent_settings_test.cc index d653c9233f7e..38611984b130 100644 --- a/cobalt/persistent_storage/persistent_settings_test.cc +++ b/cobalt/persistent_storage/persistent_settings_test.cc @@ -48,307 +48,70 @@ class PersistentSettingTest : public testing::Test { void SetUp() final { starboard::SbFileDeleteRecursive(persistent_settings_file_.c_str(), true); + persistent_settings_ = + std::make_unique(kPersistentSettingsJson); } void TearDown() final { starboard::SbFileDeleteRecursive(persistent_settings_file_.c_str(), true); } + void Fence(const base::Location& location) { + persistent_settings_->Fence(location); + } + + void WaitForPendingFileWrite() { + persistent_settings_->WaitForPendingFileWrite(); + } + base::test::TaskEnvironment scoped_task_environment_; std::string persistent_settings_file_; + std::unique_ptr persistent_settings_; }; -TEST_F(PersistentSettingTest, GetDefaultBool) { - auto persistent_settings = - std::make_unique(kPersistentSettingsJson); - persistent_settings->ValidatePersistentSettings(); - - // does not exist - ASSERT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); - ASSERT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false)); - - // exists but invalid - persistent_settings->SetPersistentSetting("key", - std::make_unique(4.2)); - EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); +TEST_F(PersistentSettingTest, Set) { + EXPECT_TRUE(persistent_settings_->Get("key").is_none()); + persistent_settings_->Set("key", base::Value(4.2)); + Fence(FROM_HERE); + EXPECT_EQ(4.2, persistent_settings_->Get("key").GetIfDouble().value_or(0)); } -TEST_F(PersistentSettingTest, GetSetBool) { - auto persistent_settings = - std::make_unique(kPersistentSettingsJson); - persistent_settings->ValidatePersistentSettings(); - - persistent_settings->SetPersistentSetting( - "key", std::make_unique(true)); - EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", false)); - persistent_settings->SetPersistentSetting( - "key", std::make_unique(false)); - EXPECT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false)); +TEST_F(PersistentSettingTest, Remove) { + persistent_settings_->Set("key", base::Value(true)); + Fence(FROM_HERE); + EXPECT_EQ(true, persistent_settings_->Get("key").GetIfBool().value_or(false)); + persistent_settings_->Remove("key"); + Fence(FROM_HERE); + EXPECT_TRUE(persistent_settings_->Get("key").is_none()); } -TEST_F(PersistentSettingTest, GetDefaultInt) { - auto persistent_settings = - std::make_unique(kPersistentSettingsJson); - persistent_settings->ValidatePersistentSettings(); - - // does not exist - ASSERT_EQ(-1, persistent_settings->GetPersistentSettingAsInt("key", -1)); - ASSERT_EQ(0, persistent_settings->GetPersistentSettingAsInt("key", 0)); - ASSERT_EQ(42, persistent_settings->GetPersistentSettingAsInt("key", 42)); - - // exists but invalid - persistent_settings->SetPersistentSetting("key", - std::make_unique(4.2)); - EXPECT_EQ(8, persistent_settings->GetPersistentSettingAsInt("key", 8)); +TEST_F(PersistentSettingTest, RemoveAll) { + persistent_settings_->Set("key", base::Value(true)); + Fence(FROM_HERE); + EXPECT_EQ(true, persistent_settings_->Get("key").GetIfBool().value_or(false)); + persistent_settings_->RemoveAll(); + Fence(FROM_HERE); + EXPECT_TRUE(persistent_settings_->Get("key").is_none()); } -TEST_F(PersistentSettingTest, GetSetInt) { - auto persistent_settings = - std::make_unique(kPersistentSettingsJson); - persistent_settings->ValidatePersistentSettings(); - - persistent_settings->SetPersistentSetting("key", - std::make_unique(-1)); - EXPECT_EQ(-1, persistent_settings->GetPersistentSettingAsInt("key", 8)); - persistent_settings->SetPersistentSetting("key", - std::make_unique(0)); - EXPECT_EQ(0, persistent_settings->GetPersistentSettingAsInt("key", 8)); - persistent_settings->SetPersistentSetting("key", - std::make_unique(42)); - EXPECT_EQ(42, persistent_settings->GetPersistentSettingAsInt("key", 8)); -} - -TEST_F(PersistentSettingTest, GetDefaultDouble) { - auto persistent_settings = - std::make_unique(kPersistentSettingsJson); - persistent_settings->ValidatePersistentSettings(); - - // does not exist - ASSERT_EQ(-1.1, - persistent_settings->GetPersistentSettingAsDouble("key", -1.1)); - ASSERT_EQ(0.1, persistent_settings->GetPersistentSettingAsDouble("key", 0.1)); - ASSERT_EQ(42.1, - persistent_settings->GetPersistentSettingAsDouble("key", 42.1)); - - // exists but invalid - persistent_settings->SetPersistentSetting( - "key", std::make_unique(true)); - EXPECT_EQ(8.1, persistent_settings->GetPersistentSettingAsDouble("key", 8.1)); -} - -TEST_F(PersistentSettingTest, GetSetDouble) { - auto persistent_settings = - std::make_unique(kPersistentSettingsJson); - persistent_settings->ValidatePersistentSettings(); - - persistent_settings->SetPersistentSetting( - "key", std::make_unique(-1.1)); - EXPECT_EQ(-1.1, - persistent_settings->GetPersistentSettingAsDouble("key", 8.1)); - persistent_settings->SetPersistentSetting("key", - std::make_unique(0.1)); - EXPECT_EQ(0.1, persistent_settings->GetPersistentSettingAsDouble("key", 8.1)); - persistent_settings->SetPersistentSetting( - "key", std::make_unique(42.1)); - EXPECT_EQ(42.1, - persistent_settings->GetPersistentSettingAsDouble("key", 8.1)); -} - -TEST_F(PersistentSettingTest, GetDefaultString) { - auto persistent_settings = - std::make_unique(kPersistentSettingsJson); - persistent_settings->ValidatePersistentSettings(); - - // does not exist - ASSERT_EQ("", persistent_settings->GetPersistentSettingAsString("key", "")); - ASSERT_EQ("hello there", persistent_settings->GetPersistentSettingAsString( - "key", "hello there")); - ASSERT_EQ("42", - persistent_settings->GetPersistentSettingAsString("key", "42")); - ASSERT_EQ("\n", - persistent_settings->GetPersistentSettingAsString("key", "\n")); - ASSERT_EQ("\\n", - persistent_settings->GetPersistentSettingAsString("key", "\\n")); - - // exists but invalid - persistent_settings->SetPersistentSetting("key", - std::make_unique(4.2)); - EXPECT_EQ("hello", - persistent_settings->GetPersistentSettingAsString("key", "hello")); -} +TEST_F(PersistentSettingTest, PersistValidatedSettings) { + EXPECT_TRUE(persistent_settings_->Get("key").is_none()); + persistent_settings_->Set("key", base::Value(true)); + Fence(FROM_HERE); + EXPECT_EQ(true, persistent_settings_->Get("key").GetIfBool().value_or(false)); -TEST_F(PersistentSettingTest, GetSetString) { - auto persistent_settings = + persistent_settings_ = std::make_unique(kPersistentSettingsJson); - persistent_settings->ValidatePersistentSettings(); - - persistent_settings->SetPersistentSetting("key", - std::make_unique("")); - EXPECT_EQ("", - persistent_settings->GetPersistentSettingAsString("key", "hello")); - persistent_settings->SetPersistentSetting( - "key", std::make_unique("hello there")); - EXPECT_EQ("hello there", - persistent_settings->GetPersistentSettingAsString("key", "hello")); - persistent_settings->SetPersistentSetting( - "key", std::make_unique("42")); - EXPECT_EQ("42", - persistent_settings->GetPersistentSettingAsString("key", "hello")); - persistent_settings->SetPersistentSetting( - "key", std::make_unique("\n")); - EXPECT_EQ("\n", - persistent_settings->GetPersistentSettingAsString("key", "hello")); - persistent_settings->SetPersistentSetting( - "key", std::make_unique("\\n")); - EXPECT_EQ("\\n", - persistent_settings->GetPersistentSettingAsString("key", "hello")); -} - -TEST_F(PersistentSettingTest, GetSetList) { - auto persistent_settings = - std::make_unique(kPersistentSettingsJson); - persistent_settings->ValidatePersistentSettings(); - - base::Value list(base::Value::Type::LIST); - list.GetList().Append("hello"); - persistent_settings->SetPersistentSetting( - "key", base::Value::ToUniquePtrValue(list.Clone())); - auto test_list = persistent_settings->GetPersistentSettingAsList("key"); - EXPECT_FALSE(test_list.empty()); - EXPECT_EQ(1, test_list.size()); - EXPECT_TRUE(test_list[0].is_string()); - EXPECT_EQ("hello", test_list[0].GetString()); - - list.GetList().Append("there"); - persistent_settings->SetPersistentSetting( - "key", base::Value::ToUniquePtrValue(list.Clone())); - test_list = persistent_settings->GetPersistentSettingAsList("key"); - EXPECT_FALSE(test_list.empty()); - EXPECT_EQ(2, test_list.size()); - EXPECT_TRUE(test_list[0].is_string()); - EXPECT_EQ("hello", test_list[0].GetString()); - EXPECT_TRUE(test_list[1].is_string()); - EXPECT_EQ("there", test_list[1].GetString()); - - list.GetList().Append(42); - persistent_settings->SetPersistentSetting( - "key", base::Value::ToUniquePtrValue(list.Clone())); - test_list = persistent_settings->GetPersistentSettingAsList("key"); - EXPECT_FALSE(test_list.empty()); - EXPECT_EQ(3, test_list.size()); - EXPECT_TRUE(test_list[0].is_string()); - EXPECT_EQ("hello", test_list[0].GetString()); - EXPECT_TRUE(test_list[1].is_string()); - EXPECT_EQ("there", test_list[1].GetString()); - EXPECT_TRUE(test_list[2].is_int()); - EXPECT_EQ(42, test_list[2].GetInt()); -} - -TEST_F(PersistentSettingTest, GetSetDictionary) { - auto persistent_settings = - std::make_unique(kPersistentSettingsJson); - persistent_settings->ValidatePersistentSettings(); - - base::Value dict(base::Value::Type::DICT); - dict.GetDict().Set("key_string", "hello"); - persistent_settings->SetPersistentSetting( - "key", base::Value::ToUniquePtrValue(dict.Clone())); - auto test_dict = persistent_settings->GetPersistentSettingAsDictionary("key"); - EXPECT_FALSE(test_dict.empty()); - EXPECT_EQ(1, test_dict.size()); - EXPECT_TRUE(test_dict["key_string"]->is_string()); - EXPECT_EQ("hello", test_dict["key_string"]->GetString()); - - dict.GetDict().Set("key_int", 42); - persistent_settings->SetPersistentSetting( - "key", base::Value::ToUniquePtrValue(dict.Clone())); - test_dict = persistent_settings->GetPersistentSettingAsDictionary("key"); - EXPECT_FALSE(test_dict.empty()); - EXPECT_EQ(2, test_dict.size()); - EXPECT_TRUE(test_dict["key_string"]->is_string()); - EXPECT_EQ("hello", test_dict["key_string"]->GetString()); - EXPECT_TRUE(test_dict["key_int"]->is_int()); - EXPECT_EQ(42, test_dict["key_int"]->GetInt()); - - dict.GetDict().Set("http://127.0.0.1:45019/", "Dictionary URL Key Works!"); - persistent_settings->SetPersistentSetting( - "key", base::Value::ToUniquePtrValue(dict.Clone())); - test_dict = persistent_settings->GetPersistentSettingAsDictionary("key"); - EXPECT_FALSE(test_dict.empty()); - EXPECT_EQ(3, test_dict.size()); - EXPECT_TRUE(test_dict["key_string"]->is_string()); - EXPECT_EQ("hello", test_dict["key_string"]->GetString()); - EXPECT_TRUE(test_dict["key_int"]->is_int()); - EXPECT_EQ(42, test_dict["key_int"]->GetInt()); - EXPECT_TRUE(test_dict["http://127.0.0.1:45019/"]->is_string()); - EXPECT_EQ("Dictionary URL Key Works!", - test_dict["http://127.0.0.1:45019/"]->GetString()); -} - -TEST_F(PersistentSettingTest, RemoveSetting) { - auto persistent_settings = - std::make_unique(kPersistentSettingsJson); - persistent_settings->ValidatePersistentSettings(); - - ASSERT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); - ASSERT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false)); - persistent_settings->SetPersistentSetting( - "key", std::make_unique(true)); - EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", false)); - persistent_settings->RemovePersistentSetting("key"); - EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false)); -} - -TEST_F(PersistentSettingTest, DeleteSettings) { - auto persistent_settings = - std::make_unique(kPersistentSettingsJson); - persistent_settings->ValidatePersistentSettings(); - - ASSERT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); - ASSERT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false)); - persistent_settings->SetPersistentSetting( - "key", std::make_unique(true)); - EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", false)); - persistent_settings->DeletePersistentSettings(); - EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false)); -} - -TEST_F(PersistentSettingTest, InvalidSettings) { - auto persistent_settings = - std::make_unique(kPersistentSettingsJson); - persistent_settings->ValidatePersistentSettings(); - - ASSERT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); - ASSERT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false)); - persistent_settings->SetPersistentSetting( - "key", std::make_unique(true), true); - EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", false)); - - persistent_settings = - std::make_unique(kPersistentSettingsJson); - ASSERT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); - ASSERT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", false)); - persistent_settings->SetPersistentSetting( - "key", std::make_unique(false), true); - EXPECT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false)); - - persistent_settings = + persistent_settings_->Validate(); + EXPECT_TRUE(persistent_settings_->Get("key").is_none()); + persistent_settings_->Set("key", base::Value(true)); + Fence(FROM_HERE); + EXPECT_EQ(true, persistent_settings_->Get("key").GetIfBool().value_or(false)); + WaitForPendingFileWrite(); + + persistent_settings_ = std::make_unique(kPersistentSettingsJson); - persistent_settings->ValidatePersistentSettings(); - ASSERT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); - ASSERT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false)); - persistent_settings->SetPersistentSetting( - "key", std::make_unique(true)); - EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", false)); + EXPECT_EQ(true, persistent_settings_->Get("key").GetIfBool().value_or(false)); } } // namespace persistent_storage diff --git a/cobalt/watchdog/watchdog.cc b/cobalt/watchdog/watchdog.cc index 3846bbb0c47b..cce9b05cff6c 100644 --- a/cobalt/watchdog/watchdog.cc +++ b/cobalt/watchdog/watchdog.cc @@ -760,8 +760,9 @@ bool Watchdog::GetPersistentSettingWatchdogEnable() { if (!persistent_settings_) return kDefaultSettingWatchdogEnable; // Gets the boolean that controls whether or not Watchdog is enabled. - return persistent_settings_->GetPersistentSettingAsBool( - kPersistentSettingWatchdogEnable, kDefaultSettingWatchdogEnable); + return persistent_settings_->Get(kPersistentSettingWatchdogEnable) + .GetIfBool() + .value_or(kDefaultSettingWatchdogEnable); } void Watchdog::SetPersistentSettingWatchdogEnable(bool enable_watchdog) { @@ -769,9 +770,8 @@ void Watchdog::SetPersistentSettingWatchdogEnable(bool enable_watchdog) { if (!persistent_settings_) return; // Sets the boolean that controls whether or not Watchdog is enabled. - persistent_settings_->SetPersistentSetting( - kPersistentSettingWatchdogEnable, - std::make_unique(enable_watchdog)); + persistent_settings_->Set(kPersistentSettingWatchdogEnable, + base::Value(enable_watchdog)); } bool Watchdog::GetPersistentSettingWatchdogCrash() { @@ -779,8 +779,9 @@ bool Watchdog::GetPersistentSettingWatchdogCrash() { if (!persistent_settings_) return kDefaultSettingWatchdogCrash; // Gets the boolean that controls whether or not crashes can be triggered. - return persistent_settings_->GetPersistentSettingAsBool( - kPersistentSettingWatchdogCrash, kDefaultSettingWatchdogCrash); + return persistent_settings_->Get(kPersistentSettingWatchdogCrash) + .GetIfBool() + .value_or(kDefaultSettingWatchdogCrash); } void Watchdog::SetPersistentSettingWatchdogCrash(bool can_trigger_crash) { @@ -788,9 +789,8 @@ void Watchdog::SetPersistentSettingWatchdogCrash(bool can_trigger_crash) { if (!persistent_settings_) return; // Sets the boolean that controls whether or not crashes can be triggered. - persistent_settings_->SetPersistentSetting( - kPersistentSettingWatchdogCrash, - std::make_unique(can_trigger_crash)); + persistent_settings_->Set(kPersistentSettingWatchdogCrash, + base::Value(can_trigger_crash)); } bool Watchdog::LogEvent(const std::string& event) { @@ -821,17 +821,17 @@ bool Watchdog::GetPersistentSettingLogtraceEnable() { if (!persistent_settings_) return kDefaultSettingLogtraceEnable; // Gets the boolean that controls whether or not LogTrace is enabled. - return persistent_settings_->GetPersistentSettingAsBool( - kPersistentSettingLogtraceEnable, kDefaultSettingLogtraceEnable); + return persistent_settings_->Get(kPersistentSettingLogtraceEnable) + .GetIfBool() + .value_or(kDefaultSettingLogtraceEnable); } void Watchdog::SetPersistentSettingLogtraceEnable(bool enable_logtrace) { if (!persistent_settings_) return; // Sets the boolean that controls whether or not LogTrace is enabled. - persistent_settings_->SetPersistentSetting( - kPersistentSettingLogtraceEnable, - std::make_unique(enable_logtrace)); + persistent_settings_->Set(kPersistentSettingLogtraceEnable, + base::Value(enable_logtrace)); } #if defined(_DEBUG) diff --git a/cobalt/watchdog/watchdog_test.cc b/cobalt/watchdog/watchdog_test.cc index c5e93d5afb14..87d48e858003 100644 --- a/cobalt/watchdog/watchdog_test.cc +++ b/cobalt/watchdog/watchdog_test.cc @@ -99,6 +99,11 @@ class WatchdogTest : public testing::Test { starboard::SbFileDeleteRecursive(path.c_str(), true); } + void Fence(PersistentSettings* persistent_settings, + const base::Location& location) { + persistent_settings->Fence(location); + } + const std::string kSettingsFileName = "test-settings.json"; watchdog::Watchdog* watchdog_; @@ -258,61 +263,61 @@ TEST_F(WatchdogTest, ViolationsJsonShouldPersistAndBeValid) { absl::optional violations_map_optional = base::JSONReader::Read(json); ASSERT_TRUE(violations_map_optional.has_value()); - std::unique_ptr violations_map = - std::make_unique( - std::move(*violations_map_optional.value().GetIfDict())); + const base::Value::Dict* violations_map = + violations_map_optional->GetIfDict(); ASSERT_NE(violations_map, nullptr); - base::Value::Dict* violation_dict = violations_map->FindDict("test-name"); + const base::Value::Dict* violation_dict = + violations_map->FindDict("test-name"); ASSERT_NE(violation_dict, nullptr); - std::string* description = violation_dict->FindString("description"); + const std::string* description = violation_dict->FindString("description"); ASSERT_NE(description, nullptr); ASSERT_EQ(*description, "test-desc"); - base::Value::List* violations = violation_dict->FindList("violations"); + const base::Value::List* violations = violation_dict->FindList("violations"); ASSERT_NE(violations, nullptr); ASSERT_EQ(violations->size(), 1); - std::string* monitor_state = + const std::string* monitor_state = (*violations)[0].GetDict().FindString("monitorState"); ASSERT_NE(monitor_state, nullptr); ASSERT_EQ( *monitor_state, std::string(GetApplicationStateString(base::kApplicationStateStarted))); - base::Value::List* ping_infos = + const base::Value::List* ping_infos = (*violations)[0].GetDict().FindList("pingInfos"); ASSERT_NE(ping_infos, nullptr); ASSERT_EQ(ping_infos->size(), 1); - std::string* info = (*ping_infos)[0].GetDict().FindString("info"); + const std::string* info = (*ping_infos)[0].GetDict().FindString("info"); ASSERT_NE(info, nullptr); ASSERT_EQ(*info, "test-ping"); - std::string* timestamp_milliseconds = + const std::string* timestamp_milliseconds = (*ping_infos)[0].GetDict().FindString("timestampMilliseconds"); ASSERT_NE(timestamp_milliseconds, nullptr); std::stoll(*timestamp_milliseconds); - base::Value::List* registered_clients = + const base::Value::List* registered_clients = (*violations)[0].GetDict().FindList("registeredClients"); ASSERT_NE(registered_clients, nullptr); ASSERT_EQ(registered_clients->size(), 1); ASSERT_EQ((*registered_clients)[0].GetString(), "test-name"); - std::string* time_interval_milliseconds = + const std::string* time_interval_milliseconds = (*violations)[0].GetDict().FindString("timeIntervalMilliseconds"); ASSERT_NE(time_interval_milliseconds, nullptr); std::stoll(*time_interval_milliseconds); - std::string* time_wait_milliseconds = + const std::string* time_wait_milliseconds = (*violations)[0].GetDict().FindString("timeWaitMilliseconds"); ASSERT_NE(time_wait_milliseconds, nullptr); std::stoll(*time_wait_milliseconds); - std::string* timestamp_last_pinged_milliseconds = + const std::string* timestamp_last_pinged_milliseconds = (*violations)[0].GetDict().FindString("timestampLastPingedMilliseconds"); ASSERT_NE(timestamp_last_pinged_milliseconds, nullptr); std::stoll(*timestamp_last_pinged_milliseconds); - std::string* timestamp_registered_milliseconds = + const std::string* timestamp_registered_milliseconds = (*violations)[0].GetDict().FindString("timestampRegisteredMilliseconds"); ASSERT_NE(timestamp_registered_milliseconds, nullptr); std::stoll(*timestamp_registered_milliseconds); - std::string* timestamp_violation_milliseconds = + const std::string* timestamp_violation_milliseconds = (*violations)[0].GetDict().FindString("timestampViolationMilliseconds"); ASSERT_NE(timestamp_violation_milliseconds, nullptr); std::stoll(*timestamp_violation_milliseconds); - std::string* violation_duration_milliseconds = + const std::string* violation_duration_milliseconds = (*violations)[0].GetDict().FindString("violationDurationMilliseconds"); ASSERT_NE(violation_duration_milliseconds, nullptr); std::stoll(*violation_duration_milliseconds); @@ -751,13 +756,13 @@ TEST_F(WatchdogTest, WatchdogMethodsAreNoopWhenWatchdogIsDisabled) { // PersistentSettings doesn't have interface so it's not mockable auto persistent_settings = std::make_unique(kSettingsFileName); - persistent_settings->ValidatePersistentSettings(); - persistent_settings->SetPersistentSetting( - kPersistentSettingWatchdogEnable, std::make_unique(false), - true); - ASSERT_FALSE(persistent_settings->GetPersistentSettingAsBool( - kPersistentSettingWatchdogEnable, true)); + persistent_settings->Set(kPersistentSettingWatchdogEnable, + base::Value(false)); + Fence(persistent_settings.get(), FROM_HERE); + ASSERT_FALSE(persistent_settings->Get(kPersistentSettingWatchdogEnable) + .GetIfBool() + .value_or(true)); watchdog_ = new watchdog::Watchdog(); watchdog_->InitializeCustom(persistent_settings.get(), @@ -786,13 +791,13 @@ TEST_F(WatchdogTest, LogtraceMethodsAreNoopWhenLogtraceIsDisabled) { // PersistentSettings doesn't have interface so it's not mockable auto persistent_settings = std::make_unique(kSettingsFileName); - persistent_settings->ValidatePersistentSettings(); - persistent_settings->SetPersistentSetting( - kPersistentSettingLogtraceEnable, std::make_unique(false), - true); - ASSERT_FALSE(persistent_settings->GetPersistentSettingAsBool( - kPersistentSettingLogtraceEnable, true)); + persistent_settings->Set(kPersistentSettingLogtraceEnable, + base::Value(false)); + Fence(persistent_settings.get(), FROM_HERE); + ASSERT_FALSE(persistent_settings->Get(kPersistentSettingLogtraceEnable) + .GetIfBool() + .value_or(true)); watchdog_ = new watchdog::Watchdog(); watchdog_->InitializeCustom(persistent_settings.get(), diff --git a/cobalt/worker/service_worker_persistent_settings.cc b/cobalt/worker/service_worker_persistent_settings.cc index 0a2112a3a65a..70fa71c05525 100644 --- a/cobalt/worker/service_worker_persistent_settings.cc +++ b/cobalt/worker/service_worker_persistent_settings.cc @@ -66,22 +66,6 @@ const char kSettingsSetOfUsedScriptsKey[] = "set_of_used_scripts"; const char kSettingsSkipWaitingKey[] = "skip_waiting"; const char kSettingsClassicScriptsImportedKey[] = "classic_scripts_imported"; const char kSettingsRawHeadersKey[] = "raw_headers"; - -bool CheckPersistentValue( - std::string key_string, std::string settings_key, - base::flat_map>& dict, - base::Value::Type type) { - if (!dict.contains(settings_key)) { - DLOG(INFO) << "Key: " << key_string << " does not contain " << settings_key; - return false; - } else if (!(dict[settings_key]->type() == type)) { - DLOG(INFO) << "Key: " << key_string << " " << settings_key - << " is of type: " << dict[settings_key]->type() - << ", but expected type is: " << type; - return false; - } - return true; -} } // namespace ServiceWorkerPersistentSettings::ServiceWorkerPersistentSettings( @@ -89,7 +73,7 @@ ServiceWorkerPersistentSettings::ServiceWorkerPersistentSettings( : options_(options) { persistent_settings_.reset(new cobalt::persistent_storage::PersistentSettings( WorkerConsts::kSettingsJson)); - persistent_settings_->ValidatePersistentSettings(); + persistent_settings_->Validate(); DCHECK(persistent_settings_); } @@ -97,69 +81,60 @@ void ServiceWorkerPersistentSettings::ReadServiceWorkerRegistrationMapSettings( std::map>& registration_map) { - std::vector key_list = - persistent_settings_->GetPersistentSettingAsList(kSettingsKeyList); + base::Value local_key_list_value = + persistent_settings_->Get(kSettingsKeyList); + const base::Value::List* key_list = local_key_list_value.GetIfList(); + if (!key_list || key_list->empty()) return; std::set unverified_key_set; - for (auto& key : key_list) { + for (auto& key : *key_list) { if (key.is_string()) { unverified_key_set.insert(key.GetString()); } } for (auto& key_string : unverified_key_set) { - auto dict = - persistent_settings_->GetPersistentSettingAsDictionary(key_string); - if (dict.empty()) { + base::Value local_dict_value = persistent_settings_->Get(key_string); + const base::Value::Dict* dict = local_dict_value.GetIfDict(); + if (!dict || dict->empty()) { DLOG(INFO) << "Key: " << key_string << " does not exist in " << WorkerConsts::kSettingsJson; continue; } - if (!CheckPersistentValue(key_string, kSettingsStorageKeyKey, dict, - base::Value::Type::STRING)) - continue; - url::Origin storage_key = - url::Origin::Create(GURL(dict[kSettingsStorageKeyKey]->GetString())); + auto storage_key = dict->FindString(kSettingsStorageKeyKey); + if (!storage_key) continue; + url::Origin storage_key_origin = url::Origin::Create(GURL(*storage_key)); - if (!CheckPersistentValue(key_string, kSettingsScopeUrlKey, dict, - base::Value::Type::STRING)) - continue; - GURL scope(dict[kSettingsScopeUrlKey]->GetString()); + auto scope_url = dict->FindString(kSettingsScopeUrlKey); + if (!scope_url) continue; + GURL scope(*scope_url); - if (!CheckPersistentValue(key_string, kSettingsUpdateViaCacheModeKey, dict, - base::Value::Type::INTEGER)) - continue; + auto update_via_cache_mode = dict->FindInt(kSettingsUpdateViaCacheModeKey); + if (!update_via_cache_mode) continue; ServiceWorkerUpdateViaCache update_via_cache = - static_cast( - dict[kSettingsUpdateViaCacheModeKey]->GetInt()); + static_cast(*update_via_cache_mode); - if (!CheckPersistentValue(key_string, kSettingsScopeStringKey, dict, - base::Value::Type::STRING)) - continue; - std::string scope_string(dict[kSettingsScopeStringKey]->GetString()); + auto scope_string = dict->FindString(kSettingsScopeStringKey); + if (!scope_string) continue; - RegistrationMapKey key(storage_key, scope_string); + RegistrationMapKey key(storage_key_origin, *scope_string); scoped_refptr registration( - new ServiceWorkerRegistrationObject(storage_key, scope, + new ServiceWorkerRegistrationObject(storage_key_origin, scope, update_via_cache)); - auto worker_key = kSettingsWaitingWorkerKey; - if (!CheckPersistentValue(key_string, worker_key, dict, - base::Value::Type::DICT)) { - worker_key = kSettingsActiveWorkerKey; - if (!CheckPersistentValue(key_string, worker_key, dict, - base::Value::Type::DICT)) - continue; + const base::Value::Dict* worker = dict->FindDict(kSettingsWaitingWorkerKey); + if (!worker) { + worker = dict->FindDict(kSettingsActiveWorkerKey); + if (!worker) continue; } - if (!ReadServiceWorkerObjectSettings( - registration, key_string, std::move(dict[worker_key]), worker_key)) + if (!ReadServiceWorkerObjectSettings(registration, key_string, *worker)) continue; - if (CheckPersistentValue(key_string, kSettingsLastUpdateCheckTimeKey, dict, - base::Value::Type::STRING)) { - int64_t last_update_check_time = - std::atol(dict[kSettingsLastUpdateCheckTimeKey]->GetString().c_str()); + auto last_update_check_time = + dict->FindString(kSettingsLastUpdateCheckTimeKey); + if (last_update_check_time) { registration->set_last_update_check_time( base::Time::FromDeltaSinceWindowsEpoch( - base::TimeDelta::FromMicroseconds(last_update_check_time))); + base::TimeDelta::FromMicroseconds( + std::atol(last_update_check_time->c_str())))); } key_set_.insert(key_string); @@ -173,7 +148,7 @@ void ServiceWorkerPersistentSettings::ReadServiceWorkerRegistrationMapSettings( registration)); auto job = options_.service_worker_context->jobs()->CreateJobWithoutPromise( - ServiceWorkerJobs::JobType::kUpdate, storage_key, scope, + ServiceWorkerJobs::JobType::kUpdate, storage_key_origin, scope, registration->waiting_worker()->script_url()); options_.service_worker_context->task_runner()->PostTask( FROM_HERE, base::BindOnce(&ServiceWorkerJobs::ScheduleJob, @@ -185,11 +160,10 @@ void ServiceWorkerPersistentSettings::ReadServiceWorkerRegistrationMapSettings( bool ServiceWorkerPersistentSettings::ReadServiceWorkerObjectSettings( scoped_refptr registration, - std::string key_string, std::unique_ptr value_dict, - std::string worker_key_string) { - std::string* options_name_value = - value_dict->FindStringKey(kSettingsOptionsNameKey); - if (options_name_value == nullptr) return false; + std::string key_string, const base::Value::Dict& value_dict) { + const std::string* options_name_value = + value_dict.FindString(kSettingsOptionsNameKey); + if (!options_name_value) return false; ServiceWorkerObject::Options options(*options_name_value, options_.web_settings, options_.network_module, registration); @@ -197,42 +171,37 @@ bool ServiceWorkerPersistentSettings::ReadServiceWorkerObjectSettings( options.web_options.service_worker_context = options_.service_worker_context; scoped_refptr worker(new ServiceWorkerObject(options)); - std::string* script_url_value = - value_dict->FindStringKey(kSettingsScriptUrlKey); - if (script_url_value == nullptr) return false; + const std::string* script_url_value = + value_dict.FindString(kSettingsScriptUrlKey); + if (!script_url_value) return false; worker->set_script_url(GURL(*script_url_value)); absl::optional skip_waiting_value = - value_dict->FindBoolKey(kSettingsSkipWaitingKey); - if (!skip_waiting_value.has_value()) return false; + value_dict.FindBool(kSettingsSkipWaitingKey); + if (!skip_waiting_value) return false; if (skip_waiting_value.value()) worker->set_skip_waiting(); absl::optional classic_scripts_imported_value = - value_dict->FindBoolKey(kSettingsClassicScriptsImportedKey); - if (!classic_scripts_imported_value.has_value()) return false; + value_dict.FindBool(kSettingsClassicScriptsImportedKey); + if (!classic_scripts_imported_value) return false; if (classic_scripts_imported_value.value()) worker->set_classic_scripts_imported(); worker->set_start_status(nullptr); - base::Value* used_scripts_value = - value_dict->FindListKey(kSettingsSetOfUsedScriptsKey); - if (used_scripts_value == nullptr) return false; - base::Value::List used_scripts_list = - std::move(*used_scripts_value).TakeList(); - for (int i = 0; i < used_scripts_list.size(); i++) { - auto script_value = std::move(used_scripts_list[i]); + const base::Value::List* used_scripts_list = + value_dict.FindList(kSettingsSetOfUsedScriptsKey); + if (!used_scripts_list) return false; + for (const auto& script_value : *used_scripts_list) { if (script_value.is_string()) { worker->AppendToSetOfUsedScripts(GURL(script_value.GetString())); } } - base::Value* script_urls_value = - value_dict->FindListKey(kSettingsScriptResourceMapScriptUrlsKey); - if (script_urls_value == nullptr) return false; - base::Value::List script_urls_list = std::move(*script_urls_value).TakeList(); + const base::Value::List* script_urls_list = + value_dict.FindList(kSettingsScriptResourceMapScriptUrlsKey); + if (!script_urls_list) return false; ScriptResourceMap script_resource_map; - for (int i = 0; i < script_urls_list.size(); i++) { - auto script_url_value = std::move(script_urls_list[i]); + for (const auto& script_url_value : *script_urls_list) { if (script_url_value.is_string()) { auto script_url_string = script_url_value.GetString(); auto script_url = GURL(script_url_string); @@ -248,9 +217,9 @@ bool ServiceWorkerPersistentSettings::ReadServiceWorkerObjectSettings( if (script_url == worker->script_url()) { // Get the persistent headers for the ServiceWorkerObject script_url_. // This is used in ServiceWorkerObject::Initialize(). - std::string* raw_header_value = - value_dict->FindStringKey(kSettingsRawHeadersKey); - if (raw_header_value == nullptr) return false; + const std::string* raw_header_value = + value_dict.FindString(kSettingsRawHeadersKey); + if (!raw_header_value) return false; const scoped_refptr headers = scoped_refptr( new net::HttpResponseHeaders(*raw_header_value)); @@ -306,8 +275,7 @@ void ServiceWorkerPersistentSettings:: for (auto& key : key_set_) { key_list.Append(key); } - persistent_settings_->SetPersistentSetting( - kSettingsKeyList, std::make_unique(std::move(key_list))); + persistent_settings_->Set(kSettingsKeyList, base::Value(std::move(key_list))); // Persist ServiceWorkerRegistrationObject's fields. dict.Set(kSettingsStorageKeyKey, registration->storage_key().GetURL().spec()); @@ -324,8 +292,7 @@ void ServiceWorkerPersistentSettings:: .ToDeltaSinceWindowsEpoch() .InMicroseconds())); - persistent_settings_->SetPersistentSetting( - key_string, std::make_unique(std::move(dict))); + persistent_settings_->Set(key_string, base::Value(std::move(dict))); } base::Value::Dict @@ -396,38 +363,32 @@ void ServiceWorkerPersistentSettings:: for (auto& key : key_set_) { key_list.Append(key); } - persistent_settings_->SetPersistentSetting( - kSettingsKeyList, std::make_unique(std::move(key_list))); + persistent_settings_->Set(kSettingsKeyList, base::Value(std::move(key_list))); // Remove the registration dictionary. - persistent_settings_->RemovePersistentSetting(key_string); + persistent_settings_->Remove(key_string); } void ServiceWorkerPersistentSettings::RemoveServiceWorkerObjectSettings( std::string key_string) { - auto dict = - persistent_settings_->GetPersistentSettingAsDictionary(key_string); - if (dict.empty()) return; + base::Value local_dict_value = persistent_settings_->Get(key_string); + const base::Value::Dict* dict = local_dict_value.GetIfDict(); + if (!dict || dict->empty()) return; std::vector worker_keys{kSettingsWaitingWorkerKey, kSettingsActiveWorkerKey}; for (std::string worker_key : worker_keys) { - if (!CheckPersistentValue(key_string, worker_key, dict, - base::Value::Type::DICT)) - continue; - auto worker_dict = std::move(dict[worker_key]); - base::Value* script_urls_value = - worker_dict->FindListKey(kSettingsScriptResourceMapScriptUrlsKey); - if (script_urls_value == nullptr) return; - base::Value::List script_urls_list = - std::move(*script_urls_value).TakeList(); - - for (int i = 0; i < script_urls_list.size(); i++) { - auto script_url_value = std::move(script_urls_list[i]); - if (script_url_value.is_string()) { - auto script_url_string = script_url_value.GetString(); + const base::Value::Dict* worker = dict->FindDict(worker_key); + if (!worker) continue; + const base::Value::List* script_urls = + worker->FindList(kSettingsScriptResourceMapScriptUrlsKey); + if (!script_urls) return; + + for (const auto& script_url : *script_urls) { + auto script_url_string = script_url.GetIfString(); + if (script_url_string) { cobalt::cache::Cache::GetInstance()->Delete( network::disk_cache::ResourceType::kServiceWorkerScript, - web::cache_utils::GetKey(key_string + script_url_string)); + web::cache_utils::GetKey(key_string + *script_url_string)); } } } @@ -435,12 +396,12 @@ void ServiceWorkerPersistentSettings::RemoveServiceWorkerObjectSettings( void ServiceWorkerPersistentSettings::RemoveAll() { for (auto& key : key_set_) { - persistent_settings_->RemovePersistentSetting(key); + persistent_settings_->Remove(key); } } void ServiceWorkerPersistentSettings::DeleteAll() { - persistent_settings_->DeletePersistentSettings(); + persistent_settings_->RemoveAll(); } } // namespace worker diff --git a/cobalt/worker/service_worker_persistent_settings.h b/cobalt/worker/service_worker_persistent_settings.h index 90b150934a9b..5eab1be87f86 100644 --- a/cobalt/worker/service_worker_persistent_settings.h +++ b/cobalt/worker/service_worker_persistent_settings.h @@ -68,8 +68,7 @@ class ServiceWorkerPersistentSettings { bool ReadServiceWorkerObjectSettings( scoped_refptr registration, - std::string key_string, std::unique_ptr value_dict, - std::string worker_type_string); + std::string key_string, const base::Value::Dict& value_dict); void WriteServiceWorkerRegistrationObjectSettings( RegistrationMapKey key, @@ -81,13 +80,13 @@ class ServiceWorkerPersistentSettings { void RemoveServiceWorkerRegistrationObjectSettings(RegistrationMapKey key); - void RemoveServiceWorkerObjectSettings(std::string key_string); - void RemoveAll(); void DeleteAll(); private: + void RemoveServiceWorkerObjectSettings(std::string key_string); + Options options_; std::unique_ptr