From 8300933170d9e409f2eb2a0cedacc91bcc44dac0 Mon Sep 17 00:00:00 2001 From: Brian Ting Date: Thu, 25 Apr 2024 19:07:11 -0700 Subject: [PATCH] Address potential Persistent Settings Getters race. While Getters and SetterHelpers both use locks to prevent race conditions, Setters do not. Given that Setters run SetterHelpers through posted tasks, it is possible for Getters to run in-between Setters and SetterHelpers causing a race condition. To resolve this, effectively add Getters into the PostTask Queue by calling PostBlockingTask. Also refactor GetValues calls so that they are run by Setters rather than Getters to limit the number of deep copies made. Now that Getters and Setters no longer race, massively clean up the tests by removing the unwieldly and now unneeded closures. b/305057554 --- .../persistent_storage/persistent_settings.cc | 111 ++-- .../persistent_storage/persistent_settings.h | 35 +- .../persistent_settings_test.cc | 581 +++++------------- cobalt/watchdog/watchdog_test.cc | 14 +- .../service_worker_persistent_settings.cc | 4 +- .../service_worker_persistent_settings.h | 2 +- .../worker/service_worker_registration_map.cc | 8 +- 7 files changed, 232 insertions(+), 523 deletions(-) diff --git a/cobalt/persistent_storage/persistent_settings.cc b/cobalt/persistent_storage/persistent_settings.cc index 4fefafbe0b7e..9aaa73f5610e 100644 --- a/cobalt/persistent_storage/persistent_settings.cc +++ b/cobalt/persistent_storage/persistent_settings.cc @@ -76,6 +76,7 @@ void PersistentSettings::InitializePrefStore() { pref_store_ = base::MakeRefCounted( base::FilePath(persistent_settings_file_)); pref_store_->ReadPrefs(); + persistent_settings_ = pref_store_->GetValues(); pref_store_initialized_.Signal(); } // Remove settings file and do not allow writes to the file until the @@ -102,42 +103,47 @@ void PersistentSettings::ValidatePersistentSettingsHelper(bool blocking) { bool PersistentSettings::GetPersistentSettingAsBool(const std::string& key, bool default_setting) { + // Wait for all previously posted tasks to finish. + PostBlockingTask(); base::AutoLock auto_lock(pref_store_lock_); - auto persistent_settings = pref_store_->GetValues(); - absl::optional result = persistent_settings.FindBool(key); + 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. + PostBlockingTask(); base::AutoLock auto_lock(pref_store_lock_); - auto persistent_settings = pref_store_->GetValues(); - absl::optional result = persistent_settings.FindInt(key); + 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. + PostBlockingTask(); base::AutoLock auto_lock(pref_store_lock_); - auto persistent_settings = pref_store_->GetValues(); - absl::optional result = persistent_settings.FindDouble(key); + 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. + PostBlockingTask(); base::AutoLock auto_lock(pref_store_lock_); - auto persistent_settings = pref_store_->GetValues(); - const std::string* result = persistent_settings.FindString(key); + 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. + PostBlockingTask(); base::AutoLock auto_lock(pref_store_lock_); - auto persistent_settings = pref_store_->GetValues(); - const base::Value::List* result = persistent_settings.FindList(key); + const base::Value::List* result = persistent_settings_.FindList(key); std::vector values; if (result) { for (const auto& value : *result) { @@ -149,9 +155,10 @@ std::vector PersistentSettings::GetPersistentSettingAsList( base::flat_map> PersistentSettings::GetPersistentSettingAsDictionary(const std::string& key) { + // Wait for all previously posted tasks to finish. + PostBlockingTask(); base::AutoLock auto_lock(pref_store_lock_); - auto persistent_settings = pref_store_->GetValues(); - base::Value::Dict* result = persistent_settings.FindDict(key); + 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(); @@ -164,70 +171,74 @@ PersistentSettings::GetPersistentSettingAsDictionary(const std::string& key) { return dict; } +void PersistentSettings::PostBlockingTask() { + base::WaitableEvent task_finished( + base::WaitableEvent::ResetPolicy::MANUAL, + base::WaitableEvent::InitialState::NOT_SIGNALED); + task_runner()->PostTask( + FROM_HERE, base::BindOnce(&PersistentSettings::PostBlockingTaskHelper, + base::Unretained(this), &task_finished)); + task_finished.Wait(); +} + +void PersistentSettings::PostBlockingTaskHelper( + base::WaitableEvent* task_finished) { + DCHECK_EQ(base::SequencedTaskRunner::GetCurrentDefault(), task_runner()); + task_finished->Signal(); +} + void PersistentSettings::SetPersistentSetting( - const std::string& key, std::unique_ptr value, - base::OnceClosure closure, bool blocking) { + 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), - std::move(closure), blocking)); + 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, - base::OnceClosure closure, bool blocking) { + const std::string& key, std::unique_ptr value, bool blocking) { DCHECK_EQ(base::SequencedTaskRunner::GetCurrentDefault(), task_runner()); - { - base::AutoLock auto_lock(pref_store_lock_); - pref_store_->SetValue(key, - base::Value::FromUniquePtrValue(std::move(value)), - WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); - if (validated_initial_settings_) { - CommitPendingWrite(blocking); - } + 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(); + if (validated_initial_settings_) { + CommitPendingWrite(blocking); } - std::move(closure).Run(); } void PersistentSettings::RemovePersistentSetting(const std::string& key, - base::OnceClosure closure, bool blocking) { task_runner()->PostTask( FROM_HERE, base::BindOnce(&PersistentSettings::RemovePersistentSettingHelper, - base::Unretained(this), key, std::move(closure), - blocking)); + base::Unretained(this), key, blocking)); } -void PersistentSettings::RemovePersistentSettingHelper( - const std::string& key, base::OnceClosure closure, bool 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); - if (validated_initial_settings_) { - CommitPendingWrite(blocking); - } + 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); } - std::move(closure).Run(); } -void PersistentSettings::DeletePersistentSettings(base::OnceClosure closure) { +void PersistentSettings::DeletePersistentSettings() { task_runner()->PostTask( FROM_HERE, base::BindOnce(&PersistentSettings::DeletePersistentSettingsHelper, - base::Unretained(this), std::move(closure))); + base::Unretained(this))); } -void PersistentSettings::DeletePersistentSettingsHelper( - base::OnceClosure closure) { +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(); - } - std::move(closure).Run(); + 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::CommitPendingWrite(bool blocking) { diff --git a/cobalt/persistent_storage/persistent_settings.h b/cobalt/persistent_storage/persistent_settings.h index 30f3b6be2ffe..6b869d03b751 100644 --- a/cobalt/persistent_storage/persistent_settings.h +++ b/cobalt/persistent_storage/persistent_settings.h @@ -72,18 +72,13 @@ class PersistentSettings : public base::CurrentThread::DestructionObserver { base::flat_map> GetPersistentSettingAsDictionary(const std::string& key); - void SetPersistentSetting( - const std::string& key, std::unique_ptr value, - base::OnceClosure closure = std::move(base::DoNothing()), - bool blocking = false); + void SetPersistentSetting(const std::string& key, + std::unique_ptr value, + bool blocking = false); - void RemovePersistentSetting( - const std::string& key, - base::OnceClosure closure = std::move(base::DoNothing()), - bool blocking = false); + void RemovePersistentSetting(const std::string& key, bool blocking = false); - void DeletePersistentSettings( - base::OnceClosure closure = std::move(base::DoNothing())); + void DeletePersistentSettings(); private: // Called by the constructor to initialize pref_store_ from @@ -92,14 +87,17 @@ class PersistentSettings : public base::CurrentThread::DestructionObserver { void ValidatePersistentSettingsHelper(bool blocking); + void PostBlockingTask(); + + void PostBlockingTaskHelper(base::WaitableEvent* task_finished); + void SetPersistentSettingHelper(const std::string& key, std::unique_ptr value, - base::OnceClosure closure, bool blocking); + bool blocking); - void RemovePersistentSettingHelper(const std::string& key, - base::OnceClosure closure, bool blocking); + void RemovePersistentSettingHelper(const std::string& key, bool blocking); - void DeletePersistentSettingsHelper(base::OnceClosure closure); + void DeletePersistentSettingsHelper(); void CommitPendingWrite(bool blocking); @@ -109,14 +107,17 @@ class PersistentSettings : public base::CurrentThread::DestructionObserver { // PrefStore used for persistent settings. scoped_refptr pref_store_; - // Flag indicating whether or not initial persistent settings have been - // validated. - bool validated_initial_settings_; + // Persistent settings dictionary. + base::Value::Dict persistent_settings_; // The thread created and owned by PersistentSettings. All pref_store_ // methods must be called from this thread. base::Thread thread_; + // 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_ = { diff --git a/cobalt/persistent_storage/persistent_settings_test.cc b/cobalt/persistent_storage/persistent_settings_test.cc index 9da560444661..d653c9233f7e 100644 --- a/cobalt/persistent_storage/persistent_settings_test.cc +++ b/cobalt/persistent_storage/persistent_settings_test.cc @@ -17,9 +17,6 @@ #include #include -#include "base/bind.h" -#include "base/functional/callback_forward.h" -#include "base/synchronization/waitable_event.h" #include "base/test/task_environment.h" #include "base/values.h" #include "starboard/common/file.h" @@ -45,26 +42,20 @@ class PersistentSettingTest : public testing::Test { std::vector storage_dir(kSbFileMaxPath + 1, 0); SbSystemGetPath(kSbSystemPathCacheDirectory, storage_dir.data(), kSbFileMaxPath); - persistent_settings_file_ = std::string(storage_dir.data()) + kSbFileSepString + kPersistentSettingsJson; } void SetUp() final { - test_done_.Reset(); starboard::SbFileDeleteRecursive(persistent_settings_file_.c_str(), true); } void TearDown() final { - test_done_.Reset(); starboard::SbFileDeleteRecursive(persistent_settings_file_.c_str(), true); } base::test::TaskEnvironment scoped_task_environment_; std::string persistent_settings_file_; - base::WaitableEvent test_done_ = { - base::WaitableEvent::ResetPolicy::MANUAL, - base::WaitableEvent::InitialState::NOT_SIGNALED}; }; TEST_F(PersistentSettingTest, GetDefaultBool) { @@ -77,17 +68,9 @@ TEST_F(PersistentSettingTest, GetDefaultBool) { ASSERT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false)); // exists but invalid - base::OnceClosure closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { test_done->Signal(); }, - persistent_settings.get(), &test_done_); - persistent_settings->SetPersistentSetting( - "key", std::make_unique(4.2), std::move(closure), true); - test_done_.Wait(); + persistent_settings->SetPersistentSetting("key", + std::make_unique(4.2)); EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_EQ(-1, persistent_settings->GetPersistentSettingAsInt("key", -1)); - EXPECT_EQ(4.2, - persistent_settings->GetPersistentSettingAsDouble("key", -1.0)); } TEST_F(PersistentSettingTest, GetSetBool) { @@ -95,37 +78,14 @@ TEST_F(PersistentSettingTest, GetSetBool) { std::make_unique(kPersistentSettingsJson); persistent_settings->ValidatePersistentSettings(); - base::OnceClosure closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_TRUE( - persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_TRUE( - persistent_settings->GetPersistentSettingAsBool("key", false)); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); - persistent_settings->SetPersistentSetting( - "key", std::make_unique(true), std::move(closure), true); - test_done_.Wait(); - test_done_.Reset(); - - closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_FALSE( - persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_FALSE( - persistent_settings->GetPersistentSettingAsBool("key", false)); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); - + "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), std::move(closure), true); - - test_done_.Wait(); + "key", std::make_unique(false)); + EXPECT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", true)); + EXPECT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false)); } TEST_F(PersistentSettingTest, GetDefaultInt) { @@ -139,17 +99,9 @@ TEST_F(PersistentSettingTest, GetDefaultInt) { ASSERT_EQ(42, persistent_settings->GetPersistentSettingAsInt("key", 42)); // exists but invalid - base::OnceClosure closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_EQ(8, persistent_settings->GetPersistentSettingAsInt("key", 8)); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); - - persistent_settings->SetPersistentSetting( - "key", std::make_unique(4.2), std::move(closure), true); - test_done_.Wait(); + persistent_settings->SetPersistentSetting("key", + std::make_unique(4.2)); + EXPECT_EQ(8, persistent_settings->GetPersistentSettingAsInt("key", 8)); } TEST_F(PersistentSettingTest, GetSetInt) { @@ -157,40 +109,51 @@ TEST_F(PersistentSettingTest, GetSetInt) { std::make_unique(kPersistentSettingsJson); persistent_settings->ValidatePersistentSettings(); - base::OnceClosure closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_EQ(-1, persistent_settings->GetPersistentSettingAsInt("key", 8)); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); + 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(-1), std::move(closure), true); - test_done_.Wait(); - test_done_.Reset(); - - closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_EQ(0, persistent_settings->GetPersistentSettingAsInt("key", 8)); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); + "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(0), std::move(closure), true); - test_done_.Wait(); - test_done_.Reset(); - - closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_EQ(42, persistent_settings->GetPersistentSettingAsInt("key", 8)); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); + "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), std::move(closure), true); - test_done_.Wait(); + "key", std::make_unique(42.1)); + EXPECT_EQ(42.1, + persistent_settings->GetPersistentSettingAsDouble("key", 8.1)); } TEST_F(PersistentSettingTest, GetDefaultString) { @@ -210,17 +173,10 @@ TEST_F(PersistentSettingTest, GetDefaultString) { persistent_settings->GetPersistentSettingAsString("key", "\\n")); // exists but invalid - base::OnceClosure closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_EQ("hello", persistent_settings->GetPersistentSettingAsString( - "key", "hello")); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); - persistent_settings->SetPersistentSetting( - "key", std::make_unique(4.2), std::move(closure), true); - test_done_.Wait(); + persistent_settings->SetPersistentSetting("key", + std::make_unique(4.2)); + EXPECT_EQ("hello", + persistent_settings->GetPersistentSettingAsString("key", "hello")); } TEST_F(PersistentSettingTest, GetSetString) { @@ -228,74 +184,26 @@ TEST_F(PersistentSettingTest, GetSetString) { std::make_unique(kPersistentSettingsJson); persistent_settings->ValidatePersistentSettings(); - base::OnceClosure closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_EQ("", persistent_settings->GetPersistentSettingAsString( - "key", "hello")); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); - - persistent_settings->SetPersistentSetting( - "key", std::make_unique(""), std::move(closure), true); - test_done_.Wait(); - test_done_.Reset(); - - closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_EQ( - "hello there", + persistent_settings->SetPersistentSetting("key", + std::make_unique("")); + EXPECT_EQ("", persistent_settings->GetPersistentSettingAsString("key", "hello")); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); - persistent_settings->SetPersistentSetting( - "key", std::make_unique("hello there"), std::move(closure), - true); - test_done_.Wait(); - test_done_.Reset(); - - closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_EQ("42", persistent_settings->GetPersistentSettingAsString( - "key", "hello")); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); + "key", std::make_unique("hello there")); + EXPECT_EQ("hello there", + persistent_settings->GetPersistentSettingAsString("key", "hello")); persistent_settings->SetPersistentSetting( - "key", std::make_unique("42"), std::move(closure), true); - test_done_.Wait(); - test_done_.Reset(); - - closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_EQ("\n", persistent_settings->GetPersistentSettingAsString( - "key", "hello")); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); + "key", std::make_unique("42")); + EXPECT_EQ("42", + persistent_settings->GetPersistentSettingAsString("key", "hello")); persistent_settings->SetPersistentSetting( - "key", std::make_unique("\n"), std::move(closure), true); - test_done_.Wait(); - test_done_.Reset(); - - closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_EQ("\\n", persistent_settings->GetPersistentSettingAsString( - "key", "hello")); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); + "key", std::make_unique("\n")); + EXPECT_EQ("\n", + persistent_settings->GetPersistentSettingAsString("key", "hello")); persistent_settings->SetPersistentSetting( - "key", std::make_unique("\\n"), std::move(closure), true); - test_done_.Wait(); - test_done_.Reset(); + "key", std::make_unique("\\n")); + EXPECT_EQ("\\n", + persistent_settings->GetPersistentSettingAsString("key", "hello")); } TEST_F(PersistentSettingTest, GetSetList) { @@ -303,69 +211,39 @@ TEST_F(PersistentSettingTest, GetSetList) { std::make_unique(kPersistentSettingsJson); persistent_settings->ValidatePersistentSettings(); - base::OnceClosure closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - 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()); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); - base::Value list(base::Value::Type::LIST); list.GetList().Append("hello"); persistent_settings->SetPersistentSetting( - "key", base::Value::ToUniquePtrValue(list.Clone()), std::move(closure), - true); - test_done_.Wait(); - test_done_.Reset(); - - closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - auto 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()); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); + "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()), std::move(closure), - true); - test_done_.Wait(); - test_done_.Reset(); - - closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - auto 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_done->Signal(); - }, - persistent_settings.get(), &test_done_); + "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()), std::move(closure), - true); - test_done_.Wait(); - test_done_.Reset(); + "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) { @@ -373,95 +251,40 @@ TEST_F(PersistentSettingTest, GetSetDictionary) { std::make_unique(kPersistentSettingsJson); persistent_settings->ValidatePersistentSettings(); - base::OnceClosure closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - 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()); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); - base::Value dict(base::Value::Type::DICT); dict.GetDict().Set("key_string", "hello"); persistent_settings->SetPersistentSetting( - "key", base::Value::ToUniquePtrValue(dict.Clone()), std::move(closure), - true); - test_done_.Wait(); - test_done_.Reset(); - - closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - auto 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()); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); + "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()), std::move(closure), - true); - test_done_.Wait(); - test_done_.Reset(); -} - -TEST_F(PersistentSettingTest, URLAsKey) { - // Tests that json_pref_store has the correct SetValue and - // RemoveValue changes for using a URL as a PersistentSettings - // Key. - auto persistent_settings = - std::make_unique(kPersistentSettingsJson); - persistent_settings->ValidatePersistentSettings(); + "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()); - base::OnceClosure closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - auto test_dict = persistent_settings->GetPersistentSettingAsDictionary( - "http://127.0.0.1:45019/"); - EXPECT_FALSE(test_dict.empty()); - EXPECT_EQ(1, test_dict.size()); - 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_done->Signal(); - }, - persistent_settings.get(), &test_done_); - - // Test that json_pref_store uses SetKey instead of Set, making URL - // keys viable. - base::Value dict(base::Value::Type::DICT); dict.GetDict().Set("http://127.0.0.1:45019/", "Dictionary URL Key Works!"); persistent_settings->SetPersistentSetting( - "http://127.0.0.1:45019/", base::Value::ToUniquePtrValue(dict.Clone()), - std::move(closure), true); - test_done_.Wait(); - test_done_.Reset(); - - closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - auto test_dict = persistent_settings->GetPersistentSettingAsDictionary( - "http://127.0.0.1:45019/"); - EXPECT_TRUE(test_dict.empty()); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); - persistent_settings->RemovePersistentSetting("http://127.0.0.1:45019/", - std::move(closure), true); - test_done_.Wait(); - test_done_.Reset(); + "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) { @@ -471,35 +294,13 @@ TEST_F(PersistentSettingTest, RemoveSetting) { ASSERT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); ASSERT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false)); - - base::OnceClosure closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_TRUE( - persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_TRUE( - persistent_settings->GetPersistentSettingAsBool("key", false)); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); persistent_settings->SetPersistentSetting( - "key", std::make_unique(true), std::move(closure), true); - test_done_.Wait(); - test_done_.Reset(); - - closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_TRUE( - persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_FALSE( - persistent_settings->GetPersistentSettingAsBool("key", false)); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); - persistent_settings->RemovePersistentSetting("key", std::move(closure), true); - test_done_.Wait(); - test_done_.Reset(); + "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) { @@ -509,35 +310,13 @@ TEST_F(PersistentSettingTest, DeleteSettings) { ASSERT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); ASSERT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false)); - - base::OnceClosure closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_TRUE( - persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_TRUE( - persistent_settings->GetPersistentSettingAsBool("key", false)); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); persistent_settings->SetPersistentSetting( - "key", std::make_unique(true), std::move(closure), true); - test_done_.Wait(); - test_done_.Reset(); - - closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_TRUE( - persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_FALSE( - persistent_settings->GetPersistentSettingAsBool("key", false)); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); - persistent_settings->DeletePersistentSettings(std::move(closure)); - test_done_.Wait(); - test_done_.Reset(); + "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) { @@ -547,105 +326,29 @@ TEST_F(PersistentSettingTest, InvalidSettings) { ASSERT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); ASSERT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false)); - - base::OnceClosure closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_TRUE( - persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_TRUE( - persistent_settings->GetPersistentSettingAsBool("key", false)); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); persistent_settings->SetPersistentSetting( - "key", std::make_unique(true), std::move(closure), true); - test_done_.Wait(); - test_done_.Reset(); + "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)); - - closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_FALSE( - persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_FALSE( - persistent_settings->GetPersistentSettingAsBool("key", false)); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); persistent_settings->SetPersistentSetting( - "key", std::make_unique(false), std::move(closure), true); - test_done_.Wait(); - test_done_.Reset(); + "key", std::make_unique(false), true); + EXPECT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", true)); + EXPECT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false)); persistent_settings = std::make_unique(kPersistentSettingsJson); persistent_settings->ValidatePersistentSettings(); ASSERT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); ASSERT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false)); - closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { - EXPECT_TRUE( - persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_TRUE( - persistent_settings->GetPersistentSettingAsBool("key", false)); - test_done->Signal(); - }, - persistent_settings.get(), &test_done_); persistent_settings->SetPersistentSetting( - "key", std::make_unique(true), std::move(closure), true); - test_done_.Wait(); - test_done_.Reset(); -} - -TEST_F(PersistentSettingTest, WriteToFileOnlyWhenValidated) { - { - auto persistent_settings = - std::make_unique(kPersistentSettingsJson); - auto closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { test_done->Signal(); }, - persistent_settings.get(), &test_done_); - // Write to memory, but not file. - persistent_settings->SetPersistentSetting( - "key", std::make_unique(true), std::move(closure), true); - test_done_.Wait(); - test_done_.Reset(); - EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", false)); - } - { - auto persistent_settings = - std::make_unique(kPersistentSettingsJson); - EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); - EXPECT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false)); - } - - { - auto persistent_settings = - std::make_unique(kPersistentSettingsJson); - auto closure = base::BindOnce( - [](PersistentSettings* persistent_settings, - base::WaitableEvent* test_done) { test_done->Signal(); }, - persistent_settings.get(), &test_done_); - // Write to memory, but not file. - persistent_settings->SetPersistentSetting( - "key", std::make_unique(true), std::move(closure), true); - test_done_.Wait(); - test_done_.Reset(); - EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", false)); - persistent_settings->ValidatePersistentSettings(/*blocking=*/true); - } - { - auto persistent_settings = - std::make_unique(kPersistentSettingsJson); - EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", false)); - } + "key", std::make_unique(true)); + EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true)); + EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", false)); } } // namespace persistent_storage diff --git a/cobalt/watchdog/watchdog_test.cc b/cobalt/watchdog/watchdog_test.cc index 95e3fa2d4567..c5e93d5afb14 100644 --- a/cobalt/watchdog/watchdog_test.cc +++ b/cobalt/watchdog/watchdog_test.cc @@ -753,12 +753,11 @@ TEST_F(WatchdogTest, WatchdogMethodsAreNoopWhenWatchdogIsDisabled) { std::make_unique(kSettingsFileName); persistent_settings->ValidatePersistentSettings(); - base::OnceClosure closure = base::BindOnce( - [](base::WaitableEvent* task_done) { task_done->Signal(); }, &task_done_); persistent_settings->SetPersistentSetting( kPersistentSettingWatchdogEnable, std::make_unique(false), - std::move(closure), true); - task_done_.Wait(); + true); + ASSERT_FALSE(persistent_settings->GetPersistentSettingAsBool( + kPersistentSettingWatchdogEnable, true)); watchdog_ = new watchdog::Watchdog(); watchdog_->InitializeCustom(persistent_settings.get(), @@ -789,12 +788,11 @@ TEST_F(WatchdogTest, LogtraceMethodsAreNoopWhenLogtraceIsDisabled) { std::make_unique(kSettingsFileName); persistent_settings->ValidatePersistentSettings(); - base::OnceClosure closure = base::BindOnce( - [](base::WaitableEvent* task_done) { task_done->Signal(); }, &task_done_); persistent_settings->SetPersistentSetting( kPersistentSettingLogtraceEnable, std::make_unique(false), - std::move(closure), true); - task_done_.Wait(); + true); + ASSERT_FALSE(persistent_settings->GetPersistentSettingAsBool( + kPersistentSettingLogtraceEnable, 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 cb803be6a091..0a2112a3a65a 100644 --- a/cobalt/worker/service_worker_persistent_settings.cc +++ b/cobalt/worker/service_worker_persistent_settings.cc @@ -439,8 +439,8 @@ void ServiceWorkerPersistentSettings::RemoveAll() { } } -void ServiceWorkerPersistentSettings::DeleteAll(base::OnceClosure closure) { - persistent_settings_->DeletePersistentSettings(std::move(closure)); +void ServiceWorkerPersistentSettings::DeleteAll() { + persistent_settings_->DeletePersistentSettings(); } } // namespace worker diff --git a/cobalt/worker/service_worker_persistent_settings.h b/cobalt/worker/service_worker_persistent_settings.h index 459d45b296b7..90b150934a9b 100644 --- a/cobalt/worker/service_worker_persistent_settings.h +++ b/cobalt/worker/service_worker_persistent_settings.h @@ -85,7 +85,7 @@ class ServiceWorkerPersistentSettings { void RemoveAll(); - void DeleteAll(base::OnceClosure closure); + void DeleteAll(); private: Options options_; diff --git a/cobalt/worker/service_worker_registration_map.cc b/cobalt/worker/service_worker_registration_map.cc index 27ea623c6ef9..e632377dea2b 100644 --- a/cobalt/worker/service_worker_registration_map.cc +++ b/cobalt/worker/service_worker_registration_map.cc @@ -70,12 +70,8 @@ void ServiceWorkerRegistrationMap::ReadPersistentSettings() { } void ServiceWorkerRegistrationMap::DeletePersistentSettings() { - base::OnceClosure closure = base::BindOnce( - [](std::map>* - registration_map) { (*registration_map).clear(); }, - ®istration_map_); - service_worker_persistent_settings_->DeleteAll(std::move(closure)); + service_worker_persistent_settings_->DeleteAll(); + registration_map_.clear(); } scoped_refptr