[sync] Significantly speed up password model association on mac

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 4b938c5a07a8d2b0bd6a8027f5ddbac7503f7d57..b17ab978cc8887fc070effd203ae55435c893019 100644
--- a/chrome/browser/password_manager/password_store_mac.cc
+++ b/chrome/browser/password_manager/password_store_mac.cc
@@ -8,6 +8,7 @@
 #include <CoreServices/CoreServices.h>
 #include <set>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/callback.h"
@@ -236,7 +237,8 @@ PasswordForm::Scheme SchemeForAuthType(SecAuthenticationType auth_type) {
 
 bool FillPasswordFormFromKeychainItem(const AppleKeychain& keychain,
                                       const SecKeychainItemRef& keychain_item,
-                                      PasswordForm* form) {
+                                      PasswordForm* form,
+                                      bool extract_password_data) {
   DCHECK(form);
 
   SecKeychainAttributeInfo attrInfo;
@@ -255,11 +257,18 @@ bool FillPasswordFormFromKeychainItem(const AppleKeychain& keychain,
 
   SecKeychainAttributeList *attrList;
   UInt32 password_length;
-  void* password_data;
+
+  // If |extract_password_data| is false, do not pass in a reference to
+  // |password_data|. ItemCopyAttributesAndData will then extract only the
+  // attributes of |keychain_item| (doesn't require OS authorization), and not
+  // attempt to extract its password data (requires OS authorization).
+  void* password_data = NULL;
+  void** password_data_ref = extract_password_data ? &password_data : NULL;
+
   OSStatus result = keychain.ItemCopyAttributesAndData(keychain_item, &attrInfo,
                                                        NULL, &attrList,
                                                        &password_length,
-                                                       &password_data);
+                                                       password_data_ref);
 
   if (result != noErr) {
     // We don't log errSecAuthFailed because that just means that the user
@@ -270,8 +279,10 @@ bool FillPasswordFormFromKeychainItem(const AppleKeychain& keychain,
     return false;
   }
 
-  UTF8ToUTF16(static_cast<const char *>(password_data), password_length,
-              &(form->password_value));
+  if (extract_password_data) {
+    UTF8ToUTF16(static_cast<const char *>(password_data), password_length,
+                &(form->password_value));
+  }
 
   int port = kAnyPort;
   std::string server;
@@ -333,7 +344,8 @@ bool FillPasswordFormFromKeychainItem(const AppleKeychain& keychain,
   // kSecNegativeItemAttr doesn't seem to actually be in widespread use. In
   // practice, other browsers seem to use a "" or " " password (and a special
   // user name) to indicated blacklist entries.
-  if (form->password_value.empty() || EqualsASCII(form->password_value, " ")) {
+  if (extract_password_data && (form->password_value.empty() ||
+                                EqualsASCII(form->password_value, " "))) {
     form->blacklisted_by_user = true;
   }
 
@@ -446,17 +458,52 @@ void MergePasswordForms(std::vector<PasswordForm*>* keychain_forms,
   DeleteVectorElementsInSet(keychain_forms, used_keychain_forms);
 }
 
+std::vector<MacKeychainPasswordFormAdapter::ItemFormPair>
+    GetAllKeychainItemAttributesAsPasswordForms(
+        std::vector<SecKeychainItemRef>* keychain_items,
+        const AppleKeychain& keychain) {
+  DCHECK(keychain_items);
+  MacKeychainPasswordFormAdapter keychain_adapter(&keychain);
+  *keychain_items = keychain_adapter.GetAllPasswordFormKeychainItems();
+  std::vector<MacKeychainPasswordFormAdapter::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) {
+  // 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<MacKeychainPasswordFormAdapter::ItemFormPair> item_form_pairs =
+      GetAllKeychainItemAttributesAsPasswordForms(&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.
   MacKeychainPasswordFormAdapter keychain_adapter(&keychain);
-
   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 =
-        keychain_adapter.PasswordsMergeableWithForm(**i);
+        keychain_adapter.ExtractPasswordsMergeableWithForm(item_form_pairs,
+                                                           **i);
     MergePasswordForms(&keychain_matches, &db_form_container, &merged_forms);
     if (db_form_container.empty()) {
       i = database_forms->erase(i);
@@ -465,6 +512,14 @@ std::vector<PasswordForm*> GetPasswordsForForms(
     }
     STLDeleteElements(&keychain_matches);
   }
+
+  // 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);
+  }
   return merged_forms;
 }
 
@@ -496,15 +551,40 @@ std::vector<PasswordForm*>
   return ConvertKeychainItemsToForms(&keychain_items);
 }
 
+bool MacKeychainPasswordFormAdapter::FormIsValidAndMatchesOtherForm(
+    const PasswordForm& query_form,
+    const PasswordForm& other_form) {
+  std::string server;
+  std::string security_domain;
+  int 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);
+}
+
 std::vector<PasswordForm*>
-    MacKeychainPasswordFormAdapter::PasswordsMergeableWithForm(
+    MacKeychainPasswordFormAdapter::ExtractPasswordsMergeableWithForm(

[stuartmorgan, 2013/09/30 22:39:12]

AFAICT this no longer operates on the keychain in any way, so it's weird to have
it be part of MacKeychainPasswordFormAdapter. Can it just be a stand-alone
helper?

[Raghu Simha, 2013/10/02 02:09:50]

We reference the keychain in line 580, while calling
FillPasswordFormFromKeychainItem, but this can just as easily be done by passing
a const reference to the keychain. The reason I originally made
ExtractPasswordsMergeableWithForm a member of MacKeychainPasswordFormAdapter was
because it calls FormIsValidAndMatchesOtherForm, which in turn calls
ExtractSignonRealmComponents, both of which are members of
MacKeychainPasswordFormAdapter. Since these methods do not actually operate on
the keychain, I've made them helper functions.

+        const std::vector<ItemFormPair>& item_form_pairs,
         const PasswordForm& query_form) {
-  std::string username = UTF16ToUTF8(query_form.username_value);
-  std::vector<SecKeychainItemRef> keychain_items =
-      MatchingKeychainItems(query_form.signon_realm, query_form.scheme,
-                            NULL, username.c_str());
-
-  return ConvertKeychainItemsToForms(&keychain_items);
+  std::vector<PasswordForm*> matches;
+  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.

[stuartmorgan, 2013/09/30 22:39:12]

Feel free to use ScopedVector anywhere it makes life easier here BTW; this code
predates it, which is the only reason it does everything manually.

[Raghu Simha, 2013/10/02 02:09:50]

I did consider using a ScopedVector here, but ended up not doing so, since the
vector of PasswordForm*s created here is processed in another function, and
therefore, cannot be deleted here.

+      PasswordForm* form_with_password = new PasswordForm();
+      internal_keychain_helpers::FillPasswordFormFromKeychainItem(
+          *keychain_,
+          *(i->first),
+          form_with_password,
+          true);  // Load password attributes and data.
+      matches.push_back(form_with_password);
+    }
+  }
+  return matches;
 }
 
 PasswordForm* MacKeychainPasswordFormAdapter::PasswordExactlyMatchingForm(
@@ -514,7 +594,8 @@ PasswordForm* MacKeychainPasswordFormAdapter::PasswordExactlyMatchingForm(
     PasswordForm* form = new PasswordForm();
     internal_keychain_helpers::FillPasswordFormFromKeychainItem(*keychain_,
                                                                 keychain_item,
-                                                                form);
+                                                                form,
+                                                                true);
     keychain_->Free(keychain_item);
     return form;
   }
@@ -535,8 +616,8 @@ bool MacKeychainPasswordFormAdapter::HasPasswordsMergeableWithForm(
   return !matches.empty();
 }
 
-std::vector<PasswordForm*>
-    MacKeychainPasswordFormAdapter::GetAllPasswordFormPasswords() {
+std::vector<SecKeychainItemRef>
+    MacKeychainPasswordFormAdapter::GetAllPasswordFormKeychainItems() {
   SecAuthenticationType supported_auth_types[] = {
     kSecAuthenticationTypeHTMLForm,
     kSecAuthenticationTypeHTTPBasic,
@@ -550,8 +631,13 @@ std::vector<PasswordForm*>
                          NULL, NULL, NULL, CreatorCodeForSearch());
     keychain_search.FindMatchingItems(&matches);
   }
+  return matches;
+}
 
-  return ConvertKeychainItemsToForms(&matches);
+std::vector<PasswordForm*>
+    MacKeychainPasswordFormAdapter::GetAllPasswordFormPasswords() {
+  std::vector<SecKeychainItemRef> items = GetAllPasswordFormKeychainItems();
+  return ConvertKeychainItemsToForms(&items);
 }
 
 bool MacKeychainPasswordFormAdapter::AddPassword(const PasswordForm& form) {
@@ -619,8 +705,8 @@ std::vector<PasswordForm*>
   for (std::vector<SecKeychainItemRef>::const_iterator i = items->begin();
        i != items->end(); ++i) {
     PasswordForm* form = new PasswordForm();
-    if (internal_keychain_helpers::FillPasswordFormFromKeychainItem(*keychain_,
-                                                                    *i, form)) {
+    if (internal_keychain_helpers::FillPasswordFormFromKeychainItem(
+            *keychain_, *i, form, true)) {
       keychain_forms.push_back(form);
     }
     keychain_->Free(*i);