[Sync::USS] Handle corrupted bookmarks metadata

At startup, the persisted sync metadata (if available) are loaded
and tracked.

This CL adds some integrity checks and makes sure that corrupted
metadata are ignored, and that Sync starts clean in such case.


Bug: 516866
Change-Id: Idf42cce2306f18a94941d40caa1816f737732156
Reviewed-on: https://chromium-review.googlesource.com/c/1274106Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599105}

components/sync_bookmarks/BUILD.gn

@@ -38,6 +38,7 @@ static_library("sync_bookmarks") {
     "//components/keyed_service/core:core",
     "//components/sync",
     "//components/undo",
+    "//ui/base",
     "//ui/gfx",
   ]
 }

components/sync_bookmarks/DEPS

@@ -7,5 +7,6 @@ include_rules = [
   "+components/prefs",
   "+components/sync",
   "+components/undo",
+  "+ui/base",
   "+ui/gfx",
 ]

components/sync_bookmarks/bookmark_model_type_processor.cc

@@ -216,10 +216,9 @@ void BookmarkModelTypeProcessor::ModelReadyToSync(
   sync_pb::BookmarkModelMetadata model_metadata;
   model_metadata.ParseFromString(metadata_str);
 
-  auto model_type_state = std::make_unique<sync_pb::ModelTypeState>();
-  model_type_state->Swap(model_metadata.mutable_model_type_state());
-
-  if (model_type_state->initial_sync_done()) {
+  if (model_metadata.model_type_state().initial_sync_done() &&
+      SyncedBookmarkTracker::BookmarkModelMatchesMetadata(model,
+                                                          model_metadata)) {
     std::vector<NodeMetadataPair> nodes_metadata;
     for (sync_pb::BookmarkMetadata& bookmark_metadata :
          *model_metadata.mutable_bookmarks_metadata()) {
@@ -227,35 +226,20 @@ void BookmarkModelTypeProcessor::ModelReadyToSync(
       // all nodes and store in a map keyed by id instead of doing a lookup for
       // every id.
       const bookmarks::BookmarkNode* node = nullptr;
-      if (bookmark_metadata.metadata().is_deleted()) {
-        if (bookmark_metadata.has_id()) {
-          DLOG(ERROR) << "Error when decoding sync metadata: Tombstones "
-                         "shouldn't have a bookmark id.";
-          continue;
-        }
-      } else {
-        if (!bookmark_metadata.has_id()) {
-          DLOG(ERROR)
-              << "Error when decoding sync metadata: Bookmark id is missing.";
-          continue;
-        }
+      if (!bookmark_metadata.metadata().is_deleted()) {
         node = GetBookmarkNodeByID(bookmark_model_, bookmark_metadata.id());
-        if (node == nullptr) {
-          DLOG(ERROR) << "Error when decoding sync metadata: Cannot find the "
-                         "bookmark node.";
-          continue;
-        }
+        DCHECK(node);
       }
       auto metadata = std::make_unique<sync_pb::EntityMetadata>();
       metadata->Swap(bookmark_metadata.mutable_metadata());
       nodes_metadata.emplace_back(node, std::move(metadata));
     }
-    // TODO(crbug.com/516866): Handle local nodes that don't have a
-    // corresponding
-    // metadata.
+    auto model_type_state = std::make_unique<sync_pb::ModelTypeState>();
+    model_type_state->Swap(model_metadata.mutable_model_type_state());
     StartTrackingMetadata(std::move(nodes_metadata),
                           std::move(model_type_state));
-  } else if (!model_metadata.bookmarks_metadata().empty()) {
+  } else if (!model_metadata.model_type_state().initial_sync_done() &&
+             !model_metadata.bookmarks_metadata().empty()) {
     DLOG(ERROR)
         << "Persisted Metadata not empty while initial sync is not done.";
   }

components/sync_bookmarks/bookmark_model_type_processor.h

@@ -63,7 +63,7 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor,
   // restored via ModelReadyToSync() below.
   std::string EncodeSyncMetadata() const;
 
-  // It mainly decodes a BookmarkModelMetadata proto seralized in
+  // It mainly decodes a BookmarkModelMetadata proto serialized in
   // |metadata_str|, and uses it to fill in the tracker and the model type state
   // objects. |model| must not be null and must outlive this object. It is used
   // to the retrieve the local node ids, and is stored in the processor to be

components/sync_bookmarks/bookmark_model_type_processor_unittest.cc

@@ -30,7 +30,11 @@ namespace sync_bookmarks {
 namespace {
 
 const char kBookmarkBarTag[] = "bookmark_bar";
+const char kOtherBookmarksTag[] = "other_bookmarks";
+const char kMobileBookmarksTag[] = "synced_bookmarks";
 const char kBookmarkBarId[] = "bookmark_bar_id";
+const char kOtherBookmarksId[] = "other_bookmarks_id";
+const char kMobileBookmarksId[] = "mobile_bookmarks_id";
 const char kBookmarksRootId[] = "root_id";
 const char kCacheGuid[] = "generated_id";
 
@@ -79,8 +83,8 @@ void AssertState(const BookmarkModelTypeProcessor* processor,
   const SyncedBookmarkTracker* tracker = processor->GetTrackerForTest();
 
   // Make sure the tracker contains all bookmarks in |bookmarks| + the
-  // bookmark bar node.
-  ASSERT_THAT(tracker->TrackedEntitiesCountForTest(), Eq(bookmarks.size() + 1));
+  // 3 permanent nodes.
+  ASSERT_THAT(tracker->TrackedEntitiesCountForTest(), Eq(bookmarks.size() + 3));
 
   for (BookmarkInfo bookmark : bookmarks) {
     const SyncedBookmarkTracker::Entity* entity =
@@ -103,22 +107,26 @@ void InitWithSyncedBookmarks(const std::vector<BookmarkInfo>& bookmarks,
   syncer::UpdateResponseDataList updates;
   syncer::UniquePosition pos = syncer::UniquePosition::InitialPosition(
       syncer::UniquePosition::RandomSuffix());
-  // Add update for the permanent folder "Bookmarks bar".
+  // Add update for the permanent folders "Bookmarks bar", "Other Bookmarks" and
+  // "Mobile Bookmarks".
   updates.push_back(
       CreateUpdateResponseData({kBookmarkBarId, std::string(), std::string(),
                                 kBookmarksRootId, kBookmarkBarTag},
                                pos, /*response_version=*/0));
+  updates.push_back(
+      CreateUpdateResponseData({kOtherBookmarksId, std::string(), std::string(),
+                                kBookmarksRootId, kOtherBookmarksTag},
+                               pos, /*response_version=*/0));
+  updates.push_back(CreateUpdateResponseData(
+      {kMobileBookmarksId, std::string(), std::string(), kBookmarksRootId,
+       kMobileBookmarksTag},
+      pos, /*response_version=*/0));
   for (BookmarkInfo bookmark : bookmarks) {
     pos = syncer::UniquePosition::After(pos,
                                         syncer::UniquePosition::RandomSuffix());
     updates.push_back(
         CreateUpdateResponseData(bookmark, pos, /*response_version=*/0));
   }
-  // TODO(crbug.com/516866): Remove after a proper positioning for remote
-  // updates is implemented. Reversing the updates because the sorting algorithm
-  // isn't stable. This is OK for now because once proper positioning is
-  // implemented, the siblings update requests order would be irrelvant.
-  std::reverse(updates.begin(), updates.end());
   processor->OnUpdateReceived(CreateDummyModelTypeState(), updates);
   AssertState(processor, bookmarks);
 }
@@ -278,11 +286,20 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldDecodeSyncMetadata) {
   // within the processor.
   sync_pb::BookmarkModelMetadata model_metadata;
   model_metadata.mutable_model_type_state()->set_initial_sync_done(true);
-  // Add an entry for bookmark bar.
+  // Add entries for the permanent nodes. TestBookmarkClient adds all of them.
   sync_pb::BookmarkMetadata* bookmark_metadata =
       model_metadata.add_bookmarks_metadata();
   bookmark_metadata->set_id(bookmark_bar_node->id());
   bookmark_metadata->mutable_metadata()->set_server_id(kBookmarkBarId);
+
+  bookmark_metadata = model_metadata.add_bookmarks_metadata();
+  bookmark_metadata->set_id(bookmark_model()->other_node()->id());
+  bookmark_metadata->mutable_metadata()->set_server_id(kOtherBookmarksId);
+
+  bookmark_metadata = model_metadata.add_bookmarks_metadata();
+  bookmark_metadata->set_id(bookmark_model()->mobile_node()->id());
+  bookmark_metadata->mutable_metadata()->set_server_id(kMobileBookmarksId);
+
   // Add an entry for the bookmark node.
   bookmark_metadata = model_metadata.add_bookmarks_metadata();
   bookmark_metadata->set_id(bookmarknode->id());
@@ -331,6 +348,50 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldDecodeEncodedSyncMetadata) {
   AssertState(&new_processor, bookmarks);
 }
 
+TEST_F(BookmarkModelTypeProcessorTest,
+       ShouldIgnoreNonEmptyMetadataWhileSyncNotDone) {
+  sync_pb::BookmarkModelMetadata model_metadata;
+  model_metadata.mutable_model_type_state()->set_initial_sync_done(false);
+  // Add entries to the metadata.
+  sync_pb::BookmarkMetadata* bookmark_metadata =
+      model_metadata.add_bookmarks_metadata();
+  bookmark_metadata->set_id(bookmark_model()->bookmark_bar_node()->id());
+  bookmark_metadata->mutable_metadata()->set_server_id(kBookmarkBarId);
+
+  // Create a new processor and init it with the metadata str.
+  BookmarkModelTypeProcessor new_processor(bookmark_undo_service());
+
+  std::string metadata_str;
+  model_metadata.SerializeToString(&metadata_str);
+  new_processor.ModelReadyToSync(metadata_str, base::DoNothing(),
+                                 bookmark_model());
+  // Metadata are corrupted, so no tracker should have been created.
+  EXPECT_THAT(new_processor.GetTrackerForTest(), IsNull());
+}
+
+TEST_F(BookmarkModelTypeProcessorTest,
+       ShouldIgnoreMetadataNotMatchingTheModel) {
+  sync_pb::BookmarkModelMetadata model_metadata;
+  model_metadata.mutable_model_type_state()->set_initial_sync_done(true);
+  // Add entries for only the bookmark bar. However, the TestBookmarkClient will
+  // create all the 3 permanent nodes.
+  sync_pb::BookmarkMetadata* bookmark_metadata =
+      model_metadata.add_bookmarks_metadata();
+  bookmark_metadata->set_id(bookmark_model()->bookmark_bar_node()->id());
+  bookmark_metadata->mutable_metadata()->set_server_id(kBookmarkBarId);
+
+  // Create a new processor and init it with the metadata str.
+  BookmarkModelTypeProcessor new_processor(bookmark_undo_service());
+
+  std::string metadata_str;
+  model_metadata.SerializeToString(&metadata_str);
+  new_processor.ModelReadyToSync(metadata_str, base::DoNothing(),
+                                 bookmark_model());
+
+  // Metadata are corrupted, so no tracker should have been created.
+  EXPECT_THAT(new_processor.GetTrackerForTest(), IsNull());
+}
+
 // Verifies that the model type state stored in the tracker gets
 // updated upon handling remote updates by assigning a new encryption
 // key name.

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -4,6 +4,7 @@
 
 #include "components/sync_bookmarks/synced_bookmark_tracker.h"
 
+#include <algorithm>
 #include <set>
 #include <utility>
 
@@ -11,11 +12,13 @@
 #include "base/sha1.h"
 #include "base/stl_util.h"
 #include "base/trace_event/memory_usage_estimator.h"
+#include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/bookmark_node.h"
 #include "components/sync/base/time.h"
 #include "components/sync/base/unique_position.h"
 #include "components/sync/model/entity_data.h"
 #include "components/sync/protocol/proto_memory_estimations.h"
+#include "ui/base/models/tree_node_iterator.h"
 
 namespace sync_bookmarks {
 
@@ -107,6 +110,58 @@ SyncedBookmarkTracker::SyncedBookmarkTracker(
 
 SyncedBookmarkTracker::~SyncedBookmarkTracker() = default;
 
+// static
+bool SyncedBookmarkTracker::BookmarkModelMatchesMetadata(
+    const bookmarks::BookmarkModel* model,
+    const sync_pb::BookmarkModelMetadata& model_metadata) {
+  DCHECK(model_metadata.model_type_state().initial_sync_done());
+
+  // Collect ids of non-deletion entries in the metadata.
+  std::vector<int> metadata_node_ids;
+  for (const sync_pb::BookmarkMetadata& bookmark_metadata :
+       model_metadata.bookmarks_metadata()) {
+    if (!bookmark_metadata.metadata().has_server_id()) {
+      DLOG(ERROR) << "Error when decoding sync metadata: Entities must contain "
+                     "server id.";
+      return false;
+    }
+    if (bookmark_metadata.metadata().is_deleted() &&
+        bookmark_metadata.has_id()) {
+      DLOG(ERROR) << "Error when decoding sync metadata: Tombstones "
+                     "shouldn't have a bookmark id.";
+      return false;
+    }
+    if (!bookmark_metadata.metadata().is_deleted() &&
+        !bookmark_metadata.has_id()) {
+      DLOG(ERROR)
+          << "Error when decoding sync metadata: Bookmark id is missing.";
+      return false;
+    }
+    // The entry is valid. If it's not a tombstone, collect its node id to
+    // compare it later with the ids in the model.
+    if (!bookmark_metadata.metadata().is_deleted()) {
+      metadata_node_ids.push_back(bookmark_metadata.id());
+    }
+  }
+
+  // Collect ids of nodes in the model.
+  std::vector<int> model_node_ids;
+  ui::TreeNodeIterator<const bookmarks::BookmarkNode> iterator(
+      model->root_node());
+  while (iterator.has_next()) {
+    const bookmarks::BookmarkNode* node = iterator.Next();
+    model_node_ids.push_back(node->id());
+  }
+
+  if (model_node_ids.size() != metadata_node_ids.size()) {
+    return false;
+  }
+  std::sort(model_node_ids.begin(), model_node_ids.end());
+  std::sort(metadata_node_ids.begin(), metadata_node_ids.end());
+  return std::equal(model_node_ids.begin(), model_node_ids.end(),
+                    metadata_node_ids.begin());
+}
+
 const SyncedBookmarkTracker::Entity* SyncedBookmarkTracker::GetEntityForSyncId(
     const std::string& sync_id) const {
   auto it = sync_id_to_entities_map_.find(sync_id);

components/sync_bookmarks/synced_bookmark_tracker.h

@@ -18,6 +18,7 @@
 #include "components/sync/protocol/unique_position.pb.h"
 
 namespace bookmarks {
+class BookmarkModel;
 class BookmarkNode;
 }
 
@@ -95,6 +96,13 @@ class SyncedBookmarkTracker {
       std::unique_ptr<sync_pb::ModelTypeState> model_type_state);
   ~SyncedBookmarkTracker();
 
+  // Checks the integrity of the |model_metadata|. It also verifies that the
+  // contents of the |model_metadata| match the contents of |model|. It should
+  // only be called if the initial sync has completed.
+  static bool BookmarkModelMatchesMetadata(
+      const bookmarks::BookmarkModel* model,
+      const sync_pb::BookmarkModelMetadata& model_metadata);
+
   // Returns null if no entity is found.
   const Entity* GetEntityForSyncId(const std::string& sync_id) const;
 

components/sync_bookmarks/synced_bookmark_tracker_unittest.cc

@@ -376,6 +376,94 @@ TEST(SyncedBookmarkTrackerTest,
   EXPECT_THAT(entities_with_local_change[3]->metadata()->server_id(), Eq(kId3));
 }
 
+TEST(SyncedBookmarkTrackerTest, ShouldMatchModelAndMetadata) {
+  std::unique_ptr<bookmarks::BookmarkModel> model =
+      bookmarks::TestBookmarkClient::CreateModel();
+
+  const bookmarks::BookmarkNode* bookmark_bar_node = model->bookmark_bar_node();
+  const bookmarks::BookmarkNode* node = model->AddFolder(
+      /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16("node0"));
+
+  sync_pb::BookmarkModelMetadata model_metadata;
+  model_metadata.mutable_model_type_state()->set_initial_sync_done(true);
+  // Add entries for all the permanent nodes. TestBookmarkClient creates all the
+  // 3 permanent nodes.
+  sync_pb::BookmarkMetadata* bookmark_metadata =
+      model_metadata.add_bookmarks_metadata();
+  bookmark_metadata->set_id(model->bookmark_bar_node()->id());
+  bookmark_metadata->mutable_metadata()->set_server_id("BookmarkBarId");
+
+  bookmark_metadata = model_metadata.add_bookmarks_metadata();
+  bookmark_metadata->set_id(model->other_node()->id());
+  bookmark_metadata->mutable_metadata()->set_server_id("OtherBookmarksId");
+
+  bookmark_metadata = model_metadata.add_bookmarks_metadata();
+  bookmark_metadata->set_id(model->mobile_node()->id());
+  bookmark_metadata->mutable_metadata()->set_server_id("MobileBookmarksId");
+
+  // Add entry for the extra node.
+  bookmark_metadata = model_metadata.add_bookmarks_metadata();
+  bookmark_metadata->set_id(node->id());
+  bookmark_metadata->mutable_metadata()->set_server_id("NodeId");
+
+  // Add a tombstone entry.
+  sync_pb::BookmarkMetadata* tombstone =
+      model_metadata.add_bookmarks_metadata();
+  tombstone->mutable_metadata()->set_server_id("tombstoneId");
+  tombstone->mutable_metadata()->set_is_deleted(true);
+
+  EXPECT_TRUE(SyncedBookmarkTracker::BookmarkModelMatchesMetadata(
+      model.get(), model_metadata));
+}
+
+TEST(SyncedBookmarkTrackerTest, ShouldNotMatchModelAndCorruptedMetadata) {
+  std::unique_ptr<bookmarks::BookmarkModel> model =
+      bookmarks::TestBookmarkClient::CreateModel();
+
+  sync_pb::BookmarkModelMetadata model_metadata;
+  model_metadata.mutable_model_type_state()->set_initial_sync_done(true);
+  // Add entries for 2 permanent nodes only. TestBookmarkClient creates all the
+  // 3 permanent nodes.
+  sync_pb::BookmarkMetadata* bookmark_metadata =
+      model_metadata.add_bookmarks_metadata();
+  bookmark_metadata->set_id(model->bookmark_bar_node()->id());
+  bookmark_metadata->mutable_metadata()->set_server_id("BookmarkBarId");
+
+  bookmark_metadata = model_metadata.add_bookmarks_metadata();
+  bookmark_metadata->set_id(model->other_node()->id());
+  bookmark_metadata->mutable_metadata()->set_server_id("OtherBookmarksId");
+
+  // The entry for the Mobile bookmarks is missing.
+  EXPECT_FALSE(SyncedBookmarkTracker::BookmarkModelMatchesMetadata(
+      model.get(), model_metadata));
+
+  // The entry for the Mobile bookmarks is missing a server id.
+  bookmark_metadata = model_metadata.add_bookmarks_metadata();
+  bookmark_metadata->set_id(model->mobile_node()->id());
+  EXPECT_FALSE(SyncedBookmarkTracker::BookmarkModelMatchesMetadata(
+      model.get(), model_metadata));
+
+  // The entry for the Mobile bookmarks is missing a node id.
+  bookmark_metadata->clear_id();
+  bookmark_metadata->mutable_metadata()->set_server_id("OtherBookmarksId");
+  EXPECT_FALSE(SyncedBookmarkTracker::BookmarkModelMatchesMetadata(
+      model.get(), model_metadata));
+
+  // The entry for the Mobile bookmarks is having a wrong node id.
+  bookmark_metadata->set_id(model->mobile_node()->id() + 1);
+  EXPECT_FALSE(SyncedBookmarkTracker::BookmarkModelMatchesMetadata(
+      model.get(), model_metadata));
+
+  // A tombstone shouldn't have a node id.
+  sync_pb::BookmarkMetadata* tombstone =
+      model_metadata.add_bookmarks_metadata();
+  tombstone->mutable_metadata()->set_server_id("tombstoneId");
+  tombstone->mutable_metadata()->set_is_deleted(true);
+  tombstone->set_id(10);
+  EXPECT_FALSE(SyncedBookmarkTracker::BookmarkModelMatchesMetadata(
+      model.get(), model_metadata));
+}
+
 }  // namespace
 
 }  // namespace sync_bookmarks