[base] Migrate Android's MessagePumpForUI to DoSomeWork().

The tricky part is the need to yield to java before declaring
idle. Previously this was done by only yielding to java if we
did any work. With DoSomeWork() however we do not know if we
did any work, we only know whether there's more work to come.
As such we use a special bit to yield to java once and declare
idle if no other work is scheduled in-between now and java
invoking us again.

Bug: 885371
Change-Id: Id9c8271cc216d06a2db7b9a95636fe87b285eb93
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1485130Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Auto-Submit: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#654883}

base/message_loop/message_pump.h

@@ -59,8 +59,8 @@ class BASE_EXPORT MessagePump {
     // |!delayed_run_time.is_max()|; and it will not be invoked again until
     // ScheduleWork() otherwise. Redundant/spurious invocations outside of those
     // guarantees are not impossible however. DoIdleWork() will not be called so
-    // long as this returns a null |delayed_run_time|. See design doc for
-    // details :
+    // long as this returns a NextWorkInfo which is_immediate(). See design doc
+    // for details :
     // https://docs.google.com/document/d/1no1JMli6F1r8gTF9KDIOvoWkUUZcXDktPf4A1IXYc3M/edit#
     virtual NextWorkInfo DoSomeWork() = 0;
 
@@ -158,12 +158,16 @@ class BASE_EXPORT MessagePump {
   // only be used on the thread that called Run.
   virtual void Quit() = 0;
 
-  // Schedule a DoWork callback to happen reasonably soon.  Does nothing if a
-  // DoWork callback is already scheduled. Once this call is made, DoWork should
-  // not be "starved" at least until it returns a value of false. Thread-safe
-  // (and callers should avoid holding a Lock at all cost while making this call
-  // as some platforms' priority boosting features have been observed to cause
-  // the caller to get descheduled : https://crbug.com/890978).
+  // Schedule a DoSomeWork callback to happen reasonably soon.  Does nothing if
+  // a DoSomeWork callback is already scheduled. Once this call is made,
+  // DoSomeWork is guaranteed to be called repeatedly at least until it returns
+  // a non-immediate NextWorkInfo (or, if this pump wasn't yet migrated,
+  // DoWork() will be called until it returns false). This call can be expensive
+  // and callers should attempt not to invoke it again before a non-immediate
+  // NextWorkInfo was returned from DoSomeWork(). Thread-safe (and callers
+  // should avoid holding a Lock at all cost while making this call as some
+  // platforms' priority boosting features have been observed to cause the
+  // caller to get descheduled : https://crbug.com/890978).
   virtual void ScheduleWork() = 0;
 
   // Schedule a DoDelayedWork callback to happen at the specified time,

base/message_loop/message_pump_android.cc

@@ -121,6 +121,8 @@ MessagePumpForUI::~MessagePumpForUI() {
 }
 
 void MessagePumpForUI::OnDelayedLooperCallback() {
+  // ALooper_pollOnce may call this after Quit() if OnNonDelayedLooperCallback()
+  // resulted in Quit() in the same round.
   if (ShouldQuit())
     return;
 
@@ -139,74 +141,98 @@ void MessagePumpForUI::OnDelayedLooperCallback() {
   // there are no obvious timing or multi-threading related issues.
   DPCHECK(ret >= 0 || errno == EAGAIN);
 
-  delayed_scheduled_time_ = base::TimeTicks();
+  delayed_scheduled_time_.reset();
+
+  Delegate::NextWorkInfo next_work_info = delegate_->DoSomeWork();
 
-  base::TimeTicks next_delayed_work_time;
-  delegate_->DoDelayedWork(&next_delayed_work_time);
-  if (!next_delayed_work_time.is_null()) {
-    ScheduleDelayedWork(next_delayed_work_time);
-  }
   if (ShouldQuit())
     return;
-  // We may be idle now, so pump the loop to find out.
-  ScheduleWork();
+
+  if (next_work_info.is_immediate()) {
+    ScheduleWork();
+    return;
+  }
+
+  DoIdleWork();
+  if (!next_work_info.delayed_run_time.is_max())
+    ScheduleDelayedWork(next_work_info.delayed_run_time);
 }
 
 void MessagePumpForUI::OnNonDelayedLooperCallback() {
-  base::TimeTicks next_delayed_work_time;
-  bool did_any_work = false;
+  // ALooper_pollOnce may call this after Quit() if OnDelayedLooperCallback()
+  // resulted in Quit() in the same round.
+  if (ShouldQuit())
+    return;
 
-  // Runs all native tasks scheduled to run, scheduling delayed work if
-  // necessary.
-  while (true) {
-    bool did_work_this_loop = false;
-    if (ShouldQuit())
-      return;
-    did_work_this_loop = delegate_->DoWork();
+  // A bit added to the |non_delayed_fd_| to keep it signaled when we yield to
+  // java tasks below.
+  constexpr uint64_t kTryJavaTasksBeforeIdleBit = uint64_t(1) << 32;
+
+  // We're about to process all the work requested by ScheduleWork().
+  // MessagePump users are expected to do their best not to invoke
+  // ScheduleWork() again before DoSomeWork() returns a non-immediate
+  // NextWorkInfo below. Hence, capturing the file descriptor's value now and
+  // resetting its contents to 0 should be okay. The value currently stored
+  // should be greater than 0 since work having been scheduled is the reason
+  // we're here. See http://man7.org/linux/man-pages/man2/eventfd.2.html
+  uint64_t pre_work_value = 0;
+  int ret = read(non_delayed_fd_, &pre_work_value, sizeof(pre_work_value));
+  DPCHECK(ret >= 0);
+  DCHECK_GT(pre_work_value, 0U);
+
+  // Note: We can't skip DoSomeWork() even if
+  // |pre_work_value == kTryJavaTasksBeforeIdleBit| here (i.e. no additional
+  // ScheduleWork() since yielding to java) as delayed tasks might have come in
+  // and we need to re-sample |next_work_info|.
+
+  // Runs all application tasks scheduled to run.
+  Delegate::NextWorkInfo next_work_info;
+  do {
     if (ShouldQuit())
       return;
 
-    did_work_this_loop |= delegate_->DoDelayedWork(&next_delayed_work_time);
+    next_work_info = delegate_->DoSomeWork();
+  } while (next_work_info.is_immediate());
 
-    did_any_work |= did_work_this_loop;
+  // Do not resignal |non_delayed_fd_| if we're quitting (this pump doesn't
+  // allow nesting so needing to resume in an outer loop is not an issue
+  // either).
+  if (ShouldQuit())
+    return;
 
-    // If we didn't do any work, we're out of native tasks to run, and we should
-    // return control to the looper to run Java tasks.
-    if (!did_work_this_loop)
-      break;
-  }
-  // If we did any work, return control to the looper to run java tasks before
-  // we call DoIdleWork(). We haven't cleared the fd yet, so we'll get woken up
-  // again soon to check for idle-ness.
-  if (did_any_work)
+  // Before declaring this loop idle, yield to java tasks and arrange to be
+  // called again (unless we're already in that second call).
+  if (pre_work_value != kTryJavaTasksBeforeIdleBit) {
+    // Note: This write() is racing with potential ScheduleWork() calls. This is
+    // fine as write() is adding this bit, not overwriting the existing value,
+    // and as such racing ScheduleWork() calls would merely add 1 to the lower
+    // bits and we would find |pre_work_value != kTryJavaTasksBeforeIdleBit| in
+    // the next cycle again, retrying this.
+    ret = write(non_delayed_fd_, &kTryJavaTasksBeforeIdleBit,
+                sizeof(kTryJavaTasksBeforeIdleBit));
+    DPCHECK(ret >= 0);
     return;
+  }
+
+  // We yielded to java tasks already and they didn't generate a ScheduleWork()
+  // request so we can declare idleness. It's possible for a ScheduleWork()
+  // request to come in racily while this method unwinds, this is fine and will
+  // merely result in it being re-invoked shortly after it returns.
+  DCHECK_EQ(pre_work_value, kTryJavaTasksBeforeIdleBit);
+
   if (ShouldQuit())
     return;
 
-  // Read the file descriptor, resetting its contents to 0 and reading back the
-  // stored value.
-  // See http://man7.org/linux/man-pages/man2/eventfd.2.html
-  uint64_t value = 0;
-  int ret = read(non_delayed_fd_, &value, sizeof(value));
-  DPCHECK(ret >= 0);
-
-  // If we read a value > 1, it means we lost the race to clear the fd before a
-  // new task was posted. This is okay, we can just re-schedule work.
-  if (value > 1) {
-    ScheduleWork();
-  } else {
-    // At this point, the java looper might not be idle - it's impossible to
-    // know pre-Android-M, so we may end up doing Idle work while java tasks are
-    // still queued up. Note that this won't cause us to fail to run java tasks
-    // using QuitWhenIdle, as the JavaHandlerThread will finish running all
-    // currently scheduled tasks before it quits. Also note that we can't just
-    // add an idle callback to the java looper, as that will fire even if native
-    // tasks are still queued up.
-    DoIdleWork();
-    if (!next_delayed_work_time.is_null()) {
-      ScheduleDelayedWork(next_delayed_work_time);
-    }
-  }
+  // At this point, the java looper might not be idle - it's impossible to know
+  // pre-Android-M, so we may end up doing Idle work while java tasks are still
+  // queued up. Note that this won't cause us to fail to run java tasks using
+  // QuitWhenIdle, as the JavaHandlerThread will finish running all currently
+  // scheduled tasks before it quits. Also note that we can't just add an idle
+  // callback to the java looper, as that will fire even if application tasks
+  // are still queued up.
+  DoIdleWork();
+  if (!next_work_info.delayed_run_time.is_max())
+    ScheduleDelayedWork(next_work_info.delayed_run_time);
 }
 
 void MessagePumpForUI::DoIdleWork() {
@@ -296,10 +322,8 @@ void MessagePumpForUI::ScheduleDelayedWork(const TimeTicks& delayed_work_time) {
   if (ShouldQuit())
     return;
 
-  if (!delayed_scheduled_time_.is_null() &&
-      delayed_work_time >= delayed_scheduled_time_) {
+  if (delayed_scheduled_time_ && *delayed_scheduled_time_ == delayed_work_time)
     return;
-  }
 
   DCHECK(!delayed_work_time.is_null());
   delayed_scheduled_time_ = delayed_work_time;

base/message_loop/message_pump_android.h

@@ -14,6 +14,7 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/message_loop/message_pump.h"
+#include "base/optional.h"
 #include "base/time/time.h"
 
 struct ALooper;
@@ -79,8 +80,10 @@ class BASE_EXPORT MessagePumpForUI : public MessagePump {
   Delegate* delegate_ = nullptr;
 
   // The time at which we are currently scheduled to wake up and perform a
-  // delayed task.
-  base::TimeTicks delayed_scheduled_time_;
+  // delayed task. This avoids redundantly scheduling |delayed_fd_| with the
+  // same timeout when subsequent work phases all go idle on the same pending
+  // delayed task; nullopt if no wakeup is currently scheduled.
+  Optional<TimeTicks> delayed_scheduled_time_;
 
   // If set, a callback to fire when the message pump is quit.
   base::OnceClosure on_quit_callback_;

base/message_loop/message_pump_unittest.cc

@@ -44,7 +44,7 @@ bool PumpTypeUsesDoSomeWork(MessageLoop::Type type) {
       // iOS uses a MessagePumpDefault for UI in unit tests, ref.
       // test_support_ios.mm::CreateMessagePumpForUIForTests().
       return true;
-#elif defined(OS_WIN)
+#elif defined(OS_WIN) || defined(OS_ANDROID)
       return true;
 #elif defined(OS_POSIX) && !defined(OS_NACL_SFI)
       // MessagePumpLibevent was migrated (ref. message_pump_for_ui.h and

base/test/test_support_android.cc

@@ -113,13 +113,8 @@ class MessagePumpForUIStub : public base::MessagePumpForUI {
           break;
       }
 
-      more_work_is_plausible = g_state->delegate->DoWork();
-      if (g_state->should_quit)
-        break;
-
-      base::TimeTicks delayed_work_time;
-      more_work_is_plausible |=
-          g_state->delegate->DoDelayedWork(&delayed_work_time);
+      Delegate::NextWorkInfo next_work_info = g_state->delegate->DoSomeWork();
+      more_work_is_plausible = next_work_info.is_immediate();
       if (g_state->should_quit)
         break;
 
@@ -130,7 +125,7 @@ class MessagePumpForUIStub : public base::MessagePumpForUI {
       if (g_state->should_quit)
         break;
 
-      more_work_is_plausible |= !delayed_work_time.is_null();
+      more_work_is_plausible |= !next_work_info.delayed_run_time.is_max();
     }
   }