Reland "[Sync] Postpone reupload of bookmarks until next updates received"

This reverts commit 8ebf771a.

Reason for revert: revert didn't fix win-asan failures, it doesn't affect them and must be safe to reland.

Original change's description:
> Revert "[Sync] Postpone reupload of bookmarks until next updates received"
> 
> This reverts commit 56f7b697.
> 
> Reason for revert: win-asan failures
> https://bugs.chromium.org/p/chromium/issues/detail?id=1106392
> 
> Original change's description:
> > [Sync] Postpone reupload of bookmarks until next updates received
> > 
> > When reupload is initiated after loading from the disk right away, it
> > may lead to upload bookmarks which were removed from the other device.
> > This would restore removed bookmarks. In practise it may be caused by
> > some devices which a rarely used.
> > 
> > Initiating reupload after receiving latest updates from the server helps
> > to prevent such behaviour.
> > 
> > Bug: 1061411
> > Change-Id: Ibc5646b19bf133de496140fa152b544bc06ebcf9
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2299721
> > Commit-Queue: Rushan Suleymanov <rushans@google.com>
> > Reviewed-by: Mikel Astiz <mastiz@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#788775}
> 
> TBR=mastiz@chromium.org,rushans@google.com
> 
> Change-Id: Ib00d0dfc3cd570aaf1081482cf1bd727b3dbdf8e
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 1061411
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2302773
> Reviewed-by: Jonah Ryan-Davis <jonahr@google.com>
> Commit-Queue: Jonah Ryan-Davis <jonahr@google.com>
> Cr-Commit-Position: refs/heads/master@{#789078}

TBR=mastiz@chromium.org,jonahr@google.com,rushans@google.com

# Not skipping CQ checks because this is a reland.

Bug: 1061411
Change-Id: Id4884cd8d26c73142fb5772ee0ab617613c7a8d4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2302752Reviewed-by: Rushan Suleymanov <rushans@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Cr-Commit-Position: refs/heads/master@{#789427}

components/sync_bookmarks/bookmark_model_type_processor.cc

@@ -246,6 +246,9 @@ void BookmarkModelTypeProcessor::OnUpdateReceived(
   bookmark_tracker_->set_model_type_state(model_type_state);
   bookmark_tracker_->UpdateLastSyncTime();
   updates_handler.Process(updates, got_new_encryption_requirements);
+  if (bookmark_tracker_->ReuploadBookmarksOnLoadIfNeeded()) {
+    NudgeForCommitIfNeeded();
+  }
   // There are cases when we receive non-empty updates that don't result in
   // model changes (e.g. reflections). In that case, issue a write to persit the
   // progress marker in order to avoid downloading those updates again.

components/sync_bookmarks/bookmark_model_type_processor_unittest.cc

@@ -19,6 +19,7 @@
 #include "components/bookmarks/test/test_bookmark_client.h"
 #include "components/favicon/core/test/mock_favicon_service.h"
 #include "components/sync/base/unique_position.h"
+#include "components/sync/engine/commit_queue.h"
 #include "components/sync/model/data_type_activation_request.h"
 #include "components/sync_bookmarks/switches.h"
 #include "components/undo/bookmark_undo_service.h"
@@ -63,6 +64,7 @@ syncer::UpdateResponseData CreateUpdateResponseData(
   data.parent_id = bookmark_info.parent_id;
   data.server_defined_unique_tag = bookmark_info.server_tag;
   data.unique_position = unique_position.ToProto();
+  data.originator_client_item_id = guid;
 
   sync_pb::BookmarkSpecifics* bookmark_specifics =
       data.specifics.mutable_bookmark();
@@ -149,6 +151,24 @@ void InitWithSyncedBookmarks(const std::vector<BookmarkInfo>& bookmarks,
   AssertState(processor, bookmarks);
 }
 
+class MockCommitQueue : public syncer::CommitQueue {
+ public:
+  MOCK_METHOD0(NudgeForCommit, void());
+};
+
+class ProxyCommitQueue : public syncer::CommitQueue {
+ public:
+  explicit ProxyCommitQueue(CommitQueue* commit_queue)
+      : commit_queue_(commit_queue) {
+    DCHECK(commit_queue_);
+  }
+
+  void NudgeForCommit() override { commit_queue_->NudgeForCommit(); }
+
+ private:
+  CommitQueue* commit_queue_ = nullptr;
+};
+
 class TestBookmarkClientWithFavicon : public bookmarks::TestBookmarkClient {
  public:
   // This method must be used to tell the bookmark_model about favicon.
@@ -192,17 +212,18 @@ class TestBookmarkClientWithFavicon : public bookmarks::TestBookmarkClient {
 class BookmarkModelTypeProcessorTest : public testing::Test {
  public:
   BookmarkModelTypeProcessorTest()
-      : processor_(&bookmark_undo_service_),
+      : processor_(std::make_unique<BookmarkModelTypeProcessor>(
+            &bookmark_undo_service_)),
         bookmark_model_(bookmarks::TestBookmarkClient::CreateModelWithClient(
             std::make_unique<TestBookmarkClientWithFavicon>())) {
     // TODO(crbug.com/516866): This class assumes model is loaded and sync has
     // started before running tests. We should test other variations (i.e. model
     // isn't loaded yet and/or sync didn't start yet).
-    processor_.SetFaviconService(&favicon_service_);
+    processor_->SetFaviconService(&favicon_service_);
   }
 
   void SimulateModelReadyToSync() {
-    processor_.ModelReadyToSync(
+    processor_->ModelReadyToSync(
         /*metadata_str=*/std::string(), schedule_save_closure_.Get(),
         bookmark_model_.get());
   }
@@ -210,7 +231,19 @@ class BookmarkModelTypeProcessorTest : public testing::Test {
   void SimulateOnSyncStarting() {
     syncer::DataTypeActivationRequest request;
     request.cache_guid = kCacheGuid;
-    processor_.OnSyncStarting(request, base::DoNothing());
+    processor_->OnSyncStarting(request, base::DoNothing());
+  }
+
+  void SimulateConnectSync() {
+    processor_->ConnectSync(
+        std::make_unique<ProxyCommitQueue>(&mock_commit_queue_));
+  }
+
+  // Simulate browser restart.
+  void ResetModelTypeProcessor() {
+    processor_ =
+        std::make_unique<BookmarkModelTypeProcessor>(&bookmark_undo_service_);
+    processor_->SetFaviconService(&favicon_service_);
   }
 
   void DestroyBookmarkModel() { bookmark_model_.reset(); }
@@ -224,7 +257,8 @@ class BookmarkModelTypeProcessorTest : public testing::Test {
     return &bookmark_undo_service_;
   }
   favicon::FaviconService* favicon_service() { return &favicon_service_; }
-  BookmarkModelTypeProcessor* processor() { return &processor_; }
+  MockCommitQueue* mock_commit_queue() { return &mock_commit_queue_; }
+  BookmarkModelTypeProcessor* processor() { return processor_.get(); }
   base::MockCallback<base::RepeatingClosure>* schedule_save_closure() {
     return &schedule_save_closure_;
   }
@@ -242,7 +276,8 @@ class BookmarkModelTypeProcessorTest : public testing::Test {
   NiceMock<base::MockCallback<base::RepeatingClosure>> schedule_save_closure_;
   BookmarkUndoService bookmark_undo_service_;
   NiceMock<favicon::MockFaviconService> favicon_service_;
-  BookmarkModelTypeProcessor processor_;
+  NiceMock<MockCommitQueue> mock_commit_queue_;
+  std::unique_ptr<BookmarkModelTypeProcessor> processor_;
   std::unique_ptr<bookmarks::BookmarkModel> bookmark_model_;
 };
 
@@ -540,6 +575,7 @@ TEST_F(BookmarkModelTypeProcessorTest,
        ShouldNotRecommitEntitiesWhenEncryptionIsUpToDate) {
   SimulateModelReadyToSync();
   SimulateOnSyncStarting();
+  SimulateConnectSync();
   // Initialize the process to make sure the tracker has been created.
   InitWithSyncedBookmarks({}, processor());
   const SyncedBookmarkTracker* tracker = processor()->GetTrackerForTest();
@@ -561,6 +597,7 @@ TEST_F(BookmarkModelTypeProcessorTest,
       /*response_version=*/0);
   response_data.encryption_key_name = kEncryptionKeyName;
 
+  EXPECT_CALL(*mock_commit_queue(), NudgeForCommit()).Times(0);
   syncer::UpdateResponseDataList updates;
   updates.push_back(std::move(response_data));
   processor()->OnUpdateReceived(model_type_state, std::move(updates));
@@ -687,31 +724,49 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldReuploadLegacyBookmarksOnStart) {
 
   SimulateModelReadyToSync();
   SimulateOnSyncStarting();
+  SimulateConnectSync();
   InitWithSyncedBookmarks(bookmarks, processor());
 
   sync_pb::BookmarkModelMetadata model_metadata =
       BuildBookmarkModelMetadataWithoutFullTitles();
-  // Ensure that bookmark is legacy.
-  model_metadata.clear_bookmarks_full_title_reuploaded();
   ASSERT_FALSE(processor()->GetTrackerForTest()->HasLocalChanges());
 
+  // Simulate browser restart, enable sync reupload and initialize the processor
+  // again.
+  ResetModelTypeProcessor();
+
   base::test::ScopedFeatureList features;
   features.InitAndEnableFeature(switches::kSyncReuploadBookmarkFullTitles);
-  BookmarkModelTypeProcessor new_processor(bookmark_undo_service());
 
   std::string metadata_str;
   model_metadata.SerializeToString(&metadata_str);
-  new_processor.ModelReadyToSync(metadata_str, base::DoNothing(),
-                                 bookmark_model());
+  processor()->ModelReadyToSync(metadata_str, base::DoNothing(),
+                                bookmark_model());
+  SimulateOnSyncStarting();
+  SimulateConnectSync();
+
+  ASSERT_THAT(processor()->GetTrackerForTest(), NotNull());
+  const SyncedBookmarkTracker::Entity* entity =
+      processor()->GetTrackerForTest()->GetEntityForSyncId(kNodeId);
+  ASSERT_THAT(entity, NotNull());
+
+  // Entity should be synced before until first update is received.
+  ASSERT_FALSE(entity->IsUnsynced());
+  ASSERT_FALSE(processor()
+                   ->GetTrackerForTest()
+                   ->BuildBookmarkModelMetadata()
+                   .bookmarks_full_title_reuploaded());
+
+  // Synchronize with the server and get any updates.
+  EXPECT_CALL(*mock_commit_queue(), NudgeForCommit());
+  processor()->OnUpdateReceived(CreateDummyModelTypeState(),
+                                syncer::UpdateResponseDataList());
 
   // Check that all entities are unsynced now and metadata is marked as
   // reuploaded.
-  const size_t entities_count =
-      processor()->GetTrackerForTest()->GetAllEntities().size();
-  EXPECT_EQ(1u, new_processor.GetTrackerForTest()
-                    ->GetEntitiesWithLocalChanges(entities_count)
-                    .size());
-  EXPECT_TRUE(new_processor.GetTrackerForTest()
+  EXPECT_TRUE(entity->IsUnsynced());
+  EXPECT_TRUE(processor()
+                  ->GetTrackerForTest()
                   ->BuildBookmarkModelMetadata()
                   .bookmarks_full_title_reuploaded());
 }

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -265,8 +265,6 @@ SyncedBookmarkTracker::CreateFromBookmarkModelAndMetadata(
     return nullptr;
   }
 
-  tracker->ReuploadBookmarksOnLoadIfNeeded();
-
   return tracker;
 }
 
@@ -732,11 +730,11 @@ SyncedBookmarkTracker::ReorderUnsyncedEntitiesExceptDeletions(
   return ordered_entities;
 }
 
-void SyncedBookmarkTracker::ReuploadBookmarksOnLoadIfNeeded() {
+bool SyncedBookmarkTracker::ReuploadBookmarksOnLoadIfNeeded() {
   if (bookmarks_full_title_reuploaded_ ||
       !base::FeatureList::IsEnabled(
           switches::kSyncReuploadBookmarkFullTitles)) {
-    return;
+    return false;
   }
   for (const auto& sync_id_and_entity : sync_id_to_entities_map_) {
     const SyncedBookmarkTracker::Entity* entity =
@@ -749,7 +747,8 @@ void SyncedBookmarkTracker::ReuploadBookmarksOnLoadIfNeeded() {
     }
     IncrementSequenceNumber(entity);
   }
-  bookmarks_full_title_reuploaded_ = true;
+  SetBookmarksFullTitleReuploaded();
+  return true;
 }
 
 void SyncedBookmarkTracker::TraverseAndAppend(

components/sync_bookmarks/synced_bookmark_tracker.h

@@ -289,6 +289,15 @@ class SyncedBookmarkTracker {
   void CheckAllNodesTracked(
       const bookmarks::BookmarkModel* bookmark_model) const;
 
+  // This method is used to mark all entities except permanent nodes as
+  // unsynced. This will cause reuploading of all bookmarks. The reupload
+  // will be initiated only when the |bookmarks_full_title_reuploaded| field in
+  // BookmarksMetadata is false. This field is used to prevent reuploading after
+  // each browser restart. Returns true if the reupload was initiated.
+  // TODO(crbug.com/1066962): remove this code when most of bookmarks are
+  // reuploaded.
+  bool ReuploadBookmarksOnLoadIfNeeded();
+
  private:
   // Enumeration of possible reasons why persisted metadata are considered
   // corrupted and don't match the bookmark model. Used in UMA metrics. Do not
@@ -334,16 +343,6 @@ class SyncedBookmarkTracker {
   std::vector<const Entity*> ReorderUnsyncedEntitiesExceptDeletions(
       const std::vector<const Entity*>& entities) const;
 
-  // This method is used to mark all entities except permanent nodes as
-  // unsynced. This will cause reuploading of all bookmarks. This reupload
-  // should be initiated only when the |bookmarks_full_title_reuploaded| field
-  // in BookmarksMetadata is false. This field is used to prevent reuploading
-  // after each browser restart. It is set to true in
-  // BuildBookmarkModelMetadata.
-  // TODO(crbug.com/1066962): remove this code when most of bookmarks are
-  // reuploaded.
-  void ReuploadBookmarksOnLoadIfNeeded();
-
   // Recursive method that starting from |node| appends all corresponding
   // entities with updates in top-down order to |ordered_entities|.
   void TraverseAndAppend(const bookmarks::BookmarkNode* node,