PasswordStore: Use ScopedVector to express ownership of forms

chrome/browser/password_manager/password_store_mac.cc

Index: chrome/browser/password_manager/password_store_mac.cc
diff --git a/chrome/browser/password_manager/password_store_mac.cc b/chrome/browser/password_manager/password_store_mac.cc
index bb50c7425f1a6208bdb54b9f42369294e273602c..9b94ce06527b5552b866e12b7f5dec0b05599c9b 100644
--- a/chrome/browser/password_manager/password_store_mac.cc
+++ b/chrome/browser/password_manager/password_store_mac.cc
@@ -15,7 +15,6 @@
 #include "base/logging.h"
 #include "base/mac/foundation_util.h"
 #include "base/mac/mac_logging.h"
-#include "base/memory/scoped_vector.h"
 #include "base/message_loop/message_loop.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
@@ -418,70 +417,63 @@ PasswordForm* BestKeychainFormForForm(
   return partial_match;
 }
 
-// Returns entries from |forms| that are blacklist entries, after removing
-// them from |forms|.
-std::vector<PasswordForm*> ExtractBlacklistForms(
-    std::vector<PasswordForm*>* forms) {
-  std::vector<PasswordForm*> blacklist_forms;
-  for (std::vector<PasswordForm*>::iterator i = forms->begin();
-       i != forms->end();) {
-    PasswordForm* form = *i;
-    if (form->blacklisted_by_user) {
-      blacklist_forms.push_back(form);
-      i = forms->erase(i);
-    } else {
-      ++i;
-    }
-  }
-  return blacklist_forms;
-}
-
-// Deletes and removes from v any element that exists in s.
-template <class T>
-void DeleteVectorElementsInSet(std::vector<T*>* v, const std::set<T*>& s) {
-  for (typename std::vector<T*>::iterator i = v->begin(); i != v->end();) {
-    T* element = *i;
-    if (s.find(element) != s.end()) {
-      delete element;
-      i = v->erase(i);
-    } else {
-      ++i;
-    }
+// Moves entries from |forms| that are blacklist entries into |blacklist|.
+void ExtractBlacklistForms(ScopedVector<autofill::PasswordForm>* forms,
+                           ScopedVector<autofill::PasswordForm>* blacklist) {
+  blacklist->reserve(blacklist->size() + forms->size());
+  ScopedVector<autofill::PasswordForm> non_blacklist;
+  for (auto& form : *forms) {
+    if (form->blacklisted_by_user)
+      blacklist->push_back(form);
+    else
+      non_blacklist.push_back(form);
+    form = nullptr;

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

you could weak_clear |forms| instead.

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

I could, but that extends the period of time when two ScopedVectors think they
own the same object. Suppose somebody added an early return in the for-loop
later, then the weak_clear() would not be called, and instead some forms would
be double-freed.

What is the reason against nullptr and for weak_clear()?

[vasilii, 2015/01/28 15:09:03]

It's performance related. You know that |forms| contains just zeros but still
iterate over it and release the pointers.
Optional: add weak_clear() before swap().

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

To record our offline discussion: a benchmark run in release mode showed that
even for vectors of size of tens of thousands, the time to destroy them is in
the order of tens of microseconds (on my machine) with or without weak_clear().
What takes time in the weak_clear() case could be the deallocation of the space
previously taken by the vector, but that's just a hypothesis.

(Just for comparison, the same benchmark run in debug mode did show the expected
asymptotic difference between the O(1) destruction with weak_clear() and the
linear-time destruction in the absence of weak_clear().)

The performance gain thus seems rather negligible, and would come on the cost of
code readability and robustness -- this whole refactoring is to get rid of
low-level memory management. So with vasilii@'s permission, I'm not addressing
this comment.


Attachment -- the benchmark, for the record:
  for (unsigned size_shift = 0; size_shift < 17; ++size_shift) {
    const size_t size = 1 << size_shift;
    std::vector<autofill::PasswordForm*> v(size, nullptr);
    base::TimeTicks t;
    {
      ScopedVector<autofill::PasswordForm> sv;
      sv.swap(v);
      t = base::TimeTicks::Now();
      // sv.weak_clear();
    }
    base::TimeDelta elapsed = base::TimeTicks::Now() - t;
    LOG(INFO) << "For size=" << size << ", no weak_clear, elapsed "
               << elapsed.InMicroseconds() << "us";
  }
  for (unsigned size_shift = 0; size_shift < 17; ++size_shift) {
    const size_t size = 1 << size_shift;
    std::vector<autofill::PasswordForm*> v(size, nullptr);
    base::TimeTicks t;
    {
      ScopedVector<autofill::PasswordForm> sv;
      sv.swap(v);
      t = base::TimeTicks::Now();
      sv.weak_clear();
    }
    base::TimeDelta elapsed = base::TimeTicks::Now() - t;
    LOG(INFO) << "For size=" << size << ", + weak_clear, elapsed "
               << elapsed.InMicroseconds() << "us";
  }

   }
+  forms->swap(non_blacklist);
 }
 
-void MergePasswordForms(std::vector<PasswordForm*>* keychain_forms,
-                        std::vector<PasswordForm*>* database_forms,
-                        std::vector<PasswordForm*>* merged_forms) {
+// Takes |keychain_forms| and |database_forms| and moves the following 2 types
+// of forms to |merged_forms|: (1) blacklisted |database_forms|, (2)
+// |database_forms| which have a corresponding entry in |keychain_forms|. The
+// database forms of type (2) have their password value updated from the
+// corresponding keychain form, and all the keychain forms corresponding to some
+// database form are removed from |keychain_forms| and deleted.
+void MergePasswordForms(ScopedVector<autofill::PasswordForm>* keychain_forms,
+                        ScopedVector<autofill::PasswordForm>* database_forms,
+                        ScopedVector<autofill::PasswordForm>* merged_forms) {
   // Pull out the database blacklist items, since they are used as-is rather
   // than being merged with keychain forms.
-  std::vector<PasswordForm*> database_blacklist_forms =
-      ExtractBlacklistForms(database_forms);
+  ExtractBlacklistForms(database_forms, merged_forms);
 
   // Merge the normal entries.
-  std::set<PasswordForm*> used_keychain_forms;
-  for (std::vector<PasswordForm*>::iterator i = database_forms->begin();
-       i != database_forms->end();) {
-    PasswordForm* db_form = *i;
-    PasswordForm* best_match = BestKeychainFormForForm(*db_form,
-                                                       keychain_forms);
+  ScopedVector<autofill::PasswordForm> unused_database_forms;
+  unused_database_forms.reserve(database_forms->size());
+  std::set<autofill::PasswordForm*> used_keychain_forms;
+  for (auto& db_form : *database_forms) {
+    PasswordForm* best_match =
+        BestKeychainFormForForm(*db_form, &keychain_forms->get());
     if (best_match) {
       used_keychain_forms.insert(best_match);
       db_form->password_value = best_match->password_value;
       merged_forms->push_back(db_form);
-      i = database_forms->erase(i);
     } else {
-      ++i;
+      unused_database_forms.push_back(db_form);
     }
+    db_form = nullptr;

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

again I'd prefer weak_clear.

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

Please see my response above.

   }
-
-  // Add in the blacklist entries from the database.
-  merged_forms->insert(merged_forms->end(),
-                       database_blacklist_forms.begin(),
-                       database_blacklist_forms.end());
+  database_forms->swap(unused_database_forms);
 
   // Clear out all the Keychain entries we used.
-  DeleteVectorElementsInSet(keychain_forms, used_keychain_forms);
+  ScopedVector<autofill::PasswordForm> unused_keychain_forms;
+  unused_keychain_forms.reserve(keychain_forms->size());
+  for (auto& keychain_form : *keychain_forms) {
+    if (!ContainsKey(used_keychain_forms, keychain_form)) {
+      using std::swap;

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

excessive here.

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

Done.

+      unused_keychain_forms.push_back(nullptr);
+      swap(keychain_form, unused_keychain_forms.back());

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

It's easier to understand

unused_keychain_forms.push_back(keychain_form);
keychain_form = nullptr;

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

Agreed. This was an overlooked relic from a past patch set.
Done.

+    }
+  }
+  keychain_forms->swap(unused_keychain_forms);
 }
 
 std::vector<ItemFormPair> ExtractAllKeychainItemAttributesIntoPasswordForms(
@@ -504,9 +496,9 @@ std::vector<ItemFormPair> ExtractAllKeychainItemAttributesIntoPasswordForms(
   return item_form_pairs;
 }
 
-std::vector<PasswordForm*> GetPasswordsForForms(
-    const AppleKeychain& keychain,
-    std::vector<PasswordForm*>* database_forms) {
+void GetPasswordsForForms(const AppleKeychain& keychain,
+                          ScopedVector<autofill::PasswordForm>* database_forms,
+                          ScopedVector<autofill::PasswordForm>* passwords) {
   // First load the attributes of all items in the keychain without loading
   // their password data, and then match items in |database_forms| against them.
   // This avoids individually searching through the keychain for passwords
@@ -522,29 +514,28 @@ std::vector<PasswordForm*> GetPasswordsForForms(
   // Next, compare the attributes of the PasswordForms in |database_forms|
   // against those in |item_form_pairs|, and extract password data for each
   // matching PasswordForm using its corresponding SecKeychainItemRef.
-  std::vector<PasswordForm*> merged_forms;
-  for (std::vector<PasswordForm*>::iterator i = database_forms->begin();
-       i != database_forms->end();) {
-    std::vector<PasswordForm*> db_form_container(1, *i);
-    std::vector<PasswordForm*> keychain_matches =
-        ExtractPasswordsMergeableWithForm(keychain, item_form_pairs, **i);
-    MergePasswordForms(&keychain_matches, &db_form_container, &merged_forms);
-    if (db_form_container.empty()) {
-      i = database_forms->erase(i);
-    } else {
-      ++i;
-    }
-    STLDeleteElements(&keychain_matches);
+  ScopedVector<autofill::PasswordForm> unused_db_forms;
+  unused_db_forms.reserve(database_forms->size());
+  for (auto& db_form : *database_forms) {
+    ScopedVector<autofill::PasswordForm> keychain_matches;
+    ExtractPasswordsMergeableWithForm(keychain, item_form_pairs, *db_form,
+                                      &keychain_matches);
+
+    ScopedVector<autofill::PasswordForm> db_form_container;
+    db_form_container.push_back(db_form);
+    db_form = nullptr;

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

I think weak_clear is better here.

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

Please see my answer above.

+    MergePasswordForms(&keychain_matches, &db_form_container, passwords);
+    unused_db_forms.insert(unused_db_forms.end(), db_form_container.begin(),
+                           db_form_container.end());
+    db_form_container.weak_clear();
   }
+  database_forms->swap(unused_db_forms);
 
-  // Clean up temporary PasswordForms and SecKeychainItemRefs.
   STLDeleteContainerPairSecondPointers(item_form_pairs.begin(),
                                        item_form_pairs.end());
-  for (std::vector<SecKeychainItemRef>::iterator i = keychain_items.begin();
-       i != keychain_items.end(); ++i) {
-    keychain.Free(*i);
+  for (auto& item : keychain_items) {

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

Here I'd prefer "SecKeychainItemRef item" but not a reference to the pointer.

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

Done.

+    keychain.Free(item);
   }
-  return merged_forms;
 }
 
 // TODO(stuartmorgan): signon_realm for proxies is not yet supported.
@@ -590,11 +581,11 @@ bool FormIsValidAndMatchesOtherForm(const PasswordForm& query_form,
       query_form, other_form, STRICT_FORM_MATCH);
 }
 
-std::vector<PasswordForm*> ExtractPasswordsMergeableWithForm(
+void ExtractPasswordsMergeableWithForm(
     const AppleKeychain& keychain,
     const std::vector<ItemFormPair>& item_form_pairs,
-    const PasswordForm& query_form) {
-  std::vector<PasswordForm*> matches;
+    const PasswordForm& query_form,
+    ScopedVector<autofill::PasswordForm>* matches) {

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

returning this object would simplify the caller.

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

Done.

(I misread the definition of ScopedVector and thought it does not have the
scoped_ptr's Pass() and friends to make it work as a return value. Now I read
base/move.h and repent my mistake. :))

   for (std::vector<ItemFormPair>::const_iterator i = item_form_pairs.begin();
        i != item_form_pairs.end(); ++i) {
     if (FormIsValidAndMatchesOtherForm(query_form, *(i->second))) {
@@ -608,10 +599,9 @@ std::vector<PasswordForm*> ExtractPasswordsMergeableWithForm(
           true);  // Load password attributes and data.
       // Do not include blacklisted items found in the keychain.
       if (!form_with_password->blacklisted_by_user)
-        matches.push_back(form_with_password.release());
+        matches->push_back(form_with_password.release());
     }
   }
-  return matches;
 }
 
 }  // namespace internal_keychain_helpers
@@ -623,13 +613,15 @@ MacKeychainPasswordFormAdapter::MacKeychainPasswordFormAdapter(
     : keychain_(keychain), finds_only_owned_(false) {
 }
 
-std::vector<PasswordForm*> MacKeychainPasswordFormAdapter::PasswordsFillingForm(
+// Gets keychain items corresponding to |signon_realm| and |scheme|, converts
+// them into PasswordForms and appends them to |forms|.
+void MacKeychainPasswordFormAdapter::PasswordsFillingForm(
     const std::string& signon_realm,
-    PasswordForm::Scheme scheme) {
+    PasswordForm::Scheme scheme,
+    ScopedVector<autofill::PasswordForm>* forms) {
   std::vector<SecKeychainItemRef> keychain_items =
       MatchingKeychainItems(signon_realm, scheme, NULL, NULL);
-
-  return ConvertKeychainItemsToForms(&keychain_items);
+  ConvertKeychainItemsToForms(&keychain_items, forms);
 }
 
 bool MacKeychainPasswordFormAdapter::HasPasswordExactlyMatchingForm(
@@ -681,10 +673,10 @@ std::vector<SecKeychainItemRef>
   return matches;
 }
 
-std::vector<PasswordForm*>
-    MacKeychainPasswordFormAdapter::GetAllPasswordFormPasswords() {
+void MacKeychainPasswordFormAdapter::GetAllPasswordFormPasswords(
+    ScopedVector<autofill::PasswordForm>* forms) {
   std::vector<SecKeychainItemRef> items = GetAllPasswordFormKeychainItems();
-  return ConvertKeychainItemsToForms(&items);
+  return ConvertKeychainItemsToForms(&items, forms);
 }
 
 bool MacKeychainPasswordFormAdapter::AddPassword(const PasswordForm& form) {
@@ -745,21 +737,18 @@ void MacKeychainPasswordFormAdapter::SetFindsOnlyOwnedItems(
   finds_only_owned_ = finds_only_owned;
 }
 
-std::vector<PasswordForm*>
-    MacKeychainPasswordFormAdapter::ConvertKeychainItemsToForms(
-        std::vector<SecKeychainItemRef>* items) {
-  std::vector<PasswordForm*> keychain_forms;
-  for (std::vector<SecKeychainItemRef>::const_iterator i = items->begin();
-       i != items->end(); ++i) {
-    PasswordForm* form = new PasswordForm();
+void MacKeychainPasswordFormAdapter::ConvertKeychainItemsToForms(
+    std::vector<SecKeychainItemRef>* items,
+    ScopedVector<autofill::PasswordForm>* forms) {

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

returning this would simplify the code.

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

Done.

+  for (const auto& item : *items) {

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

You are iterating over pointers. I'd prefer "SecKeychainItemRef item"

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

Done.

+    scoped_ptr<PasswordForm> form(new PasswordForm());
     if (internal_keychain_helpers::FillPasswordFormFromKeychainItem(
-            *keychain_, *i, form, true)) {
-      keychain_forms.push_back(form);
+            *keychain_, item, form.get(), true)) {
+      forms->push_back(form.release());
     }
-    keychain_->Free(*i);
+    keychain_->Free(item);
   }
   items->clear();
-  return keychain_forms;
 }
 
 SecKeychainItemRef MacKeychainPasswordFormAdapter::KeychainItemForForm(
@@ -983,15 +972,15 @@ PasswordStoreChangeList PasswordStoreMac::RemoveLoginsCreatedBetweenImpl(
     base::Time delete_begin,
     base::Time delete_end) {
   PasswordStoreChangeList changes;
-  ScopedVector<PasswordForm> forms;
+  ScopedVector<PasswordForm> forms_to_remove;
   if (login_metadata_db_ &&
       login_metadata_db_->GetLoginsCreatedBetween(delete_begin, delete_end,
-                                                  &forms.get()) &&
+                                                  &forms_to_remove) &&
       login_metadata_db_->RemoveLoginsCreatedBetween(delete_begin,
                                                      delete_end)) {
-    RemoveKeychainForms(forms.get());
-    CleanOrphanedForms(&forms.get());
-    changes = FormsToRemoveChangeList(forms.get());
+    RemoveKeychainForms(forms_to_remove.get());
+    CleanOrphanedForms(&forms_to_remove);  // Add the orphaned forms.
+    changes = FormsToRemoveChangeList(forms_to_remove.get());
     LogStatsForBulkDeletion(changes.size());
   }
   return changes;
@@ -1001,14 +990,14 @@ PasswordStoreChangeList PasswordStoreMac::RemoveLoginsSyncedBetweenImpl(
     base::Time delete_begin,
     base::Time delete_end) {
   PasswordStoreChangeList changes;
-  ScopedVector<PasswordForm> forms;
+  ScopedVector<PasswordForm> forms_to_remove;
   if (login_metadata_db_ &&
       login_metadata_db_->GetLoginsSyncedBetween(delete_begin, delete_end,
-                                                 &forms.get()) &&
+                                                 &forms_to_remove) &&
       login_metadata_db_->RemoveLoginsSyncedBetween(delete_begin, delete_end)) {
-    RemoveKeychainForms(forms.get());
-    CleanOrphanedForms(&forms.get());
-    changes = FormsToRemoveChangeList(forms.get());
+    RemoveKeychainForms(forms_to_remove.get());
+    CleanOrphanedForms(&forms_to_remove);  // Add the orphaned forms_to_remove.
+    changes = FormsToRemoveChangeList(forms_to_remove.get());
     LogStatsForBulkDeletionDuringRollback(changes.size());
   }
   return changes;
@@ -1022,97 +1011,96 @@ void PasswordStoreMac::GetLoginsImpl(
       prompt_policy == ALLOW_PROMPT);
 
   if (!login_metadata_db_) {
-    callback_runner.Run(std::vector<PasswordForm*>());
+    ScopedVector<autofill::PasswordForm> dummy_matched_forms;
+    callback_runner.Run(&dummy_matched_forms);
     return;
   }
 
   ScopedVector<PasswordForm> database_forms;
-  login_metadata_db_->GetLogins(form, &database_forms.get());
+  login_metadata_db_->GetLogins(form, &database_forms);
 
   // Let's gather all signon realms we want to match with keychain entries.
   std::set<std::string> realm_set;
   realm_set.insert(form.signon_realm);
-  for (std::vector<PasswordForm*>::const_iterator db_form =
-           database_forms.begin();
-       db_form != database_forms.end();
-       ++db_form) {
+  for (const auto& db_form : database_forms) {

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

const reference to pointer seems to be an overhead.

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

Done.

     // TODO(vabr): We should not be getting different schemes here.
     // http://crbug.com/340112
-    if (form.scheme != (*db_form)->scheme)
+    if (form.scheme != db_form->scheme)
       continue;  // Forms with different schemes never match.
-    const std::string& original_singon_realm((*db_form)->original_signon_realm);
+    const std::string& original_singon_realm(db_form->original_signon_realm);
     if (!original_singon_realm.empty())
       realm_set.insert(original_singon_realm);
   }
-  std::vector<PasswordForm*> keychain_forms;
+  ScopedVector<autofill::PasswordForm> keychain_forms;
   for (std::set<std::string>::const_iterator realm = realm_set.begin();
        realm != realm_set.end();
        ++realm) {
     MacKeychainPasswordFormAdapter keychain_adapter(keychain_.get());
-    std::vector<PasswordForm*> temp_keychain_forms =
-        keychain_adapter.PasswordsFillingForm(*realm, form.scheme);
-    keychain_forms.insert(keychain_forms.end(),
-                          temp_keychain_forms.begin(),
-                          temp_keychain_forms.end());
+    keychain_adapter.PasswordsFillingForm(*realm, form.scheme, &keychain_forms);
   }
 
-  std::vector<PasswordForm*> matched_forms;
-  internal_keychain_helpers::MergePasswordForms(&keychain_forms,
-                                                &database_forms.get(),
-                                                &matched_forms);
+  ScopedVector<autofill::PasswordForm> matched_forms;
+  internal_keychain_helpers::MergePasswordForms(
+      &keychain_forms, &database_forms, &matched_forms);
 
   // Strip any blacklist entries out of the unused Keychain array, then take
   // all the entries that are left (which we can use as imported passwords).
   ScopedVector<PasswordForm> keychain_blacklist_forms;
-  internal_keychain_helpers::ExtractBlacklistForms(&keychain_forms).swap(
-      keychain_blacklist_forms.get());
+  internal_keychain_helpers::ExtractBlacklistForms(&keychain_forms,
+                                                   &keychain_blacklist_forms);
   matched_forms.insert(matched_forms.end(),
                        keychain_forms.begin(),
                        keychain_forms.end());
-  keychain_forms.clear();
+  keychain_forms.weak_clear();
 
   if (!database_forms.empty()) {
     RemoveDatabaseForms(database_forms.get());
     NotifyLoginsChanged(FormsToRemoveChangeList(database_forms.get()));
   }
 
-  callback_runner.Run(matched_forms);
+  callback_runner.Run(&matched_forms);
 }
 
 void PasswordStoreMac::GetBlacklistLoginsImpl(GetLoginsRequest* request) {
-  FillBlacklistLogins(request->result());
+  ScopedVector<autofill::PasswordForm> obtained_forms;
+  FillBlacklistLogins(&obtained_forms);
+  request->result()->swap(obtained_forms.get());
+  obtained_forms.weak_clear();  // TODO(vabr): Change request to have a

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

Seems to be a leak.

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

You are right. I honestly don't know what I was thinking.
Done.

+                                // ScopedVector instead.
   ForwardLoginsResult(request);
 }
 
 void PasswordStoreMac::GetAutofillableLoginsImpl(GetLoginsRequest* request) {
-  FillAutofillableLogins(request->result());
+  ScopedVector<autofill::PasswordForm> obtained_forms;
+  FillAutofillableLogins(&obtained_forms);
+  request->result()->swap(obtained_forms.get());
+  obtained_forms.weak_clear();  // TODO(vabr): Change request to have a

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

Same here.

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

Done.

+                                // ScopedVector instead.
   ForwardLoginsResult(request);
 }
 
 bool PasswordStoreMac::FillAutofillableLogins(
-         std::vector<PasswordForm*>* forms) {
+    ScopedVector<autofill::PasswordForm>* forms) {
   DCHECK(thread_->message_loop() == base::MessageLoop::current());
 
   ScopedVector<PasswordForm> database_forms;
   if (!login_metadata_db_ ||
-      !login_metadata_db_->GetAutofillableLogins(&database_forms.get()))
+      !login_metadata_db_->GetAutofillableLogins(&database_forms))
     return false;
 
-  std::vector<PasswordForm*> merged_forms =
-      internal_keychain_helpers::GetPasswordsForForms(*keychain_,
-                                                      &database_forms.get());
+  internal_keychain_helpers::GetPasswordsForForms(*keychain_, &database_forms,
+                                                  forms);
 
   if (!database_forms.empty()) {
     RemoveDatabaseForms(database_forms.get());
     NotifyLoginsChanged(FormsToRemoveChangeList(database_forms.get()));
   }
 
-  forms->insert(forms->end(), merged_forms.begin(), merged_forms.end());
   return true;
 }
 
 bool PasswordStoreMac::FillBlacklistLogins(
-         std::vector<PasswordForm*>* forms) {
+    ScopedVector<autofill::PasswordForm>* forms) {
   DCHECK(thread_->message_loop() == base::MessageLoop::current());
   return login_metadata_db_ && login_metadata_db_->GetBlacklistLogins(forms);
 }
@@ -1129,21 +1117,19 @@ bool PasswordStoreMac::DatabaseHasFormMatchingKeychainForm(
     const autofill::PasswordForm& form) {
   DCHECK(login_metadata_db_);
   bool has_match = false;
-  std::vector<PasswordForm*> database_forms;
+  ScopedVector<autofill::PasswordForm> database_forms;
   login_metadata_db_->GetLogins(form, &database_forms);
-  for (std::vector<PasswordForm*>::iterator i = database_forms.begin();
-       i != database_forms.end(); ++i) {
+  for (const auto& db_form : database_forms) {

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

a reference to the pointer

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

Done.

     // Below we filter out forms with non-empty original_signon_realm, because
     // those signal fuzzy matches, and we are only interested in exact ones.
-    if ((*i)->original_signon_realm.empty() &&
+    if (db_form->original_signon_realm.empty() &&
         internal_keychain_helpers::FormsMatchForMerge(
-            form, **i, internal_keychain_helpers::STRICT_FORM_MATCH) &&
-        (*i)->origin == form.origin) {
+            form, *db_form, internal_keychain_helpers::STRICT_FORM_MATCH) &&
+        db_form->origin == form.origin) {
       has_match = true;
       break;
     }
   }
-  STLDeleteElements(&database_forms);
   return has_match;
 }
 
@@ -1166,19 +1152,24 @@ void PasswordStoreMac::RemoveKeychainForms(
   }
 }
 
-void PasswordStoreMac::CleanOrphanedForms(std::vector<PasswordForm*>* forms) {
-  DCHECK(forms);
+void PasswordStoreMac::CleanOrphanedForms(
+    ScopedVector<autofill::PasswordForm>* orphaned_forms) {
+  DCHECK(orphaned_forms);
   DCHECK(login_metadata_db_);
 
-  std::vector<PasswordForm*> database_forms;
+  ScopedVector<autofill::PasswordForm> database_forms;
   login_metadata_db_->GetAutofillableLogins(&database_forms);
 
-  ScopedVector<PasswordForm> merged_forms;
-  merged_forms.get() = internal_keychain_helpers::GetPasswordsForForms(
-      *keychain_, &database_forms);
+  // Filter forms with corresponding Keychain entry out of |database_forms|.
+  ScopedVector<PasswordForm> forms_with_keychain_entry;
+  internal_keychain_helpers::GetPasswordsForForms(*keychain_, &database_forms,
+                                                  &forms_with_keychain_entry);
 
   // Clean up any orphaned database entries.
-  RemoveDatabaseForms(database_forms);
+  RemoveDatabaseForms(database_forms.get());
 
-  forms->insert(forms->end(), database_forms.begin(), database_forms.end());
+  // Move the orphaned DB forms to the output parameter.
+  orphaned_forms->insert(orphaned_forms->end(), database_forms.begin(),
+                         database_forms.end());
+  database_forms.weak_clear();
 }