PasswordSyncableService shouldn't push any sync changes if MergeDataAndStartSyncing() failed.

BUG=411521

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

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

components/password_manager/core/browser/password_syncable_service.cc

@@ -128,8 +128,6 @@ syncer::SyncMergeResult PasswordSyncableService::MergeDataAndStartSyncing(
   DCHECK_EQ(syncer::PASSWORDS, type);
   base::AutoReset<bool> processing_changes(&is_processing_sync_changes_, true);
   syncer::SyncMergeResult merge_result(type);
-  sync_error_factory_ = sync_error_factory.Pass();
-  sync_processor_ = sync_processor.Pass();
 
   // We add all the db entries as |new_local_entries| initially. During model
   // association entries that match a sync entry will be removed and this list
@@ -137,20 +135,24 @@ syncer::SyncMergeResult PasswordSyncableService::MergeDataAndStartSyncing(
   ScopedVector<autofill::PasswordForm> password_entries;
   PasswordEntryMap new_local_entries;
   if (!ReadFromPasswordStore(&password_entries, &new_local_entries)) {
-    DCHECK(sync_error_factory_);
-    merge_result.set_error(sync_error_factory_->CreateAndUploadError(
+    merge_result.set_error(sync_error_factory->CreateAndUploadError(
         FROM_HERE,
         "Failed to get passwords from store."));
     return merge_result;
   }
 
   if (password_entries.size() != new_local_entries.size()) {
-    merge_result.set_error(sync_error_factory_->CreateAndUploadError(
+    merge_result.set_error(sync_error_factory->CreateAndUploadError(
         FROM_HERE,
         "There are passwords with identical sync tags in the database."));
     return merge_result;
   }
 
+  // Save |sync_processor_| only if reading the PasswordStore succeeded. In case
+  // of failure Sync shouldn't receive any updates from the PasswordStore.
+  sync_error_factory_ = sync_error_factory.Pass();
+  sync_processor_ = sync_processor.Pass();
+
   merge_result.set_num_items_before_association(new_local_entries.size());
 
   SyncEntries sync_entries;
@@ -301,14 +303,14 @@ bool PasswordSyncableService::ReadFromPasswordStore(
 void PasswordSyncableService::WriteToPasswordStore(const SyncEntries& entries) {
   PasswordStoreChangeList changes;
   WriteEntriesToDatabase(&PasswordStoreSync::AddLoginImpl,
-                entries.new_entries.get(),
-                &changes);
+                         entries.new_entries.get(),
+                         &changes);
   WriteEntriesToDatabase(&PasswordStoreSync::UpdateLoginImpl,
-                entries.updated_entries.get(),
-                &changes);
+                         entries.updated_entries.get(),
+                         &changes);
   WriteEntriesToDatabase(&PasswordStoreSync::RemoveLoginImpl,
-                entries.deleted_entries.get(),
-                &changes);
+                         entries.deleted_entries.get(),
+                         &changes);
 
   // We have to notify password store observers of the change by hand since
   // we use internal password store interfaces to make changes synchronously.

components/password_manager/core/browser/password_syncable_service_unittest.cc

@@ -16,7 +16,7 @@
 #include "components/password_manager/core/browser/mock_password_store.h"
 #include "sync/api/sync_change_processor.h"
 #include "sync/api/sync_error.h"
-#include "sync/api/sync_error_factory.h"
+#include "sync/api/sync_error_factory_mock.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -480,6 +480,31 @@ TEST_F(PasswordSyncableServiceTest, StartSyncFlare) {
   service()->ActOnPasswordStoreChanges(list);
 }
 
+// Start syncing with an error. Subsequent password store updates shouldn't be
+// propagated to Sync.
+TEST_F(PasswordSyncableServiceTest, FailedReadFromPasswordStore) {
+  scoped_ptr<syncer::SyncErrorFactoryMock> error_factory(
+      new syncer::SyncErrorFactoryMock);
+  EXPECT_CALL(*password_store(), FillAutofillableLogins(_))
+      .WillOnce(Return(false));
+  EXPECT_CALL(*error_factory, CreateAndUploadError(_, _))
+      .WillOnce(Return(SyncError()));
+  // ActOnPasswordStoreChanges() below shouldn't generate any changes for Sync.
+  // |processor_| will be destroyed in MergeDataAndStartSyncing().
+  EXPECT_CALL(*processor_, ProcessSyncChanges(_, _)).Times(0);
+  service()->MergeDataAndStartSyncing(
+      syncer::PASSWORDS,
+      syncer::SyncDataList(),
+      processor_.PassAs<syncer::SyncChangeProcessor>(),
+      error_factory.PassAs<syncer::SyncErrorFactory>());
+
+  autofill::PasswordForm form;
+  form.signon_realm = "abc";
+  PasswordStoreChangeList list;
+  list.push_back(PasswordStoreChange(PasswordStoreChange::ADD, form));
+  service()->ActOnPasswordStoreChanges(list);
+}
+
 }  // namespace
 
 }  // namespace password_manager