PasswordStore: Use ScopedVector to express ownership of forms

chrome/browser/password_manager/password_store_mac.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/password_manager/password_store_mac.h"
 #include "chrome/browser/password_manager/password_store_mac_internal.h"
 
 #include <CoreServices/CoreServices.h>
 #include <set>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/callback.h"
 #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"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/mac/security_wrappers.h"
 #include "components/password_manager/core/browser/login_database.h"
 #include "components/password_manager/core/browser/password_store_change.h"
 #include "content/public/browser/browser_thread.h"
 #include "crypto/apple_keychain.h"
 
@@ skipping 382 matching lines @@
       if (base_form.origin == (*i)->origin) {
         return *i;
       } else if (!partial_match) {
         partial_match = *i;
       }
     }
   }
   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;
-    }
+// 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";
  }

   }
-  return blacklist_forms;
+  forms->swap(non_blacklist);
 }
 
-// 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;
-    }
-  }
-}
-
-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.

+  }
+  database_forms->swap(unused_database_forms);
+
+  // Clear out all the Keychain entries we used.
+  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.

     }
   }
-
-  // Add in the blacklist entries from the database.
-  merged_forms->insert(merged_forms->end(),
-                       database_blacklist_forms.begin(),
-                       database_blacklist_forms.end());
-
-  // Clear out all the Keychain entries we used.
-  DeleteVectorElementsInSet(keychain_forms, used_keychain_forms);
+  keychain_forms->swap(unused_keychain_forms);
 }
 
 std::vector<ItemFormPair> ExtractAllKeychainItemAttributesIntoPasswordForms(
     std::vector<SecKeychainItemRef>* keychain_items,
     const AppleKeychain& keychain) {
   DCHECK(keychain_items);
   MacKeychainPasswordFormAdapter keychain_adapter(&keychain);
   *keychain_items = keychain_adapter.GetAllPasswordFormKeychainItems();
   std::vector<ItemFormPair> item_form_pairs;
   for (std::vector<SecKeychainItemRef>::iterator i = keychain_items->begin();
        i != keychain_items->end(); ++i) {
     PasswordForm* form_without_password = new PasswordForm();
     internal_keychain_helpers::FillPasswordFormFromKeychainItem(
         keychain,
         *i,
         form_without_password,
         false);  // Load password attributes, but not password data.
     item_form_pairs.push_back(std::make_pair(&(*i), form_without_password));
   }
   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
   // matching each form in |database_forms|, and results in a significant
   // performance gain, replacing O(N) keychain search operations with a single
   // operation that loads all keychain items, and then selective reads of only
   // the relevant passwords. See crbug.com/263685.
   std::vector<SecKeychainItemRef> keychain_items;
   std::vector<ItemFormPair> item_form_pairs =
       ExtractAllKeychainItemAttributesIntoPasswordForms(&keychain_items,
                                                         keychain);
 
   // 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.
 bool ExtractSignonRealmComponents(const std::string& signon_realm,
                                   std::string* server,
                                   UInt32* port,
                                   bool* is_secure,
                                   std::string* security_domain) {
   // The signon_realm will be the Origin portion of a URL for an HTML form,
   // and the same but with the security domain as a path for HTTP auth.
@@ skipping 25 matching lines @@
   UInt32 port;
   bool is_secure;
   if (!ExtractSignonRealmComponents(query_form.signon_realm, &server, &port,
                                     &is_secure, &security_domain)) {
     return false;
   }
   return internal_keychain_helpers::FormsMatchForMerge(
       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))) {
       // Create a new object, since the caller is responsible for deleting the
       // returned forms.
       scoped_ptr<PasswordForm> form_with_password(new PasswordForm());
       internal_keychain_helpers::FillPasswordFormFromKeychainItem(
           keychain,
           *(i->first),
           form_with_password.get(),
           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
 
 #pragma mark -
 
 MacKeychainPasswordFormAdapter::MacKeychainPasswordFormAdapter(
     const AppleKeychain* keychain)
     : 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(
     const PasswordForm& query_form) {
   SecKeychainItemRef keychain_item = KeychainItemForForm(query_form);
   if (keychain_item) {
     keychain_->Free(keychain_item);
     return true;
   }
   return false;
@@ skipping 31 matching lines @@
                          &supported_auth_types[i],
                          NULL,
                          NULL,
                          NULL,
                          creator ? &creator : NULL);
     keychain_search.FindMatchingItems(&matches);
   }
   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) {
   // We should never be trying to store a blacklist in the keychain.
   DCHECK(!form.blacklisted_by_user);
 
   std::string server;
   std::string security_domain;
   UInt32 port;
   bool is_secure;
@@ skipping 40 matching lines @@
   OSStatus result = keychain_->ItemDelete(keychain_item);
   keychain_->Free(keychain_item);
   return result == noErr;
 }
 
 void MacKeychainPasswordFormAdapter::SetFindsOnlyOwnedItems(
     bool finds_only_owned) {
   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(
     const PasswordForm& form) {
   // We don't store blacklist entries in the keychain, so the answer to "what
   // Keychain item goes with this form" is always "nothing" for blacklists.
   if (form.blacklisted_by_user) {
     return NULL;
   }
 
@@ skipping 203 matching lines @@
 
     changes.push_back(PasswordStoreChange(PasswordStoreChange::REMOVE, form));
   }
   return changes;
 }
 
 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;
 }
 
 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;
 }
 
 void PasswordStoreMac::GetLoginsImpl(
     const autofill::PasswordForm& form,
     AuthorizationPromptPolicy prompt_policy,
     const ConsumerCallbackRunner& callback_runner) {
   chrome::ScopedSecKeychainSetUserInteractionAllowed user_interaction_allowed(
       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);
 }
 
 bool PasswordStoreMac::AddToKeychainIfNecessary(const PasswordForm& form) {
   if (form.blacklisted_by_user) {
     return true;
   }
   MacKeychainPasswordFormAdapter keychainAdapter(keychain_.get());
   return keychainAdapter.AddPassword(form);
 }
 
 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;
 }
 
 void PasswordStoreMac::RemoveDatabaseForms(
     const std::vector<PasswordForm*>& forms) {
   DCHECK(login_metadata_db_);
   for (std::vector<PasswordForm*>::const_iterator i = forms.begin();
        i != forms.end(); ++i) {
     login_metadata_db_->RemoveLogin(**i);
   }
 }
 
 void PasswordStoreMac::RemoveKeychainForms(
     const std::vector<PasswordForm*>& forms) {
   MacKeychainPasswordFormAdapter owned_keychain_adapter(keychain_.get());
   owned_keychain_adapter.SetFindsOnlyOwnedItems(true);
   for (std::vector<PasswordForm*>::const_iterator i = forms.begin();
        i != forms.end(); ++i) {
     owned_keychain_adapter.RemovePassword(**i);
   }
 }
 
-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();
 }