Enforce bookmark GUID uniqueness as model invariant

A bookmark's GUID must be unique and otherwise BookmarkModel's invariant
can be considered violated.

In this patch, the invariant is enforced via DCHECKs upon node creation,
which is sufficient because GUIDs are inmutable.

Change-Id: Ia3f48e98027ae9a8468bd0dc578843047af79712
Bug: 978430
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2031024
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#744286}

components/bookmarks/browser/bookmark_model.cc

@@ -115,6 +115,18 @@ class EmptyUndoDelegate : public BookmarkUndoDelegate {
   DISALLOW_COPY_AND_ASSIGN(EmptyUndoDelegate);
 };
 
+#if DCHECK_IS_ON()
+void AddGuidsToIndexRecursive(const BookmarkNode* node,
+                              std::set<std::string>* guid_index) {
+  bool success = guid_index->insert(node->guid()).second;
+  DCHECK(success);
+
+  // Recurse through children.
+  for (size_t i = node->children().size(); i > 0; --i)
+    AddGuidsToIndexRecursive(node->children()[i - 1].get(), guid_index);
+}
+#endif  // DCHECK_IS_ON()
+
 }  // namespace
 
 // BookmarkModel --------------------------------------------------------------
@@ -414,9 +426,11 @@ void BookmarkModel::SetURL(const BookmarkNode* node, const GURL& url) {
   for (BookmarkModelObserver& observer : observers_)
     observer.OnWillChangeBookmarkNode(this, node);
 
+  // The title index doesn't support changing the URL, instead we remove then
+  // add it back.
   titled_url_index_->Remove(mutable_node);
   url_index_->SetUrl(mutable_node, url);
-  AddNodeToIndexRecursive(mutable_node);
+  titled_url_index_->Add(mutable_node);
 
   if (store_)
     store_->ScheduleSave();
@@ -830,6 +844,12 @@ void BookmarkModel::RemoveNodeFromIndexRecursive(BookmarkNode* node) {
   if (node->is_url())
     titled_url_index_->Remove(node);
 
+  // Note that |guid_index_| is used for DCHECK-enabled builds only.
+#if DCHECK_IS_ON()
+  DCHECK(guid_index_.erase(node->guid()))
+      << "Bookmark GUID missing in index: " << node->guid();
+#endif  // DCHECK_IS_ON()
+
   CancelPendingFaviconLoadRequests(node);
 
   // Recurse through children.
@@ -863,6 +883,10 @@ void BookmarkModel::DoneLoading(std::unique_ptr<BookmarkLoadDetails> details) {
   other_node_ = details->other_folder_node();
   mobile_node_ = details->mobile_folder_node();
 
+#if DCHECK_IS_ON()
+  AddGuidsToIndexRecursive(root_, &guid_index_);
+#endif  // DCHECK_IS_ON()
+
   titled_url_index_->SetNodeSorter(
       std::make_unique<TypedCountSorter>(client_.get()));
   // Sorting the permanent nodes has to happen on the main thread, so we do it
@@ -910,6 +934,14 @@ void BookmarkModel::AddNodeToIndexRecursive(BookmarkNode* node) {
 
   if (node->is_url())
     titled_url_index_->Add(node);
+
+  // The node's GUID must be unique. Note that |guid_index_| is used for
+  // DCHECK-enabled builds only.
+#if DCHECK_IS_ON()
+  DCHECK(guid_index_.insert(node->guid()).second)
+      << "Duplicate bookmark GUID: " << node->guid();
+#endif  // DCHECK_IS_ON()
+
   for (const auto& child : node->children())
     AddNodeToIndexRecursive(child.get());
 }

components/bookmarks/browser/bookmark_model.h

@@ -14,6 +14,7 @@
 #include <vector>
 
 #include "base/compiler_specific.h"
+#include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
@@ -425,6 +426,11 @@ class BookmarkModel : public BookmarkUndoProvider,
 
   std::unique_ptr<TitledUrlIndex> titled_url_index_;
 
+#if DCHECK_IS_ON()
+  // GUID index used to verify uniqueness in DCHECK-enabled builds.
+  std::set<std::string> guid_index_;
+#endif  // DCHECK_IS_ON()
+
   // Owned by |model_loader_|.
   // WARNING: in some tests this does *not* refer to
   // |ModelLoader::history_bookmark_model_|. This is because some tests

components/sync_bookmarks/bookmark_specifics_conversions_unittest.cc

@@ -244,7 +244,6 @@ TEST(BookmarkSpecificsConversionsTest, ShouldCreateBookmarkNodeFromSpecifics) {
 
 TEST(BookmarkSpecificsConversionsTest,
      ShouldCreateBookmarkNodeFromSpecificsWithIllegalTitle) {
-  const std::string kGuid = base::GenerateGUID();
   std::unique_ptr<bookmarks::BookmarkModel> model =
       bookmarks::TestBookmarkClient::CreateModel();
   testing::NiceMock<favicon::MockFaviconService> favicon_service;
@@ -256,7 +255,7 @@ TEST(BookmarkSpecificsConversionsTest,
     sync_pb::EntitySpecifics specifics;
     sync_pb::BookmarkSpecifics* bm_specifics = specifics.mutable_bookmark();
     bm_specifics->set_url("http://www.url.com");
-    bm_specifics->set_guid(kGuid);
+    bm_specifics->set_guid(base::GenerateGUID());
     // Legacy clients append an extra space to illegal clients.
     bm_specifics->set_title(illegal_title + " ");
     const bookmarks::BookmarkNode* node = CreateBookmarkNodeFromSpecifics(