Rewrite the glib UI pump.

base/message_pump_glib.cc

Index: base/message_pump_glib.cc
diff --git a/base/message_pump_glib.cc b/base/message_pump_glib.cc
index f837f597c12569f5104b7b921eaf7b4d7915cf9b..e4efd8b715531362157377e720f8a6853fd9dc4f 100644
--- a/base/message_pump_glib.cc
+++ b/base/message_pump_glib.cc
@@ -11,47 +11,79 @@
 #include "base/logging.h"
 #include "base/platform_thread.h"
 
-namespace base {
-
-static const char kWorkScheduled = '\0';
-static const char kDelayedWorkScheduled = '\1';
+namespace {
 
-// I wish these could be const, but g_source_new wants a non-const GSourceFunc
-// pointer.
-
-// static
-GSourceFuncs MessagePumpForUI::WorkSourceFuncs = {
-  WorkSourcePrepare,
-  WorkSourceCheck,
-  WorkSourceDispatch,
-  NULL
-};
+// We send a byte across a pipe to wakeup the event loop.
+const char kWorkScheduled = '\0';
 
-// static
-GSourceFuncs MessagePumpForUI::IdleSourceFuncs = {
-  IdleSourcePrepare,
-  IdleSourceCheck,
-  IdleSourceDispatch,
-  NULL
-};
-
-static int GetTimeIntervalMilliseconds(Time from) {
+// Return a timeout suitable for the glib loop, -1 to block forever,
+// 0 to return right away, or a timeout in milliseconds from now.
+int GetTimeIntervalMilliseconds(base::Time from) {
   if (from.is_null())
     return -1;
 
   // Be careful here.  TimeDelta has a precision of microseconds, but we want a
   // value in milliseconds.  If there are 5.5ms left, should the delay be 5 or
   // 6?  It should be 6 to avoid executing delayed work too early.
-  double timeout = ceil((from - Time::Now()).InMillisecondsF());
+  int delay = static_cast<int>(
+      ceil((from - base::Time::Now()).InMillisecondsF()));
 
   // If this value is negative, then we need to run delayed work soon.
-  int delay = static_cast<int>(timeout);
-  if (delay < 0)
-    delay = 0;
+  return delay < 0 ? 0 : delay;
+}
 
-  return delay;
+// A brief refresher on GLib:
+//     GLib sources have four callbacks: Prepare, Check, Dispatch and Finalize.
+// On each iteration of the GLib pump, it calls each source's Prepare function.
+// This function should return TRUE if it wants GLib to call its Dispatch, and
+// FALSE otherwise.  It can also set a timeout in this case for the next time
+// Prepare should be called again (it may be called sooner).
+//     After the Prepare calls, GLib does a poll to check for events from the
+// system.  File descriptors can be attached to the sources.  The poll may block
+// if none of the Prepare calls returned TRUE.  It will block indefinitely, or
+// by the minimum time returned by a source in Prepare.
+//     After the poll, GLib calls Check for each source that returned FALSE
+// from Prepare.  The return value of Check has the same meaning as for Prepare,
+// making Check a second chance to tell GLib we are ready for Dispatch.
+//     Finally, GLib calls Dispatch for each source that is ready.  If Dispatch
+// 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.
+
+struct WorkSource : public GSource {
+  int timeout_ms;
+};
+
+gboolean WorkSourcePrepare(GSource* source,
+                           gint* timeout_ms) {
+  *timeout_ms = static_cast<WorkSource*>(source)->timeout_ms;
+  return FALSE;
+}
+
+gboolean WorkSourceCheck(GSource* source) {
+  return FALSE;
 }
 
+gboolean WorkSourceDispatch(GSource* source,
+                            GSourceFunc unused_func,
+                            gpointer unused_data) {
+  NOTREACHED();
+  return TRUE;
+}
+
+// I wish these could be const, but g_source_new wants non-const.
+GSourceFuncs WorkSourceFuncs = {
+  WorkSourcePrepare,
+  WorkSourceCheck,
+  WorkSourceDispatch,
+  NULL
+};
+
+}  // namespace
+
+
+namespace base {
+
 MessagePumpForUI::MessagePumpForUI()
     : state_(NULL),
       context_(g_main_context_default()),
@@ -70,8 +102,12 @@ MessagePumpForUI::MessagePumpForUI()
       "Could not set file descriptor to non-blocking!";
   work_source_poll_fd_->fd = read_fd_work_scheduled_;
   work_source_poll_fd_->events = G_IO_IN | G_IO_HUP | G_IO_ERR;
-  work_source_ = AddSource(&WorkSourceFuncs, G_PRIORITY_DEFAULT,
-                           work_source_poll_fd_.get());
+
+  work_source_ = g_source_new(&WorkSourceFuncs, sizeof(WorkSource));
+  // This is needed to allow Run calls inside Dispatch.
+  g_source_set_can_recurse(work_source_, TRUE);
+  g_source_add_poll(work_source_, work_source_poll_fd_.get());
+  g_source_attach(work_source_, context_);
 }
 
 MessagePumpForUI::~MessagePumpForUI() {
@@ -81,50 +117,77 @@ MessagePumpForUI::~MessagePumpForUI() {
   g_source_unref(work_source_);
 }
 
-struct ThreadIdTraits {
-  static void New(void* instance) {
-    int* thread_id = static_cast<int*>(instance);
-    *thread_id = PlatformThread::CurrentId();
-  }
-  static void Delete(void* instance) {
-  }
-};
-
 void MessagePumpForUI::Run(Delegate* delegate) {
+#ifndef NDEBUG
   // Make sure we only run this on one thread.  GTK only has one message pump
   // so we can only have one UI loop per process.
-  static LazyInstance<int, ThreadIdTraits> thread_id(base::LINKER_INITIALIZED);
-  DCHECK(thread_id.Get() == PlatformThread::CurrentId()) <<
+  static int thread_id = PlatformThread::CurrentId();
+  DCHECK(thread_id == PlatformThread::CurrentId()) <<
       "Running MessagePumpForUI on two different threads; "
       "this is unsupported by GLib!";
+#endif
 
   RunState state;
   state.delegate = delegate;
-  state.keep_running = true;
-  // We emulate the behavior of MessagePumpDefault and try to do work at once.
-  state.should_do_work = true;
-  state.should_do_idle_work = false;
-  state.idle_source = NULL;
+  state.should_quit = false;
+  state.run_depth = state_ ? state_->run_depth + 1 : 1;
 
   RunState* previous_state = state_;
   state_ = &state;
 
-  while (state.keep_running)
-    g_main_context_iteration(context_, true);
-
-  if (state.idle_source) {
-    // This removes the source from the context and releases GLib's hold on it.
-    g_source_destroy(state.idle_source);
-    // This releases our hold and destroys the source.
-    g_source_unref(state.idle_source);
+  // 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.
+  bool more_work_is_plausible = true;
+  for (;;) {
+    // Set up our timeout for any delayed work.
+    static_cast<WorkSource*>(work_source_)->timeout_ms =
+        GetTimeIntervalMilliseconds(delayed_work_time_);
+
+    // Process a single iteration of the event loop.
+    g_main_context_iteration(context_, !more_work_is_plausible);
+    if (state_->should_quit)
+      break;
+
+    more_work_is_plausible = false;
+
+    // Drain our wakeup pipe, this is a non-blocking read.

[darin (slow to review), 2008/11/12 18:12:02]

Hmm... have you ever seen this have more than 1 byte?  (Might be good to at
least comment that this should ordinarily be a 1 byte read.)

+    char tempbuf[16];
+    while (read(read_fd_work_scheduled_, tempbuf, sizeof(tempbuf)) > 0) { }
+
+    if (state_->delegate->DoWork())

[dsh, 2008/11/12 19:24:42]

This is much simpler, but the problem is like you said on chat: A GTK widget may
run a loop in the default context (which we are using) which means that this
code will not execute during that time.  This is the reason all the work was
done in the GSource callbacks.

+      more_work_is_plausible = true;
+
+    if (state_->should_quit)
+      break;
+
+    if (state_->delegate->DoDelayedWork(&delayed_work_time_))
+      more_work_is_plausible = true;
+    if (state_->should_quit)
+      break;
+
+    // Don't do idle work if we think there are more important things
+    // that we could be doing.
+    if (more_work_is_plausible)
+      continue;
+
+    if (state_->delegate->DoIdleWork())
+      more_work_is_plausible = true;
+    if (state_->should_quit)
+      break;
   }
 
   state_ = previous_state;
 }
 
 void MessagePumpForUI::Quit() {
-  DCHECK(state_) << "Quit called outside Run!";
-  state_->keep_running = false;
+  if (state_) {
+    state_->should_quit = true;
+  } else {
+    NOTREACHED() << "Quit called outside Run!";
+  }
 }
 
 void MessagePumpForUI::ScheduleWork() {
@@ -132,174 +195,16 @@ void MessagePumpForUI::ScheduleWork() {
   // variables as we would then need locks all over.  This ensures that if
   // we are sleeping in a poll that we will wake up, and we check the pipe
   // so we know when work was scheduled.
-  CHECK(1 == write(write_fd_work_scheduled_, &kWorkScheduled, 1)) <<
-      "Could not write to pipe!";
+  if (write(write_fd_work_scheduled_, &kWorkScheduled, 1) != 1) {
+    NOTREACHED() << "Could not write to the UI message loop wakeup pipe!";
+  }
 }
 
 void MessagePumpForUI::ScheduleDelayedWork(const Time& delayed_work_time) {
+  // We need to wake up the loop in case the poll timeout needs to be
+  // adjusted.  This will cause us to try to do work, but that's ok.
   delayed_work_time_ = delayed_work_time;
-  // This is an optimization.  Delayed work may not be imminent, we may just
-  // need to update our timeout to poll.  Hence we don't want to go overkill
-  // with kWorkScheduled.
-  CHECK(1 == write(write_fd_work_scheduled_, &kDelayedWorkScheduled, 1)) <<
-      "Could not write to pipe!";
-}
-
-// A brief refresher on GLib:
-//     GLib sources have four callbacks: Prepare, Check, Dispatch and Finalize.
-// On each iteration of the GLib pump, it calls each source's Prepare function.
-// This function should return TRUE if it wants GLib to call its Dispatch, and
-// FALSE otherwise.  It can also set a timeout in this case for the next time
-// Prepare should be called again (it may be called sooner).
-//     After the Prepare calls, GLib does a poll to check for events from the
-// system.  File descriptors can be attached to the sources.  The poll may block
-// if none of the Prepare calls returned TRUE.  It will block indefinitely, or
-// by the minimum time returned by a source in Prepare.
-//     After the poll, GLib calls Check for each source that returned FALSE
-// from Prepare.  The return value of Check has the same meaning as for Prepare,
-// making Check a second chance to tell GLib we are ready for Dispatch.
-//     Finally, GLib calls Dispatch for each source that is ready.  If Dispatch
-// 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.
-
-// static
-gboolean MessagePumpForUI::WorkSourcePrepare(GSource* source,
-                                             gint* timeout_ms) {
-  MessagePumpForUI* self = static_cast<WorkSource*>(source)->self;
-  RunState* state = self->state_;
-
-  if (state->should_do_work) {
-    state->should_do_idle_work = false;
-    *timeout_ms = 0;
-    return TRUE;
-  }
-
-  *timeout_ms = GetTimeIntervalMilliseconds(self->delayed_work_time_);
-
-  state->should_do_idle_work = true;

[dsh, 2008/11/12 19:24:42]

It's not really necessary to redesign the whole thing, esp since this is not
correct WRT recursive runs of the main loop via g_main_loop_run (in a GTK
widget).  I think to get it correct it will end up looking a lot like it looks
now.

The current problem is just a bug that could be fixed by adding another state to
RunState that says we tried idle work and can go to sleep on the poll call now.

-  // We want to do idle work right before poll goes to sleep.  Obviously
-  // we are not currently asleep, but we may be about to since we have
-  // no work to do.  If we don't have an idle source ready to go it's
-  // probably because it fired already (or we just started and it hasn't
-  // been added yet) and we should add one for when we are ready.  Note
-  // that this new source will get Prepare called on this current pump
-  // iteration since it gets added at the end of the source list.
-  if (!state->idle_source) {
-    state->idle_source =
-        self->AddSource(&IdleSourceFuncs, G_PRIORITY_DEFAULT_IDLE, NULL);
-  }
-
-  return FALSE;
-}
-
-// static
-gboolean MessagePumpForUI::WorkSourceCheck(GSource* source) {
-  MessagePumpForUI* self = static_cast<WorkSource*>(source)->self;
-  RunState* state = self->state_;
-
-  // Make sure we don't attempt idle work until we are really sure we don't
-  // have other work to do.  We'll know this in the call to Prepare.
-  state->should_do_idle_work = false;
-
-  // Check if ScheduleWork or ScheduleDelayedWork was called.  This is a
-  // non-blocking read.
-  char byte;
-  while (0 < read(self->read_fd_work_scheduled_, &byte, 1)) {
-    // Don't assume we actually have work yet unless the stronger ScheduleWork
-    // was called.
-    if (byte == kWorkScheduled)
-      state->should_do_work = true;
-  }
-
-  if (state->should_do_work)
-    return TRUE;
-
-  if (!self->delayed_work_time_.is_null())
-    return self->delayed_work_time_ <= Time::Now();
-
-  return FALSE;
-}
-
-// static
-gboolean MessagePumpForUI::WorkSourceDispatch(GSource* source,
-                                              GSourceFunc unused_func,
-                                              gpointer unused_data) {
-  MessagePumpForUI* self = static_cast<WorkSource*>(source)->self;
-  RunState* state = self->state_;
-  DCHECK(!state->should_do_idle_work) <<
-      "Idle work should not be flagged while regular work exists.";
-
-  // Note that in this function we never return FALSE.  This source is owned
-  // by GLib and shared by multiple calls to Run.  It will only finally get
-  // destroyed when the loop is destroyed.
-
-  state->should_do_work = state->delegate->DoWork();
-  if (!state->keep_running)
-    return TRUE;
-
-  state->should_do_work |=
-      state->delegate->DoDelayedWork(&self->delayed_work_time_);
-
-  return TRUE;
-}
-
-// static
-gboolean MessagePumpForUI::IdleSourcePrepare(GSource* source,
-                                             gint* timeout_ms) {
-  RunState* state = static_cast<WorkSource*>(source)->self->state_;
-  *timeout_ms = 0;
-  return state->should_do_idle_work;
-}
-
-// static
-gboolean MessagePumpForUI::IdleSourceCheck(GSource* source) {
-  RunState* state = static_cast<WorkSource*>(source)->self->state_;
-  return state->should_do_idle_work;
-}
-
-// static
-gboolean MessagePumpForUI::IdleSourceDispatch(GSource* source,
-                                              GSourceFunc unused_func,
-                                              gpointer unused_data) {
-  RunState* state = static_cast<WorkSource*>(source)->self->state_;
-  // We should not do idle work unless we didn't have other work to do.
-  DCHECK(!state->should_do_work) << "Doing idle work in non-idle time!";
-  state->should_do_idle_work = false;
-  state->should_do_work = state->delegate->DoIdleWork();
-
-  // This is an optimization.  We could always remove ourselves right now,
-  // but we will just get re-added when WorkSourceCheck eventually returns
-  // FALSE.
-  if (!state->should_do_work) {
-    // This is so that when we return FALSE, GLib will not only remove us
-    // from the context, but since it holds the last reference, it will
-    // destroy us as well.
-    g_source_unref(source);
-    state->idle_source = NULL;
-  }
-
-  return state->should_do_work;
-}
-
-GSource* MessagePumpForUI::AddSource(GSourceFuncs* funcs, gint priority,
-                                     GPollFD *optional_poll_fd) {
-  GSource* source = g_source_new(funcs, sizeof(WorkSource));
-
-  // Setting the priority is actually a bit expensive since it causes GLib
-  // to resort an internal list.
-  if (priority != G_PRIORITY_DEFAULT)
-    g_source_set_priority(source, priority);
-
-  // This is needed to allow Run calls inside Dispatch.
-  g_source_set_can_recurse(source, TRUE);
-  static_cast<WorkSource*>(source)->self = this;
-
-  if (optional_poll_fd)
-    g_source_add_poll(source, optional_poll_fd);
-
-  g_source_attach(source, context_);
-  return source;
+  ScheduleWork();
 }
 
 }  // namespace base