Skip to content

Commit

Permalink
Address potential Persistent Settings Getters race.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@briantting
briantting committed Apr 27, 2024
1 parent 00c3930 commit 80f11b8
Show file tree
Hide file tree
Showing 7 changed files with 232 additions and 523 deletions.
111 changes: 61 additions & 50 deletions cobalt/persistent_storage/persistent_settings.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ void PersistentSettings::InitializePrefStore() {
pref_store_ = base::MakeRefCounted<JsonPrefStore>(
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
Expand All @@ -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<bool> result = persistent_settings.FindBool(key);
absl::optional<bool> 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<int> result = persistent_settings.FindInt(key);
absl::optional<int> 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<double> result = persistent_settings.FindDouble(key);
absl::optional<double> 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<base::Value> 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<base::Value> values;
if (result) {
for (const auto& value : *result) {
Expand All @@ -149,9 +155,10 @@ std::vector<base::Value> PersistentSettings::GetPersistentSettingAsList(

base::flat_map<std::string, std::unique_ptr<base::Value>>
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<std::string, std::unique_ptr<base::Value>> dict;
if (result) {
for (base::Value::Dict::iterator it = result->begin(); it != result->end();
Expand All @@ -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<base::Value> value,
base::OnceClosure closure, bool blocking) {
const std::string& key, std::unique_ptr<base::Value> 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<base::Value> value,
base::OnceClosure closure, bool blocking) {
const std::string& key, std::unique_ptr<base::Value> 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) {
Expand Down
35 changes: 18 additions & 17 deletions cobalt/persistent_storage/persistent_settings.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,18 +72,13 @@ class PersistentSettings : public base::CurrentThread::DestructionObserver {
base::flat_map<std::string, std::unique_ptr<base::Value>>
GetPersistentSettingAsDictionary(const std::string& key);

void SetPersistentSetting(
const std::string& key, std::unique_ptr<base::Value> value,
base::OnceClosure closure = std::move(base::DoNothing()),
bool blocking = false);
void SetPersistentSetting(const std::string& key,
std::unique_ptr<base::Value> 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
Expand All @@ -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<base::Value> 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);

Expand All @@ -109,14 +107,17 @@ class PersistentSettings : public base::CurrentThread::DestructionObserver {
// PrefStore used for persistent settings.
scoped_refptr<PersistentPrefStore> 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_ = {
Expand Down
Loading

0 comments on commit 80f11b8

Please sign in to comment.