Skip to content

Commit

Permalink
Address potential Persistent Settings Getters race. (#3095)
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 WaitForFence. 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 authored May 9, 2024
1 parent 2df15ff commit feba187
Show file tree
Hide file tree
Showing 7 changed files with 212 additions and 523 deletions.
95 changes: 45 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.
base::task_runner_util::WaitForFence(thread_.task_runner(), FROM_HERE);
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.
base::task_runner_util::WaitForFence(thread_.task_runner(), FROM_HERE);
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.
base::task_runner_util::WaitForFence(thread_.task_runner(), FROM_HERE);
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.
base::task_runner_util::WaitForFence(thread_.task_runner(), FROM_HERE);
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.
base::task_runner_util::WaitForFence(thread_.task_runner(), FROM_HERE);
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.
base::task_runner_util::WaitForFence(thread_.task_runner(), FROM_HERE);
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 @@ -165,69 +172,57 @@ PersistentSettings::GetPersistentSettingAsDictionary(const std::string& key) {
}

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
31 changes: 14 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 @@ -94,12 +89,11 @@ class PersistentSettings : public base::CurrentThread::DestructionObserver {

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 +103,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 feba187

Please sign in to comment.