[Sync::USS] Bookmark sync conflict resolution

This patch addresses the problem of conflict in bookmarks upon
receiving a remote update for a node that has local changes.

The general polic that is implemented is that server wins except in
case of deletion.

Bug: 516866
Change-Id: I50a85bde068dd9414e4e22bc1a33529fb3dfffa1
Reviewed-on: https://chromium-review.googlesource.com/1157006
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580436}

chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc

@@ -1679,7 +1679,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest, MC_DeleteBookmark) {
 
 // Test a scenario of updating the name of the same bookmark from two clients at
 // the same time.
-IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest,
+IN_PROC_BROWSER_TEST_P(TwoClientBookmarksSyncTestIncludingUssTests,
                        MC_BookmarkNameChangeConflict) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
@@ -1704,7 +1704,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest,
 
 // Test a scenario of updating the URL of the same bookmark from two clients at
 // the same time.
-IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest,
+IN_PROC_BROWSER_TEST_P(TwoClientBookmarksSyncTestIncludingUssTests,
                        MC_BookmarkURLChangeConflict) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
@@ -1731,7 +1731,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest,
 
 // Test a scenario of updating the BM Folder name from two clients at the same
 // time.
-IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest,
+IN_PROC_BROWSER_TEST_P(TwoClientBookmarksSyncTestIncludingUssTests,
                        MC_FolderNameChangeConflict) {
   ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
   DisableVerifier();
@@ -1796,6 +1796,101 @@ IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest,
   ASSERT_FALSE(ContainsDuplicateBookmarks(0));
 }
 
+// Test a scenario of updating an empty BM Folder name from one client and
+// deleting it from another client at the same time.
+IN_PROC_BROWSER_TEST_P(TwoClientBookmarksSyncTestIncludingUssTests,
+                       MC_EmptyFolderNameChangeAndDeletionConflict) {
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+  DisableVerifier();
+
+  const BookmarkNode* folders[2];
+
+  // Create empty folder on both clients.
+  folders[0] = AddFolder(0, IndexedFolderName(0));
+  ASSERT_NE(nullptr, folders[0]);
+  folders[1] = AddFolder(1, IndexedFolderName(0));
+  ASSERT_NE(nullptr, folders[1]);
+
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(BookmarksMatchChecker().Wait());
+  ASSERT_FALSE(ContainsDuplicateBookmarks(0));
+
+  // Simultaneously rename the folder and delete it.
+  SetTitle(0, folders[0], "Folder A++");
+  Remove(1, GetBookmarkBarNode(1), 0);
+  // Both model should match.
+  ASSERT_TRUE(BookmarksMatchChecker().Wait());
+  ASSERT_FALSE(ContainsDuplicateBookmarks(0));
+}
+
+// Test a scenario of updating a non-empty BM Folder name from one client and
+// deleting it from another client at the same time.
+IN_PROC_BROWSER_TEST_P(TwoClientBookmarksSyncTestIncludingUssTests,
+                       MC_NonEmptyFolderNameChangeAndDeletionConflict) {
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+  DisableVerifier();
+
+  const BookmarkNode* folderA[2];
+  const BookmarkNode* folderB[2];
+
+  // Create empty folder A on both clients.
+  folderA[0] = AddFolder(0, IndexedFolderName(0));
+  ASSERT_NE(nullptr, folderA[0]);
+  folderA[1] = AddFolder(1, IndexedFolderName(0));
+  ASSERT_NE(nullptr, folderA[1]);
+
+  // Create folder B under folder A on both clients.
+  folderB[0] = AddFolder(0, folderA[0], 0, IndexedFolderName(1));
+  ASSERT_NE(nullptr, folderB[0]);
+  folderB[1] = AddFolder(1, folderA[1], 0, IndexedFolderName(1));
+  ASSERT_NE(nullptr, folderB[1]);
+
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(BookmarksMatchChecker().Wait());
+  ASSERT_FALSE(ContainsDuplicateBookmarks(0));
+
+  // Simultaneously delete folder A from one client and delete it on the other.
+  SetTitle(0, folderA[0], "Folder A++");
+  Remove(1, GetBookmarkBarNode(1), 0);
+  ASSERT_TRUE(BookmarksMatchChecker().Wait());
+  ASSERT_FALSE(ContainsDuplicateBookmarks(0));
+}
+
+// Test a scenario of deleting a parent BM Folder name from one client and
+// renaming a child in another client at the same time. Deleting the parent will
+// also delete the child and hence there would be a conflict at the child node.
+IN_PROC_BROWSER_TEST_P(TwoClientBookmarksSyncTestIncludingUssTests,
+                       MC_ParentDeleteChildRenameConflict) {
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+  DisableVerifier();
+
+  const BookmarkNode* folderA[2];
+  const BookmarkNode* folderB[2];
+
+  // Create empty folder A on both clients.
+  folderA[0] = AddFolder(0, IndexedFolderName(0));
+  ASSERT_NE(nullptr, folderA[0]);
+  folderA[1] = AddFolder(1, IndexedFolderName(0));
+  ASSERT_NE(nullptr, folderA[1]);
+
+  // Create folder B under folder A on both clients.
+  folderB[0] = AddFolder(0, folderA[0], 0, IndexedFolderName(1));
+  ASSERT_NE(nullptr, folderB[0]);
+  folderB[1] = AddFolder(1, folderA[1], 0, IndexedFolderName(1));
+  ASSERT_NE(nullptr, folderB[1]);
+
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(BookmarksMatchChecker().Wait());
+  ASSERT_FALSE(ContainsDuplicateBookmarks(0));
+
+  // Simultaneously delete folder A from one client and rename folder B on the
+  // other.
+  SetTitle(0, folderB[0], "Folder B++");
+  Remove(1, GetBookmarkBarNode(1), 0);
+  ASSERT_TRUE(BookmarksMatchChecker().Wait());
+  ASSERT_FALSE(ContainsDuplicateBookmarks(0));
+}
+
 IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest,
                        SingleClientEnabledEncryption) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";

components/sync_bookmarks/bookmark_remote_updates_handler.h

@@ -65,7 +65,7 @@ class BookmarkRemoteUpdatesHandler {
   void ProcessRemoteUpdate(const syncer::UpdateResponseData& update,
                            const SyncedBookmarkTracker::Entity* tracked_entity);
 
-  // Process a remote delete of a bookmark node. |update_entity| must not be a
+  // Processes a remote delete of a bookmark node. |update_entity| must not be a
   // deletion. |tracked_entity| is the tracked entity for that server_id. It is
   // passed as a dependency instead of performing a lookup inside
   // ProcessRemoteDelete() to avoid wasting CPU cycles for doing another lookup
@@ -73,6 +73,15 @@ class BookmarkRemoteUpdatesHandler {
   void ProcessRemoteDelete(const syncer::EntityData& update_entity,
                            const SyncedBookmarkTracker::Entity* tracked_entity);
 
+  // Processes a conflict where the bookmark has been changed both locally and
+  // remotely. It applies the general policy the server wins expcet in the case
+  // of remote deletions in which local wins. |tracked_entity| is the tracked
+  // entity for that server_id. It is passed as a dependency instead of
+  // performing a lookup inside ProcessRemoteDelete() to avoid wasting CPU
+  // cycles for doing another lookup (this code runs on the UI thread).
+  void ProcessConflict(const syncer::UpdateResponseData& update,
+                       const SyncedBookmarkTracker::Entity* tracked_entity);
+
   // Recursively removes the entities corresponding to |node| and its children
   // from |bookmark_tracker_|.
   void RemoveEntityAndChildrenFromTracker(const bookmarks::BookmarkNode* node);

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -136,7 +136,7 @@ void SyncedBookmarkTracker::Update(
   auto it = sync_id_to_entities_map_.find(sync_id);
   Entity* entity = it->second.get();
   DCHECK(entity);
-  entity->metadata()->set_server_id(sync_id);
+  DCHECK_EQ(entity->metadata()->server_id(), sync_id);
   entity->metadata()->set_server_version(server_version);
   entity->metadata()->set_modification_time(
       syncer::TimeToProtoTime(modification_time));
@@ -146,6 +146,14 @@ void SyncedBookmarkTracker::Update(
   // |ordered_local_tombstones_| as well if it has been locally deleted.
 }
 
+void SyncedBookmarkTracker::UpdateServerVersion(const std::string& sync_id,
+                                                int64_t server_version) {
+  auto it = sync_id_to_entities_map_.find(sync_id);
+  Entity* entity = it->second.get();
+  DCHECK(entity);
+  entity->metadata()->set_server_version(server_version);
+}
+
 void SyncedBookmarkTracker::MarkDeleted(const std::string& sync_id) {
   auto it = sync_id_to_entities_map_.find(sync_id);
   Entity* entity = it->second.get();
@@ -225,7 +233,7 @@ SyncedBookmarkTracker::GetEntitiesWithLocalChanges(size_t max_entries) const {
        sync_id_to_entities_map_) {
     Entity* entity = pair.second.get();
     if (entity->metadata()->is_deleted()) {
-      // Deletion are stored sorted in |ordered_local_tombstones_| and will be
+      // Deletions are stored sorted in |ordered_local_tombstones_| and will be
       // added later.
       continue;
     }
@@ -344,6 +352,18 @@ void SyncedBookmarkTracker::UpdateUponCommitResponse(
   }
 }
 
+void SyncedBookmarkTracker::AckSequenceNumber(const std::string& sync_id) {
+  auto it = sync_id_to_entities_map_.find(sync_id);
+  Entity* entity =
+      it != sync_id_to_entities_map_.end() ? it->second.get() : nullptr;
+  if (!entity) {
+    DLOG(WARNING) << "Trying to update a non existing entity.";
+    return;
+  }
+  entity->metadata()->set_acked_sequence_number(
+      entity->metadata()->sequence_number());
+}
+
 bool SyncedBookmarkTracker::IsEmpty() const {
   return sync_id_to_entities_map_.empty();
 }

components/sync_bookmarks/synced_bookmark_tracker.h

@@ -106,6 +106,9 @@ class SyncedBookmarkTracker {
               const sync_pb::UniquePosition& unique_position,
               const sync_pb::EntitySpecifics& specifics);
 
+  // Updates the server version of an existing entry for the |sync_id|.
+  void UpdateServerVersion(const std::string& sync_id, int64_t server_version);
+
   // This class maintains the order of calls to this method and the same order
   // is gauaranteed when returning local changes in
   // GetEntitiesWithLocalChanges() as well as in BuildBookmarkModelMetadata().
@@ -146,6 +149,11 @@ class SyncedBookmarkTracker {
                                 int64_t acked_sequence_number,
                                 int64_t server_version);
 
+  // Set the value of |EntityMetadata.acked_sequence_number| in the entity with
+  // |sync_id| to be equal to |EntityMetadata.sequence_number| such that it is
+  // not returned in GetEntitiesWithLocalChanges().
+  void AckSequenceNumber(const std::string& sync_id);
+
   // Whether the tracker is empty or not.
   bool IsEmpty() const;
 

components/sync_bookmarks/synced_bookmark_tracker_unittest.cc

@@ -150,6 +150,38 @@ TEST(SyncedBookmarkTrackerTest,
   // request in a separate test probably.
 }
 
+TEST(SyncedBookmarkTrackerTest, ShouldAckSequenceNumber) {
+  SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
+                                std::make_unique<sync_pb::ModelTypeState>());
+  const std::string kSyncId = "SYNC_ID";
+  const int64_t kId = 1;
+  const int64_t kServerVersion = 1000;
+  const base::Time kModificationTime(base::Time::Now() -
+                                     base::TimeDelta::FromSeconds(1));
+  const sync_pb::UniquePosition unique_position;
+  const sync_pb::EntitySpecifics specifics =
+      GenerateSpecifics(/*title=*/std::string(), /*url=*/std::string());
+  bookmarks::BookmarkNode node(kId, GURL());
+  tracker.Add(kSyncId, &node, kServerVersion, kModificationTime,
+              unique_position, specifics);
+
+  // Test simple scenario of ack'ing an incrememented sequence number.
+  EXPECT_THAT(tracker.HasLocalChanges(), Eq(false));
+  tracker.IncrementSequenceNumber(kSyncId);
+  EXPECT_THAT(tracker.HasLocalChanges(), Eq(true));
+  tracker.AckSequenceNumber(kSyncId);
+  EXPECT_THAT(tracker.HasLocalChanges(), Eq(false));
+
+  // Test ack'ing of a mutliple times incremented sequence number.
+  tracker.IncrementSequenceNumber(kSyncId);
+  EXPECT_THAT(tracker.HasLocalChanges(), Eq(true));
+  tracker.IncrementSequenceNumber(kSyncId);
+  tracker.IncrementSequenceNumber(kSyncId);
+  EXPECT_THAT(tracker.HasLocalChanges(), Eq(true));
+  tracker.AckSequenceNumber(kSyncId);
+  EXPECT_THAT(tracker.HasLocalChanges(), Eq(false));
+}
+
 TEST(SyncedBookmarkTrackerTest, ShouldUpdateUponCommitResponseWithNewId) {
   SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
                                 std::make_unique<sync_pb::ModelTypeState>());