[M115] Fix an underflow when ModelReadyToSync is called

This CL fixes an underflow that is happening when
ReadingListSyncBridge::ModelReadyToSync() is called from
ReadingListModelImpl::StoreLoaded() by suppressing the observer
notification when ReadingListSyncBridge::ModelReadyToSync() called.

When ReadingListSyncBridge::ModelReadyToSync() is called from
ReadingListModelImpl::StoreLoaded(), it's called before
ReadingListModelLoaded() is called, and it could reach to
ReadingListModelImpl::DeleteAllEntries() which means notifying deletions
to DualReadingListModel, which decrement the
DualReadingListModel::unread_entry_count_ and
DualReadingListModel::read_entry_count_ before they are initialized,
which causes the underflow.

(cherry picked from commit 4c6016c)

Fixed: 1446140
Change-Id: I6968bba233a505ae4bbc9a568fc5e074945a0603
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4552758
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Mahmoud Rashad <mmrashad@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1148616}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4568075
Cr-Commit-Position: refs/branch-heads/5790@{#120}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}

components/reading_list/core/dual_reading_list_model_unittest.cc

@@ -13,6 +13,7 @@
 #include "components/reading_list/core/reading_list_entry.h"
 #include "components/reading_list/core/reading_list_model_impl.h"
 #include "components/sync/base/storage_type.h"
+#include "components/sync/model/client_tag_based_model_type_processor.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ -189,11 +190,9 @@ class DualReadingListModelTest : public testing::Test {
         /*initial_account_entries_builders=*/{});
   }
 
-  bool ResetStorageAndMimicSignedInSyncDisabled(
+  bool TriggerStorageLoadCompletionSignedInSyncDisabled(
       std::vector<TestEntryBuilder> initial_local_entries_builders = {},
       std::vector<TestEntryBuilder> initial_account_entries_builders = {}) {
-    ResetStorage();
-
     auto metadata_batch = std::make_unique<syncer::MetadataBatch>();
     sync_pb::ModelTypeState state;
     state.set_initial_sync_state(
@@ -217,6 +216,14 @@ class DualReadingListModelTest : public testing::Test {
                std::move(initial_account_entries), std::move(metadata_batch));
   }
 
+  bool ResetStorageAndMimicSignedInSyncDisabled(
+      std::vector<TestEntryBuilder> initial_local_entries_builders = {},
+      std::vector<TestEntryBuilder> initial_account_entries_builders = {}) {
+    ResetStorage();
+    return TriggerStorageLoadCompletionSignedInSyncDisabled(
+        initial_local_entries_builders, initial_account_entries_builders);
+  }
+
   bool ResetStorageAndMimicSyncEnabled(
       std::vector<TestEntryBuilder> initial_syncable_entries_builders = {}) {
     ResetStorage();
@@ -276,6 +283,27 @@ TEST_F(DualReadingListModelTest, ModelLoadFailure) {
   EXPECT_FALSE(dual_model_->loaded());
 }
 
+TEST_F(DualReadingListModelTest, MetaDataClearedBeforeModelLoaded) {
+  ResetStorage();
+  static_cast<syncer::ClientTagBasedModelTypeProcessor*>(
+      account_model_ptr_->GetSyncBridgeForTest()->change_processor())
+      ->ClearMetadataWhileStopped();
+
+  EXPECT_CALL(observer_, ReadingListModelBeganBatchUpdates).Times(0);
+  EXPECT_CALL(observer_, ReadingListModelCompletedBatchUpdates).Times(0);
+  EXPECT_CALL(observer_, ReadingListWillRemoveEntry).Times(0);
+  EXPECT_CALL(observer_, ReadingListDidRemoveEntry).Times(0);
+  EXPECT_CALL(observer_, ReadingListDidApplyChanges).Times(0);
+  EXPECT_CALL(observer_, ReadingListModelLoaded);
+  TriggerStorageLoadCompletionSignedInSyncDisabled(
+      /*initial_local_entries_builders=*/{},
+      /*initial_account_entries_builders=*/{
+          TestEntryBuilder(kUrl, clock_.Now())});
+
+  EXPECT_EQ(0ul, account_model_ptr_->size());
+  EXPECT_EQ(0ul, dual_model_->size());
+}
+
 TEST_F(DualReadingListModelTest, ReturnAccountModelSize) {
   ASSERT_TRUE(ResetStorageAndTriggerLoadCompletion(
       /*initial_local_or_syncable_entries_builders=*/{},

components/reading_list/core/reading_list_model_impl.cc

@@ -275,8 +275,11 @@ void ReadingListModelImpl::RemoveEntryByURLImpl(const GURL& url,
   if (!entry)
     return;
 
-  for (auto& observer : observers_)
-    observer.ReadingListWillRemoveEntry(this, url);
+  if (!suppress_deletions_batch_updates_notifications_) {
+    for (auto& observer : observers_) {
+      observer.ReadingListWillRemoveEntry(this, url);
+    }
+  }
 
   std::unique_ptr<ReadingListModelStorage::ScopedBatchUpdate> batch =
       storage_layer_->EnsureBatchCreated();
@@ -290,9 +293,11 @@ void ReadingListModelImpl::RemoveEntryByURLImpl(const GURL& url,
 
   entries_.erase(url);
 
-  for (auto& observer : observers_) {
-    observer.ReadingListDidRemoveEntry(this, url);
-    observer.ReadingListDidApplyChanges(this);
+  if (!suppress_deletions_batch_updates_notifications_) {
+    for (auto& observer : observers_) {
+      observer.ReadingListDidRemoveEntry(this, url);
+      observer.ReadingListDidApplyChanges(this);
+    }
   }
 }
 
@@ -582,7 +587,8 @@ ReadingListModelImpl::BeginBatchUpdatesWithSyncMetadata() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   auto token = std::make_unique<ScopedReadingListBatchUpdateImpl>(this);
   ++current_batch_updates_count_;
-  if (current_batch_updates_count_ == 1) {
+  if (current_batch_updates_count_ == 1 &&
+      !suppress_deletions_batch_updates_notifications_) {
     for (auto& observer : observers_) {
       observer.ReadingListModelBeganBatchUpdates(this);
     }
@@ -646,8 +652,15 @@ void ReadingListModelImpl::StoreLoaded(
   DCHECK_EQ(read_entry_count_ + unread_entry_count_, entries_.size());
   loaded_ = true;
 
-  sync_bridge_.ModelReadyToSync(/*model=*/this,
-                                std::move(result_or_error.value().second));
+  {
+    // In rare cases, ModelReadyToSync() leads to the deletion of all local
+    // entries. Such deletions should not be propagated to observers, because
+    // ReadingListModelLoaded hasn't been broadcasted yet.
+    base::AutoReset<bool> auto_reset_suppress_observer_notifications(
+        &suppress_deletions_batch_updates_notifications_, true);
+    sync_bridge_.ModelReadyToSync(/*model=*/this,
+                                  std::move(result_or_error.value().second));
+  }
 
   base::UmaHistogramCounts1000("ReadingList.Unread.Count.OnModelLoaded",
                                unread_entry_count_);
@@ -664,7 +677,8 @@ void ReadingListModelImpl::EndBatchUpdates() {
   DCHECK(IsPerformingBatchUpdates());
   DCHECK(current_batch_updates_count_ > 0);
   --current_batch_updates_count_;
-  if (current_batch_updates_count_ == 0) {
+  if (current_batch_updates_count_ == 0 &&
+      !suppress_deletions_batch_updates_notifications_) {
     for (auto& observer : observers_) {
       observer.ReadingListModelCompletedBatchUpdates(this);
     }

components/reading_list/core/reading_list_model_impl.h

@@ -179,6 +179,10 @@ class ReadingListModelImpl : public ReadingListModel {
 
   bool loaded_ = false;
 
+  // Used to suppress deletions and batch updates notifications when
+  // ReadingListModelLoaded is not broadcasted yet.
+  bool suppress_deletions_batch_updates_notifications_ = false;
+
   std::map<GURL, scoped_refptr<ReadingListEntry>> entries_;
   size_t unread_entry_count_ = 0;
   size_t read_entry_count_ = 0;