[M115] Fix the DualReadingListModel response to notifications

This CL fixes the DualReadingListModel response to observer
notifications when at least one of its two ReadingListModelImpl
underlying instances did not notify loaded yet.

(cherry picked from commit 80baa21)

Fixed: 1450890
Change-Id: I6a8dafe7ceff4bfc662291008ce17dd06e3c5719
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4584674
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Mahmoud Rashad <mmrashad@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1153154}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4590590
Cr-Commit-Position: refs/branch-heads/5790@{#396}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}

components/reading_list/core/dual_reading_list_model.cc

@@ -97,6 +97,7 @@ base::flat_set<GURL> DualReadingListModel::GetKeys() const {
 
 size_t DualReadingListModel::size() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(loaded());
   DCHECK_EQ(unread_entry_count_ + read_entry_count_, GetKeys().size());
 
   return unread_entry_count_ + read_entry_count_;
@@ -496,6 +497,11 @@ void DualReadingListModel::RemoveObserver(ReadingListModelObserver* observer) {
 
 void DualReadingListModel::ReadingListModelBeganBatchUpdates(
     const ReadingListModel* model) {
+  DCHECK(!suppress_observer_notifications_);
+
+  if (!loaded()) {
+    return;
+  }
   ++current_batch_updates_count_;
   if (current_batch_updates_count_ == 1) {
     for (auto& observer : observers_) {
@@ -506,6 +512,11 @@ void DualReadingListModel::ReadingListModelBeganBatchUpdates(
 
 void DualReadingListModel::ReadingListModelCompletedBatchUpdates(
     const ReadingListModel* model) {
+  DCHECK(!suppress_observer_notifications_);
+
+  if (!loaded()) {
+    return;
+  }
   --current_batch_updates_count_;
   if (current_batch_updates_count_ == 0) {
     for (auto& observer : observers_) {
@@ -533,7 +544,7 @@ void DualReadingListModel::ReadingListModelLoaded(
 void DualReadingListModel::ReadingListWillRemoveEntry(
     const ReadingListModel* model,
     const GURL& url) {
-  if (suppress_observer_notifications_) {
+  if (!loaded() || suppress_observer_notifications_) {
     return;
   }
 
@@ -553,7 +564,7 @@ void DualReadingListModel::ReadingListWillRemoveEntry(
 void DualReadingListModel::ReadingListDidRemoveEntry(
     const ReadingListModel* model,
     const GURL& url) {
-  if (suppress_observer_notifications_) {
+  if (!loaded() || suppress_observer_notifications_) {
     return;
   }
 
@@ -572,7 +583,7 @@ void DualReadingListModel::ReadingListDidRemoveEntry(
 void DualReadingListModel::ReadingListWillMoveEntry(
     const ReadingListModel* model,
     const GURL& url) {
-  if (suppress_observer_notifications_) {
+  if (!loaded() || suppress_observer_notifications_) {
     return;
   }
 
@@ -586,7 +597,7 @@ void DualReadingListModel::ReadingListWillMoveEntry(
 void DualReadingListModel::ReadingListDidMoveEntry(
     const ReadingListModel* model,
     const GURL& url) {
-  if (suppress_observer_notifications_) {
+  if (!loaded() || suppress_observer_notifications_) {
     return;
   }
 
@@ -600,7 +611,7 @@ void DualReadingListModel::ReadingListDidMoveEntry(
 void DualReadingListModel::ReadingListWillAddEntry(
     const ReadingListModel* model,
     const ReadingListEntry& entry) {
-  if (suppress_observer_notifications_) {
+  if (!loaded() || suppress_observer_notifications_) {
     return;
   }
 
@@ -623,7 +634,7 @@ void DualReadingListModel::ReadingListDidAddEntry(
     const ReadingListModel* model,
     const GURL& url,
     reading_list::EntrySource source) {
-  if (suppress_observer_notifications_) {
+  if (!loaded() || suppress_observer_notifications_) {
     return;
   }
 
@@ -665,9 +676,10 @@ void DualReadingListModel::ReadingListDidUpdateEntry(
 }
 
 void DualReadingListModel::ReadingListDidApplyChanges(ReadingListModel* model) {
-  if (!suppress_observer_notifications_) {
-    NotifyObserversWithDidApplyChanges();
+  if (!loaded() || suppress_observer_notifications_) {
+    return;
   }
+  NotifyObserversWithDidApplyChanges();
 }
 
 DualReadingListModel::StorageStateForTesting

components/reading_list/core/dual_reading_list_model_unittest.cc

@@ -190,6 +190,24 @@ class DualReadingListModelTest : public testing::Test {
         /*initial_account_entries_builders=*/{});
   }
 
+  bool TriggerAccountStorageLoadCompletionSignedInSyncDisabled(
+      std::vector<TestEntryBuilder> initial_account_entries_builders = {}) {
+    auto metadata_batch = std::make_unique<syncer::MetadataBatch>();
+    sync_pb::ModelTypeState state;
+    state.set_initial_sync_state(
+        sync_pb::ModelTypeState_InitialSyncState_INITIAL_SYNC_DONE);
+    state.set_authenticated_account_id(kTestAccountId);
+    metadata_batch->SetModelTypeState(state);
+
+    std::vector<scoped_refptr<ReadingListEntry>> initial_account_entries;
+    for (auto entry_builder : initial_account_entries_builders) {
+      initial_account_entries.push_back(entry_builder.Build());
+    }
+
+    return account_model_storage_ptr_->TriggerLoadCompletion(
+        std::move(initial_account_entries), std::move(metadata_batch));
+  }
+
   bool TriggerStorageLoadCompletionSignedInSyncDisabled(
       std::vector<TestEntryBuilder> initial_local_entries_builders = {},
       std::vector<TestEntryBuilder> initial_account_entries_builders = {}) {
@@ -304,6 +322,25 @@ TEST_F(DualReadingListModelTest, MetaDataClearedBeforeModelLoaded) {
   EXPECT_EQ(0ul, dual_model_->size());
 }
 
+TEST_F(DualReadingListModelTest, UpdatesFromSyncBeforeTheLocalModelIsLoaded) {
+  ResetStorage();
+  TriggerAccountStorageLoadCompletionSignedInSyncDisabled(
+      /*initial_account_entries_builders=*/{
+          TestEntryBuilder(kUrl, clock_.Now())});
+
+  EXPECT_CALL(observer_, ReadingListWillRemoveEntry).Times(0);
+  EXPECT_CALL(observer_, ReadingListDidRemoveEntry).Times(0);
+  EXPECT_CALL(observer_, ReadingListWillAddEntry).Times(0);
+  EXPECT_CALL(observer_, ReadingListDidAddEntry).Times(0);
+  EXPECT_CALL(observer_, ReadingListDidApplyChanges).Times(0);
+
+  // DCHECKs verify that sync updates are issued as batch updates.
+  auto token = account_model_ptr_->BeginBatchUpdates();
+  account_model_ptr_->SyncRemoveEntry(kUrl);
+  account_model_ptr_->AddEntry(TestEntryBuilder(kUrl, clock_.Now()).Build(),
+                               reading_list::ADDED_VIA_SYNC);
+}
+
 TEST_F(DualReadingListModelTest, ReturnAccountModelSize) {
   ASSERT_TRUE(ResetStorageAndTriggerLoadCompletion(
       /*initial_local_or_syncable_entries_builders=*/{},