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

base/message_loop.h

Index: base/message_loop.h
diff --git a/base/message_loop.h b/base/message_loop.h
index d78b58addc2019ad46b61e79adf8c918f432a9dd..c9baa4f70e9f0810053eeb2e0d60bc5bde71b4e1 100644
--- a/base/message_loop.h
+++ b/base/message_loop.h
@@ -217,30 +217,111 @@ class BASE_EXPORT MessageLoop : public base::MessagePump::Delegate {
         this, from_here, object);
   }
 
+ protected:
+  class RunState;
+
+  // RunIDObject is used by Run and QuitNowWithID to quit a particular nested
+  // call to MessageLoop::Run. Call GetRunID to get a RunIDObject for use with
+  // one pair of Run/Quit calls. You can not reuse a RunIDObject.
+  class RunIDObject : public base::RefCounted<RunIDObject> {
+   private:
+    friend class base::RefCounted<RunIDObject>;
+    friend class MessageLoop;
+
+    RunIDObject() : run_state_(NULL), used_(false), quit_now_received_(false) {}
+    ~RunIDObject() {}
+
+    RunState* run_state_;
+    bool used_;
+
+    // Used to record that QuitNowWithID() was called at this run_depth_.
+    bool quit_now_received_;
+
+    DISALLOW_COPY_AND_ASSIGN(RunIDObject);
+  };
+
+ public:
+
+  typedef scoped_refptr<RunIDObject> RunID;
+
   // Run the message loop.
   void Run();
 
+  // Run the message loop with the given |run_id|. |run_id| must be retrieved by
+  // an earlier call to GetRunID.
+  void RunWithID(const RunID& run_id);

[darin (slow to review), 2012/06/19 17:55:51]

[Sorry, detour incoming!!]

I think it would be helpful to abstract this API a bit more.

What we really have are two distinct things:

  1-  A queue of messages / tasks to process.
  2-  A function that loops over the queue of messages / tasks and runs them.

Perhaps part of the problem stems from the fact that we are trying to use
a single class to represent both things.

Imagine if we could write code like this:

  RunLoop run_loop;
  run_loop.Start();
  ...
  WeakPtr<RunLoop> weak_run_loop(run_loop.AsWeakPtr());
  ...
  if (weak_run_loop)
    weak_run_loop->Stop();

The RunLoop constructor would use MessageLoop::current() to locate
the queue of messages / tasks for the current thread.

If we switched to this model, then the approach for running a nested
loop would look like this:

  void StopRunLoop(const base::WeakPtr<RunLoop>& run_loop) {
    run_loop->Stop();
  }

  void RunNestedLoop() {
    RunLoop nested_run_loop;
    MessageLoop::current()->PostDelayedTask(
        FROM_HERE,
        base::Bind(&StopNestedLoop, nested_run_loop.AsWeakPtr()));
    nested_run_loop.Start();
  }

  void main() {
    RunLoop main_run_loop;
    MessageLoop::current()->PostTask(
        FROM_HERE,
        base::Bind(&RunNestedLoop));
    main_run_loop.Start();
  }

In other words, the RunLoop embodies the concept of MessageLoop::RunID.
I think this creates a much simpler programming model for consumers.

[jbates, 2012/06/22 02:12:51]

RunLoop done with some slight semantic changes.

I started with RunLoop::Start, but kept getting tripped up by a blocking "Start"
method. Start sounds like it kicks off some asynchronous thing and then returns,
while Run sounds more like it blocks to me. Also, RunUntilIdle sounds better
than StartUntilIdle :)

+
   // Process all pending tasks, windows messages, etc., but don't wait/sleep.
   // Return as soon as all items that can be run are taken care of.
   void RunAllPending();
 
-  // Signals the Run method to return after it is done processing all pending
-  // messages.  This method may only be called on the same thread that called
-  // Run, and Run must still be on the call stack.
+  // TODO(jbates) remove this. crbug.com/131220. See QuitWhenIdle().
+  void Quit() { QuitWhenIdle(); }
+
+  // Signals the Run method to return when it becomes idle. It will continue to
+  // process pending messages and future messages as long as they are enqueued.
+  // Warning: if the MessageLoop remains busy, it may never quit. Only use this
+  // Quit method when looping procedures (such as web pages) have been shut
+  // down.
   //
-  // Use QuitClosure if you need to Quit another thread's MessageLoop, but note
-  // that doing so is fairly dangerous if the target thread makes nested calls
-  // to MessageLoop::Run.  The problem being that you won't know which nested
-  // run loop you are quitting, so be careful!
-  void Quit();
+  // This method may only be called on the same thread that called Run, and Run
+  // must still be on the call stack.
+  //
+  // Use Quit*Closure variants if you need to Quit another thread's MessageLoop,
+  // but note that doing so is fairly dangerous if the target thread makes
+  // nested calls to MessageLoop::Run.  The problem being that you won't know
+  // which nested run loop you are quitting, so be careful!
+  void QuitWhenIdle();
 
   // This method is a variant of Quit, that does not wait for pending messages
   // to be processed before returning from Run.
   void QuitNow();
 
-  // Invokes Quit on the current MessageLoop when run. Useful to schedule an
-  // arbitrary MessageLoop to Quit.
-  static base::Closure QuitClosure();
+  // This method is a variant of QuitNow that applies to a specific nested call
+  // to MessageLoop::Run(). Although nesting is strongly discouraged, it can be
+  // handy for tests. Use QuitNowWithID to make nesting and quit behavior more
+  // predictable. |run_id| is returned by an earlier call to
+  // GetRunID(). QuitWithID has the following behavior:
+  // - If the corresponding MessageLoop::RunWithID call has already completed,
+  //   nothing happens.
+  // - If the corresponding MessageLoop::RunWithID has not been called yet, it
+  //   will become a no-op when it is eventually called.
+  // - If there are one or more nested calls to MessageLoop::Run inside the
+  //   target RunWithID call, the QuitNow is deferred until all nested Run calls
+  //   have completed.
+  // WARNING: You must NEVER assume that a call to QuitNowWithID will terminate
+  // the targetted message loop. If a nested message loop continues running, the
+  // target may NEVER terminate. It is very easy to livelock (run forever) in
+  // such a case.
+  void QuitNowWithID(const RunID& run_id);
+
+  // TODO(jbates) remove this. crbug.com/131220. See QuitWhenIdleClosure().
+  static base::Closure QuitClosure() { return QuitWhenIdleClosure(); }
+
+  // Construct a Closure that will call QuitWhenIdle(). Useful to schedule an
+  // arbitrary MessageLoop to QuitWhenIdle.
+  static base::Closure QuitWhenIdleClosure();
+
+  // Construct a Closure that will call QuitNow(). Useful to schedule an
+  // arbitrary MessageLoop to QuitNow.
+  static base::Closure QuitNowClosure();
+
+  // Construct a Closure that will call QuitNowWithID(run_id). Useful to
+  // schedule an arbitrary MessageLoop to QuitNowWithID.
+  static base::Closure QuitNowWithIDClosure(const RunID& run_id);
+
+  // Get a run id for use with MessageLoop::Run and MessageLoop::QuitNowWithID.
+  // Use this value to quit a specific nested call to MessageLoop::Run. The
+  // value returned by GetRunID is only for use on the same MessageLoop
+  // instance. For example:
+  //
+  //  // Record GetRunID before running the MessageLoop:
+  //  run_id_ = MessageLoop::current()->GetRunID();
+  //  MessageLoop::current()->RunWithID(run_id_);
+  //
+  //  // Then, during a task or callback during the above call to Run:
+  //  MessageLoop::current()->QuitNowWithID(run_id_);
+  RunID GetRunID();
 
   // Returns the type passed to the constructor.
   Type type() const { return type_; }
@@ -362,17 +443,25 @@ class BASE_EXPORT MessageLoop : public base::MessagePump::Delegate {
 
   //----------------------------------------------------------------------------
  protected:
-  struct RunState {
+  class BASE_EXPORT RunState {
+   public:
+    RunState();
+
+    // Used to distinguish this RunState from other nested RunStates.
+    RunID run_state_id_;
+
     // Used to count how many Run() invocations are on the stack.
-    int run_depth;
+    int run_depth_;
 
     // Used to record that Quit() was called, or that we should quit the pump
     // once it becomes idle.
-    bool quit_received;
+    bool quit_received_;
 
 #if !defined(OS_MACOSX) && !defined(OS_ANDROID)
-    Dispatcher* dispatcher;
+    Dispatcher* dispatcher_;
 #endif
+   protected:
+    ~RunState();
   };
 
 #if defined(OS_ANDROID)
@@ -382,7 +471,7 @@ class BASE_EXPORT MessageLoop : public base::MessagePump::Delegate {
 #endif
   class BASE_EXPORT AutoRunState : RunState {
    public:
-    explicit AutoRunState(MessageLoop* loop);
+    AutoRunState(MessageLoop* loop, const RunID& run_id);
     ~AutoRunState();
    private:
     MessageLoop* loop_;
@@ -572,6 +661,7 @@ class BASE_EXPORT MessageLoopForUI : public MessageLoop {
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
   void RunWithDispatcher(Dispatcher* dispatcher);
+  void RunWithDispatcher(Dispatcher* dispatcher, const RunID& run_id);
   void RunAllPendingWithDispatcher(Dispatcher* dispatcher);
 
  protected: