Fix flakiness in MetricsServiceTest.CrashRenderers

chrome/browser/metrics/metrics_service_browsertest.cc

Index: chrome/browser/metrics/metrics_service_browsertest.cc
diff --git a/chrome/browser/metrics/metrics_service_browsertest.cc b/chrome/browser/metrics/metrics_service_browsertest.cc
index aee98aa62f370034e05b44b4377035a8665ccef4..211bac28a2d11ca222522c9ceebd97b18d8339e0 100644
--- a/chrome/browser/metrics/metrics_service_browsertest.cc
+++ b/chrome/browser/metrics/metrics_service_browsertest.cc
@@ -74,8 +74,7 @@ IN_PROC_BROWSER_TEST_F(MetricsServiceTest, CloseRenderersNormally) {
   // exits... it's not clear to me how to test that.
 }
 
-// Flaky: http://crbug.com/18738
-IN_PROC_BROWSER_TEST_F(MetricsServiceTest, FLAKY_CrashRenderers) {
+IN_PROC_BROWSER_TEST_F(MetricsServiceTest, CrashRenderers) {
   OpenTabs();
 
   // Kill the process for one of the tabs.
@@ -84,6 +83,9 @@ IN_PROC_BROWSER_TEST_F(MetricsServiceTest, FLAKY_CrashRenderers) {
       content::NotificationService::AllSources());
   ui_test_utils::NavigateToURL(browser(), GURL(chrome::kChromeUICrashURL));
   observer.Wait();
+  // The MetricsService listens for the same notification, so process any
+  // pending tasks in the message loop to avoid race conditions.
+  ui_test_utils::RunAllPendingInMessageLoop();

[jar (doing other things), 2012/04/07 02:46:48]

This pattern always scares me... as it is most commonly used to run a nested
message loop :-(.... which is evil.  In this case, I *think* you are not nested,
so I'm less scared... but..

In this case, you've indicated there is probably a race for two threads to post
into the message loop, and one caused the message loop to terminate, presumably
before the other one got its (notification) message posted.

Sadly, this RunAllPendingInMessageLoop() *might* execute before the delayed
notification message arrives, if your race scenario is correct.  <sigh>

How about looping, calling RunAllPendingInMessageLoop() again and again, until
we get the expected kStabilitityLaunchCount (if that is the one we are expecting
to be slow to grow?  If we printed out each time with cycled, then we'd only
fail if we never got the notification... and the we'd fail in a specific test,
with a LOT of output (meaning it would make it to the log ;-) ).

WDYT?

Am I missing something??

[Ilya Sherman, 2012/04/10 01:00:25]

I think you're absolutely right.  Added the loop.  I don't think adding extra
logging is very helpful though... if the crash count is never incremented, the
test will eventually time out, and we'll know that this hasn't fully fixed the
race.  I'm not sure what we'd gain from knowing how many times we looped before
timing out.

[jar (doing other things), 2012/04/10 01:32:22]

The reason I was after outputting something is that IF we get killed by the
infrastructure for "taking too long" we might not get to tell anyone that it was
*this* test that hung.  So long as you're sure we can tell it was *this* test
that fails (since we're designing it to fail via a multi-minute infrastructure
kill), I'm fine with it as is.  I just hate when I get some funky "browser test
failed and timed out" but I don't know what caused it :-(.

On 2012/04/10 01:00:25, Ilya Sherman wrote:

[Ilya Sherman, 2012/04/10 01:33:53]

Yes, the test logs definitely clearly indicate which test it was that timed out.
 That information doesn't always successfully make it into the failure summary,
but if you look at the logs, it's quite clear :)

 
   // Verify that the expected stability metrics were recorded.
   const PrefService* prefs = g_browser_process->local_state();