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 199 matching lines @@
   // MessageLoop::Run().  If this is not the same as the thread that calls
   // PostDelayedTask(FROM_HERE, ), then T MUST inherit from
   // RefCountedThreadSafe<T>!
   template <class T>
   void ReleaseSoon(const tracked_objects::Location& from_here,
                    const T* object) {
     base::subtle::ReleaseHelperInternal<T, void>::ReleaseViaSequencedTaskRunner(
         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_;

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

Why not use the bool in the run_state?  

I later saw that you handle calling quit before a Run(), which I guess means we
need this... but  I wasn't clear about that use case.

[jbates, 2012/06/18 23:14:03]

Added this use case to the QuitNowWithID comments. I think it's helpful to make
the API easier to use and less error prone. Often, tests will kick off an
asynchronous process and then later wait for the process to complete by calling
ML::RunWithID(id). It simplifies the test code if they don't have to track
whether they have called run yet, or whether they have called quit yet. This
design allows them to quit at any time, even multiple times on the same RunID
and it will always affect a specific past/present/future call to RunWithID.

+
+    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);
+
   // 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(). 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.

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

Please add the following additional warning.  I don't think we can say this
enough :-/  :

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 deadlock (run forever) in such a case.

[jbates, 2012/06/18 23:14:03]

Done.

+  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_; }
 
   // 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 101 matching lines @@
   bool os_modal_loop() const {
     return os_modal_loop_;
   }
 #endif  // OS_WIN
 
   // Can only be called from the thread that owns the MessageLoop.
   bool is_running() const;
 
   //----------------------------------------------------------------------------
  protected:
-  struct RunState {
+  class BASE_EXPORT RunState {
+   public:
+    RunState();
+
+    // Used to distinguish this RunState from other nested RunStates.
+    RunID run_state_id;

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

nit: member names need a trailing underscore.

[jbates, 2012/06/18 23:14:03]

Done.

+
     // Used to count how many Run() invocations are on the stack.
     int run_depth;
 
     // Used to record that Quit() was called, or that we should quit the pump
     // once it becomes idle.
     bool quit_received;
 
 #if !defined(OS_MACOSX) && !defined(OS_ANDROID)
     Dispatcher* dispatcher;
 #endif
+   protected:
+    ~RunState();
   };
 
 #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(MessageLoop* loop, const RunID& run_id);
     ~AutoRunState();
    private:
     MessageLoop* loop_;
     RunState* previous_state_;
   };
 #if defined(OS_ANDROID)
  protected:
 #endif
 
 #if defined(OS_WIN)
@@ skipping 169 matching lines @@
   // On Android, the UI message loop is handled by Java side. So Run() should
   // never be called. Instead use Start(), which will forward all the native UI
   // events to the Java message loop.
   void Start();
 #elif !defined(OS_MACOSX)
   // Please see message_pump_win/message_pump_glib for definitions of these
   // methods.
   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:
   // TODO(rvargas): Make this platform independent.
   base::MessagePumpForUI* pump_ui() {
     return static_cast<base::MessagePumpForUI*>(pump_.get());
   }
 #endif  // !defined(OS_MACOSX)
 };
 
@@ skipping 74 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_