Fix MultipleClientPasswordsSyncTest.Sanity

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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266770

[rlarocque, 2014-04-26 00:26:06]

This should fix the test.  I don't like it as a long-term solution, but it works
well enough for now.

I've tested it locally with --gtest_repeat=20 and it worked just fine, whereas
it used to crash ~80% of the time previously.

Please review.

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

https://codereview.chromium.org/250943005/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/passwords_helper.cc (right):

https://codereview.chromium.org/250943005/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/passwords_helper.cc:57:
base::MessageLoopForUI::current()->Quit();
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:57]

https://codereview.chromium.org/250943005/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/passwords_helper.cc (right):

https://codereview.chromium.org/250943005/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/passwords_helper.cc:57:
base::MessageLoopForUI::current()->Quit();
On 2014/04/28 22:14:47, Nicolas Zea wrote:
> I wonder if we should instead not be using this, and rather have our own
> RunLoops and be quitting specific run loops?

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.

[Nicolas Zea, 2014-04-28 23:26:38]

FYI, Run loops have RunUntilIdle, which I think results in the same behavior as
QuitWhenIdle.

That said, if it looks like switching to RunLoops is a significant refactor
worthy of a separate CL, this LGTM

[rlarocque, 2014-04-28 23:35:40]

On 2014/04/28 23:26:38, Nicolas Zea wrote:
> FYI, Run loops have RunUntilIdle, which I think results in the same behavior
as
> QuitWhenIdle.

As far as I can tell, they're not quite the same.  Our test harness calls Run(),
then quits it with QuitWhenIdle().  I don't think it would be correct to replace
the call to Run() with RunUntilIdle(), and I don't know what effect calling
RunUntilIdle() after Run() would have.

[rlarocque, 2014-04-28 23:36:01]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-04-28 23:38:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/250943005/1

[commit-bot: I haz the power, 2014-04-29 00:19:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 00:19:48]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[rlarocque, 2014-04-29 00:22:17]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-04-29 00:22:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/250943005/1

[commit-bot: I haz the power, 2014-04-29 07:38:21]

Change committed as 266770