Skip to content

Commit

Permalink
Cherry pick PR #3052: Always allow writing to persistent settings mem…
Browse files Browse the repository at this point in the history 
…ory. (#3140)

Refer to the original PR: #3052

b/335325208

Co-authored-by: aee <117306596+aee-google@users.noreply.github.com>
  • Loading branch information
@cobalt-github-releaser-bot @aee-google
cobalt-github-releaser-bot and aee-google authored May 2, 2024
1 parent 007bc51 commit e7c2cc7
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 47 deletions.
56 changes: 19 additions & 37 deletions cobalt/persistent_storage/persistent_settings.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,13 @@
namespace cobalt {
namespace persistent_storage {

namespace {
// Protected persistent settings key indicating whether or not the persistent
// settings file has been validated.
const char kValidated[] = "validated";
} // namespace

void PersistentSettings::WillDestroyCurrentMessageLoop() {
// Clear all member variables allocated from the thread.
pref_store_.reset();
}

PersistentSettings::PersistentSettings(const std::string& file_name)
: thread_("PersistentSettings") {
: thread_("PersistentSettings"), validated_initial_settings_(false) {
if (!thread_.Start()) return;
DCHECK(task_runner());

Expand Down Expand Up @@ -76,35 +70,33 @@ 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<JsonPrefStore>(
base::FilePath(persistent_settings_file_));
pref_store_->ReadPrefs();
pref_store_initialized_.Signal();
}
validated_initial_settings_ = GetPersistentSettingAsBool(kValidated, false);
if (!validated_initial_settings_) {
starboard::SbFileDeleteRecursive(persistent_settings_file_.c_str(), true);
}
// 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();
}

void PersistentSettings::ValidatePersistentSettings() {
void PersistentSettings::ValidatePersistentSettings(bool blocking) {
task_runner()->PostTask(
FROM_HERE,
base::BindOnce(&PersistentSettings::ValidatePersistentSettingsHelper,
base::Unretained(this)));
base::Unretained(this), blocking));
}

void PersistentSettings::ValidatePersistentSettingsHelper() {
void PersistentSettings::ValidatePersistentSettingsHelper(bool blocking) {
DCHECK_EQ(base::SequencedTaskRunner::GetCurrentDefault(), task_runner());
if (!validated_initial_settings_) {
base::AutoLock auto_lock(pref_store_lock_);
pref_store_->SetValue(kValidated, base::Value(true),
WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
CommitPendingWrite(false);
validated_initial_settings_ = true;
CommitPendingWrite(blocking);
}
}

Expand Down Expand Up @@ -175,10 +167,6 @@ PersistentSettings::GetPersistentSettingAsDictionary(const std::string& key) {
void PersistentSettings::SetPersistentSetting(
const std::string& key, std::unique_ptr<base::Value> value,
base::OnceClosure closure, bool blocking) {
if (key == kValidated) {
LOG(ERROR) << "Cannot set protected persistent setting: " << key;
return;
}
task_runner()->PostTask(
FROM_HERE, base::BindOnce(&PersistentSettings::SetPersistentSettingHelper,
base::Unretained(this), key, std::move(value),
Expand All @@ -189,16 +177,14 @@ void PersistentSettings::SetPersistentSettingHelper(
const std::string& key, std::unique_ptr<base::Value> value,
base::OnceClosure closure, bool blocking) {
DCHECK_EQ(base::SequencedTaskRunner::GetCurrentDefault(), task_runner());
if (validated_initial_settings_) {
{
base::AutoLock auto_lock(pref_store_lock_);
pref_store_->SetValue(kValidated, base::Value(false),
WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
pref_store_->SetValue(key,
base::Value::FromUniquePtrValue(std::move(value)),
WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
CommitPendingWrite(blocking);
} else {
LOG(ERROR) << "Cannot set persistent setting while unvalidated: " << key;
if (validated_initial_settings_) {
CommitPendingWrite(blocking);
}
}
std::move(closure).Run();
}
Expand All @@ -216,14 +202,12 @@ void PersistentSettings::RemovePersistentSetting(const std::string& key,
void PersistentSettings::RemovePersistentSettingHelper(
const std::string& key, base::OnceClosure closure, bool blocking) {
DCHECK_EQ(base::SequencedTaskRunner::GetCurrentDefault(), task_runner());
if (validated_initial_settings_) {
{
base::AutoLock auto_lock(pref_store_lock_);
pref_store_->SetValue(kValidated, base::Value(false),
WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
pref_store_->RemoveValue(key, WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
CommitPendingWrite(blocking);
} else {
LOG(ERROR) << "Cannot remove persistent setting while unvalidated: " << key;
if (validated_initial_settings_) {
CommitPendingWrite(blocking);
}
}
std::move(closure).Run();
}
Expand All @@ -238,12 +222,10 @@ void PersistentSettings::DeletePersistentSettings(base::OnceClosure closure) {
void PersistentSettings::DeletePersistentSettingsHelper(
base::OnceClosure closure) {
DCHECK_EQ(base::SequencedTaskRunner::GetCurrentDefault(), task_runner());
if (validated_initial_settings_) {
starboard::SbFileDeleteRecursive(persistent_settings_file_.c_str(), true);
{
base::AutoLock auto_lock(pref_store_lock_);
starboard::SbFileDeleteRecursive(persistent_settings_file_.c_str(), true);
pref_store_->ReadPrefs();
} else {
LOG(ERROR) << "Cannot delete persistent setting while unvalidated.";
}
std::move(closure).Run();
}
Expand Down
4 changes: 2 additions & 2 deletions cobalt/persistent_storage/persistent_settings.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class PersistentSettings : public base::CurrentThread::DestructionObserver {
void WillDestroyCurrentMessageLoop() override;

// Validates persistent settings by restoring the file on successful start up.
void ValidatePersistentSettings();
void ValidatePersistentSettings(bool blocking = false);

// Getters and Setters for persistent settings.
bool GetPersistentSettingAsBool(const std::string& key, bool default_setting);
Expand Down Expand Up @@ -90,7 +90,7 @@ class PersistentSettings : public base::CurrentThread::DestructionObserver {
// the dedicated thread_ as a JSONPrefStore.
void InitializePrefStore();

void ValidatePersistentSettingsHelper();
void ValidatePersistentSettingsHelper(bool blocking);

void SetPersistentSettingHelper(const std::string& key,
std::unique_ptr<base::Value> value,
Expand Down
73 changes: 65 additions & 8 deletions cobalt/persistent_storage/persistent_settings_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,15 @@ TEST_F(PersistentSettingTest, GetDefaultBool) {
// exists but invalid
base::OnceClosure closure = base::BindOnce(
[](PersistentSettings* persistent_settings,
base::WaitableEvent* test_done) {
EXPECT_TRUE(
persistent_settings->GetPersistentSettingAsInt("key", true));
test_done->Signal();
},
base::WaitableEvent* test_done) { test_done->Signal(); },
persistent_settings.get(), &test_done_);
persistent_settings->SetPersistentSetting(
"key", std::make_unique<base::Value>(4.2), std::move(closure), true);
test_done_.Wait();
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) {
Expand Down Expand Up @@ -571,9 +571,9 @@ TEST_F(PersistentSettingTest, InvalidSettings) {
closure = base::BindOnce(
[](PersistentSettings* persistent_settings,
base::WaitableEvent* test_done) {
EXPECT_TRUE(
EXPECT_FALSE(
persistent_settings->GetPersistentSettingAsBool("key", true));
EXPECT_TRUE(
EXPECT_FALSE(
persistent_settings->GetPersistentSettingAsBool("key", false));
test_done->Signal();
},
Expand All @@ -586,9 +586,66 @@ TEST_F(PersistentSettingTest, InvalidSettings) {
persistent_settings =
std::make_unique<PersistentSettings>(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<base::Value>(true), std::move(closure), true);
test_done_.Wait();
test_done_.Reset();
}

TEST_F(PersistentSettingTest, WriteToFileOnlyWhenValidated) {
{
auto persistent_settings =
std::make_unique<PersistentSettings>(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<base::Value>(true), std::move(closure), true);
test_done_.Wait();
test_done_.Reset();
EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", false));
}
{
auto persistent_settings =
std::make_unique<PersistentSettings>(kPersistentSettingsJson);
EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", true));
EXPECT_FALSE(persistent_settings->GetPersistentSettingAsBool("key", false));
}

{
auto persistent_settings =
std::make_unique<PersistentSettings>(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<base::Value>(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<PersistentSettings>(kPersistentSettingsJson);
EXPECT_TRUE(persistent_settings->GetPersistentSettingAsBool("key", false));
}
}

} // namespace persistent_storage
Expand Down

0 comments on commit e7c2cc7

Please sign in to comment.