Fix parameter in the BulkWeakCheck

This CL fixes a bug in the StartWeakCheck(). The bug was that we passed
to BindOnce |base::span| of password forms that were read on another
thread. It created a shallow copy of the password forms. Now, we pass to
BindOnce a |base::flat_set| of passwords that are deeply copied.

Bug: 1119752
Change-Id: I59f32a494e27f2ce50f0e615aabcd14c13f893c8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2414238
Commit-Queue: Irina Fedorova <irfedorova@google.com>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807966}

components/password_manager/core/browser/ui/insecure_credentials_manager.cc

@@ -158,6 +158,16 @@ std::vector<CredentialWithPassword> ExtractInsecureCredentials(
   return credentials;
 }
 
+base::flat_set<base::string16> ExtractPasswords(
+    SavedPasswordsPresenter::SavedPasswordsView password_forms) {
+  std::vector<base::string16> passwords;
+  passwords.reserve(password_forms.size());
+  for (const auto& form : password_forms) {
+    passwords.push_back(form.password_value);
+  }
+  return base::flat_set<base::string16>(std::move(passwords));
+}
+
 }  // namespace
 
 CredentialView::CredentialView(std::string signon_realm,
@@ -228,7 +238,8 @@ void InsecureCredentialsManager::Init() {
 void InsecureCredentialsManager::StartWeakCheck() {
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
-      base::BindOnce(&BulkWeakCheck, presenter_->GetSavedPasswords()),
+      base::BindOnce(&BulkWeakCheck,
+                     ExtractPasswords(presenter_->GetSavedPasswords())),
       base::BindOnce(&InsecureCredentialsManager::OnWeakCheckDone,
                      weak_ptr_factory_.GetWeakPtr()));
 }

components/password_manager/core/browser/ui/weak_check_utility.cc

@@ -58,16 +58,11 @@ int PasswordWeakCheck(const base::string16& password) {
 }  // namespace
 
 base::flat_set<base::string16> BulkWeakCheck(
-    SavedPasswordsPresenter::SavedPasswordsView saved_passwords) {
-  std::vector<base::string16> weak_passwords;
-
-  for (const auto& password : saved_passwords) {
-    if (PasswordWeakCheck(password.password_value) <= kLowSeverityScore) {
-      weak_passwords.push_back(password.password_value);
-    }
-  }
-
-  return base::flat_set<base::string16>(std::move(weak_passwords));
+    base::flat_set<base::string16> passwords) {
+  base::EraseIf(passwords, [](const auto& password) {
+    return kLowSeverityScore < PasswordWeakCheck(password);
+  });
+  return passwords;
 }
 
 }  // namespace password_manager

components/password_manager/core/browser/ui/weak_check_utility.h

@@ -11,9 +11,10 @@
 
 namespace password_manager {
 
-// Checks each password for weakness.
+// Checks each password for weakness and removes strong passwords from the
+// |passwords|.
 base::flat_set<base::string16> BulkWeakCheck(
-    SavedPasswordsPresenter::SavedPasswordsView passwords);
+    base::flat_set<base::string16> passwords);
 
 }  // namespace password_manager
 

components/password_manager/core/browser/ui/weak_check_utility_unittest.cc

@@ -4,9 +4,7 @@
 
 #include "components/password_manager/core/browser/ui/weak_check_utility.h"
 
-#include <vector>
 #include "base/strings/utf_string_conversions.h"
-#include "components/autofill/core/common/password_form.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -14,9 +12,6 @@ namespace password_manager {
 
 namespace {
 
-constexpr char kUsername1[] = "alice";
-constexpr char kUsername2[] = "bob";
-
 constexpr char kWeakShortPassword[] = "123456";
 constexpr char kWeakLongPassword[] =
     "abcdabcdabcdabcdabcdabcdabcdabcdabcdabcda";
@@ -24,34 +19,24 @@ constexpr char kStrongShortPassword[] = "fnlsr4@cm^mdls@fkspnsg3d";
 constexpr char kStrongLongPassword[] =
     "pmsFlsnoab4nsl#losb@skpfnsbkjb^klsnbs!cns";
 
-using autofill::PasswordForm;
 using ::testing::ElementsAre;
 
-PasswordForm MakeSavedPassword(base::StringPiece username,
-                               base::StringPiece password) {
-  PasswordForm form;
-  form.signon_realm = "https://example.com";
-  form.username_value = base::ASCIIToUTF16(username);
-  form.password_value = base::ASCIIToUTF16(password);
-  return form;
-}
-
 }  // namespace
 
 TEST(WeakCheckUtilityTest, WeakPasswordsNotFound) {
-  std::vector<PasswordForm> passwords = {
-      MakeSavedPassword(kUsername1, kStrongShortPassword),
-      MakeSavedPassword(kUsername2, kStrongLongPassword)};
+  base::flat_set<base::string16> passwords = {
+      base::ASCIIToUTF16(kStrongShortPassword),
+      base::ASCIIToUTF16(kStrongLongPassword)};
 
   EXPECT_THAT(BulkWeakCheck(passwords), testing::IsEmpty());
 }
 
 TEST(WeakCheckUtilityTest, DetectedShortAndLongWeakPasswords) {
-  std::vector<PasswordForm> passwords = {
-      MakeSavedPassword(kUsername1, kStrongLongPassword),
-      MakeSavedPassword(kUsername1, kWeakShortPassword),
-      MakeSavedPassword(kUsername1, kStrongShortPassword),
-      MakeSavedPassword(kUsername2, kWeakLongPassword)};
+  base::flat_set<base::string16> passwords = {
+      base::ASCIIToUTF16(kStrongLongPassword),
+      base::ASCIIToUTF16(kWeakShortPassword),
+      base::ASCIIToUTF16(kStrongShortPassword),
+      base::ASCIIToUTF16(kWeakLongPassword)};
 
   base::flat_set<base::string16> weak_passwords = BulkWeakCheck(passwords);
 
@@ -60,15 +45,4 @@ TEST(WeakCheckUtilityTest, DetectedShortAndLongWeakPasswords) {
                           base::ASCIIToUTF16(kWeakLongPassword)));
 }
 
-TEST(WeakCheckUtilityTest, WeakPasswordsSetContainsNoCopies) {
-  std::vector<PasswordForm> passwords = {
-      MakeSavedPassword(kUsername1, kWeakShortPassword),
-      MakeSavedPassword(kUsername2, kWeakShortPassword)};
-
-  base::flat_set<base::string16> weak_passwords = BulkWeakCheck(passwords);
-
-  EXPECT_THAT(weak_passwords,
-              ElementsAre(base::ASCIIToUTF16(kWeakShortPassword)));
-}
-
 }  // namespace password_manager