PasswordStore: Use ScopedVector to express ownership of forms

chrome/browser/password_manager/password_store_x_unittest.cc

Index: chrome/browser/password_manager/password_store_x_unittest.cc
diff --git a/chrome/browser/password_manager/password_store_x_unittest.cc b/chrome/browser/password_manager/password_store_x_unittest.cc
index c5418479ca18c2f1507362c89124427d6e6ccf5d..de7d005ae027b3a51b01ec90b81e7d611c13f160 100644
--- a/chrome/browser/password_manager/password_store_x_unittest.cc
+++ b/chrome/browser/password_manager/password_store_x_unittest.cc
@@ -33,8 +33,6 @@ using testing::_;
 using testing::ElementsAreArray;
 using testing::WithArg;
 
-typedef std::vector<PasswordForm*> VectorOfForms;
-
 namespace {
 
 class MockPasswordStoreConsumer
@@ -78,12 +76,19 @@ class FailingBackend : public PasswordStoreX::NativeBackend {
     return false;
   }
 
-  bool GetLogins(const PasswordForm& form, PasswordFormList* forms) override {
+  bool GetLogins(const PasswordForm& form,
+                 ScopedVector<autofill::PasswordForm>* forms) override {
     return false;
   }
 
-  bool GetAutofillableLogins(PasswordFormList* forms) override { return false; }
-  bool GetBlacklistLogins(PasswordFormList* forms) override { return false; }
+  bool GetAutofillableLogins(
+      ScopedVector<autofill::PasswordForm>* forms) override {
+    return false;
+  }
+  bool GetBlacklistLogins(
+      ScopedVector<autofill::PasswordForm>* forms) override {
+    return false;
+  }
 };
 
 class MockBackend : public PasswordStoreX::NativeBackend {
@@ -142,21 +147,24 @@ class MockBackend : public PasswordStoreX::NativeBackend {
     return true;
   }
 
-  bool GetLogins(const PasswordForm& form, PasswordFormList* forms) override {
+  bool GetLogins(const PasswordForm& form,
+                 ScopedVector<autofill::PasswordForm>* forms) override {
     for (size_t i = 0; i < all_forms_.size(); ++i)
       if (all_forms_[i].signon_realm == form.signon_realm)
         forms->push_back(new PasswordForm(all_forms_[i]));
     return true;
   }
 
-  bool GetAutofillableLogins(PasswordFormList* forms) override {
+  bool GetAutofillableLogins(
+      ScopedVector<autofill::PasswordForm>* forms) override {
     for (size_t i = 0; i < all_forms_.size(); ++i)
       if (!all_forms_[i].blacklisted_by_user)
         forms->push_back(new PasswordForm(all_forms_[i]));
     return true;
   }
 
-  bool GetBlacklistLogins(PasswordFormList* forms) override {
+  bool GetBlacklistLogins(
+      ScopedVector<autofill::PasswordForm>* forms) override {
     for (size_t i = 0; i < all_forms_.size(); ++i)
       if (all_forms_[i].blacklisted_by_user)
         forms->push_back(new PasswordForm(all_forms_[i]));
@@ -193,16 +201,18 @@ class MockLoginDatabaseReturn {
 void LoginDatabaseQueryCallback(password_manager::LoginDatabase* login_db,
                                 bool autofillable,
                                 MockLoginDatabaseReturn* mock_return) {
-  std::vector<PasswordForm*> forms;
+  ScopedVector<autofill::PasswordForm> forms;
   if (autofillable)
     login_db->GetAutofillableLogins(&forms);
   else
     login_db->GetBlacklistLogins(&forms);
-  mock_return->OnLoginDatabaseQueryDone(forms);
+  mock_return->OnLoginDatabaseQueryDone(forms.get());
 }
 
 // Generate |count| expected logins, either auto-fillable or blacklisted.
-void InitExpectedForms(bool autofillable, size_t count, VectorOfForms* forms) {
+void InitExpectedForms(bool autofillable,
+                       size_t count,
+                       ScopedVector<autofill::PasswordForm>* forms) {
   const char* domain = autofillable ? "example" : "blacklisted";
   for (size_t i = 0; i < count; ++i) {
     std::string realm = base::StringPrintf("http://%zu.%s.com", i, domain);
@@ -339,10 +349,10 @@ TEST_P(PasswordStoreXTest, Notifications) {
 }
 
 TEST_P(PasswordStoreXTest, NativeMigration) {
-  VectorOfForms expected_autofillable;
+  ScopedVector<autofill::PasswordForm> expected_autofillable;
   InitExpectedForms(true, 50, &expected_autofillable);
 
-  VectorOfForms expected_blacklisted;
+  ScopedVector<autofill::PasswordForm> expected_blacklisted;
   InitExpectedForms(false, 50, &expected_blacklisted);
 
   const base::FilePath login_db_file = test_login_db_file_path();
@@ -356,13 +366,11 @@ TEST_P(PasswordStoreXTest, NativeMigration) {
   ASSERT_TRUE(base::GetFileInfo(login_db_file, &db_file_start_info));
 
   // Populate the login DB with logins that should be migrated.
-  for (VectorOfForms::iterator it = expected_autofillable.begin();
-       it != expected_autofillable.end(); ++it) {
-    login_db->AddLogin(**it);
+  for (const auto& form : expected_autofillable) {

[vasilii, 2015/01/27 20:45:52]

const auto*

[vabr (Chromium), 2015/01/28 13:27:36]

Done.

+    login_db->AddLogin(*form);
   }
-  for (VectorOfForms::iterator it = expected_blacklisted.begin();
-       it != expected_blacklisted.end(); ++it) {
-    login_db->AddLogin(**it);
+  for (const auto& form : expected_blacklisted) {

[vasilii, 2015/01/27 20:45:51]

see above

[vabr (Chromium), 2015/01/28 13:27:36]

Done.

+    login_db->AddLogin(*form);
   }
 
   // Get the new size of the login DB file. We expect it to be larger.
@@ -380,34 +388,32 @@ TEST_P(PasswordStoreXTest, NativeMigration) {
   MockPasswordStoreConsumer consumer;
 
   // The autofillable forms should have been migrated to the native backend.
-  EXPECT_CALL(consumer,
-      OnGetPasswordStoreResults(
-          ContainsAllPasswordForms(expected_autofillable)))
+  EXPECT_CALL(consumer, OnGetPasswordStoreResults(ContainsAllPasswordForms(
+                            expected_autofillable.get())))
       .WillOnce(WithArg<0>(STLDeleteElements0()));

[vasilii, 2015/01/27 20:45:52]

This causes crashes in the tests. You don't need it anymore.

[vabr (Chromium), 2015/01/28 13:27:36]

Actually, the STLDeleteElements still needs to be here, because
OnGetPasswordStoreResults is still responsible for deleting its argument (see
MaybeCallConsumerCallback in password_store.cc).

The failures in the ASAN tests were due to line 417 below, which I'll now
remove.

 
   store->GetAutofillableLogins(&consumer);
   base::RunLoop().RunUntilIdle();
 
   // The blacklisted forms should have been migrated to the native backend.
-  EXPECT_CALL(consumer,
-      OnGetPasswordStoreResults(ContainsAllPasswordForms(expected_blacklisted)))
+  EXPECT_CALL(consumer, OnGetPasswordStoreResults(ContainsAllPasswordForms(
+                            expected_blacklisted.get())))
       .WillOnce(WithArg<0>(STLDeleteElements0()));
 
   store->GetBlacklistLogins(&consumer);
   base::RunLoop().RunUntilIdle();
 
-  VectorOfForms empty;
+  ScopedVector<autofill::PasswordForm> empty;
   MockLoginDatabaseReturn ld_return;
 
   if (GetParam() == WORKING_BACKEND) {
     // No autofillable logins should be left in the login DB.
-    EXPECT_CALL(ld_return,
-                OnLoginDatabaseQueryDone(ContainsAllPasswordForms(empty)));
+    EXPECT_CALL(ld_return, OnLoginDatabaseQueryDone(
+                               ContainsAllPasswordForms(empty.get())));
   } else {
     // The autofillable logins should still be in the login DB.
-    EXPECT_CALL(ld_return,
-                OnLoginDatabaseQueryDone(
-                    ContainsAllPasswordForms(expected_autofillable)))
+    EXPECT_CALL(ld_return, OnLoginDatabaseQueryDone(ContainsAllPasswordForms(
+                               expected_autofillable.get())))
         .WillOnce(WithArg<0>(STLDeleteElements0()));
   }
 
@@ -418,13 +424,12 @@ TEST_P(PasswordStoreXTest, NativeMigration) {
 
   if (GetParam() == WORKING_BACKEND) {
     // Likewise, no blacklisted logins should be left in the login DB.
-    EXPECT_CALL(ld_return,
-                OnLoginDatabaseQueryDone(ContainsAllPasswordForms(empty)));
+    EXPECT_CALL(ld_return, OnLoginDatabaseQueryDone(
+                               ContainsAllPasswordForms(empty.get())));
   } else {
     // The blacklisted logins should still be in the login DB.
-    EXPECT_CALL(ld_return,
-                OnLoginDatabaseQueryDone(
-                    ContainsAllPasswordForms(expected_blacklisted)))
+    EXPECT_CALL(ld_return, OnLoginDatabaseQueryDone(ContainsAllPasswordForms(
+                               expected_blacklisted.get())))
         .WillOnce(WithArg<0>(STLDeleteElements0()));
   }
 
@@ -444,9 +449,6 @@ TEST_P(PasswordStoreXTest, NativeMigration) {
     EXPECT_EQ(db_file_start_info.size, db_file_end_info.size);
   }
 
-  STLDeleteElements(&expected_autofillable);
-  STLDeleteElements(&expected_blacklisted);
-
   store->Shutdown();
 }