linux: fix main loop issues with windowless plugins

In certain conditions (e.g. NPN_InvalidateRect taking long), the flash plugin runs its own event loop before returning. It looks roughly like this:
while (gtk_events_pending()) {
  gtk_main_iteration();
}

The problem is that our worker source used to force gtk_events_pending() to be always true, because the Check handler always returned TRUE (and that also made HandleDispatch to always run). That caused flash animations to stop and other bad behavior (100% CPU).
This CL changes the Check function to return TRUE only if HandleDispatch should be run (i.e., more_work_is_plausible set to true), while also checking whether the loop was woken up, or the delayed work timer expired.
HandlePrepare was forcing more_work_is_plausible to be always true apparently to avoid starvation. I removed this, I'm not sure why it is needed. If we get starvation issue, we should raise the priority of WorkSource (for reference, most events run at prio 0, except redraws that run at prio 120. WorkSource runs at prio 200). Another possibility is to force more_work_is_plausible but only every n calls to HandlePrepare.

BUG=8202,11843,12278

Review URL: http://codereview.chromium.org/115812
Patch from Antoine Labour <piman@google.com>.

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

1 files changed, 91 insertions, 38 deletions

diff --git a/base/message_pump_glib.cc b/base/message_pump_glib.cc
index e032de4..1b973cb 100644
--- a/base/message_pump_glib.cc
+++ b/base/message_pump_glib.cc
@@ -50,6 +50,37 @@ int GetTimeIntervalMilliseconds(base::Time from) {
 // returns FALSE, GLib will destroy the source.  Dispatch calls may be recursive
 // (i.e., you can call Run from them), but Prepare and Check cannot.
 //     Finalize is called when the source is destroyed.
+// NOTE: It is common for subsytems to want to process pending events while
+// doing intensive work, for example the flash plugin. They usually use the
+// following pattern (recommended by the GTK docs):
+// while (gtk_events_pending()) {
+//   gtk_main_iteration();
+// }
+//
+// gtk_events_pending just calls g_main_context_pending, which does the
+// following:
+// - call prepare on all the sources
+// - do the poll with a timeout of 0 (not blocking)
+// - call check on all the sources
+// - *does not* call dispatch on the sources
+// - return true iff any of prepare() or check() returned true
+//
+// gtk_main_iteration just calls g_main_context_iteration, which does the whole
+// thing, respecting the timeout for the poll (and block, although it is
+// expected not to if gtk_events_pending returned true), and call dispatch.
+//
+// Thus it is important to only return true from prepare or check if we
+// actually have events or work to do. We also need to make sure we keep
+// internal state consistent so that if prepare/check return true when called
+// from gtk_events_pending, they will still return true when called right
+// after, from gtk_main_iteration.
+//
+// For the GLib pump we try to follow the Windows UI pump model:
+// - whenever we receive a wakeup event or the timer for delayed work expires,
+// we run DoWork and/or DoDelayedWork. That part will also run in the other
+// event pumps
+// - we also run DoWork, DoDelayedWork, and possibly DoIdleWork in the main
+// loop, around event handling.
 
 struct WorkSource : public GSource {
   base::MessagePumpForUI* pump;
@@ -65,8 +96,8 @@ gboolean WorkSourcePrepare(GSource* source,
 }
 
 gboolean WorkSourceCheck(GSource* source) {
-  // Always return TRUE, and Dispatch will be called.
-  return TRUE;
+  // Only return TRUE if Dispatch should be called.
+  return static_cast<WorkSource*>(source)->pump->HandleCheck();
 }
 
 gboolean WorkSourceDispatch(GSource* source,
@@ -136,46 +167,64 @@ void MessagePumpForUI::Run(Delegate* delegate) {
   state.delegate = delegate;
   state.should_quit = false;
   state.run_depth = state_ ? state_->run_depth + 1 : 1;
+  state.has_work = false;
+
+  RunState* previous_state = state_;
+  state_ = &state;
+
   // We really only do a single task for each iteration of the loop.  If we
   // have done something, assume there is likely something more to do.  This
   // will mean that we don't block on the message pump until there was nothing
   // more to do.  We also set this to true to make sure not to block on the
   // first iteration of the loop, so RunAllPending() works correctly.
-  state.more_work_is_plausible = true;
-
-  RunState* previous_state = state_;
-  state_ = &state;
+  bool more_work_is_plausible = true;
 
   // We run our own loop instead of using g_main_loop_quit in one of the
   // callbacks.  This is so we only quit our own loops, and we don't quit
   // nested loops run by others.  TODO(deanm): Is this what we want?
-  while (!state_->should_quit)
-    g_main_context_iteration(context_, true);
+  for (;;) {
+    // Don't block if we think we have more work to do.
+    bool block = !more_work_is_plausible;
+
+    // g_main_context_iteration returns true if events have been dispatched.
+    more_work_is_plausible = g_main_context_iteration(context_, block);
+    if (state_->should_quit)
+      break;
+
+    more_work_is_plausible |= state_->delegate->DoWork();
+    if (state_->should_quit)
+      break;
+
+    more_work_is_plausible |=
+        state_->delegate->DoDelayedWork(&delayed_work_time_);
+
+    if (state_->should_quit)
+      break;
+
+    if (more_work_is_plausible)
+      continue;
+
+    more_work_is_plausible = state_->delegate->DoIdleWork();
+    if (state_->should_quit)
+      break;
+  }
 
   state_ = previous_state;
 }
 
 // Return the timeout we want passed to poll.
 int MessagePumpForUI::HandlePrepare() {
-  // If it's likely that we have more work, don't let the pump
-  // block so that we can do some processing.
-  if (state_->more_work_is_plausible)
+  // We know we have work, but we haven't called HandleDispatch yet. Don't let
+  // the pump block so that we can do some processing.
+  if (state_->has_work)
     return 0;
 
-  // Work wasn't plausible, so we'll block.  In the case where glib fires
-  // our Dispatch(), |more_work_is_plausible| will be reset to whatever it
-  // should be.  However, so we don't get starved by more important work,
-  // we set |more_work_is_plausible| to true.  This means if we come back
-  // here without having been through Dispatch(), we will get a chance to be
-  // fired and properly do our work in Dispatch().
-  state_->more_work_is_plausible = true;
-
   // We don't think we have work to do, but make sure not to block
   // longer than the next time we need to run delayed work.
   return GetTimeIntervalMilliseconds(delayed_work_time_);
 }
 
-void MessagePumpForUI::HandleDispatch() {
+bool MessagePumpForUI::HandleCheck() {
   // We should only ever have a single message on the wakeup pipe, since we
   // are only signaled when the queue went from empty to non-empty.  The glib
   // poll will tell us whether there was data, so this read shouldn't block.
@@ -184,33 +233,37 @@ void MessagePumpForUI::HandleDispatch() {
     if (HANDLE_EINTR(read(wakeup_pipe_read_, &msg, 1)) != 1 || msg != '!') {
       NOTREACHED() << "Error reading from the wakeup pipe.";
     }
+    // Since we ate the message, we need to record that we have more work,
+    // because HandleCheck() may be called without HandleDispatch being called
+    // afterwards.
+    state_->has_work = true;
   }
 
-  if (state_->should_quit)
-    return;
+  if (state_->has_work)
+    return true;
 
-  state_->more_work_is_plausible = false;
+  if (GetTimeIntervalMilliseconds(delayed_work_time_) == 0) {
+    // The timer has expired. That condition will stay true until we process
+    // that delayed work, so we don't need to record this differently.
+    return true;
+  }
 
-  if (state_->delegate->DoWork())
-    state_->more_work_is_plausible = true;
+  return false;
+}
 
-  if (state_->should_quit)
-    return;
+void MessagePumpForUI::HandleDispatch() {
+  state_->has_work = false;
+  if (state_->delegate->DoWork()) {
+    // NOTE: on Windows at this point we would call ScheduleWork. But here,
+    // instead of posting a message on the wakeup pipe, we can avoid the
+    // syscalls and just signal that we have more work.
+    state_->has_work = true;
+  }
 
-  if (state_->delegate->DoDelayedWork(&delayed_work_time_))
-    state_->more_work_is_plausible = true;
   if (state_->should_quit)
     return;
 
-  // Don't do idle work if we think there are more important things
-  // that we could be doing.
-  if (state_->more_work_is_plausible)
-    return;
-
-  if (state_->delegate->DoIdleWork())
-    state_->more_work_is_plausible = true;
-  if (state_->should_quit)
-    return;
+  state_->delegate->DoDelayedWork(&delayed_work_time_);
 }
 
 void MessagePumpForUI::AddObserver(Observer* observer) {