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

base/message_loop.h

 // 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.
 
 #ifndef BASE_MESSAGE_LOOP_H_
 #define BASE_MESSAGE_LOOP_H_
 #pragma once
 
 #include <queue>
 #include <string>
@@ skipping 206 matching lines @@
         this, from_here, object);
   }
 
   // Run the message loop.
   void Run();
 
   // 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(). |run_id| is returned by an earlier call to
+  // next_run_id(). If the corresponding MessageLoop::Run call has already
+  // completed, nothing happens. If there are one or more nested calls to
+  // MessageLoop::Run inside the target Run call, the QuitNow is deferred until
+  // all nested Run calls have completed.
+  void QuitNowByID(int run_id);

[jar (doing other things), 2012/06/14 04:50:29]

This is an interesting extension.
My general feeling is that I want to always massively discourage nested message
loops, and although this helps some scenarios, it is still murderously hard to
use these without messing up (use only with fully re-entrant tasks!!!).

From an API perspective, I'd rather not expose next_run_id(), which gives a
caller the ability to mess-up, and get the wrong id (some how).  I'd rather have
a 

Run(int* generated_run_id)  // arg is output value only.

This would put all the onus of manipulating these ids as a private affair in the
message loop, but allow a thread that called Run() to get a copy of the id
pushed to an accessible place, for use in a Quit(int
previously_generated_run_id).

[jbates, 2012/06/15 02:20:38]

Done as discussed over chat.

+
+  // 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 QuitNowByID(run_id). Useful to schedule
+  // an arbitrary MessageLoop to QuitNowByID.
+  static base::Closure QuitNowByIDClosure(int run_id);
+
+  // Get the run id for the next call to MessageLoop::Run. Use this
+  // value to quit a specific nested call to MessageLoop::Run. For example:
+  //
+  //  // Record next_run_id before running the MessageLoop:
+  //  run_id_ = MessageLoop::current()->next_run_id();
+  //  MessageLoop::current()->Run();

[jar (doing other things), 2012/06/14 04:50:29]

With the proposed API (preventing others from manipulating run_ids).  This could
would read:
int run_id = 0;
MessageLoop::current()->Run(&run_id);

IMO, if you failed to pass in a run_id pointer, or passed in NULL, then *no*
run_id would even be generated (and hence no need for a global lock, etc.).

+  //
+  //  // Then, during a task or callback during the above call to Run:
+  //  MessageLoop::current()->QuitNowByID(run_id_);
+  int next_run_id() const { return current_run_state_id_ + 1; }
 
   // Returns the type passed to the constructor.
   Type type() const { return type_; }
 
   // Optional call to connect the thread name with this loop.
   void set_thread_name(const std::string& thread_name) {
     DCHECK(thread_name_.empty()) << "Should not rename this thread!";
     thread_name_ = thread_name;
   }
   const std::string& thread_name() const { return thread_name_; }
@@ skipping 105 matching lines @@
 
   // Can only be called from the thread that owns the MessageLoop.
   bool is_running() const;
 
   //----------------------------------------------------------------------------
  protected:
   struct RunState {
     // Used to count how many Run() invocations are on the stack.
     int run_depth;
 
+    // Used to distinguish this RunState from other nested RunStates.
+    int run_state_id;
+
     // Used to record that Quit() was called, or that we should quit the pump
     // once it becomes idle.
     bool quit_received;
 
+    // Used to record that QuitNow() was called at this run_depth.
+    bool quit_now_received;
+
 #if !defined(OS_MACOSX) && !defined(OS_ANDROID)
     Dispatcher* dispatcher;
 #endif
+    RunState* previous_state;
   };
 
 #if defined(OS_ANDROID)
   // Android Java process manages the UI thread message loop. So its
   // MessagePumpForUI needs to keep the RunState.
  public:
 #endif
   class BASE_EXPORT AutoRunState : RunState {
    public:
     explicit AutoRunState(MessageLoop* loop);
     ~AutoRunState();
    private:
     MessageLoop* loop_;
-    RunState* previous_state_;
   };
 #if defined(OS_ANDROID)
  protected:
 #endif
 
 #if defined(OS_WIN)
   base::MessagePumpWin* pump_win() {
     return static_cast<base::MessagePumpWin*>(pump_.get());
   }
 #elif defined(OS_POSIX)
@@ skipping 115 matching lines @@
 
   // The next sequence number to use for delayed tasks.
   int next_sequence_num_;
 
   ObserverList<TaskObserver> task_observers_;
 
   // The message loop proxy associated with this message loop, if one exists.
   scoped_refptr<base::MessageLoopProxy> message_loop_proxy_;
   scoped_ptr<base::ThreadTaskRunnerHandle> thread_task_runner_handle_;
 
+  // Unique identifier (within the scope of this MessageLoop) for AutoRunState
+  // instances (AutoRunState is associated with calls to Run).

[jar (doing other things), 2012/06/14 04:50:29]

It might be nice to expand on this statement.  Is the ID globally unique for the
process? Is it generated automagically for the message loop (thread?), or is it
the responsibility of the caller of Run() to select one?

[jbates, 2012/06/15 02:20:38]

Removed.

+  int current_run_state_id_;
+
  private:
   template <class T, class R> friend class base::subtle::DeleteHelperInternal;
   template <class T, class R> friend class base::subtle::ReleaseHelperInternal;
 
   void DeleteSoonInternal(const tracked_objects::Location& from_here,
                           void(*deleter)(const void*),
                           const void* object);
   void ReleaseSoonInternal(const tracked_objects::Location& from_here,
                            void(*releaser)(const void*),
                            const void* object);
 

[jar (doing other things), 2012/06/14 04:50:29]

nit: extra blank line probably wasn't intentional.

 
   DISALLOW_COPY_AND_ASSIGN(MessageLoop);
 };
 
 //-----------------------------------------------------------------------------
 // MessageLoopForUI extends MessageLoop with methods that are particular to a
 // MessageLoop instantiated with TYPE_UI.
 //
 // This class is typically used like so:
 //   MessageLoopForUI::current()->...call some method...
@@ skipping 113 matching lines @@
 #endif  // defined(OS_POSIX)
 };
 
 // Do not add any member variables to MessageLoopForIO!  This is important b/c
 // MessageLoopForIO is often allocated via MessageLoop(TYPE_IO).  Any extra
 // data that you need should be stored on the MessageLoop's pump_ instance.
 COMPILE_ASSERT(sizeof(MessageLoop) == sizeof(MessageLoopForIO),
                MessageLoopForIO_should_not_have_extra_member_variables);
 
 #endif  // BASE_MESSAGE_LOOP_H_