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 Post Task 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.

b/305057554

cobalt/persistent_storage/persistent_settings.cc

@@ -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
@@ -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) {
@@ -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();
@@ -182,6 +189,7 @@ void PersistentSettings::SetPersistentSettingHelper(
     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);
     }
@@ -205,6 +213,7 @@ void PersistentSettings::RemovePersistentSettingHelper(
   {
     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);
     }
@@ -226,6 +235,7 @@ void PersistentSettings::DeletePersistentSettingsHelper(
     base::AutoLock auto_lock(pref_store_lock_);
     starboard::SbFileDeleteRecursive(persistent_settings_file_.c_str(), true);
     pref_store_->ReadPrefs();
+    persistent_settings_ = pref_store_->GetValues();
   }
   std::move(closure).Run();
 }

cobalt/persistent_storage/persistent_settings.h

@@ -109,14 +109,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_ = {