fixing the case of duplicate profiles.

BUG=
TEST=

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

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

4 files changed, 84 insertions, 105 deletions

diff --git a/chrome/browser/sync/glue/autofill_model_associator.cc b/chrome/browser/sync/glue/autofill_model_associator.cc
index f1340df..289d5fb 100644
--- a/chrome/browser/sync/glue/autofill_model_associator.cc
+++ b/chrome/browser/sync/glue/autofill_model_associator.cc
@@ -237,8 +237,7 @@ bool AutofillModelAssociator::TraverseAndAssociateAllSyncNodes(
 
   bool autofill_profile_not_migrated = HasNotMigratedYet(write_trans);
 
-  if (MigrationLoggingEnabled() &&
-     autofill_profile_not_migrated) {
+  if (VLOG_IS_ON(1) && autofill_profile_not_migrated) {
     VLOG(1) << "[AUTOFILL MIGRATION]"
             << "Printing profiles from web db";
 
@@ -251,7 +250,7 @@ bool AutofillModelAssociator::TraverseAndAssociateAllSyncNodes(
     }
   }
 
-  if (MigrationLoggingEnabled() && autofill_profile_not_migrated) {
+  if (autofill_profile_not_migrated) {
     VLOG(1) << "[AUTOFILL MIGRATION]"
             << "Iterating over sync db";
   }
@@ -271,11 +270,9 @@ bool AutofillModelAssociator::TraverseAndAssociateAllSyncNodes(
     } else if (autofill.has_profile()) {
       // Ignore autofill profiles if we are not upgrading.
       if (autofill_profile_not_migrated) {
-        if (MigrationLoggingEnabled()) {
-          VLOG(1) << "[AUTOFILL MIGRATION] Looking for "
-                  << autofill.profile().name_first()
-                  << autofill.profile().name_last();
-        }
+        VLOG(1) << "[AUTOFILL MIGRATION] Looking for "
+                << autofill.profile().name_first()
+                << autofill.profile().name_last();
         AddNativeProfileIfNeeded(
             autofill.profile(),
             bundle,
@@ -355,19 +352,15 @@ void AutofillModelAssociator::AddNativeProfileIfNeeded(
       profile, all_profiles_from_db);
 
   if (profile_in_web_db != NULL) {
-    if (MigrationLoggingEnabled()) {
-      VLOG(1) << "[AUTOFILL MIGRATION]"
-              << "Node found in web db. So associating";
-    }
+    VLOG(1) << "[AUTOFILL MIGRATION]"
+            << "Node found in web db. So associating";
     int64 sync_id = node.GetId();
     std::string guid = profile_in_web_db->guid();
     Associate(&guid, sync_id);
     return;
   } else {  // Create a new node.
-    if (MigrationLoggingEnabled()) {
-      VLOG(1) << "[AUTOFILL MIGRATION]"
-              << "Node not found in web db so creating and associating";
-    }
+    VLOG(1) << "[AUTOFILL MIGRATION]"
+            << "Node not found in web db so creating and associating";
     std::string guid = guid::GenerateGUID();
     Associate(&guid, node.GetId());
     AutoFillProfile* p = new AutoFillProfile(guid);
@@ -555,11 +548,9 @@ bool AutofillModelAssociator::HasNotMigratedYet(
   }
 
   if (autofill_migration_state == syncable::INSUFFICIENT_INFO_TO_DETERMINE) {
-      if (MigrationLoggingEnabled()) {
-        VLOG(1) << "[AUTOFILL MIGRATION]"
-                << "current autofill migration state is insufficient info to"
-                << "determine.";
-      }
+      VLOG(1) << "[AUTOFILL MIGRATION]"
+              << "current autofill migration state is insufficient info to"
+              << "determine.";
       sync_api::ReadNode autofill_profile_root_node(trans);
       if (!autofill_profile_root_node.InitByTagLookup(
           browser_sync::kAutofillProfileTag) ||
@@ -568,29 +559,21 @@ bool AutofillModelAssociator::HasNotMigratedYet(
         sync_service()->backend()->SetAutofillMigrationState(
             syncable::NOT_MIGRATED);
 
-        if (MigrationLoggingEnabled()) {
-          VLOG(1) << "[AUTOFILL MIGRATION]"
-                  << "Current autofill migration state is NOT Migrated because"
-                  << "legacy autofill root node is present whereas new "
-                  << "Autofill profile root node is absent.";
-        }
+        VLOG(1) << "[AUTOFILL MIGRATION]"
+                << "Current autofill migration state is NOT Migrated because"
+                << "legacy autofill root node is present whereas new "
+                << "Autofill profile root node is absent.";
         return true;
       }
 
       sync_service()->backend()->SetAutofillMigrationState(syncable::MIGRATED);
 
-      if (MigrationLoggingEnabled()) {
-          VLOG(1) << "[AUTOFILL MIGRATION]"
-                  << "Current autofill migration state is migrated.";
-      }
+      VLOG(1) << "[AUTOFILL MIGRATION]"
+              << "Current autofill migration state is migrated.";
   }
 
   return false;
 }
 
-bool AutofillModelAssociator::MigrationLoggingEnabled() {
-  // [TODO] enable logging via a command line flag.
-  return false;
-}
 }  // namespace browser_sync
 
diff --git a/chrome/browser/sync/glue/autofill_model_associator.h b/chrome/browser/sync/glue/autofill_model_associator.h
index 3f1104e..553580f 100644
--- a/chrome/browser/sync/glue/autofill_model_associator.h
+++ b/chrome/browser/sync/glue/autofill_model_associator.h
@@ -173,8 +173,6 @@ class AutofillModelAssociator
   // user requested an abort.
   bool IsAbortPending();
 
-  bool MigrationLoggingEnabled();
-
   ProfileSyncService* sync_service_;
   WebDatabase* web_database_;
   PersonalDataManager* personal_data_;
diff --git a/chrome/browser/sync/glue/autofill_profile_model_associator.cc b/chrome/browser/sync/glue/autofill_profile_model_associator.cc
index b2d2e2c..c517e2c 100644
--- a/chrome/browser/sync/glue/autofill_profile_model_associator.cc
+++ b/chrome/browser/sync/glue/autofill_profile_model_associator.cc
@@ -46,7 +46,7 @@ bool AutofillProfileModelAssociator::TraverseAndAssociateChromeAutoFillProfiles(
     std::vector<AutoFillProfile*>* new_profiles,
     std::vector<std::string>* profiles_to_delete) {
 
-  if (MigrationLoggingEnabled()) {
+  if (VLOG_IS_ON(1)) {
     VLOG(1) << "[AUTOFILL MIGRATION]"
             << "Printing profiles from web db";
 
@@ -60,10 +60,9 @@ bool AutofillProfileModelAssociator::TraverseAndAssociateChromeAutoFillProfiles(
     }
   }
 
-  if (MigrationLoggingEnabled()) {
-    VLOG(1) << "[AUTOFILL MIGRATION]"
-            << "Looking for the above data in sync db..";
-  }
+  VLOG(1) << "[AUTOFILL MIGRATION]"
+          << "Looking for the above data in sync db..";
+
   // Alias the all_profiles_from_db so we fit in 80 characters
   const std::vector<AutoFillProfile*>& profiles(all_profiles_from_db);
   for (std::vector<AutoFillProfile*>::const_iterator ix = profiles.begin();
@@ -72,15 +71,18 @@ bool AutofillProfileModelAssociator::TraverseAndAssociateChromeAutoFillProfiles(
     std::string guid((*ix)->guid());
 
     ReadNode node(write_trans);
-    if (node.InitByClientTagLookup(syncable::AUTOFILL_PROFILE, guid)) {
-      if (MigrationLoggingEnabled()) {
-        VLOG(1) << "[AUTOFILL MIGRATION]"
-                << " Found in sync db: "
-                << (*ix)->GetFieldText(AutoFillType(NAME_FIRST))
-                << (*ix)->GetFieldText(AutoFillType(NAME_LAST))
-                << (*ix)->guid()
-                << " so associating";
-      }
+    if (node.InitByClientTagLookup(syncable::AUTOFILL_PROFILE, guid) &&
+        // The following check is to ensure the given sync node is not already
+        // associated with another profile. That could happen if the user has
+        // the same profile duplicated.
+        current_profiles->find(guid) == current_profiles->end()) {
+
+      VLOG(1) << "[AUTOFILL MIGRATION]"
+              << " Found in sync db: "
+              << (*ix)->GetFieldText(AutoFillType(NAME_FIRST))
+              << (*ix)->GetFieldText(AutoFillType(NAME_LAST))
+              << (*ix)->guid()
+              << " so associating with node id " << node.GetId();
       const sync_pb::AutofillProfileSpecifics& autofill(
           node.GetAutofillProfileSpecifics());
       if (OverwriteProfileWithServerData(*ix, autofill)) {
@@ -138,11 +140,9 @@ bool AutofillProfileModelAssociator::AssociateModels() {
     return false;
   }
 
-  if (MigrationLoggingEnabled()) {
-    VLOG(1) << "[AUTOFILL MIGRATION]"
-            << " Now associating to the new autofill profile model associator"
-            << " root node";
-  }
+  VLOG(1) << "[AUTOFILL MIGRATION]"
+          << " Now associating to the new autofill profile model associator"
+          << " root node";
   DataBundle bundle;
   {
     // The write transaction lock is held inside this block.
@@ -251,7 +251,8 @@ bool AutofillProfileModelAssociator::OverwriteProfileWithServerData(
 int64 AutofillProfileModelAssociator::FindSyncNodeWithProfile(
     sync_api::WriteTransaction* trans,
     const sync_api::BaseNode& autofill_root,
-    const AutoFillProfile& profile_from_db) {
+    const AutoFillProfile& profile_from_db,
+    std::set<std::string>* current_profiles) {
   int64 sync_child_id = autofill_root.GetFirstChildId();
   while (sync_child_id != sync_api::kInvalidId) {
     ReadNode read_node(trans);
@@ -263,9 +264,14 @@ int64 AutofillProfileModelAssociator::FindSyncNodeWithProfile(
     }
     const sync_pb::AutofillProfileSpecifics& autofill_specifics(
         read_node.GetAutofillProfileSpecifics());
-    OverwriteProfileWithServerData(&p, autofill_specifics);
-    if (p.Compare(profile_from_db) == 0) {
-      return sync_child_id;
+
+    // This find should be fast as the set uses tree.
+    if (current_profiles->find(autofill_specifics.guid())
+        == current_profiles->end()) {
+      OverwriteProfileWithServerData(&p, autofill_specifics);
+      if (p.Compare(profile_from_db) == 0) {
+        return sync_child_id;
+      }
     }
     sync_child_id = read_node.GetSuccessorId();
   }
@@ -280,7 +286,10 @@ bool AutofillProfileModelAssociator::MakeNewAutofillProfileSyncNodeIfNeeded(
     std::set<std::string>* current_profiles,
     std::vector<std::string>* profiles_to_delete) {
 
-  int64 sync_node_id = FindSyncNodeWithProfile(trans, autofill_root, profile);
+  int64 sync_node_id = FindSyncNodeWithProfile(trans,
+      autofill_root,
+      profile,
+      current_profiles);
   if (sync_node_id != sync_api::kInvalidId) {
     // In case of duplicates throw away the local profile and apply the
     // server profile.(The only difference between the 2 profiles are the guids)
@@ -298,14 +307,13 @@ bool AutofillProfileModelAssociator::MakeNewAutofillProfileSyncNodeIfNeeded(
     std::string guid = autofill_specifics.guid();
     Associate(&guid, sync_node_id);
     current_profiles->insert(autofill_specifics.guid());
-    if (MigrationLoggingEnabled()) {
-      VLOG(1) << "[AUTOFILL MIGRATION]"
-              << "Found in sync db but with a different guid: "
-              << UTF16ToUTF8(profile.GetFieldText(AutoFillType(NAME_FIRST)))
-              << UTF16ToUTF8(profile.GetFieldText(AutoFillType(NAME_LAST)))
-              << "New guid " << autofill_specifics.guid()
-              << " so associating";
-    }
+    VLOG(1) << "[AUTOFILL MIGRATION]"
+            << "Found in sync db but with a different guid: "
+            << UTF16ToUTF8(profile.GetFieldText(AutoFillType(NAME_FIRST)))
+            << UTF16ToUTF8(profile.GetFieldText(AutoFillType(NAME_LAST)))
+            << "New guid " << autofill_specifics.guid() << " sync node id "
+            << sync_node_id << " so associating. Profile to be deleted "
+            << profile.guid();
   } else {
     sync_api::WriteNode node(trans);
     if (!node.InitUniqueByCreation(
@@ -314,16 +322,17 @@ bool AutofillProfileModelAssociator::MakeNewAutofillProfileSyncNodeIfNeeded(
       return false;
     }
     node.SetTitle(UTF8ToWide(profile.guid()));
-    if (MigrationLoggingEnabled()) {
-      VLOG(1) << "[AUTOFILL MIGRATION]"
-              << "NOT Found in sync db  "
-              << UTF16ToUTF8(profile.GetFieldText(AutoFillType(NAME_FIRST)))
-              << UTF16ToUTF8(profile.GetFieldText(AutoFillType(NAME_LAST)))
-              << profile.guid()
-              << " so creating a new sync node.";
-    }
+    VLOG(1) << "[AUTOFILL MIGRATION]"
+            << "NOT Found in sync db  "
+            << UTF16ToUTF8(profile.GetFieldText(AutoFillType(NAME_FIRST)))
+            << UTF16ToUTF8(profile.GetFieldText(AutoFillType(NAME_LAST)))
+            << profile.guid()
+            << " so creating a new sync node. Sync node id "
+            << node.GetId();
     AutofillProfileChangeProcessor::WriteAutofillProfile(profile, &node);
     current_profiles->insert(profile.guid());
+    std::string guid = profile.guid();
+    Associate(&guid, node.GetId());
     number_of_profiles_created_++;
 
   }
@@ -335,11 +344,9 @@ bool AutofillProfileModelAssociator::TraverseAndAssociateAllSyncNodes(
     const sync_api::ReadNode& autofill_root,
     DataBundle* bundle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
+  VLOG(1) << "[AUTOFILL MIGRATION] "
+          << " Iterating over sync nodes of autofill profile root node";
 
-  if (MigrationLoggingEnabled()) {
-    VLOG(1) << "[AUTOFILL MIGRATION] "
-            << " Iterating over sync nodes of autofill profile root node";
-  }
   int64 sync_child_id = autofill_root.GetFirstChildId();
   while (sync_child_id != sync_api::kInvalidId) {
     ReadNode sync_child(write_trans);
@@ -363,14 +370,15 @@ void AutofillProfileModelAssociator::AddNativeProfileIfNeeded(
     const sync_api::ReadNode& node) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
 
-  if (MigrationLoggingEnabled()) {
-    VLOG(1) << "[AUTOFILL MIGRATION] "
-            << "Trying to lookup "
-            << profile.name_first()
-            << " "
-            << profile.name_last()
-            << " in the web db";
-  }
+  VLOG(1) << "[AUTOFILL MIGRATION] "
+          << "Trying to lookup "
+          << profile.name_first()
+          << " "
+          << profile.name_last()
+          << "sync node id " << node.GetId()
+          << " Guid " << profile.guid()
+          << " in the web db";
+
   if (bundle->current_profiles.find(profile.guid()) ==
       bundle->current_profiles.end()) {
     std::string guid(profile.guid());
@@ -378,15 +386,11 @@ void AutofillProfileModelAssociator::AddNativeProfileIfNeeded(
     AutoFillProfile* p = new AutoFillProfile(profile.guid());
     OverwriteProfileWithServerData(p, profile);
     bundle->new_profiles.push_back(p);
-    if (MigrationLoggingEnabled()) {
-      VLOG(1) << "[AUTOFILL MIGRATION] "
-              << " Did not find one so creating it on web db";
-    }
+    VLOG(1) << "[AUTOFILL MIGRATION] "
+            << " Did not find one so creating it on web db";
   } else {
-    if (MigrationLoggingEnabled()) {
-      VLOG(1) << "[AUTOFILL MIGRATION] "
-              << " Found it on web db. Moving on ";
-    }
+    VLOG(1) << "[AUTOFILL MIGRATION] "
+            << " Found it on web db. Moving on ";
   }
 }
 
@@ -469,10 +473,5 @@ bool AutofillProfileModelAssociator::IsAbortPending() {
   return abort_association_pending_;
 }
 
-bool AutofillProfileModelAssociator::MigrationLoggingEnabled() {
-  // TODO(lipalani) enable logging via a command line flag.
-  return false;
-}
-
 }  // namespace browser_sync
 
diff --git a/chrome/browser/sync/glue/autofill_profile_model_associator.h b/chrome/browser/sync/glue/autofill_profile_model_associator.h
index f636b63..e2abdaf 100644
--- a/chrome/browser/sync/glue/autofill_profile_model_associator.h
+++ b/chrome/browser/sync/glue/autofill_profile_model_associator.h
@@ -161,9 +161,8 @@ class AutofillProfileModelAssociator
 
   int64 FindSyncNodeWithProfile(sync_api::WriteTransaction* trans,
       const sync_api::BaseNode& autofill_root,
-      const AutoFillProfile& profile);
-
-  bool MigrationLoggingEnabled();
+      const AutoFillProfile& profile,
+      std::set<std::string>* current_profiles);
 
   ProfileSyncService* sync_service_;
   WebDatabase* web_database_;