Use external ID to match native model nodes during bookmark association.

This is a resubmit of https://codereview.chromium.org/904083002
which was reverted due to an unrelated flaky test. There are no
changes compared to the original patch, hence skipping the code
review (zea@ was the original reviewer).

Original patch description:

The fix improves matching of nodes in BookmarkModelAssociator in
situations where there are multiple bookmarks or folders with the
same titles or URLs.
This will address one particular scenario leading to bookmark
duplication (see crbug.com/118105).

1) In BookmarkModelAssociator::BuildAssociations, when there are
multiple native model nodes with matching title / URL, a secondary
match on external ID is used to pick a preferred one; otherwise
the first matching node is returned.
The preferred match on external ID should be applicable in most
situations except when the native model has been rebuilt from scratch.
Picking a wrong folder during the association process results in
duplicating the entire subtree within the wrong folder. This issue
should be addressed now.
2) In BookmarkModelAssociator::ApplyDeletesFromSyncJournal the external
ID match is now the primary criteria for selecting a native model
node to be deleted. The previous implementation would pick an arbitrary
native model node based on just the title / URL match anywhere in the
node hierarchy. That would happen every time after deleting a bookmark
or folder and recreating it in another place.
Since external IDs might be reused, there is a secondary match on
title and URL to ensure that the right node gets deleted. To avoid
costly O(N*M) algorithm (where N is number of bookmarks and M is
number of entries in delete journal), the implementation uses a set
of external IDs to reduce the cost to O(N*logM).

BUG=456228
TBR=zea@chromium.org

Review URL: https://codereview.chromium.org/912693002

Cr-Commit-Position: refs/heads/master@{#317688}

chrome/browser/sync/glue/bookmark_model_associator.h

@@ -140,10 +140,6 @@ class BookmarkModelAssociator
       const bookmarks::BookmarkNode* permanent_node,
       const std::string& tag) WARN_UNUSED_RESULT;
 
-  // Compare the properties of a pair of nodes from either domain.
-  bool NodesMatch(const bookmarks::BookmarkNode* bookmark,
-                  const syncer::BaseNode* sync_node) const;
-
   // Check whether bookmark model and sync model are synced by comparing
   // their transaction versions.
   // Returns a PERSISTENCE_ERROR if a transaction mismatch was detected where

chrome/browser/sync/profile_sync_service_bookmark_unittest.cc

@@ -43,7 +43,7 @@
 #include "sync/internal_api/public/write_node.h"
 #include "sync/internal_api/public/write_transaction.h"
 #include "sync/internal_api/syncapi_internal.h"
-#include "sync/syncable/mutable_entry.h"  // TODO(tim): Remove. Bug 131130.
+#include "sync/syncable/mutable_entry.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -381,16 +381,17 @@ class ProfileSyncServiceBookmarkTest : public testing::Test {
   // the sync model directly after ModelAssociation.  This function can be
   // invoked prior to model association to set up first-time sync model
   // association scenarios.
-  int64 AddBookmarkToShare(syncer::WriteTransaction *trans,
+  int64 AddBookmarkToShare(syncer::WriteTransaction* trans,
                            int64 parent_id,
-                           std::string title) {
+                           const std::string& title,
+                           const std::string& url) {
     EXPECT_FALSE(model_associator_);
 
     syncer::ReadNode parent(trans);
     EXPECT_EQ(BaseNode::INIT_OK, parent.InitByIdLookup(parent_id));
 
     sync_pb::BookmarkSpecifics specifics;
-    specifics.set_url("http://www.google.com/search?q=" + title);
+    specifics.set_url(url);
     specifics.set_title(title);
 
     syncer::WriteNode node(trans);
@@ -795,12 +796,12 @@ TEST_F(ProfileSyncServiceBookmarkTest, InitialModelAssociate) {
   {
     syncer::WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
     for (int i = 0; i < kNumFolders; ++i) {
-      int64 folder_id = AddFolderToShare(&trans,
-                                         base::StringPrintf("folder%05d", i));
+      int64 folder_id =
+          AddFolderToShare(&trans, base::StringPrintf("folder%05d", i));
       for (int j = 0; j < kNumBookmarksPerFolder; ++j) {
-        AddBookmarkToShare(&trans,
-                           folder_id,
-                           base::StringPrintf("bookmark%05d", j));
+        AddBookmarkToShare(
+            &trans, folder_id, base::StringPrintf("bookmark%05d", j),
+            base::StringPrintf("http://www.google.com/search?q=%05d", j));
       }
     }
   }
@@ -811,6 +812,189 @@ TEST_F(ProfileSyncServiceBookmarkTest, InitialModelAssociate) {
   ExpectModelMatch();
 }
 
+// Tests bookmark association when nodes exists in the native model only.
+// These entries should be copied to Sync directory during association process.
+TEST_F(ProfileSyncServiceBookmarkTest,
+       InitialModelAssociateWithBookmarkModelNodes) {
+  LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);
+  const BookmarkNode* folder =
+      model_->AddFolder(model_->other_node(), 0, base::ASCIIToUTF16("foobar"));
+  model_->AddFolder(folder, 0, base::ASCIIToUTF16("nested"));
+  model_->AddURL(folder, 0, base::ASCIIToUTF16("Internets #1 Pies Site"),
+                 GURL("http://www.easypie.com/"));
+  model_->AddURL(folder, 1, base::ASCIIToUTF16("Airplanes"),
+                 GURL("http://www.easyjet.com/"));
+
+  StartSync();
+  ExpectModelMatch();
+}
+
+// Tests bookmark association case when there is an entry in the delete journal
+// that matches one of native bookmarks.
+TEST_F(ProfileSyncServiceBookmarkTest, InitialModelAssociateWithDeleteJournal) {
+  LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);
+  const BookmarkNode* folder = model_->AddFolder(model_->bookmark_bar_node(), 0,
+                                                 base::ASCIIToUTF16("foobar"));
+  const BookmarkNode* bookmark =
+      model_->AddURL(folder, 0, base::ASCIIToUTF16("Airplanes"),
+                     GURL("http://www.easyjet.com/"));
+
+  CreatePermanentBookmarkNodes();
+
+  // Create entries matching the folder and the bookmark above.
+  int64 folder_id, bookmark_id;
+  {
+    syncer::WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+    folder_id = AddFolderToShare(&trans, "foobar");
+    bookmark_id = AddBookmarkToShare(&trans, folder_id, "Airplanes",
+                                     "http://www.easyjet.com/");
+  }
+
+  // Associate the bookmark sync node with the native model one and make
+  // it deleted.
+  {
+    syncer::WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+    syncer::WriteNode node(&trans);
+    EXPECT_EQ(BaseNode::INIT_OK, node.InitByIdLookup(bookmark_id));
+
+    node.GetMutableEntryForTest()->PutLocalExternalId(bookmark->id());
+    node.GetMutableEntryForTest()->PutServerIsDel(true);
+    node.GetMutableEntryForTest()->PutIsDel(true);
+  }
+
+  ASSERT_TRUE(AssociateModels());
+  ExpectModelMatch();
+
+  // The bookmark node should be deleted.
+  EXPECT_EQ(0, folder->child_count());
+}
+
+// Tests that the external ID is used to match the right folder amoung
+// multiple folders with the same name during the native model traversal.
+// Also tests that the external ID is used to match the right bookmark
+// among multiple identical bookmarks when dealing with the delete journal.
+TEST_F(ProfileSyncServiceBookmarkTest,
+       InitialModelAssociateVerifyExternalIdMatch) {
+  LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);
+  const int kNumFolders = 10;
+  const int kNumBookmarks = 10;
+  const int kFolderToIncludeBookmarks = 7;
+  const int kBookmarkToDelete = 4;
+
+  int64 folder_ids[kNumFolders];
+  int64 bookmark_ids[kNumBookmarks];
+
+  // Create native folders and bookmarks with identical names. Only
+  // one of the folders contains bookmarks and others are empty. Here is the
+  // expected tree shape:
+  // Bookmarks bar
+  // +- folder (#0)
+  // +- folder (#1)
+  // ...
+  // +- folder (#7) <-- Only this folder contains bookmarks.
+  //    +- bookmark (#0)
+  //    +- bookmark (#1)
+  //    ...
+  //    +- bookmark (#4) <-- Only this one bookmark should be removed later.
+  //    ...
+  //    +- bookmark (#9)
+  // ...
+  // +- folder (#9)
+
+  const BookmarkNode* parent_folder = nullptr;
+  for (int i = 0; i < kNumFolders; i++) {
+    const BookmarkNode* folder = model_->AddFolder(
+        model_->bookmark_bar_node(), i, base::ASCIIToUTF16("folder"));
+    folder_ids[i] = folder->id();
+    if (i == kFolderToIncludeBookmarks) {
+      parent_folder = folder;
+    }
+  }
+
+  for (int i = 0; i < kNumBookmarks; i++) {
+    const BookmarkNode* bookmark =
+        model_->AddURL(parent_folder, i, base::ASCIIToUTF16("bookmark"),
+                       GURL("http://www.google.com/"));
+    bookmark_ids[i] = bookmark->id();
+  }
+
+  // Number of nodes in bookmark bar before association.
+  int total_node_count = model_->bookmark_bar_node()->GetTotalNodeCount();
+
+  CreatePermanentBookmarkNodes();
+
+  int64 sync_bookmark_id_to_delete = 0;
+  {
+    // Create sync folders matching native folders above.
+    int64 parent_id = 0;
+    syncer::WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+    // Create in reverse order because AddFolderToShare passes NULL for
+    // |predecessor| argument.
+    for (int i = kNumFolders - 1; i >= 0; i--) {
+      int64 id = AddFolderToShare(&trans, "folder");
+
+      // Pre-map sync folders to native folders by setting
+      // external ID. This will verify that the association algorithm picks
+      // the right ones despite all of them having identical names.
+      // More specifically this will help to avoid cloning bookmarks from
+      // a wrong folder.
+      syncer::WriteNode node(&trans);
+      EXPECT_EQ(BaseNode::INIT_OK, node.InitByIdLookup(id));
+      node.GetMutableEntryForTest()->PutLocalExternalId(folder_ids[i]);
+
+      if (i == kFolderToIncludeBookmarks) {
+        parent_id = id;
+      }
+    }
+
+    // Create sync bookmark matching native bookmarks above in reverse order
+    // because AddBookmarkToShare passes NULL for |predecessor| argument.
+    for (int i = kNumBookmarks - 1; i >= 0; i--) {
+      int id = AddBookmarkToShare(&trans, parent_id, "bookmark",
+                                  "http://www.google.com/");
+
+      // Pre-map sync bookmarks to native bookmarks by setting
+      // external ID. This will verify that the association algorithm picks
+      // the right ones despite all of them having identical names and URLs.
+      syncer::WriteNode node(&trans);
+      EXPECT_EQ(BaseNode::INIT_OK, node.InitByIdLookup(id));
+      node.GetMutableEntryForTest()->PutLocalExternalId(bookmark_ids[i]);
+
+      if (i == kBookmarkToDelete) {
+        sync_bookmark_id_to_delete = id;
+      }
+    }
+  }
+
+  // Make one bookmark deleted.
+  {
+    syncer::WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+    syncer::WriteNode node(&trans);
+    EXPECT_EQ(BaseNode::INIT_OK,
+              node.InitByIdLookup(sync_bookmark_id_to_delete));
+
+    node.GetMutableEntryForTest()->PutServerIsDel(true);
+    node.GetMutableEntryForTest()->PutIsDel(true);
+  }
+
+  // Perform association.
+  ASSERT_TRUE(AssociateModels());
+  ExpectModelMatch();
+
+  // Only one native node should have been deleted and no nodes cloned due to
+  // matching folder names.
+  EXPECT_EQ(kNumFolders, model_->bookmark_bar_node()->child_count());
+  EXPECT_EQ(kNumBookmarks - 1, parent_folder->child_count());
+  EXPECT_EQ(total_node_count - 1,
+            model_->bookmark_bar_node()->GetTotalNodeCount());
+
+  // Verify that the right bookmark got deleted and no bookmarks reordered.
+  for (int i = 0; i < parent_folder->child_count(); i++) {
+    int index_in_bookmark_ids = (i < kBookmarkToDelete) ? i : i + 1;
+    EXPECT_EQ(bookmark_ids[index_in_bookmark_ids],
+              parent_folder->GetChild(i)->id());
+  }
+}
 
 TEST_F(ProfileSyncServiceBookmarkTest, BookmarkModelOperations) {
   LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);

sync/internal_api/delete_journal.cc

@@ -21,6 +21,8 @@ void DeleteJournal::GetBookmarkDeleteJournals(
       deleted_entries.begin(); i != deleted_entries.end(); ++i) {
     delete_journal_list->push_back(BookmarkDeleteJournal());
     delete_journal_list->back().id = (*i)->ref(syncer::syncable::META_HANDLE);
+    delete_journal_list->back().external_id =
+        (*i)->ref(syncer::syncable::LOCAL_EXTERNAL_ID);
     delete_journal_list->back().is_folder = (*i)->ref(syncer::syncable::IS_DIR);
 
     const sync_pb::EntitySpecifics& specifics = (*i)->ref(

sync/internal_api/public/delete_journal.h

@@ -17,6 +17,7 @@ class BaseTransaction;
 
 struct BookmarkDeleteJournal {
   int64 id;   // Metahandle of delete journal entry.
+  int64 external_id;  // Bookmark ID in the native model.
   bool is_folder;
   sync_pb::EntitySpecifics specifics;
 };