[Sync::USS] Ignore mobile bookmarks folder when validating metadata

Mobile bookmarks folder always exists in the local model, however it
gets created on the server only after signing in with a mobile device.

Therefore, when checking persisted metadata for corruption, the mobile
bookmarks should be treated as optional and its absence shouldn't signal
metadata corruption.

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

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -177,6 +177,14 @@ bool SyncedBookmarkTracker::BookmarkModelMatchesMetadata(
       if (!model->client()->CanSyncNode(node)) {
         continue;
       }
+      // Mobile bookmarks folder is created on the server only after signing-in
+      // with a mobile client. Therefore, it should be considered for validation
+      // only if a corresponding node exists in the metadata.
+      if (node == model->mobile_node() &&
+          std::count(metadata_node_ids.begin(), metadata_node_ids.end(),
+                     node->id()) == 0) {
+        continue;
+      }
       model_node_ids.push_back(node->id());
     }
 

components/sync_bookmarks/synced_bookmark_tracker_unittest.cc

@@ -464,14 +464,17 @@ TEST(SyncedBookmarkTrackerTest, ShouldMatchModelAndMetadata) {
       /*sample=*/ExpectedCorruptionReason::NO_CORRUPTION, /*count=*/1);
 }
 
-TEST(SyncedBookmarkTrackerTest, ShouldNotMatchModelAndCorruptedMetadata) {
+TEST(SyncedBookmarkTrackerTest,
+     ShouldMatchModelAndMetadataEvenIfMissingMobileFolder) {
   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.
+  // Add entries for all the permanent nodes except for the Mobile bookmarks
+  // folder. This simulates a user who has never signed it to sync on a mobile
+  // device, and hence the Mobile bookmarks folder has never be created on the
+  // server.
   sync_pb::BookmarkMetadata* bookmark_metadata =
       model_metadata.add_bookmarks_metadata();
   bookmark_metadata->set_id(model->bookmark_bar_node()->id());
@@ -482,16 +485,37 @@ TEST(SyncedBookmarkTrackerTest, ShouldNotMatchModelAndCorruptedMetadata) {
   bookmark_metadata->mutable_metadata()->set_server_id("OtherBookmarksId");
 
   base::HistogramTester histogram_tester;
-  // The entry for the Mobile bookmarks is missing.
+  EXPECT_TRUE(SyncedBookmarkTracker::BookmarkModelMatchesMetadata(
+      model.get(), model_metadata));
+  histogram_tester.ExpectUniqueSample(
+      "Sync.BookmarksModelMetadataCorruptionReason",
+      /*sample=*/ExpectedCorruptionReason::NO_CORRUPTION, /*count=*/1);
+}
+
+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");
+
+  base::HistogramTester histogram_tester;
+  // The entry for the Other bookmarks is missing.
   EXPECT_FALSE(SyncedBookmarkTracker::BookmarkModelMatchesMetadata(
       model.get(), model_metadata));
   histogram_tester.ExpectBucketCount(
       "Sync.BookmarksModelMetadataCorruptionReason",
       /*sample=*/ExpectedCorruptionReason::COUNT_MISMATCH, /*count=*/1);
 
-  // The entry for the Mobile bookmarks is missing a server id.
+  // The entry for the Other bookmarks is missing a server id.
   bookmark_metadata = model_metadata.add_bookmarks_metadata();
-  bookmark_metadata->set_id(model->mobile_node()->id());
+  bookmark_metadata->set_id(model->other_node()->id());
   EXPECT_FALSE(SyncedBookmarkTracker::BookmarkModelMatchesMetadata(
       model.get(), model_metadata));
 
@@ -499,7 +523,7 @@ TEST(SyncedBookmarkTrackerTest, ShouldNotMatchModelAndCorruptedMetadata) {
       "Sync.BookmarksModelMetadataCorruptionReason",
       /*sample=*/ExpectedCorruptionReason::MISSING_SERVER_ID, /*count=*/1);
 
-  // The entry for the Mobile bookmarks is missing a node id.
+  // The entry for the Other bookmarks is missing a node id.
   bookmark_metadata->clear_id();
   bookmark_metadata->mutable_metadata()->set_server_id("OtherBookmarksId");
   EXPECT_FALSE(SyncedBookmarkTracker::BookmarkModelMatchesMetadata(
@@ -508,8 +532,8 @@ TEST(SyncedBookmarkTrackerTest, ShouldNotMatchModelAndCorruptedMetadata) {
       "Sync.BookmarksModelMetadataCorruptionReason",
       /*sample=*/ExpectedCorruptionReason::MISSING_BOOKMARK_ID, /*count=*/1);
 
-  // The entry for the Mobile bookmarks is having a wrong node id.
-  bookmark_metadata->set_id(model->mobile_node()->id() + 1);
+  // The entry for the Other bookmarks is having a wrong node id.
+  bookmark_metadata->set_id(model->other_node()->id() + 1000);
   EXPECT_FALSE(SyncedBookmarkTracker::BookmarkModelMatchesMetadata(
       model.get(), model_metadata));
   histogram_tester.ExpectBucketCount(