PasswordStoreMac::RemoveLoginsCreatedBetween() shouldn't affect other profiles.

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/mac_logging.h"
 #include "base/mac/mac_util.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 924 matching lines @@
 
     changes.push_back(PasswordStoreChange(PasswordStoreChange::REMOVE, form));
   }
   return changes;
 }
 
 PasswordStoreChangeList PasswordStoreMac::RemoveLoginsCreatedBetweenImpl(
     base::Time delete_begin,
     base::Time delete_end) {
   PasswordStoreChangeList changes;
-  std::vector<PasswordForm*> forms;
+  ScopedVector<PasswordForm> forms;

[vabr (Chromium), 2014/07/02 13:15:55]

It's great that you caught the leak.
Looking at the code of LoginDatabase::GetLogins* and similar methods in
LoginDatabase which return a vector of PasswordForm*, they all pass the
ownership to the caller.
Could you please add comment to the declarations of those methods to make that
clear, so that people are less likely to reintroduce the same error again?

[vasilii, 2014/07/02 14:02:58]

Done.

   if (login_metadata_db_->GetLoginsCreatedBetween(delete_begin, delete_end,
-                                                  &forms)) {
+                                                  &forms.get())) {
     if (login_metadata_db_->RemoveLoginsCreatedBetween(delete_begin,
                                                        delete_end)) {
-      // We can't delete from the Keychain by date because we may be sharing
-      // items with database entries that weren't in the delete range. Instead,
-      // we find all the Keychain items we own but aren't using any more and
-      // delete those.
-      std::vector<PasswordForm*> orphan_keychain_forms =
-          GetUnusedKeychainForms();
-      // This is inefficient, since we have to re-look-up each keychain item
-      // one at a time to delete it even though the search step already had a
-      // list of Keychain item references. If this turns out to be noticeably
-      // slow we'll need to rearchitect to allow the search and deletion steps
-      // to share.
-      RemoveKeychainForms(orphan_keychain_forms);
-      STLDeleteElements(&orphan_keychain_forms);
+      RemoveKeychainForms(forms.get());
 
       for (std::vector<PasswordForm*>::const_iterator it = forms.begin();
            it != forms.end(); ++it) {
         changes.push_back(PasswordStoreChange(PasswordStoreChange::REMOVE,
                                               **it));
       }
       LogStatsForBulkDeletion(changes.size());
     }
   }
   return changes;
 }
 
 PasswordStoreChangeList PasswordStoreMac::RemoveLoginsSyncedBetweenImpl(
     base::Time delete_begin,
     base::Time delete_end) {
   PasswordStoreChangeList changes;
-  std::vector<PasswordForm*> forms;
+  ScopedVector<PasswordForm> forms;
   if (login_metadata_db_->GetLoginsSyncedBetween(
-          delete_begin, delete_end, &forms)) {
+          delete_begin, delete_end, &forms.get())) {
     if (login_metadata_db_->RemoveLoginsSyncedBetween(delete_begin,
                                                       delete_end)) {
-      // We can't delete from the Keychain by date because we may be sharing
-      // items with database entries that weren't in the delete range. Instead,
-      // we find all the Keychain items we own but aren't using any more and
-      // delete those.
-      std::vector<PasswordForm*> orphan_keychain_forms =
-          GetUnusedKeychainForms();
-      // This is inefficient, since we have to re-look-up each keychain item
-      // one at a time to delete it even though the search step already had a
-      // list of Keychain item references. If this turns out to be noticeably
-      // slow we'll need to rearchitect to allow the search and deletion steps
-      // to share.
-      RemoveKeychainForms(orphan_keychain_forms);
-      STLDeleteElements(&orphan_keychain_forms);
+      RemoveKeychainForms(forms.get());
 
       for (std::vector<PasswordForm*>::const_iterator it = forms.begin();
            it != forms.end();
            ++it) {
         changes.push_back(
             PasswordStoreChange(PasswordStoreChange::REMOVE, **it));
       }
     }
   }
   return changes;
@@ skipping 115 matching lines @@
             form, **i, internal_keychain_helpers::STRICT_FORM_MATCH) &&
         (*i)->origin == form.origin) {
       has_match = true;
       break;
     }
   }
   STLDeleteElements(&database_forms);
   return has_match;
 }
 
-std::vector<PasswordForm*> PasswordStoreMac::GetUnusedKeychainForms() {
-  std::vector<PasswordForm*> database_forms;
-  login_metadata_db_->GetAutofillableLogins(&database_forms);
-
-  MacKeychainPasswordFormAdapter owned_keychain_adapter(keychain_.get());
-  owned_keychain_adapter.SetFindsOnlyOwnedItems(true);
-  std::vector<PasswordForm*> owned_keychain_forms =
-      owned_keychain_adapter.GetAllPasswordFormPasswords();
-
-  // Run a merge; anything left in owned_keychain_forms when we are done no
-  // longer has a matching database entry.
-  std::vector<PasswordForm*> merged_forms;
-  internal_keychain_helpers::MergePasswordForms(&owned_keychain_forms,
-                                                &database_forms,
-                                                &merged_forms);
-  STLDeleteElements(&merged_forms);
-  STLDeleteElements(&database_forms);
-
-  return owned_keychain_forms;
-}
-
 void PasswordStoreMac::RemoveDatabaseForms(
     const std::vector<PasswordForm*>& forms) {
   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);
   }
 }