content/public/test: Fix raciness in NestedMessagePumpAndroid

This patch fixes raciness and thread safety issues in
NestedMessagePumpAndroid by reimplementing it on top of the regular
MessagePumpAndroid. This allows us to remove the complicated 100ms
timeslicing logic between Java and C++ tasks and just share the
underlying Looper directly.

Bug: 872269
Change-Id: Id5f5e42f2dac6f065769d69743d78b1fb04bb916
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2207171Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Auto-Submit: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792326}

base/message_loop/message_pump_android.h

@@ -58,6 +58,7 @@ class BASE_EXPORT MessagePumpForUI : public MessagePump {
 
  protected:
   void SetDelegate(Delegate* delegate) { delegate_ = delegate; }
+  void ResetShouldQuit() { quit_ = false; }
   virtual bool IsTestImplementation() const;
 
  private:

content/public/test/android/javatests/src/org/chromium/content_public/browser/test/NestedSystemMessageHandler.java

@@ -57,24 +57,18 @@ public class NestedSystemMessageHandler {
 
     /**
      * Runs a single nested task on the provided MessageQueue
-     *
-     * @return whether the current loop should quit.
      */
-    public static boolean runSingleNestedLooperTask(MessageQueue queue)
+    public static void runSingleNestedLooperTask(MessageQueue queue)
             throws IllegalArgumentException, IllegalAccessException, SecurityException,
                    InvocationTargetException {
-        boolean quitLoop = false;
+        // This call will block if there are no messages in the queue. It will
+        // also run or more pending C++ tasks as a side effect before returning
+        // |msg|.
         Message msg = (Message) sNextMethod.invoke(queue);
-        if (msg == null) return true;
-        if (msg.what == QUIT_MESSAGE) {
-            quitLoop = true;
-        }
+        if (msg == null) return;
         Handler target = (Handler) sMessageTargetField.get(msg);
 
-        if (target == null) {
-            // No target is a magic identifier for the quit message.
-            quitLoop = true;
-        } else {
+        if (target != null) {
             target.dispatchMessage(msg);
         }
 
@@ -83,27 +77,23 @@ public class NestedSystemMessageHandler {
         sMessageFlagsField.set(msg, oldFlags & ~(1 << 0 /* FLAG_IN_USE */));
 
         msg.recycle();
-        return quitLoop;
     }
 
     /**
-     * Processes messages from the current MessageQueue till the queue becomes idle.
+     * Dispatches the first message from the current MessageQueue, blocking
+     * until a task becomes available if the queue is empty. Callbacks for
+     * other event handlers registered to the thread's looper (e.g.,
+     * MessagePumpAndroid) may also be processed as a side-effect.
+     *
+     * Returns true if task dispatching succeeded, or false if an exception was
+     * thrown.
      */
     @SuppressWarnings("unused")
     @CalledByNative
-    private static boolean runNestedLoopTillIdle() {
+    private static boolean dispatchOneMessage() {
         MessageQueue queue = Looper.myQueue();
-        queue.addIdleHandler(new MessageQueue.IdleHandler() {
-            @Override
-            public boolean queueIdle() {
-                sHandler.sendMessage(sHandler.obtainMessage(QUIT_MESSAGE));
-                return false;
-            }
-        });
-
         try {
-            while (!runSingleNestedLooperTask(queue)) {
-            }
+            runSingleNestedLooperTask(queue);
         } catch (IllegalArgumentException | IllegalAccessException | SecurityException
                 | InvocationTargetException e) {
             e.printStackTrace();
@@ -111,4 +101,16 @@ public class NestedSystemMessageHandler {
         }
         return true;
     }
+
+    /*
+     * Causes a previous call to dispatchOneMessage() to stop blocking and
+     * return.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private static void postQuitMessage() {
+        // Causes MessageQueue.next() to return in case it was blocking waiting
+        // for more messages.
+        sHandler.sendMessage(sHandler.obtainMessage(QUIT_MESSAGE));
+    }
 }

content/public/test/nested_message_pump_android.cc

@@ -5,120 +5,44 @@
 #include "content/public/test/nested_message_pump_android.h"
 
 #include "base/android/jni_android.h"
-#include "base/android/scoped_java_ref.h"
-#include "base/check_op.h"
-#include "base/synchronization/waitable_event.h"
-#include "base/threading/thread_restrictions.h"
-#include "base/time/time.h"
 #include "content/public/test/android/test_support_content_jni_headers/NestedSystemMessageHandler_jni.h"
 
 namespace content {
 
-struct NestedMessagePumpAndroid::RunState {
-  RunState(base::MessagePump::Delegate* delegate, int run_depth)
-      : delegate(delegate),
-        run_depth(run_depth),
-        should_quit(false),
-        waitable_event(base::WaitableEvent::ResetPolicy::AUTOMATIC,
-                       base::WaitableEvent::InitialState::NOT_SIGNALED) {
-    waitable_event.declare_only_used_while_idle();
-  }
-
-  base::MessagePump::Delegate* delegate;
-
-  // Used to count how many Run() invocations are on the stack.
-  int run_depth;
-
-  // Used to flag that the current Run() invocation should return ASAP.
-  bool should_quit;
-
-  // Used to sleep until there is more work to do.
-  base::WaitableEvent waitable_event;
-};
-
-NestedMessagePumpAndroid::NestedMessagePumpAndroid()
-    : state_(NULL) {
-}
-
-NestedMessagePumpAndroid::~NestedMessagePumpAndroid() {
-}
-
-void NestedMessagePumpAndroid::Run(Delegate* delegate) {
-  RunState state(delegate, state_ ? state_->run_depth + 1 : 1);
-  RunState* previous_state = state_;
-  state_ = &state;
-
-  JNIEnv* env = base::android::AttachCurrentThread();
-  DCHECK(env);
-
-  // Need to cap the wait time to allow task processing on the java
-  // side. Otherwise, a long wait time on the native will starve java
-  // tasks.
-  base::TimeDelta max_delay = base::TimeDelta::FromMilliseconds(100);
-
-  for (;;) {
-    if (state_->should_quit)
-      break;
-
-    Delegate::NextWorkInfo next_work_info = delegate->DoWork();
-    bool has_more_immediate_work = next_work_info.is_immediate();
-    if (state_->should_quit)
-      break;
-
-    if (has_more_immediate_work)
-      continue;
-
-    has_more_immediate_work = state_->delegate->DoIdleWork();
-    if (state_->should_quit)
-      break;
-
-    if (has_more_immediate_work)
-      continue;
-
-    // No native tasks to process right now. Process tasks from the Java
-    // System message handler. This will return when the java message queue
-    // is idle.
-    bool ret = Java_NestedSystemMessageHandler_runNestedLoopTillIdle(env);
+NestedMessagePumpAndroid::NestedMessagePumpAndroid() = default;
+NestedMessagePumpAndroid::~NestedMessagePumpAndroid() = default;
+
+// We need to override Run() instead of using the implementation from
+// base::MessagePumpForUI because one of the side-effects of
+// dispatchOneMessage() is calling Looper::pollOnce(). If that happens while we
+// are inside Alooper_pollOnce(), we get a crash because Android Looper
+// isn't re-entrant safe. Instead, we keep the entire run loop in Java (in
+// MessageQueue.next()).
+void NestedMessagePumpAndroid::Run(base::MessagePump::Delegate* delegate) {
+  auto* env = base::android::AttachCurrentThread();
+  SetDelegate(delegate);
+  ResetShouldQuit();
+  ScheduleWork();
+
+  while (!ShouldQuit()) {
+    // Dispatch the first available Java message and one or more C++ messages as
+    // a side effect. If there are no Java messages available, this call will
+    // block until one becomes available (while continuing to process C++ work).
+    bool ret = Java_NestedSystemMessageHandler_dispatchOneMessage(env);
     CHECK(ret) << "Error running java message loop, tests will likely fail.";
-
-    if (next_work_info.delayed_run_time.is_max()) {
-      state_->waitable_event.TimedWait(max_delay);
-    } else {
-      base::TimeDelta delay = next_work_info.remaining_delay();
-      if (delay > max_delay)
-        delay = max_delay;
-      DCHECK_GT(delay, base::TimeDelta());
-      state_->waitable_event.TimedWait(delay);
-    }
   }
-
-  state_ = previous_state;
 }
 
-void NestedMessagePumpAndroid::Attach(base::MessagePump::Delegate* delegate) {}
-
 void NestedMessagePumpAndroid::Quit() {
-  if (state_) {
-    state_->should_quit = true;
-    state_->waitable_event.Signal();
-    return;
-  }
-}
-
-void NestedMessagePumpAndroid::ScheduleWork() {
-  if (state_) {
-    state_->waitable_event.Signal();
-    return;
-  }
+  // Wake up the Java message dispatcher to exit the loop in Run().
+  auto* env = base::android::AttachCurrentThread();
+  Java_NestedSystemMessageHandler_postQuitMessage(env);
+  MessagePumpForUI::Quit();
 }
 
-void NestedMessagePumpAndroid::ScheduleDelayedWork(
-    const base::TimeTicks& delayed_work_time) {
-  // Since this is always called from the same thread as Run(), there is nothing
-  // to do as the loop is already running. It will wait in Run() with the
-  // correct timeout when it's out of immediate tasks.
-  // TODO(gab): Consider removing ScheduleDelayedWork() when all pumps function
-  // this way (bit.ly/merge-message-pump-do-work).
-}
+// Since this pump allows Run() to be called on the UI thread, there's no need
+// to also attach the pump on that thread. Making attach a no-op also prevents
+// the top level run loop from being considered a nested one.
+void NestedMessagePumpAndroid::Attach(Delegate*) {}
 
 }  // namespace content

content/public/test/nested_message_pump_android.h

@@ -5,7 +5,6 @@
 #ifndef CONTENT_PUBLIC_TEST_NESTED_MESSAGE_PUMP_ANDROID_H_
 #define CONTENT_PUBLIC_TEST_NESTED_MESSAGE_PUMP_ANDROID_H_
 
-#include "base/android/jni_android.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/message_loop/message_pump_android.h"
@@ -19,19 +18,11 @@ class NestedMessagePumpAndroid : public base::MessagePumpForUI {
   NestedMessagePumpAndroid();
   ~NestedMessagePumpAndroid() override;
 
-  void Run(Delegate* delegate) override;
+  void Run(Delegate*) override;
   void Quit() override;
-  void ScheduleWork() override;
-  void ScheduleDelayedWork(const base::TimeTicks& delayed_work_time) override;
-  void Attach(Delegate* delegate) override;
+  void Attach(Delegate*) override;
 
  private:
-  // We may make recursive calls to Run, so we save state that needs to be
-  // separate between them in this structure type.
-  struct RunState;
-
-  RunState* state_;
-
   DISALLOW_COPY_AND_ASSIGN(NestedMessagePumpAndroid);
 };