[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}

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,