Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Address potential Persistent Settings Getters race. #3095

Merged
merged 1 commit into from
May 9, 2024
Merged

Address potential Persistent Settings Getters race. #3095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 45 additions & 50 deletions cobalt/persistent_storage/persistent_settings.cc
View file
Open in desktop
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
Open in desktop
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
Loading