MessagePump implementations should call DoWork and DoDelayedWork at equal

priority.

BUG=72007
TEST=base_unittests --gtest_filter=MessageLoopTest.RecursiveDenial3
Review URL: http://codereview.chromium.org/6507017

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@75294 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 101 insertions, 95 deletions

diff --git a/base/message_loop_unittest.cc b/base/message_loop_unittest.cc
index 9576a58..2869bb8 100644
--- a/base/message_loop_unittest.cc
+++ b/base/message_loop_unittest.cc
@@ -768,6 +768,18 @@ class RecursiveTask : public OrderedTasks {
   bool is_reentrant_;
 };
 
+class RecursiveSlowTask : public RecursiveTask {
+ public:
+  RecursiveSlowTask(int depth, TaskList* order, int cookie, bool is_reentrant)
+      : RecursiveTask(depth, order, cookie, is_reentrant) {
+  }
+
+  virtual void Run() {
+    RecursiveTask::Run();
+    PlatformThread::Sleep(10);  // milliseconds
+  }
+};
+
 class QuitTask : public OrderedTasks {
  public:
   QuitTask(TaskList* order, int cookie)
@@ -893,6 +905,42 @@ void RunTest_RecursiveDenial1(MessageLoop::Type message_loop_type) {
   EXPECT_EQ(order[13], TaskItem(RECURSIVE, 2, false));
 }
 
+void RunTest_RecursiveDenial3(MessageLoop::Type message_loop_type) {
+  MessageLoop loop(message_loop_type);
+
+  EXPECT_TRUE(MessageLoop::current()->NestableTasksAllowed());
+  TaskList order;
+  MessageLoop::current()->PostTask(FROM_HERE,
+                                   new RecursiveSlowTask(2, &order, 1, false));
+  MessageLoop::current()->PostTask(FROM_HERE,
+                                   new RecursiveSlowTask(2, &order, 2, false));
+  MessageLoop::current()->PostDelayedTask(FROM_HERE,
+                                          new OrderedTasks(&order, 3), 5);
+  MessageLoop::current()->PostDelayedTask(FROM_HERE,
+                                          new QuitTask(&order, 4), 5);
+
+  MessageLoop::current()->Run();
+
+  // FIFO order.
+  ASSERT_EQ(16U, order.size());
+  EXPECT_EQ(order[ 0], TaskItem(RECURSIVE, 1, true));
+  EXPECT_EQ(order[ 1], TaskItem(RECURSIVE, 1, false));
+  EXPECT_EQ(order[ 2], TaskItem(RECURSIVE, 2, true));
+  EXPECT_EQ(order[ 3], TaskItem(RECURSIVE, 2, false));
+  EXPECT_EQ(order[ 4], TaskItem(RECURSIVE, 1, true));
+  EXPECT_EQ(order[ 5], TaskItem(RECURSIVE, 1, false));
+  EXPECT_EQ(order[ 6], TaskItem(ORDERERD, 3, true));
+  EXPECT_EQ(order[ 7], TaskItem(ORDERERD, 3, false));
+  EXPECT_EQ(order[ 8], TaskItem(RECURSIVE, 2, true));
+  EXPECT_EQ(order[ 9], TaskItem(RECURSIVE, 2, false));
+  EXPECT_EQ(order[10], TaskItem(QUITMESSAGELOOP, 4, true));
+  EXPECT_EQ(order[11], TaskItem(QUITMESSAGELOOP, 4, false));
+  EXPECT_EQ(order[12], TaskItem(RECURSIVE, 1, true));
+  EXPECT_EQ(order[13], TaskItem(RECURSIVE, 1, false));
+  EXPECT_EQ(order[14], TaskItem(RECURSIVE, 2, true));
+  EXPECT_EQ(order[15], TaskItem(RECURSIVE, 2, false));
+}
+
 void RunTest_RecursiveSupport1(MessageLoop::Type message_loop_type) {
   MessageLoop loop(message_loop_type);
 
@@ -1426,6 +1474,12 @@ TEST(MessageLoopTest, RecursiveDenial1) {
   RunTest_RecursiveDenial1(MessageLoop::TYPE_IO);
 }
 
+TEST(MessageLoopTest, RecursiveDenial3) {
+  RunTest_RecursiveDenial3(MessageLoop::TYPE_DEFAULT);
+  RunTest_RecursiveDenial3(MessageLoop::TYPE_UI);
+  RunTest_RecursiveDenial3(MessageLoop::TYPE_IO);
+}
+
 TEST(MessageLoopTest, RecursiveSupport1) {
   RunTest_RecursiveSupport1(MessageLoop::TYPE_DEFAULT);
   RunTest_RecursiveSupport1(MessageLoop::TYPE_UI);
diff --git a/base/message_pump.h b/base/message_pump.h
index a354724..866b33a 100644
--- a/base/message_pump.h
+++ b/base/message_pump.h
@@ -22,7 +22,8 @@ class MessagePump : public RefCountedThreadSafe<MessagePump> {
 
     // Called from within Run in response to ScheduleWork or when the message
     // pump would otherwise call DoDelayedWork.  Returns true to indicate that
-    // work was done.  DoDelayedWork will not be called if DoWork returns true.
+    // work was done.  DoDelayedWork will still be called if DoWork returns
+    // true, but DoIdleWork will not.
     virtual bool DoWork() = 0;
 
     // Called from within Run in response to ScheduleDelayedWork or when the
@@ -61,7 +62,8 @@ class MessagePump : public RefCountedThreadSafe<MessagePump> {
   //     if (should_quit_)
   //       break;
   //
-  //     did_work |= delegate_->DoDelayedWork();
+  //     TimeTicks next_time;
+  //     did_work |= delegate_->DoDelayedWork(&next_time);
   //     if (should_quit_)
   //       break;
   //
@@ -84,9 +86,9 @@ class MessagePump : public RefCountedThreadSafe<MessagePump> {
   // blocks until there is more work of any type to do.
   //
   // Notice that the run loop cycles between calling DoInternalWork, DoWork,
-  // and DoDelayedWork methods.  This helps ensure that neither work queue
-  // starves the other.  This is important for message pumps that are used to
-  // drive animations, for example.
+  // and DoDelayedWork methods.  This helps ensure that none of these work
+  // queues starve the others.  This is important for message pumps that are
+  // used to drive animations, for example.
   //
   // Notice also that after each callout to foreign code, the run loop checks
   // to see if it should quit.  The Quit method is responsible for setting this
diff --git a/base/message_pump_mac.h b/base/message_pump_mac.h
index c30a8ea..b903a1a 100644
--- a/base/message_pump_mac.h
+++ b/base/message_pump_mac.h
@@ -86,24 +86,17 @@ class MessagePumpCFRunLoopBase : public MessagePump {
 
  private:
   // Timer callback scheduled by ScheduleDelayedWork.  This does not do any
-  // work, but it signals delayed_work_source_ so that delayed work can be
-  // performed within the appropriate priority constraints.
+  // work, but it signals work_source_ so that delayed work can be performed
+  // within the appropriate priority constraints.
   static void RunDelayedWorkTimer(CFRunLoopTimerRef timer, void* info);
 
   // Perform highest-priority work.  This is associated with work_source_
-  // signalled by ScheduleWork.  The static method calls the instance method;
-  // the instance method returns true if work was done.
+  // signalled by ScheduleWork or RunDelayedWorkTimer.  The static method calls
+  // the instance method; the instance method returns true if it resignalled
+  // work_source_ to be called again from the loop.
   static void RunWorkSource(void* info);
   bool RunWork();
 
-  // Perform delayed-priority work.  This is associated with
-  // delayed_work_source_ signalled by RunDelayedWorkTimer, and is responsible
-  // for calling ScheduleDelayedWork again if appropriate.  The static method
-  // calls the instance method; the instance method returns true if more
-  // delayed work is available.
-  static void RunDelayedWorkSource(void* info);
-  bool RunDelayedWork();
-
   // Perform idle-priority work.  This is normally called by PreWaitObserver,
   // but is also associated with idle_work_source_.  When this function
   // actually does perform idle work, it will resignal that source.  The
@@ -162,7 +155,6 @@ class MessagePumpCFRunLoopBase : public MessagePump {
   // callbacks.
   CFRunLoopTimerRef delayed_work_timer_;
   CFRunLoopSourceRef work_source_;
-  CFRunLoopSourceRef delayed_work_source_;
   CFRunLoopSourceRef idle_work_source_;
   CFRunLoopSourceRef nesting_deferred_work_source_;
   CFRunLoopObserverRef pre_wait_observer_;
@@ -202,7 +194,6 @@ class MessagePumpCFRunLoopBase : public MessagePump {
   // any call to Run on the stack.  The Run method will check for delegateless
   // work on entry and redispatch it as needed once a delegate is available.
   bool delegateless_work_;
-  bool delegateless_delayed_work_;
   bool delegateless_idle_work_;
 
   DISALLOW_COPY_AND_ASSIGN(MessagePumpCFRunLoopBase);
diff --git a/base/message_pump_mac.mm b/base/message_pump_mac.mm
index 8c5461c..b9f188e 100644
--- a/base/message_pump_mac.mm
+++ b/base/message_pump_mac.mm
@@ -51,7 +51,6 @@ MessagePumpCFRunLoopBase::MessagePumpCFRunLoopBase()
       run_nesting_level_(0),
       deepest_nesting_level_(0),
       delegateless_work_(false),
-      delegateless_delayed_work_(false),
       delegateless_idle_work_(false) {
   run_loop_ = CFRunLoopGetCurrent();
   CFRetain(run_loop_);
@@ -78,15 +77,9 @@ MessagePumpCFRunLoopBase::MessagePumpCFRunLoopBase()
                                        &source_context);
   CFRunLoopAddSource(run_loop_, work_source_, kCFRunLoopCommonModes);
 
-  source_context.perform = RunDelayedWorkSource;
-  delayed_work_source_ = CFRunLoopSourceCreate(NULL,  // allocator
-                                               2,     // priority
-                                               &source_context);
-  CFRunLoopAddSource(run_loop_, delayed_work_source_, kCFRunLoopCommonModes);
-
   source_context.perform = RunIdleWorkSource;
   idle_work_source_ = CFRunLoopSourceCreate(NULL,  // allocator
-                                            3,     // priority
+                                            2,     // priority
                                             &source_context);
   CFRunLoopAddSource(run_loop_, idle_work_source_, kCFRunLoopCommonModes);
 
@@ -169,9 +162,6 @@ MessagePumpCFRunLoopBase::~MessagePumpCFRunLoopBase() {
   CFRunLoopRemoveSource(run_loop_, idle_work_source_, kCFRunLoopCommonModes);
   CFRelease(idle_work_source_);
 
-  CFRunLoopRemoveSource(run_loop_, delayed_work_source_, kCFRunLoopCommonModes);
-  CFRelease(delayed_work_source_);
-
   CFRunLoopRemoveSource(run_loop_, work_source_, kCFRunLoopCommonModes);
   CFRelease(work_source_);
 
@@ -199,10 +189,6 @@ void MessagePumpCFRunLoopBase::Run(Delegate* delegate) {
       CFRunLoopSourceSignal(work_source_);
       delegateless_work_ = false;
     }
-    if (delegateless_delayed_work_) {
-      CFRunLoopSourceSignal(delayed_work_source_);
-      delegateless_delayed_work_ = false;
-    }
     if (delegateless_idle_work_) {
       CFRunLoopSourceSignal(idle_work_source_);
       delegateless_idle_work_ = false;
@@ -260,11 +246,10 @@ void MessagePumpCFRunLoopBase::RunDelayedWorkTimer(CFRunLoopTimerRef timer,
   self->delayed_work_fire_time_ = kCFTimeIntervalMax;
 
   // CFRunLoopTimers fire outside of the priority scheme for CFRunLoopSources.
-  // In order to establish the proper priority where delegate_->DoDelayedWork
-  // can only be called if delegate_->DoWork returns false, the timer used
-  // to schedule delayed work must signal a CFRunLoopSource set at a lower
-  // priority than the one used for delegate_->DoWork.
-  CFRunLoopSourceSignal(self->delayed_work_source_);
+  // In order to establish the proper priority in which work and delayed work
+  // are processed one for one, the timer used to schedule delayed work must
+  // signal a CFRunLoopSource used to dispatch both work and delayed work.
+  CFRunLoopSourceSignal(self->work_source_);
 }
 
 // Called from the run loop.
@@ -291,58 +276,39 @@ bool MessagePumpCFRunLoopBase::RunWork() {
   // released promptly even in the absence of UI events.
   MessagePumpScopedAutoreleasePool autorelease_pool(this);
 
-  // Call DoWork once, and if something was done, arrange to come back here
-  // again as long as the loop is still running.
+  // Call DoWork and DoDelayedWork once, and if something was done, arrange to
+  // come back here again as long as the loop is still running.
   bool did_work = delegate_->DoWork();
-  if (did_work) {
-    CFRunLoopSourceSignal(work_source_);
-  }
-
-  return did_work;
-}
-
-// Called from the run loop.
-// static
-void MessagePumpCFRunLoopBase::RunDelayedWorkSource(void* info) {
-  MessagePumpCFRunLoopBase* self = static_cast<MessagePumpCFRunLoopBase*>(info);
-  self->RunDelayedWork();
-}
-
-// Called by MessagePumpCFRunLoopBase::RunDelayedWorkSource.
-bool MessagePumpCFRunLoopBase::RunDelayedWork() {
-  if (!delegate_) {
-    // This point can be reached with a NULL delegate_ if Run is not on the
-    // stack but foreign code is spinning the CFRunLoop.  Arrange to come back
-    // here when a delegate is available.
-    delegateless_delayed_work_ = true;
-    return false;
-  }
-
-  // The NSApplication-based run loop only drains the autorelease pool at each
-  // UI event (NSEvent).  The autorelease pool is not drained for each
-  // CFRunLoopSource target that's run.  Use a local pool for any autoreleased
-  // objects if the app is not currently handling a UI event to ensure they're
-  // released promptly even in the absence of UI events.
-  MessagePumpScopedAutoreleasePool autorelease_pool(this);
+  bool resignal_work_source = did_work;
 
   TimeTicks next_time;
   delegate_->DoDelayedWork(&next_time);
-
-  bool more_work = !next_time.is_null();
-  if (more_work) {
-    TimeDelta delay = next_time - TimeTicks::Now();
-    if (delay > TimeDelta()) {
-      // There's more delayed work to be done in the future.
-      ScheduleDelayedWork(next_time);
-    } else {
-      // There's more delayed work to be done, and its time is in the past.
-      // Arrange to come back here directly as long as the loop is still
-      // running.
-      CFRunLoopSourceSignal(delayed_work_source_);
+  if (!did_work) {
+    // Determine whether there's more delayed work, and if so, if it needs to
+    // be done at some point in the future or if it's already time to do it.
+    // Only do these checks if did_work is false. If did_work is true, this
+    // function, and therefore any additional delayed work, will get another
+    // chance to run before the loop goes to sleep.
+    bool more_delayed_work = !next_time.is_null();
+    if (more_delayed_work) {
+      TimeDelta delay = next_time - TimeTicks::Now();
+      if (delay > TimeDelta()) {
+        // There's more delayed work to be done in the future.
+        ScheduleDelayedWork(next_time);
+      } else {
+        // There's more delayed work to be done, and its time is in the past.
+        // Arrange to come back here directly as long as the loop is still
+        // running.
+        resignal_work_source = true;
+      }
     }
   }
 
-  return more_work;
+  if (resignal_work_source) {
+    CFRunLoopSourceSignal(work_source_);
+  }
+
+  return resignal_work_source;
 }
 
 // Called from the run loop.
@@ -398,19 +364,12 @@ bool MessagePumpCFRunLoopBase::RunNestingDeferredWork() {
 
   // Immediately try work in priority order.
   if (!RunWork()) {
-    if (!RunDelayedWork()) {
-      if (!RunIdleWork()) {
-        return false;
-      }
-    } else {
-      // There was no work, and delayed work was done.  Arrange for the loop
-      // to try non-nestable idle work on a subsequent pass.
-      CFRunLoopSourceSignal(idle_work_source_);
+    if (!RunIdleWork()) {
+      return false;
     }
   } else {
-    // Work was done.  Arrange for the loop to try non-nestable delayed and
-    // idle work on a subsequent pass.
-    CFRunLoopSourceSignal(delayed_work_source_);
+    // Work was done.  Arrange for the loop to try non-nestable idle work on
+    // a subsequent pass.
     CFRunLoopSourceSignal(idle_work_source_);
   }