Fix MultipleClientPasswordsSyncTest.Sanity

This test had some issues with accidentally nested RunLoops.  The
attached bug includes a small essay that thoroughly describes the issue.

The solution implemented here is to prevent nesting by not allowing
multiple calls to IsExitConditionRequired() to occur on the stack at the
same time.  It also ensures that the requests to check the exit
condition are not fully ignored; it will retry the check if a call was
attempted and rejected while the current one was in progress.

This CL also disables self-notifications for the
MultipleClientPasswordsSyncTest.  Since the test no longer depends on
AwaitQuiescence, self-notifications should be no longer necessary.

BUG=363247

Review URL: https://codereview.chromium.org/250943005

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

chrome/browser/sync/test/integration/multiple_client_passwords_sync_test.cc

@@ -21,13 +21,15 @@ class MultipleClientPasswordsSyncTest : public SyncTest {
   MultipleClientPasswordsSyncTest() : SyncTest(MULTIPLE_CLIENT) {}
   virtual ~MultipleClientPasswordsSyncTest() {}
 
+  virtual bool TestUsesSelfNotifications() OVERRIDE {
+    return false;
+  }
+
  private:
   DISALLOW_COPY_AND_ASSIGN(MultipleClientPasswordsSyncTest);
 };
 
-// This test was flaky on the Mac buildbot and also appears to be flaky
-// in local Linux (debug) builds. See crbug.com/363247.
-IN_PROC_BROWSER_TEST_F(MultipleClientPasswordsSyncTest, DISABLED_Sanity) {
+IN_PROC_BROWSER_TEST_F(MultipleClientPasswordsSyncTest, Sanity) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
   for (int i = 0; i < num_clients(); ++i) {

chrome/browser/sync/test/integration/passwords_helper.cc

@@ -207,16 +207,50 @@ class SamePasswordFormsChecker : public MultiClientStatusChangeChecker {
 
   virtual bool IsExitConditionSatisfied() OVERRIDE;
   virtual std::string GetDebugMessage() const OVERRIDE;
+
+ private:
+  bool in_progress_;
+  bool needs_recheck_;
 };
 
 SamePasswordFormsChecker::SamePasswordFormsChecker()
     : MultiClientStatusChangeChecker(
-        sync_datatype_helper::test()->GetSyncServices()) {}
+        sync_datatype_helper::test()->GetSyncServices()),
+      in_progress_(false),
+      needs_recheck_(false) {}
 
 SamePasswordFormsChecker::~SamePasswordFormsChecker() {}
 
+// This method needs protection against re-entrancy.
+//
+// This function indirectly calls GetLogins(), which starts a RunLoop on the UI
+// thread.  This can be a problem, since the next task to execute could very
+// well contain a ProfileSyncService::OnStateChanged() event, which would
+// trigger another call to this here function, and start another layer of
+// nested RunLoops.  That makes the StatusChangeChecker's Quit() method
+// ineffective.
+//
+// The work-around is to not allow re-entrancy.  But we can't just drop
+// IsExitConditionSatisifed() calls if one is already in progress.  Instead, we
+// set a flag to ask the current execution of IsExitConditionSatisfied() to be
+// re-run.  This ensures that the return value is always based on the most
+// up-to-date state.
 bool SamePasswordFormsChecker::IsExitConditionSatisfied() {
-  return AllProfilesContainSamePasswordForms();
+  if (in_progress_) {
+    LOG(WARNING) << "Setting flag and returning early to prevent nesting.";
+    needs_recheck_ = true;
+    return false;
+  }
+
+  // Keep retrying until we get a good reading.
+  bool result = false;
+  in_progress_ = true;
+  do {
+    needs_recheck_ = false;
+    result = AllProfilesContainSamePasswordForms();
+  } while (needs_recheck_);
+  in_progress_ = false;
+  return result;
 }
 
 std::string SamePasswordFormsChecker::GetDebugMessage() const {