Add base::RunLoop and update ui_test_utils to use it to reduce flakiness

content/test/test_navigation_observer.cc

Index: content/test/test_navigation_observer.cc
diff --git a/content/test/test_navigation_observer.cc b/content/test/test_navigation_observer.cc
index 89ff36fd575feb8ec98c98463596d8c7442c5a2e..ac46c429f3770eab4f0f0dba41915d46f1711037 100644
--- a/content/test/test_navigation_observer.cc
+++ b/content/test/test_navigation_observer.cc
@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/message_loop.h"
+#include "base/run_loop.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_view_host_observer.h"
@@ -14,6 +15,22 @@
 
 namespace content {
 
+namespace {
+
+// Allow some pending tasks and up to |num_deferrals| generations to complete.
+static void DeferredQuitRunLoop(
+    const base::WeakPtr<base::RunLoop>& run_loop,
+    int num_quit_deferrals) {
+  if (num_quit_deferrals <= 0) {
+    if (run_loop) run_loop->Quit();

[jar (doing other things), 2012/06/23 02:43:19]

(as commented in other file): When run_loop is null, you may as well just return
before line 24.

[jbates, 2012/06/25 20:04:57]

Done.

+  } else {
+    MessageLoop::current()->PostTask(FROM_HERE,
+        base::Bind(&DeferredQuitRunLoop, run_loop, num_quit_deferrals - 1));
+  }
+}
+
+}  // namespace
+
 // This class observes |render_view_host| and calls OnJsInjectionReady() of
 // |js_injection_ready_observer| when the time is right to inject JavaScript
 // into the page.
@@ -84,11 +101,13 @@ void TestNavigationObserver::WaitForObservation(
 }
 
 void TestNavigationObserver::Wait() {
+  base::RunLoop run_loop;
+  // Number of times to repost QuitNow task to allow pending tasks to complete:
+  const int num_quit_deferrals = 5;

[jar (doing other things), 2012/06/23 02:43:19]

Is this a hack number guess, or is this meaningful?

[jbates, 2012/06/25 20:04:57]

It's approaching being meaningful. 20 worked but may have been overkill. 2
caused an uncomfortable number of test failures. Now I'm at 5 seeing if the
trybots agree.

[jar (doing other things), 2012/06/25 21:19:43]

The larger number (20) should have no measurable impact on performance (20
minimal tasks posted and run should probably complete nicely in less than 1ms). 
The downside risk of "getting this wrong" is a flaky test, and a revisit.

IMO, you should document how you tried to pull this number form the air, mention
it can be increased if the test proves flaky, and be very conservative (choose
big... like 20?) in your selection of a constant.


On 2012/06/25 20:04:57, jbates wrote:

[jbates, 2012/06/27 02:06:37]

Docs done. Went with 10, because some perf tests might have some largish 200ms
frames, and 200ms*20 is starting to seem too long of a wait per test Wait. 10
should be enough, watching the bots...

   WaitForObservation(
-      base::Bind(&MessageLoop::Run,
-                 base::Unretained(MessageLoop::current())),
-      base::Bind(&MessageLoop::Quit,
-                 base::Unretained(MessageLoop::current())));
+      base::Bind(&base::RunLoop::Run, run_loop.AsWeakPtr()),
+      base::Bind(&DeferredQuitRunLoop, run_loop.AsWeakPtr(),
+                 num_quit_deferrals));
 }
 
 TestNavigationObserver::TestNavigationObserver(
@@ -131,8 +150,10 @@ void TestNavigationObserver::Observe(
           ++navigations_completed_ == number_of_navigations_) {
         navigation_started_ = false;
         done_ = true;
-        if (running_)
+        if (running_) {
+          running_ = false;
           done_callback_.Run();
+        }
       }
       break;
     case NOTIFICATION_RENDER_VIEW_HOST_CREATED: