Add timeout to Instrumentation#waitForIdleSync

waitForIdleSync can easily block forever in the case of a running
animation or similar, and is used throughout our test infrastructure.
This can lead to shard timeouts, especially with batched tests
that have longer timeouts, making issues hard to debug.

This change limits how long we can wait for the MessageQueue to be
idle.

Bug: 1108566
Change-Id: I449da5e71a49bee3513fd47ca659662f16f41d3e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2314989
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792014}

PRESUBMIT.py

@@ -124,6 +124,14 @@ _BANNED_JAVA_FUNCTIONS = (
       ),
       False,
     ),
+    (
+      '.waitForIdleSync()',
+      (
+       'Do not use waitForIdleSync as it masks underlying issues. There is '
+       'almost always something else you should wait on instead.',
+      ),
+      False,
+    ),
 )
 
 # Format: Sequence of tuples containing:

base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java

@@ -58,17 +58,16 @@ import java.util.Enumeration;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 
 /**
- * A custom AndroidJUnitRunner that supports multidex installer and list out test information.
+ * A custom AndroidJUnitRunner that supports multidex installer and lists out test information.
+ * Also customizes various TestRunner and Instrumentation behaviors, like when Activities get
+ * finished, and adds a timeout to waitForIdleSync.
  *
- * This class is the equivalent of BaseChromiumInstrumentationTestRunner in JUnit3. Please
- * beware that is this not a class runner. It is declared in test apk AndroidManifest.xml
+ * Please beware that is this not a class runner. It is declared in test apk AndroidManifest.xml
  * <instrumentation>
- *
- * TODO(yolandyan): remove this class after all tests are converted to JUnit4. Use class runner
- * for test listing.
  */
 @MainDex
 public class BaseChromiumAndroidJUnitRunner extends AndroidJUnitRunner {
@@ -105,6 +104,8 @@ public class BaseChromiumAndroidJUnitRunner extends AndroidJUnitRunner {
     // crashed.
     private static final String BUNDLE_STACK_ID = "stack";
 
+    private static final long WAIT_FOR_IDLE_TIMEOUT_MS = ScalableTimeout.scaleTimeout(10000L);
+
     private static final long FINISH_APP_TASKS_TIMEOUT_MS = ScalableTimeout.scaleTimeout(3000L);
     private static final long FINISH_APP_TASKS_POLL_INTERVAL_MS = 100;
 
@@ -193,6 +194,28 @@ public class BaseChromiumAndroidJUnitRunner extends AndroidJUnitRunner {
         }
     }
 
+    // The Instrumentation implementation of waitForIdleSync does not have a timeout and can wait
+    // indefinitely in the case of animations, etc.
+    //
+    // You should never use this function in new code, as waitForIdleSync hides underlying issues.
+    // There's almost always a better condition to wait on.
+    @Override
+    public void waitForIdleSync() {
+        final CallbackHelper idleCallback = new CallbackHelper();
+        runOnMainSync(() -> {
+            Looper.myQueue().addIdleHandler(() -> {
+                idleCallback.notifyCalled();
+                return false;
+            });
+        });
+
+        try {
+            idleCallback.waitForFirst((int) WAIT_FOR_IDLE_TIMEOUT_MS, TimeUnit.MILLISECONDS);
+        } catch (TimeoutException ex) {
+            Log.w(TAG, "Timeout while waiting for idle main thread.");
+        }
+    }
+
     // TODO(yolandyan): Move this to test harness side once this class gets removed
     private void addTestListPackage(Bundle bundle) {
         PackageManager pm = getContext().getPackageManager();