Fix MultipleClientPasswordsSyncTest.Sanity

chrome/browser/sync/test/integration/passwords_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/passwords_helper.h"
 
 #include "base/compiler_specific.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/waitable_event.h"
@@ skipping 36 matching lines @@
       const std::vector<PasswordForm*>& result) OVERRIDE {
     result_->clear();
     for (std::vector<PasswordForm*>::const_iterator it = result.begin();
          it != result.end();
          ++it) {
       result_->push_back(**it);
       delete *it;
     }
 
     // Quit the message loop to wake up passwords_helper::GetLogins.
     base::MessageLoopForUI::current()->Quit();

[Nicolas Zea, 2014/04/28 22:14:47]

I wonder if we should instead not be using this, and rather have our own
RunLoops and be quitting specific run loops?

[rlarocque, 2014/04/28 23:18:58]

In theory, I think switching to RunLoops here and in the
MultiClientStatusChangeChecker should fix the issue without the need for the
changes in this CL.  In practice, though, it doesn't seem to be working.

I'm also a bit reluctant to make that change, since there's a chance of subtle
side effects.  RunLoops don't support QuitWhenIdle, which is used by the
StatusChangeChecker.  That could have strange effects across all existing
multi-client tests.

I think we should start with the approach in this CL, but also try to transition
to RunLoops in future changes.  I'll open a bug for that.

   }
 
  private:
   std::vector<PasswordForm>* result_;
 
   DISALLOW_COPY_AND_ASSIGN(PasswordStoreConsumerHelper);
 };
 
 }  // namespace
 
@@ skipping 132 matching lines @@
 
 // Helper class used in the implementation of
 // AwaitAllProfilesContainSamePasswordForms.
 class SamePasswordFormsChecker : public MultiClientStatusChangeChecker {
  public:
   SamePasswordFormsChecker();
   virtual ~SamePasswordFormsChecker();
 
   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 {
   return "Waiting for matching passwords";
 }
 
 }  //  namespace
 
 bool AwaitAllProfilesContainSamePasswordForms() {
   SamePasswordFormsChecker checker;
@@ skipping 19 matching lines @@
   form.origin = GURL(base::StringPrintf(kIndexedFakeOrigin, index));
   form.username_value =
       base::ASCIIToUTF16(base::StringPrintf("username%d", index));
   form.password_value =
       base::ASCIIToUTF16(base::StringPrintf("password%d", index));
   form.date_created = base::Time::Now();
   return form;
 }
 
 }  // namespace passwords_helper