AutoFill Mac fails to fill Address Book card data

Changes the Mac implementation of auxiliary profile retrieval (Address Book data) to properly
manage unique ids assigned to profiles originating from the Address Book.

BUG=49657
TEST=PersonalDataManagerTest.*, and manual steps of (1) load a fillable HTML form, (2) click fillable field, (3) pick profile originating from Address Book, (4) observe that it fills the form.

Review URL: http://codereview.chromium.org/3055011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@53390 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 40 insertions, 18 deletions

diff --git a/chrome/browser/autofill/personal_data_manager.cc b/chrome/browser/autofill/personal_data_manager.cc
index e566909..61b47b7b 100644
--- a/chrome/browser/autofill/personal_data_manager.cc
+++ b/chrome/browser/autofill/personal_data_manager.cc
@@ -271,11 +271,8 @@ void PersonalDataManager::SetProfiles(std::vector<AutoFillProfile>* profiles) {
     // unique ID.  This also means we need to add this profile to the web
     // database.
     if (iter->unique_id() == 0) {
-      iter->set_unique_id(CreateNextUniqueID(&unique_ids_));
+      iter->set_unique_id(CreateNextUniqueIDFor(&unique_profile_ids_));
       wds->AddAutoFillProfile(*iter);
-
-      // Update the list of unique profile IDs.
-      unique_profile_ids_.insert(iter->unique_id());
     }
   }
 
@@ -352,11 +349,8 @@ void PersonalDataManager::SetCreditCards(
     // unique ID.  This also means we need to add this credit card to the web
     // database.
     if (iter->unique_id() == 0) {
-      iter->set_unique_id(CreateNextUniqueID(&unique_ids_));
+      iter->set_unique_id(CreateNextUniqueIDFor(&unique_creditcard_ids_));
       wds->AddCreditCard(*iter);
-
-      // Update the list of unique credit card IDs.
-      unique_creditcard_ids_.insert(iter->unique_id());
     }
   }
 
@@ -446,9 +440,7 @@ AutoFillProfile* PersonalDataManager::CreateNewEmptyAutoFillProfileForDBThread(
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
   AutoLock lock(unique_ids_lock_);
   AutoFillProfile* p = new AutoFillProfile(label,
-      CreateNextUniqueID(&unique_ids_));
-  // Also update the unique profile IDs.
-  unique_profile_ids_.insert(p->unique_id());
+      CreateNextUniqueIDFor(&unique_profile_ids_));
   return p;
 }
 
@@ -470,14 +462,15 @@ void PersonalDataManager::Init(Profile* profile) {
   LoadCreditCards();
 }
 
-int PersonalDataManager::CreateNextUniqueID(std::set<int>* unique_ids) {
+int PersonalDataManager::CreateNextUniqueIDFor(std::set<int>* id_set) {
   // Profile IDs MUST start at 1 to allow 0 as an error value when reading
   // the ID from the WebDB (see LoadData()).
   unique_ids_lock_.AssertAcquired();
   int id = 1;
-  while (unique_ids->count(id) != 0)
+  while (unique_ids_.count(id) != 0)
     ++id;
-  unique_ids->insert(id);
+  unique_ids_.insert(id);
+  id_set->insert(id);
   return id;
 }
 
diff --git a/chrome/browser/autofill/personal_data_manager.h b/chrome/browser/autofill/personal_data_manager.h
index 05a37b0..87c909e 100644
--- a/chrome/browser/autofill/personal_data_manager.h
+++ b/chrome/browser/autofill/personal_data_manager.h
@@ -163,8 +163,11 @@ class PersonalDataManager
   // Returns the profile of the tab contents.
   Profile* profile();
 
-  // This will create and reserve a new unique ID for a profile.
-  int CreateNextUniqueID(std::set<int>* unique_ids);
+  // This will create and reserve a new unique ID for the id pool |id_set|.
+  // The |id_set| is typically |unique_profile_ids_| or
+  // |unique_creditcard_ids_|.  The global pool |unique_ids_| is used to ensure
+  // uniqueness of ids across all pools.  The new (next) unique id is returned.
+  int CreateNextUniqueIDFor(std::set<int>* id_set);
 
   // Loads the saved profiles from the web database.
   virtual void LoadProfiles();
@@ -213,6 +216,10 @@ class PersonalDataManager
   // profile ID.
   std::set<int> unique_profile_ids_;
 
+  // The set of already created unique profile IDs for auxiliary profiles, used
+  // to create a new unique auxiliary profile ID.
+  std::set<int> unique_auxiliary_profile_ids_;
+
   // The set of already created unique credit card IDs, used to create a new
   // unique credit card ID.
   std::set<int> unique_creditcard_ids_;
diff --git a/chrome/browser/autofill/personal_data_manager_mac.mm b/chrome/browser/autofill/personal_data_manager_mac.mm
index ff199af..b2bf60d 100644
--- a/chrome/browser/autofill/personal_data_manager_mac.mm
+++ b/chrome/browser/autofill/personal_data_manager_mac.mm
@@ -250,6 +250,28 @@ void AuxiliaryProfilesImpl::GetAddressBookPhoneNumbers(
 
 // Populate |auxiliary_profiles_| with the Address Book data.
 void PersonalDataManager::LoadAuxiliaryProfiles() {
+  AutoLock lock(unique_ids_lock_);
+
+  // Before loading new auxiliary profiles remove the unique ids from the
+  // id pools.  The |GetAddressBookMeCard()| call below clears the
+  // |auxiliary_profiles_|.
+  unique_auxiliary_profile_ids_.clear();
+  for (ScopedVector<AutoFillProfile>::iterator iter
+           = auxiliary_profiles_.begin();
+       iter != auxiliary_profiles_.end(); ++iter) {
+    if ((*iter)->unique_id() != 0) {
+      unique_ids_.erase((*iter)->unique_id());
+    }
+  }
+
   AuxiliaryProfilesImpl impl(&auxiliary_profiles_);
   impl.GetAddressBookMeCard();
+
+  // For newly fetched auxiliary profiles, ensure that we have unique ids set.
+  for (ScopedVector<AutoFillProfile>::iterator iter
+           = auxiliary_profiles_.begin();
+       iter != auxiliary_profiles_.end(); ++iter) {
+    (*iter)->set_unique_id(
+        CreateNextUniqueIDFor(&unique_auxiliary_profile_ids_));
+  }
 }
diff --git a/chrome/browser/autofill/personal_data_manager_unittest.cc b/chrome/browser/autofill/personal_data_manager_unittest.cc
index b60ef41..7f61789 100644
--- a/chrome/browser/autofill/personal_data_manager_unittest.cc
+++ b/chrome/browser/autofill/personal_data_manager_unittest.cc
@@ -82,8 +82,8 @@ class PersonalDataManagerTest : public testing::Test {
   AutoFillProfile* MakeProfile() {
     AutoLock lock(personal_data_->unique_ids_lock_);
     return new AutoFillProfile(string16(),
-        personal_data_->CreateNextUniqueID(
-        &personal_data_->unique_profile_ids_));
+        personal_data_->CreateNextUniqueIDFor(
+            &personal_data_->unique_profile_ids_));
   }
 
   MessageLoopForUI message_loop_;