[prefs] Add PersistentPrefStore::HasReadErrorDelegate()

This method can be used as proxy to detect if ReadPrefsAsync() has been
called on a pref store.

Bug: 336776819
Change-Id: I21ba0a5f4d5a00d1710303359989277e4a940410
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5535432
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Ankush Singh <ankushkush@google.com>
Cr-Commit-Position: refs/heads/main@{#1301154}

components/prefs/in_memory_pref_store.cc

@@ -90,3 +90,7 @@ void InMemoryPrefStore::ReportValueChanged(const std::string& key,
 bool InMemoryPrefStore::IsInMemoryPrefStore() const {
   return true;
 }
+
+bool InMemoryPrefStore::HasReadErrorDelegate() const {
+  return false;
+}

components/prefs/in_memory_pref_store.h

@@ -54,6 +54,7 @@ class COMPONENTS_PREFS_EXPORT InMemoryPrefStore : public PersistentPrefStore {
   void OnStoreDeletionFromDisk() override {}
   bool IsInMemoryPrefStore() const override;
   void RemoveValuesByPrefixSilently(const std::string& prefix) override;
+  bool HasReadErrorDelegate() const override;
 
  protected:
   ~InMemoryPrefStore() override;

components/prefs/json_pref_store.cc

@@ -296,7 +296,7 @@ void JsonPrefStore::ReadPrefsAsync(ReadErrorDelegate* error_delegate) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   initialized_ = false;
-  error_delegate_.reset(error_delegate);
+  error_delegate_.emplace(error_delegate);
 
   // Weakly binds the read task so that it doesn't kick in during shutdown.
   file_task_runner_->PostTaskAndReplyWithResult(
@@ -527,8 +527,10 @@ void JsonPrefStore::FinalizeFileRead(bool initialization_successful,
   if (schedule_write)
     ScheduleWrite(DEFAULT_PREF_WRITE_FLAGS);
 
-  if (error_delegate_ && read_error_ != PREF_READ_ERROR_NONE)
+  if (error_delegate_.has_value() && error_delegate_.value() &&
-    error_delegate_->OnError(read_error_);
+      read_error_ != PREF_READ_ERROR_NONE) {
+    error_delegate_.value()->OnError(read_error_);
+  }
 
   for (PrefStore::Observer& observer : observers_)
     observer.OnInitializationCompleted(true);
@@ -546,3 +548,7 @@ void JsonPrefStore::ScheduleWrite(uint32_t flags) {
     writer_.ScheduleWriteWithBackgroundDataSerializer(this);
   }
 }
+
+bool JsonPrefStore::HasReadErrorDelegate() const {
+  return error_delegate_.has_value();
+}

components/prefs/json_pref_store.h

@@ -95,6 +95,7 @@ class COMPONENTS_PREFS_EXPORT JsonPrefStore final
   void RemoveValue(const std::string& key, uint32_t flags) override;
   bool ReadOnly() const override;
   PrefReadError GetReadError() const override;
+  bool HasReadErrorDelegate() const override;
   // Note this method may be asynchronous if this instance has a |pref_filter_|
   // in which case it will return PREF_READ_ERROR_ASYNCHRONOUS_TASK_INCOMPLETE.
   // See details in pref_filter.h.
@@ -114,6 +115,7 @@ class COMPONENTS_PREFS_EXPORT JsonPrefStore final
   // Just like RemoveValue(), but removes all the prefs that start with
   // |prefix|. Used for pref-initialization cleanup.
   void RemoveValuesByPrefixSilently(const std::string& prefix) override;
+
   // Registers |on_next_successful_write_reply| to be called once, on the next
   // successful write event of |writer_|.
   // |on_next_successful_write_reply| will be called on the thread from which
@@ -202,7 +204,8 @@ class COMPONENTS_PREFS_EXPORT JsonPrefStore final
   std::unique_ptr<PrefFilter> pref_filter_;
   base::ObserverList<PrefStore::Observer, true>::Unchecked observers_;
 
-  std::unique_ptr<ReadErrorDelegate> error_delegate_;
+  // Optional so we can differentiate `nullopt` from `nullptr`.
+  std::optional<std::unique_ptr<ReadErrorDelegate>> error_delegate_;
 
   bool initialized_;
   bool filtering_in_progress_;

components/prefs/json_pref_store_unittest.cc

@@ -558,6 +558,29 @@ TEST_P(JsonPrefStoreTest, RemoveValuesByPrefix) {
   EXPECT_TRUE(pref_store->GetValue(other_name, &value));
 }
 
+TEST_P(JsonPrefStoreTest, HasReadErrorDelegate) {
+  base::FilePath bogus_input_file = temp_dir_.GetPath().AppendASCII("read.txt");
+  ASSERT_FALSE(PathExists(bogus_input_file));
+  auto pref_store = base::MakeRefCounted<JsonPrefStore>(bogus_input_file);
+
+  EXPECT_FALSE(pref_store->HasReadErrorDelegate());
+
+  pref_store->ReadPrefsAsync(new MockReadErrorDelegate);
+  EXPECT_TRUE(pref_store->HasReadErrorDelegate());
+}
+
+TEST_P(JsonPrefStoreTest, HasReadErrorDelegateWithNullDelegate) {
+  base::FilePath bogus_input_file = temp_dir_.GetPath().AppendASCII("read.txt");
+  ASSERT_FALSE(PathExists(bogus_input_file));
+  auto pref_store = base::MakeRefCounted<JsonPrefStore>(bogus_input_file);
+
+  EXPECT_FALSE(pref_store->HasReadErrorDelegate());
+
+  pref_store->ReadPrefsAsync(nullptr);
+  // Returns true even though no instance was passed.
+  EXPECT_TRUE(pref_store->HasReadErrorDelegate());
+}
+
 INSTANTIATE_TEST_SUITE_P(
     JsonPrefStoreTestVariations,
     JsonPrefStoreTest,

components/prefs/overlay_user_pref_store.cc

@@ -244,3 +244,7 @@ bool OverlayUserPrefStore::ShallBeStoredInPersistent(
     std::string_view key) const {
   return persistent_names_set_.find(key) != persistent_names_set_.end();
 }
+
+bool OverlayUserPrefStore::HasReadErrorDelegate() const {
+  return persistent_user_pref_store_->HasReadErrorDelegate();
+}

components/prefs/overlay_user_pref_store.h

@@ -73,6 +73,7 @@ class COMPONENTS_PREFS_EXPORT OverlayUserPrefStore
   void RegisterPersistentPref(const std::string& key);
 
   void OnStoreDeletionFromDisk() override;
+  bool HasReadErrorDelegate() const override;
 
  protected:
   ~OverlayUserPrefStore() override;

components/prefs/overlay_user_pref_store_unittest.cc

@@ -243,4 +243,13 @@ TEST_F(OverlayUserPrefStoreTest, CommitPendingWriteWithCallback) {
   TestCommitPendingWriteWithCallback(overlay_.get(), &task_environment_);
 }
 
+TEST_F(OverlayUserPrefStoreTest, HasReadErrorDelegate) {
+  ASSERT_FALSE(underlay_->HasReadErrorDelegate());
+  EXPECT_FALSE(overlay_->HasReadErrorDelegate());
+
+  underlay_->ReadPrefsAsync(nullptr);
+  ASSERT_TRUE(underlay_->HasReadErrorDelegate());
+  EXPECT_TRUE(overlay_->HasReadErrorDelegate());
+}
+
 }  // namespace base

components/prefs/persistent_pref_store.h

@@ -83,8 +83,14 @@ class COMPONENTS_PREFS_EXPORT PersistentPrefStore : public WriteablePrefStore {
   // TODO(crbug.com/41447167) Remove this after fixing the bug.
   virtual bool IsInMemoryPrefStore() const;
 
+  // Returns whether this holds a read error delegate (can be null), passed
+  // during ReadPrefsAsync().
+  // When used in conjugation with IsInitializationComplete() and
+  // GetReadError(), this can be used to identity if there's a pending read.
+  virtual bool HasReadErrorDelegate() const = 0;
+
  protected:
-  ~PersistentPrefStore() override {}
+  ~PersistentPrefStore() override = default;
 };
 
 #endif  // COMPONENTS_PREFS_PERSISTENT_PREF_STORE_H_

components/prefs/segregated_pref_store.cc

@@ -42,10 +42,11 @@ void SegregatedPrefStore::UnderlyingPrefStoreObserver::
   if (!outer_->IsInitializationComplete())
     return;
 
-  if (outer_->read_error_delegate_) {
+  if (outer_->read_error_delegate_.has_value() &&
+      outer_->read_error_delegate_.value()) {
     PersistentPrefStore::PrefReadError read_error = outer_->GetReadError();
     if (read_error != PersistentPrefStore::PREF_READ_ERROR_NONE)
-      outer_->read_error_delegate_->OnError(read_error);
+      outer_->read_error_delegate_.value()->OnError(read_error);
   }
 
   for (auto& observer : outer_->observers_)
@@ -171,9 +172,9 @@ PersistentPrefStore::PrefReadError SegregatedPrefStore::ReadPrefs() {
 }
 
 void SegregatedPrefStore::ReadPrefsAsync(ReadErrorDelegate* error_delegate) {
-  read_error_delegate_.reset(error_delegate);
+  read_error_delegate_.emplace(error_delegate);
-  default_pref_store_->ReadPrefsAsync(NULL);
+  default_pref_store_->ReadPrefsAsync(nullptr);
-  selected_pref_store_->ReadPrefsAsync(NULL);
+  selected_pref_store_->ReadPrefsAsync(nullptr);
 }
 
 void SegregatedPrefStore::CommitPendingWrite(
@@ -226,3 +227,7 @@ const PersistentPrefStore* SegregatedPrefStore::StoreForKey(
                                                           : default_pref_store_)
       .get();
 }
+
+bool SegregatedPrefStore::HasReadErrorDelegate() const {
+  return read_error_delegate_.has_value();
+}

components/prefs/segregated_pref_store.h

@@ -8,6 +8,7 @@
 #include <stdint.h>
 
 #include <memory>
+#include <optional>
 #include <set>
 #include <string>
 #include <string_view>
@@ -81,6 +82,7 @@ class COMPONENTS_PREFS_EXPORT SegregatedPrefStore : public PersistentPrefStore {
           base::OnceClosure()) override;
   void SchedulePendingLossyWrites() override;
   void OnStoreDeletionFromDisk() override;
+  bool HasReadErrorDelegate() const override;
 
  protected:
   ~SegregatedPrefStore() override;
@@ -121,7 +123,9 @@ class COMPONENTS_PREFS_EXPORT SegregatedPrefStore : public PersistentPrefStore {
   const scoped_refptr<PersistentPrefStore> selected_pref_store_;
   const PrefNameSet selected_preference_names_;
 
-  std::unique_ptr<PersistentPrefStore::ReadErrorDelegate> read_error_delegate_;
+  // Optional so we can differentiate `nullopt` from `nullptr`.
+  std::optional<std::unique_ptr<PersistentPrefStore::ReadErrorDelegate>>
+      read_error_delegate_;
   base::ObserverList<PrefStore::Observer, true>::Unchecked observers_;
   UnderlyingPrefStoreObserver default_observer_;
   UnderlyingPrefStoreObserver selected_observer_;

components/prefs/segregated_pref_store_unittest.cc

@@ -383,6 +383,21 @@ TEST_F(SegregatedPrefStoreTest, GetValues) {
   EXPECT_EQ(base::Value(kValue1), *value);
 }
 
+TEST_F(SegregatedPrefStoreTest, HasReadErrorDelegate) {
+  EXPECT_FALSE(segregated_store_->HasReadErrorDelegate());
+
+  segregated_store_->ReadPrefsAsync(GetReadErrorDelegate().release());
+  EXPECT_TRUE(segregated_store_->HasReadErrorDelegate());
+}
+
+TEST_F(SegregatedPrefStoreTest, HasReadErrorDelegateWithNullDelegate) {
+  EXPECT_FALSE(segregated_store_->HasReadErrorDelegate());
+
+  segregated_store_->ReadPrefsAsync(nullptr);
+  // Returns true even though no instance was passed.
+  EXPECT_TRUE(segregated_store_->HasReadErrorDelegate());
+}
+
 INSTANTIATE_TEST_SUITE_P(
     WithoutCallback,
     SegregatedPrefStoreTest,

components/prefs/testing_pref_store.cc

@@ -147,7 +147,7 @@ PersistentPrefStore::PrefReadError TestingPrefStore::ReadPrefs() {
 
 void TestingPrefStore::ReadPrefsAsync(ReadErrorDelegate* error_delegate) {
   DCHECK(!pending_async_read_);
-  error_delegate_.reset(error_delegate);
+  error_delegate_.emplace(error_delegate);
   if (block_async_read_)
     pending_async_read_ = true;
   else
@@ -176,8 +176,10 @@ void TestingPrefStore::NotifyPrefValueChanged(const std::string& key) {
 void TestingPrefStore::NotifyInitializationCompleted() {
   DCHECK(!init_complete_);
   init_complete_ = true;
-  if (read_success_ && read_error_ != PREF_READ_ERROR_NONE && error_delegate_)
+  if (read_success_ && read_error_ != PREF_READ_ERROR_NONE &&
-    error_delegate_->OnError(read_error_);
+      error_delegate_.has_value() && error_delegate_.value()) {
+    error_delegate_.value()->OnError(read_error_);
+  }
   for (Observer& observer : observers_)
     observer.OnInitializationCompleted(read_success_);
 }
@@ -294,3 +296,7 @@ void TestingPrefStore::CheckPrefIsSerializable(const std::string& key,
   EXPECT_TRUE(base::JSONWriter::Write(value, &json))
       << "Pref \"" << key << "\" is not serializable as JSON.";
 }
+
+bool TestingPrefStore::HasReadErrorDelegate() const {
+  return error_delegate_.has_value();
+}

components/prefs/testing_pref_store.h

@@ -7,6 +7,7 @@
 
 #include <stdint.h>
 
+#include <optional>
 #include <string>
 #include <string_view>
 
@@ -53,6 +54,7 @@ class TestingPrefStore : public PersistentPrefStore {
   void CommitPendingWrite(base::OnceClosure reply_callback,
                           base::OnceClosure synchronous_done_callback) override;
   void SchedulePendingLossyWrites() override;
+  bool HasReadErrorDelegate() const override;
 
   // Marks the store as having completed initialization.
   void SetInitializationCompleted();
@@ -127,7 +129,8 @@ class TestingPrefStore : public PersistentPrefStore {
   // mutation.
   bool committed_;
 
-  std::unique_ptr<ReadErrorDelegate> error_delegate_;
+  // Optional so we can differentiate `nullopt` from `nullptr`.
+  std::optional<std::unique_ptr<ReadErrorDelegate>> error_delegate_;
   base::ObserverList<PrefStore::Observer, true>::Unchecked observers_;
 };
 

components/sync_preferences/dual_layer_user_pref_store.cc

@@ -68,10 +68,11 @@ void DualLayerUserPrefStore::UnderlyingPrefStoreObserver::
 
   // Forward error if any of the underlying store reported error upon
   // ReadPrefsAsync().
-  if (outer_->read_error_delegate_) {
+  if (outer_->read_error_delegate_.has_value() &&
+      outer_->read_error_delegate_.value()) {
     if (auto read_error = outer_->GetReadError();
         read_error != PersistentPrefStore::PREF_READ_ERROR_NONE) {
-      outer_->read_error_delegate_->OnError(read_error);
+      outer_->read_error_delegate_.value()->OnError(read_error);
     }
   }
 
@@ -377,7 +378,7 @@ void DualLayerUserPrefStore::ReadPrefsAsync(ReadErrorDelegate* error_delegate) {
   // The store is expected to take ownership of `error_delegate`, thus it's not
   // valid to forward the same to the two underlying stores. Instead, if any
   // error occurs, it's reported in OnInitializationCompleted() handle.
-  read_error_delegate_.reset(error_delegate);
+  read_error_delegate_.emplace(error_delegate);
   local_pref_store_->ReadPrefsAsync(nullptr);
   account_pref_store_->ReadPrefsAsync(nullptr);
 }
@@ -798,4 +799,8 @@ void DualLayerUserPrefStore::SetValueInAccountStoreOnly(const std::string& key,
   }
 }
 
+bool DualLayerUserPrefStore::HasReadErrorDelegate() const {
+  return read_error_delegate_.has_value();
+}
+
 }  // namespace sync_preferences

components/sync_preferences/dual_layer_user_pref_store.h

@@ -105,6 +105,7 @@ class DualLayerUserPrefStore : public PersistentPrefStore,
           base::OnceClosure()) override;
   void SchedulePendingLossyWrites() override;
   void OnStoreDeletionFromDisk() override;
+  bool HasReadErrorDelegate() const override;
 
   // SyncServiceObserver implementation
   void OnStateChanged(syncer::SyncService* sync_service) override;
@@ -205,7 +206,9 @@ class DualLayerUserPrefStore : public PersistentPrefStore,
   UnderlyingPrefStoreObserver local_pref_store_observer_;
   UnderlyingPrefStoreObserver account_pref_store_observer_;
 
-  std::unique_ptr<PersistentPrefStore::ReadErrorDelegate> read_error_delegate_;
+  // Optional so we can differentiate `nullopt` from `nullptr`.
+  std::optional<std::unique_ptr<PersistentPrefStore::ReadErrorDelegate>>
+      read_error_delegate_;
 
   // List of preference types currently syncing.
   base::flat_set<syncer::ModelType> active_types_;

components/sync_preferences/dual_layer_user_pref_store_unittest.cc

@@ -323,6 +323,22 @@ TEST_F(DualLayerUserPrefStoreInitializationTest,
   EXPECT_TRUE(store()->IsInitializationComplete());
 }
 
+TEST_F(DualLayerUserPrefStoreInitializationTest, HasReadErrorDelegate) {
+  EXPECT_FALSE(store()->HasReadErrorDelegate());
+
+  store()->ReadPrefsAsync(new MockReadErrorDelegate);
+  EXPECT_TRUE(store()->HasReadErrorDelegate());
+}
+
+TEST_F(DualLayerUserPrefStoreInitializationTest,
+       HasReadErrorDelegateWithNullDelegate) {
+  EXPECT_FALSE(store()->HasReadErrorDelegate());
+
+  store()->ReadPrefsAsync(nullptr);
+  // Returns true even though no instance was passed.
+  EXPECT_TRUE(store()->HasReadErrorDelegate());
+}
+
 TEST_F(DualLayerUserPrefStoreInitializationTest,
        ShouldReportInitializationCompleteAsyncReadAsync) {
   // Should report init completion after async read for underlying stores is