[Passwords] ShouldRecoverPasswordsDuringMerge() returns false on Windows

Before this patch:
ShouldRecoverPasswordsDuringMerge() checked the feature
kDeleteCorruptedPasswords on all platforms to decide whether to try to
delete undecryptable passwords during merge.

After this patch:
The feature is checked only on MacOS. On other platforms, the method
always returns false.

Change-Id: Icdc621c2b45e91541a088ce2db6f0b32c968a552
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1995382
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Auto-Submit: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#730209}

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

@@ -12,6 +12,7 @@
 #include "base/metrics/histogram_functions.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
+#include "build/build_config.h"
 #include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/core/browser/password_manager_metrics_util.h"
 #include "components/password_manager/core/common/password_manager_features.h"
@@ -168,8 +169,20 @@ bool AreLocalAndRemotePasswordsEqual(
               .Serialize() == password_form.federation_origin.Serialize());
 }
 
+// Whether we should try to recover undecryptable local passwords by deleting
+// the local copy, to be replaced by the remote version coming from Sync during
+// merge.
 bool ShouldRecoverPasswordsDuringMerge() {
+  // Delete the local undecryptable copy under the following conditions:
+  // 1. This is MacOS only.
+  // 2. The more general feature kDeleteCorruptedPasswords is disabled.
+  //    kDeleteCorruptedPasswords takes cares of deleting undecryptable entities
+  //    for Sync and non-Sync users upon reading from the LoginDatabase.
+#if defined(OS_MACOSX) && !defined(OS_IOS)
   return !base::FeatureList::IsEnabled(features::kDeleteCorruptedPasswords);
+#else
+  return false;
+#endif
 }
 
 // A simple class for scoping a password store sync transaction. If the

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

@@ -12,6 +12,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/mock_callback.h"
+#include "build/build_config.h"
 #include "components/password_manager/core/browser/password_store_sync.h"
 #include "components/sync/base/client_tag_hash.h"
 #include "components/sync/model/data_batch.h"
@@ -789,6 +790,7 @@ TEST_F(PasswordSyncBridgeTest,
                             mock_password_store_sync(), base::DoNothing());
 }
 
+#if defined(OS_MACOSX) && !defined(OS_IOS)
 // Tests that in case ReadAllLogins() during initial merge returns encryption
 // service failure, the bridge would try to do a DB clean up.
 TEST_F(PasswordSyncBridgeTest, ShouldDeleteUndecryptableLoginsDuringMerge) {
@@ -806,6 +808,7 @@ TEST_F(PasswordSyncBridgeTest, ShouldDeleteUndecryptableLoginsDuringMerge) {
       bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(), {});
   EXPECT_FALSE(error);
 }
+#endif
 
 TEST_F(PasswordSyncBridgeTest,
        ShouldDeleteSyncMetadataWhenApplyStopSyncChanges) {

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

@@ -14,6 +14,7 @@
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/utf_string_conversions.h"
+#include "build/build_config.h"
 #include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/core/browser/password_manager_metrics_util.h"
 #include "components/password_manager/core/browser/password_store_sync.h"
@@ -450,8 +451,20 @@ void PasswordSyncableService::CreateOrUpdateEntry(
   }
 }
 
+// Whether we should try to recover undecryptable local passwords by deleting
+// the local copy, to be replaced by the remote version coming from Sync during
+// merge.
 bool PasswordSyncableService::ShouldRecoverPasswordsDuringMerge() const {
+  // Delete the local undecryptable copy under the following conditions:
+  // 1. This is MacOS only.
+  // 2. The more general feature kDeleteCorruptedPasswords is disabled.
+  //    kDeleteCorruptedPasswords takes cares of deleting undecryptable entities
+  //    for Sync and non-Sync users upon reading from the LoginDatabase.
+#if defined(OS_MACOSX) && !defined(OS_IOS)
   return !base::FeatureList::IsEnabled(features::kDeleteCorruptedPasswords);
+#else
+  return false;
+#endif
 }
 
 syncer::SyncData SyncDataFromPassword(

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

@@ -31,6 +31,7 @@ using syncer::SyncChange;
 using syncer::SyncData;
 using syncer::SyncDataList;
 using syncer::SyncError;
+using testing::_;
 using testing::AnyNumber;
 using testing::DoAll;
 using testing::ElementsAre;
@@ -41,7 +42,6 @@ using testing::Matches;
 using testing::Return;
 using testing::SetArgPointee;
 using testing::UnorderedElementsAre;
-using testing::_;
 
 namespace password_manager {
 
@@ -515,8 +515,10 @@ TEST_F(PasswordSyncableServiceTest, FailedReadFromPasswordStore) {
                           syncer::PASSWORDS);
   EXPECT_CALL(*password_store(), FillAutofillableLogins(_))
       .WillOnce(Return(false));
+#if defined(OS_MACOSX) && !defined(OS_IOS)
   EXPECT_CALL(*password_store(), DeleteUndecryptableLogins())
       .WillOnce(Return(DatabaseCleanupResult::kDatabaseUnavailable));
+#endif
   EXPECT_CALL(*error_factory, CreateAndUploadError(_, _))
       .WillOnce(Return(error));
   // ActOnPasswordStoreChanges() below shouldn't generate any changes for Sync.
@@ -545,6 +547,7 @@ class PasswordSyncableServiceTestWithoutDeleteCorruptedPasswords
   base::test::ScopedFeatureList scoped_feature_list_;
 };
 
+#if defined(OS_MACOSX) && !defined(OS_IOS)
 // Test that passwords are recovered for Sync users using the older logic (i.e.
 // recover passwords only for Sync users) when the feature for deleting
 // corrupted passwords for all users is disabled.
@@ -563,6 +566,7 @@ TEST_F(PasswordSyncableServiceTestWithoutDeleteCorruptedPasswords,
       syncer::PASSWORDS, SyncDataList(), std::move(processor_), nullptr);
   EXPECT_FALSE(result.error().IsSet());
 }
+#endif
 
 class PasswordSyncableServiceTestWithDeleteCorruptedPasswords
     : public PasswordSyncableServiceTest {
@@ -597,6 +601,7 @@ TEST_F(PasswordSyncableServiceTestWithDeleteCorruptedPasswords,
   EXPECT_TRUE(result.error().IsSet());
 }
 
+#if defined(OS_MACOSX) && !defined(OS_IOS)
 // Database cleanup fails because encryption is unavailable.
 TEST_F(PasswordSyncableServiceTestWithoutDeleteCorruptedPasswords,
        FailedDeleteUndecryptableLogins) {
@@ -618,6 +623,7 @@ TEST_F(PasswordSyncableServiceTestWithoutDeleteCorruptedPasswords,
       std::move(error_factory));
   EXPECT_TRUE(result.error().IsSet());
 }
+#endif
 
 // Start syncing with an error in ProcessSyncChanges. Subsequent password store
 // updates shouldn't be propagated to Sync.