[Sync] Fix flaky TwoClientAutofillSyncTest.ConflictingFields.

chrome/browser/sync/test/integration/autofill_helper.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/sync/test/integration/autofill_helper.h"
 
 #include <stddef.h>
 
 #include <map>
+#include <utility>
 
 #include "base/guid.h"
 #include "base/run_loop.h"
 #include "chrome/browser/autofill/personal_data_manager_factory.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/profile_sync_test_util.h"
 #include "chrome/browser/sync/test/integration/sync_datatype_helper.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/web_data_service_factory.h"
@@ skipping 31 matching lines @@
   event->Signal();
 }
 
 class MockWebDataServiceObserver
     : public AutofillWebDataServiceObserverOnDBThread {
  public:
   MOCK_METHOD1(AutofillEntriesChanged,
                void(const AutofillChangeList& changes));
 };
 
-class MockPersonalDataManagerObserver : public PersonalDataManagerObserver {
- public:
-  MOCK_METHOD0(OnPersonalDataChanged, void());
-};
-
 void RunOnDBThreadAndSignal(base::Closure task,
                             base::WaitableEvent* done_event) {
   if (!task.is_null()) {
     task.Run();
   }
   done_event->Signal();
 }
 
 void RunOnDBThreadAndBlock(base::Closure task) {
   WaitableEvent done_event(base::WaitableEvent::ResetPolicy::AUTOMATIC,
@@ skipping 49 matching lines @@
 // the sync. This function blocks until after this scheduled sync is complete by
 // scheduling additional empty task on DB Thread. Call after AddKeys/RemoveKey.
 void BlockForPendingDBThreadTasks() {
   // The order of the notifications is undefined, so sync change sometimes is
   // posted after the notification for observer_helper. Post new task to db
   // thread that guaranteed to be after sync and would be blocking until
   // completion.
   RunOnDBThreadAndBlock(base::Closure());
 }
 
+bool ProfilesMatchImpl(
+    int profile_a,
+    const std::vector<AutofillProfile*>& autofill_profiles_a,
+    int profile_b,
+    const std::vector<AutofillProfile*>& autofill_profiles_b) {
+  std::map<std::string, AutofillProfile> autofill_profiles_a_map;
+  for (AutofillProfile* p : autofill_profiles_a) {
+    autofill_profiles_a_map[p->guid()] = *p;
+  }
+
+  // This seems to be a transient state that will eventually be rectified by
+  // model type logic. We don't need to check b for duplicates directly because
+  // after the first is erased from |autofill_profiles_a_map| the second will
+  // not be found.
+  if (autofill_profiles_a.size() != autofill_profiles_a_map.size()) {
+    DVLOG(1) << "Profile " << profile_a << " contains duplicate GUID(s).";
+    return false;
+  }
+
+  for (AutofillProfile* p : autofill_profiles_b) {
+    if (!autofill_profiles_a_map.count(p->guid())) {
+      DVLOG(1) << "GUID " << p->guid() << " not found in profile " << profile_b
+               << ".";
+      return false;
+    }
+    AutofillProfile* expected_profile = &autofill_profiles_a_map[p->guid()];
+    expected_profile->set_guid(p->guid());
+    if (*expected_profile != *p) {
+      DVLOG(1) << "Mismatch in profile with GUID " << p->guid() << ".";
+      return false;
+    }
+    autofill_profiles_a_map.erase(p->guid());
+  }
+
+  if (autofill_profiles_a_map.size()) {
+    DVLOG(1) << "Entries present in Profile " << profile_a << " but not in "
+             << profile_b << ".";
+    return false;
+  }
+  return true;
+}
+
 }  // namespace
 
 namespace autofill_helper {
 
 AutofillProfile CreateAutofillProfile(ProfileType type) {
   AutofillProfile profile;
   switch (type) {
     case PROFILE_MARION:
       autofill::test::SetProfileInfoWithGuid(&profile,
           "C837507A-6C3B-4872-AC14-5113F157D668",
@@ skipping 42 matching lines @@
       test()->GetProfile(index), ServiceAccessType::EXPLICIT_ACCESS);
 }
 
 PersonalDataManager* GetPersonalDataManager(int index) {
   return autofill::PersonalDataManagerFactory::GetForProfile(
       test()->GetProfile(index));
 }
 
 void AddKeys(int profile, const std::set<AutofillKey>& keys) {
   std::vector<FormFieldData> form_fields;
-  for (std::set<AutofillKey>::const_iterator i = keys.begin();
-       i != keys.end();
-       ++i) {
+  for (const AutofillKey& key : keys) {
     FormFieldData field;
-    field.name = i->name();
-    field.value = i->value();
+    field.name = key.name();
+    field.value = key.value();
     form_fields.push_back(field);
   }
 
   WaitableEvent done_event(base::WaitableEvent::ResetPolicy::AUTOMATIC,
                            base::WaitableEvent::InitialState::NOT_SIGNALED);
   MockWebDataServiceObserver mock_observer;
   EXPECT_CALL(mock_observer, AutofillEntriesChanged(_))
       .WillOnce(SignalEvent(&done_event));
 
   scoped_refptr<AutofillWebDataService> wds = GetWebDataService(profile);
@@ skipping 13 matching lines @@
   RunOnDBThreadAndBlock(Bind(remove_observer_func, wds, &mock_observer));
 }
 
 void RemoveKey(int profile, const AutofillKey& key) {
   RemoveKeyDontBlockForSync(profile, key);
   BlockForPendingDBThreadTasks();
 }
 
 void RemoveKeys(int profile) {
   std::set<AutofillEntry> keys = GetAllKeys(profile);
-  for (std::set<AutofillEntry>::const_iterator it = keys.begin();
-       it != keys.end(); ++it) {
-    RemoveKeyDontBlockForSync(profile, it->key());
+  for (const AutofillEntry& entry : keys) {
+    RemoveKeyDontBlockForSync(profile, entry.key());
   }
   BlockForPendingDBThreadTasks();
 }
 
 std::set<AutofillEntry> GetAllKeys(int profile) {
   scoped_refptr<AutofillWebDataService> wds = GetWebDataService(profile);
   std::vector<AutofillEntry> all_entries = GetAllAutofillEntries(wds.get());
-  std::set<AutofillEntry> all_keys;
-  for (std::vector<AutofillEntry>::const_iterator it = all_entries.begin();
-       it != all_entries.end(); ++it) {
-    all_keys.insert(*it);
-  }
-  return all_keys;
+  return std::set<AutofillEntry>(all_entries.begin(), all_entries.end());
 }
 
 bool KeysMatch(int profile_a, int profile_b) {
   return GetAllKeys(profile_a) == GetAllKeys(profile_b);
 }
 
 void SetProfiles(int profile, std::vector<AutofillProfile>* autofill_profiles) {
-  MockPersonalDataManagerObserver observer;
-  EXPECT_CALL(observer, OnPersonalDataChanged()).
-      WillOnce(QuitUIMessageLoop());
-  PersonalDataManager* pdm = GetPersonalDataManager(profile);
-  pdm->AddObserver(&observer);
-  pdm->SetProfiles(autofill_profiles);
-  base::RunLoop().Run();
-  pdm->RemoveObserver(&observer);
+  GetPersonalDataManager(profile)->SetProfiles(autofill_profiles);
 }
 
 void SetCreditCards(int profile, std::vector<CreditCard>* credit_cards) {
-  MockPersonalDataManagerObserver observer;
-  EXPECT_CALL(observer, OnPersonalDataChanged()).
-      WillOnce(QuitUIMessageLoop());
-  PersonalDataManager* pdm = GetPersonalDataManager(profile);
-  pdm->AddObserver(&observer);
-  pdm->SetCreditCards(credit_cards);
-  base::RunLoop().Run();
-  pdm->RemoveObserver(&observer);
+  GetPersonalDataManager(profile)->SetCreditCards(credit_cards);
 }
 
 void AddProfile(int profile, const AutofillProfile& autofill_profile) {
-  const std::vector<AutofillProfile*>& all_profiles =
-      GetAllAutoFillProfiles(profile);
   std::vector<AutofillProfile> autofill_profiles;
-  for (size_t i = 0; i < all_profiles.size(); ++i)
-    autofill_profiles.push_back(*all_profiles[i]);
+  for (AutofillProfile* profile : GetAllAutoFillProfiles(profile)) {
+    autofill_profiles.push_back(*profile);
+  }
   autofill_profiles.push_back(autofill_profile);
   autofill_helper::SetProfiles(profile, &autofill_profiles);
 }
 
 void RemoveProfile(int profile, const std::string& guid) {
-  const std::vector<AutofillProfile*>& all_profiles =
-      GetAllAutoFillProfiles(profile);
   std::vector<AutofillProfile> autofill_profiles;
-  for (size_t i = 0; i < all_profiles.size(); ++i) {
-    if (all_profiles[i]->guid() != guid)
-      autofill_profiles.push_back(*all_profiles[i]);
+  for (AutofillProfile* profile : GetAllAutoFillProfiles(profile)) {
+    if (profile->guid() != guid) {
+      autofill_profiles.push_back(*profile);
+    }
   }
   autofill_helper::SetProfiles(profile, &autofill_profiles);
 }
 
 void UpdateProfile(int profile,
                    const std::string& guid,
                    const AutofillType& type,
                    const base::string16& value) {
-  const std::vector<AutofillProfile*>& all_profiles =
-      GetAllAutoFillProfiles(profile);
   std::vector<AutofillProfile> profiles;
-  for (size_t i = 0; i < all_profiles.size(); ++i) {
-    profiles.push_back(*all_profiles[i]);
-    if (all_profiles[i]->guid() == guid)
+  for (AutofillProfile* profile : GetAllAutoFillProfiles(profile)) {
+    profiles.push_back(*profile);
+    if (profile->guid() == guid) {
       profiles.back().SetRawInfo(type.GetStorableType(), value);
+    }
   }
   autofill_helper::SetProfiles(profile, &profiles);
 }
 
 std::vector<AutofillProfile*> GetAllAutoFillProfiles(int profile) {
-  MockPersonalDataManagerObserver observer;
-  EXPECT_CALL(observer, OnPersonalDataChanged()).
-      WillOnce(QuitUIMessageLoop());
   PersonalDataManager* pdm = GetPersonalDataManager(profile);
-  pdm->AddObserver(&observer);
   pdm->Refresh();
-  base::RunLoop().Run();
-  pdm->RemoveObserver(&observer);
+
+  // PersonalDataManager::web_profiles() simply returns the current values that
+  // have been last reported to the UI thread. PersonalDataManager::Refresh()
+  // will post a task to the DB thread to read back the latest values, and we
+  // very much want the latest values. Unfortunately, the Refresh() call is
+  // asynchronous and there's no way to pass a callback that's run when our
+  // Refresh() call finishes. A PersonalDataManagerObserver won't completely fix
+  // the problem either since there could be multiple outstanding modifications
+  // scheduled, and we cannot ensure that we have the latest view. Instead post
+  // a task to the DB thread and wait for it run. This will ensure that our
+  // Refresh() was executed first, as long as the DB thread is sequenced. It is
+  // possible for another write to sneak in between our Refresh() and the task
+  // that is blocked for, causing the web_profiles() read to return even more
+  // current data, but this shouldn't cause problems. While PersonalDataManager
+  // will cancel outstanding queries, this is only instigated on the UI thread,
+  // which we are about to block, which means we are safe.
+  BlockForPendingDBThreadTasks();
+
   return pdm->web_profiles();
 }
 
 int GetProfileCount(int profile) {
   return GetAllAutoFillProfiles(profile).size();
 }
 
 int GetKeyCount(int profile) {
   return GetAllKeys(profile).size();
 }
 
-namespace {
-
-bool ProfilesMatchImpl(
-    int profile_a,
-    const std::vector<AutofillProfile*>& autofill_profiles_a,
-    int profile_b,
-    const std::vector<AutofillProfile*>& autofill_profiles_b) {
-  std::map<std::string, AutofillProfile> autofill_profiles_a_map;
-  for (size_t i = 0; i < autofill_profiles_a.size(); ++i) {
-    const AutofillProfile* p = autofill_profiles_a[i];
-    autofill_profiles_a_map[p->guid()] = *p;
-  }
-
-  for (size_t i = 0; i < autofill_profiles_b.size(); ++i) {
-    const AutofillProfile* p = autofill_profiles_b[i];
-    if (!autofill_profiles_a_map.count(p->guid())) {
-      DVLOG(1) << "GUID " << p->guid() << " not found in profile " << profile_b
-               << ".";
-      return false;
-    }
-    AutofillProfile* expected_profile = &autofill_profiles_a_map[p->guid()];
-    expected_profile->set_guid(p->guid());
-    if (*expected_profile != *p) {
-      DVLOG(1) << "Mismatch in profile with GUID " << p->guid() << ".";
-      return false;
-    }
-    autofill_profiles_a_map.erase(p->guid());
-  }
-
-  if (autofill_profiles_a_map.size()) {
-    DVLOG(1) << "Entries present in Profile " << profile_a << " but not in "
-             << profile_b << ".";
-    return false;
-  }
-  return true;
-}
-
-}  // namespace
-
 bool ProfilesMatch(int profile_a, int profile_b) {
   const std::vector<AutofillProfile*>& autofill_profiles_a =
       GetAllAutoFillProfiles(profile_a);
   const std::vector<AutofillProfile*>& autofill_profiles_b =
       GetAllAutoFillProfiles(profile_b);
   return ProfilesMatchImpl(
       profile_a, autofill_profiles_a, profile_b, autofill_profiles_b);
 }
 
-bool AllProfilesMatch() {
-  for (int i = 1; i < test()->num_clients(); ++i) {
-    if (!ProfilesMatch(0, i)) {
-      DVLOG(1) << "Profile " << i << "does not contain the same autofill "
-                                     "profiles as profile 0.";
-      return false;
-    }
-  }
-  return true;
-}
-
 }  // namespace autofill_helper
 
 AutofillKeysChecker::AutofillKeysChecker(int profile_a, int profile_b)
     : MultiClientStatusChangeChecker(
           sync_datatype_helper::test()->GetSyncServices()),
       profile_a_(profile_a),
       profile_b_(profile_b) {}
 
 bool AutofillKeysChecker::IsExitConditionSatisfied() {
   return autofill_helper::KeysMatch(profile_a_, profile_b_);
@@ skipping 10 matching lines @@
 }
 
 AutofillProfileChecker::~AutofillProfileChecker() {
   autofill_helper::GetPersonalDataManager(profile_a_)->RemoveObserver(this);
   autofill_helper::GetPersonalDataManager(profile_b_)->RemoveObserver(this);
 }
 
 bool AutofillProfileChecker::Wait() {
   autofill_helper::GetPersonalDataManager(profile_a_)->Refresh();
   autofill_helper::GetPersonalDataManager(profile_b_)->Refresh();
+  // Similar to GetAllAutoFillProfiles() we need to make sure we are not reading
+  // before any locally instigated async writes. This is run exactly one time
+  // before the first IsExitConditionSatisfied() is called.
+  BlockForPendingDBThreadTasks();
   return StatusChangeChecker::Wait();
 }
 
 bool AutofillProfileChecker::IsExitConditionSatisfied() {
   const std::vector<AutofillProfile*>& autofill_profiles_a =
       autofill_helper::GetPersonalDataManager(profile_a_)->web_profiles();
   const std::vector<AutofillProfile*>& autofill_profiles_b =
       autofill_helper::GetPersonalDataManager(profile_b_)->web_profiles();
-  return autofill_helper::ProfilesMatchImpl(profile_a_, autofill_profiles_a,
-                                            profile_b_, autofill_profiles_b);
+  return ProfilesMatchImpl(profile_a_, autofill_profiles_a, profile_b_,
+                           autofill_profiles_b);
 }
 
 std::string AutofillProfileChecker::GetDebugMessage() const {
   return "Waiting for matching autofill profiles";
 }
 
 void AutofillProfileChecker::OnPersonalDataChanged() {
   CheckExitCondition();
 }