[Sync] Delete Password Sync Metadata when not readable

Bug: 1044365
Change-Id: I9fd23b258c86f68e01be02f17c49d0adcbe7a1e5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2064940
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#743087}

components/password_manager/core/browser/sync/password_sync_bridge.cc

@@ -29,6 +29,12 @@ namespace password_manager {
 
 namespace {
 
+// Controls whether we should delete the sync metadata when they aren't
+// readable.
+const base::Feature kDeletePasswordSyncMetadataWhenNoReadable{
+    "DeletePasswordSyncMetadataWhenNoReadable",
+    base::FEATURE_ENABLED_BY_DEFAULT};
+
 // Error values for reading sync metadata.
 // Used in metrics: "PasswordManager.SyncMetadataReadError". These values
 // are persisted to logs. Entries should not be renumbered and numeric values
@@ -252,16 +258,24 @@ PasswordSyncBridge::PasswordSyncBridge(
   } else {
     batch = password_store_sync_->GetMetadataStore()->GetAllSyncMetadata();
     if (!batch) {
-      this->change_processor()->ReportError(
-          {FROM_HERE,
-           "Failed reading passwords metadata from password store."});
+      if (base::FeatureList::IsEnabled(
+              kDeletePasswordSyncMetadataWhenNoReadable)) {
+        // If the metadata cannot be read, it's mostly a persistent error, and
+        // hence we should drop the metadata to go throw the initial sync flow.
+        password_store_sync_->GetMetadataStore()->DeleteAllSyncMetadata();
+        batch = std::make_unique<syncer::MetadataBatch>();
+      } else {
+        this->change_processor()->ReportError(
+            {FROM_HERE,
+             "Failed reading passwords metadata from password store."});
+      }
       sync_metadata_read_error = SyncMetadataReadError::kReadFailed;
     }
   }
   base::UmaHistogramEnumeration("PasswordManager.SyncMetadataReadError",
                                 sync_metadata_read_error);
 
-  if (sync_metadata_read_error == SyncMetadataReadError::kNone) {
+  if (batch) {
     this->change_processor()->ModelReadyToSync(std::move(batch));
   }
 }

components/password_manager/core/browser/sync/password_sync_bridge_unittest.cc

@@ -673,6 +673,24 @@ TEST_F(PasswordSyncBridgeTest,
   EXPECT_FALSE(error);
 }
 
+// This tests that if reading sync metadata from the store fails,
+// metadata should be deleted and Sync starts without error.
+TEST_F(
+    PasswordSyncBridgeTest,
+    ShouldMergeSyncRemoteAndLocalPasswordsWithoutErrorWhenMetadataReadFails) {
+  // Simulate a failed GetAllSyncMetadata() by returning a nullptr.
+  ON_CALL(*mock_sync_metadata_store_sync(), GetAllSyncMetadata())
+      .WillByDefault(testing::ReturnNull());
+
+  EXPECT_CALL(*mock_sync_metadata_store_sync(), DeleteAllSyncMetadata());
+  EXPECT_CALL(mock_processor(), ModelReadyToSync(MetadataBatchContains(
+                                    /*state=*/syncer::HasNotInitialSyncDone(),
+                                    /*entities=*/testing::SizeIs(0))));
+  auto bridge = std::make_unique<PasswordSyncBridge>(
+      mock_processor().CreateForwardingProcessor(), mock_password_store_sync(),
+      base::DoNothing());
+}
+
 // This tests that if reading logins from the store fails,
 // ShouldMergeSync() would return an error without crashing.
 TEST_F(PasswordSyncBridgeTest,

components/sync/test/test_matchers.h

@@ -74,6 +74,11 @@ MATCHER(HasInitialSyncDone, "") {
   return arg.initial_sync_done();
 }
 
+// Matcher for sync_pb::ModelTypeState: verifies that initial sync is not done.
+MATCHER(HasNotInitialSyncDone, "") {
+  return !arg.initial_sync_done();
+}
+
 }  // namespace syncer
 
 #endif  // COMPONENTS_SYNC_TEST_TEST_MATCHERS_H_