Bug 1553515 - Replace loopUntilIdle -> waitForCondition. r=snorp
☠☠ backed out by b6e6ade7d550 ☠ ☠
authorAgi Sferro <agi@sferro.dev>
Thu, 27 Jun 2019 19:00:45 +0000
changeset 543257 bbc184402f36cdd1d76f29a0a01b7305020b172a
parent 543256 8fe9f5d7fef92d8966dfde646aa680685c9c59ce
child 543258 080d770c7b35ebbb52c48a025c60fc74ce599dfe
push id2131
push userffxbld-merge
push dateMon, 26 Aug 2019 18:30:20 +0000
treeherdermozilla-release@b19ffb3ca153 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssnorp
bugs1553515
milestone69.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1553515 - Replace loopUntilIdle -> waitForCondition. r=snorp Differential Revision: https://phabricator.services.mozilla.com/D32583
mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/GeckoResultTest.java
mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt
mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/SessionLifecycleTest.kt
mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java
mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/util/RuntimeCreator.java
mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/util/UiThreadUtils.java
--- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/GeckoResultTest.java
+++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/GeckoResultTest.java
@@ -1,13 +1,12 @@
 package org.mozilla.geckoview.test;
 
+import org.mozilla.gecko.util.ThreadUtils;
 import org.mozilla.geckoview.GeckoResult;
-import org.mozilla.geckoview.GeckoResult.OnExceptionListener;
-import org.mozilla.geckoview.GeckoResult.OnValueListener;
 import org.mozilla.geckoview.test.util.Environment;
 import org.mozilla.geckoview.test.util.UiThreadUtils;
 
 import android.os.Handler;
 import android.os.Looper;
 import android.support.test.annotation.UiThreadTest;
 import android.support.test.filters.MediumTest;
 import android.support.test.runner.AndroidJUnit4;
@@ -29,29 +28,21 @@ public class GeckoResultTest {
     }
 
     private boolean mDone;
 
     private final Environment mEnv = new Environment();
 
     private void waitUntilDone() {
         assertThat("We should not be done", mDone, equalTo(false));
-
-        while (!mDone) {
-            UiThreadUtils.loopUntilIdle(mEnv.getDefaultTimeoutMillis());
-        }
+        UiThreadUtils.waitForCondition(() -> mDone, mEnv.getDefaultTimeoutMillis());
     }
 
     private void done() {
-        UiThreadUtils.HANDLER.post(new Runnable() {
-            @Override
-            public void run() {
-                mDone = true;
-            }
-        });
+        UiThreadUtils.HANDLER.post(() -> mDone = true);
     }
 
     @Before
     public void setup() {
         mDone = false;
     }
 
     @Test
--- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt
+++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt
@@ -25,16 +25,17 @@ import org.mozilla.geckoview.test.util.C
 import android.support.test.filters.MediumTest
 import android.support.test.runner.AndroidJUnit4
 import org.hamcrest.Matchers.*
 import org.junit.After
 import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.mozilla.geckoview.test.util.HttpBin
+import org.mozilla.geckoview.test.util.UiThreadUtils
 import java.net.URI
 
 @RunWith(AndroidJUnit4::class)
 @MediumTest
 class NavigationDelegateTest : BaseSessionTest() {
     companion object {
         val TEST_ENDPOINT: String = "http://localhost:4242"
     }
@@ -1188,16 +1189,17 @@ class NavigationDelegateTest : BaseSessi
                 return GeckoResult.fromValue(sessionRule.createOpenSession())
             }
         })
 
         sessionRule.session.evaluateJS("$('#targetBlankLink').click()")
 
         sessionRule.session.waitUntilCalled(GeckoSession.NavigationDelegate::class,
                                             "onNewSession")
+        UiThreadUtils.loopUntilIdle(sessionRule.env.defaultTimeoutMillis)
     }
 
     @Test
     fun processSwitching() {
         // This loads in the parent process
         mainSession.loadUri("about:config")
         sessionRule.waitForPageStop()
 
--- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/SessionLifecycleTest.kt
+++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/SessionLifecycleTest.kt
@@ -476,27 +476,17 @@ class SessionLifecycleTest : BaseSession
             Log.d(LOGTAG, "Dumped hprof to $dest")
         } catch (e: IOException) {
             Log.e(LOGTAG, "Failed to dump hprof", e)
         }
 
     }
 
     private fun waitUntilCollected(ref: QueuedWeakReference<*>) {
-        val start = SystemClock.uptimeMillis()
-        while (ref.queue.poll() == null) {
-            val elapsed = SystemClock.uptimeMillis() - start
-            if (elapsed > sessionRule.timeoutMillis) {
-                dumpHprof()
-                throw UiThreadUtils.TimeoutException("Timed out after " + elapsed + "ms")
-            }
-
-            try {
-                UiThreadUtils.loopUntilIdle(100)
-            } catch (e: UiThreadUtils.TimeoutException) {
-            }
+        UiThreadUtils.waitForCondition({
             Runtime.getRuntime().gc()
-        }
+            ref.queue.poll() != null
+        }, sessionRule.timeoutMillis)
     }
 
     class QueuedWeakReference<T> @JvmOverloads constructor(obj: T, var queue: ReferenceQueue<T> =
             ReferenceQueue()) : WeakReference<T>(obj, queue)
 }
--- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java
+++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java
@@ -1050,17 +1050,17 @@ public class GeckoSessionTestRule implem
             return (RuntimeException) cause;
         } else if (e instanceof RuntimeException) {
             return (RuntimeException) e;
         }
 
         return new RuntimeException(cause != null ? cause : e);
     }
 
-    protected void prepareStatement(final Description description) throws Throwable {
+    protected void prepareStatement(final Description description) {
         final GeckoSessionSettings settings = new GeckoSessionSettings(mDefaultSettings);
         mTimeoutMillis = env.getDefaultTimeoutMillis();
         mNullDelegates = new HashSet<>();
         mClosedSession = false;
         mWithDevTools = false;
         mIgnoreCrash = false;
 
         applyAnnotations(Arrays.asList(description.getTestClass().getAnnotations()), settings);
@@ -1190,19 +1190,23 @@ public class GeckoSessionTestRule implem
             mDisplay.surfaceChanged(mDisplaySurface, mDisplaySize.x, mDisplaySize.y);
         }
 
         if (!mClosedSession) {
             openSession(mMainSession);
         }
     }
 
-    protected void prepareSession(final GeckoSession session) throws Throwable {
+    protected void prepareSession(final GeckoSession session) {
         for (final Class<?> cls : DEFAULT_DELEGATES) {
-            setDelegate(cls, session, mNullDelegates.contains(cls) ? null : mCallbackProxy);
+            try {
+                setDelegate(cls, session, mNullDelegates.contains(cls) ? null : mCallbackProxy);
+            } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) {
+                throw new RuntimeException(e);
+            }
         }
     }
 
     /**
      * Call open() on a session, and ensure it's ready for use by the test. In particular,
      * remove any extra calls recorded as part of opening the session.
      *
      * @param session Session to open.
@@ -1229,16 +1233,17 @@ public class GeckoSessionTestRule implem
                 mRDPTabs = new HashMap<>();
             }
             final Tab tab = sRDPConnection.getMostRecentTab();
             mRDPTabs.put(session, tab);
         }
     }
 
     private void waitForInitialLoad(final GeckoSession session) {
+        ThreadUtils.assertOnUiThread();
         // We receive an initial about:blank load; don't expose that to the test. The initial
         // load ends with the first onPageStop call, so ignore everything from the session
         // until the first onPageStop call.
 
         try {
             // We cannot detect initial page load without progress delegate.
             assertThat("ProgressDelegate cannot be null-delegate when opening session",
                        GeckoSession.ProgressDelegate.class, not(isIn(mNullDelegates)));
@@ -1290,17 +1295,17 @@ public class GeckoSessionTestRule implem
 
     protected void deleteCrashDumps() {
         File dumpDir = new File(getRuntime().getProfileDir(), "minidumps");
         for (final File dump : dumpDir.listFiles()) {
             dump.delete();
         }
     }
 
-    protected void cleanupStatement() throws Throwable {
+    protected void cleanupStatement() {
         mWaitScopeDelegates.clear();
         mTestScopeDelegates.clear();
 
         for (final GeckoSession session : mSubSessions) {
             cleanupSession(session);
         }
 
         cleanupSession(mMainSession);
@@ -1334,31 +1339,29 @@ public class GeckoSessionTestRule implem
     }
 
     @Override
     public Statement apply(final Statement base, final Description description) {
         return new Statement() {
             @Override
             public void evaluate() throws Throwable {
                 final AtomicReference<Throwable> exceptionRef = new AtomicReference<>();
-                mInstrumentation.runOnMainSync(new Runnable() {
-                    @Override
-                    public void run() {
+
+                mInstrumentation.runOnMainSync(() -> {
+                    try {
+                        prepareStatement(description);
+                        base.evaluate();
+                        performTestEndCheck();
+                    } catch (Throwable t) {
+                        exceptionRef.set(t);
+                    } finally {
                         try {
-                            prepareStatement(description);
-                            base.evaluate();
-                            performTestEndCheck();
+                            cleanupStatement();
                         } catch (Throwable t) {
-                            exceptionRef.set(t);
-                        } finally {
-                            try {
-                                cleanupStatement();
-                            } catch (Throwable t) {
-                                exceptionRef.set(t);
-                            }
+                            exceptionRef.compareAndSet(null, t);
                         }
                     }
                 });
 
                 Throwable throwable = exceptionRef.get();
                 if (throwable != null) {
                     throw throwable;
                 }
@@ -1548,19 +1551,21 @@ public class GeckoSessionTestRule implem
         assertThat("Delegate should implement a GeckoSession delegate " +
                            "or registered external delegate",
                    isSessionCallback, equalTo(true));
 
         waitUntilCalled(session, callback.getClass(), methodCalls);
         forCallbacksDuringWait(session, callback);
     }
 
-    protected void waitUntilCalled(final @Nullable GeckoSession session,
-                                   final @NonNull Class<?> delegate,
-                                   final @NonNull List<MethodCall> methodCalls) {
+    private void waitUntilCalled(final @Nullable GeckoSession session,
+                                 final @NonNull Class<?> delegate,
+                                 final @NonNull List<MethodCall> methodCalls) {
+        ThreadUtils.assertOnUiThread();
+
         if (session != null && !session.equals(mMainSession)) {
             assertThat("Session should be wrapped through wrapSession",
                        session, isIn(mSubSessions));
         }
 
         // Make sure all handlers are set though #delegateUntilTestEnd or #delegateDuringNextWait,
         // instead of through GeckoSession directly, so that we can still record calls even with
         // custom handlers set.
@@ -1597,25 +1602,18 @@ public class GeckoSessionTestRule implem
 
         boolean calledAny = false;
         int index = mLastWaitEnd;
         long startTime = SystemClock.uptimeMillis();
 
         beforeWait();
 
         while (!calledAny || !methodCalls.isEmpty()) {
-            while (index >= mCallRecords.size()) {
-                UiThreadUtils.loopUntilIdle(mTimeoutMillis);
-                // We could loop forever here if the UI thread keeps receiving
-                // messages that don't result in any methods being called.
-                // Check whether we've exceeded our allotted time and bail out.
-                if (SystemClock.uptimeMillis() - startTime > mTimeoutMillis) {
-                    break;
-                }
-            }
+            final int checkIndex = index;
+            UiThreadUtils.waitForCondition(() -> (checkIndex < mCallRecords.size()), mTimeoutMillis);
 
             if (SystemClock.uptimeMillis() - startTime > mTimeoutMillis) {
                 throw new UiThreadUtils.TimeoutException("Timed out after " + mTimeoutMillis + "ms");
             }
 
             final MethodCall recorded = mCallRecords.get(index).methodCall;
             calledAny |= recorded.method.getDeclaringClass().isAssignableFrom(delegate);
             index++;
--- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/util/RuntimeCreator.java
+++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/util/RuntimeCreator.java
@@ -1,20 +1,22 @@
 package org.mozilla.geckoview.test.util;
 
 import org.mozilla.geckoview.GeckoRuntime;
 import org.mozilla.geckoview.GeckoRuntimeSettings;
 import org.mozilla.geckoview.test.TestCrashHandler;
 
 import android.os.Process;
+import android.support.annotation.UiThread;
 import android.support.test.InstrumentationRegistry;
 
 public class RuntimeCreator {
     private static GeckoRuntime sRuntime;
 
+    @UiThread
     public static GeckoRuntime getRuntime() {
         if (sRuntime != null) {
             return sRuntime;
         }
 
         final GeckoRuntimeSettings.Builder runtimeSettingsBuilder =
                 new GeckoRuntimeSettings.Builder();
         runtimeSettingsBuilder.arguments(new String[]{"-purgecaches"})
@@ -25,18 +27,13 @@ public class RuntimeCreator {
         if (new Environment().isAutomation()) {
             runtimeSettingsBuilder.crashHandler(TestCrashHandler.class);
         }
 
         sRuntime = GeckoRuntime.create(
                 InstrumentationRegistry.getTargetContext(),
                 runtimeSettingsBuilder.build());
 
-        sRuntime.setDelegate(new GeckoRuntime.Delegate() {
-            @Override
-            public void onShutdown() {
-                Process.killProcess(Process.myPid());
-            }
-        });
+        sRuntime.setDelegate(() -> Process.killProcess(Process.myPid()));
 
         return sRuntime;
     }
 }
--- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/util/UiThreadUtils.java
+++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/util/UiThreadUtils.java
@@ -1,32 +1,34 @@
 /* -*- Mode: Java; c-basic-offset: 4; tab-width: 4; indent-tabs-mode: nil; -*-
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.geckoview.test.util;
 
+import org.mozilla.gecko.util.ThreadUtils;
 import org.mozilla.geckoview.GeckoResult;
 
 import android.os.Handler;
 import android.os.Looper;
 import android.os.Message;
 import android.os.MessageQueue;
 import android.os.SystemClock;
 import android.support.annotation.NonNull;
+import android.support.test.InstrumentationRegistry;
+import android.support.test.internal.runner.InstrumentationConnection;
 import android.util.Log;
 
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Function;
 
 public class UiThreadUtils {
-    private static final String LOGTAG = "UiThreadUtils";
-    private static long sLongestWait;
-
     private static Method sGetNextMessage = null;
     static {
         try {
             sGetNextMessage = MessageQueue.class.getDeclaredMethod("next");
             sGetNextMessage.setAccessible(true);
         } catch (NoSuchMethodException e) {
             throw new IllegalStateException(e);
         }
@@ -54,26 +56,16 @@ public class UiThreadUtils {
         @Override
         public void run() {
             throw new TimeoutException("Timed out after " + timeout + "ms");
         }
     }
 
     public static final Handler HANDLER = new Handler(Looper.getMainLooper());
     private static final TimeoutRunnable TIMEOUT_RUNNABLE = new TimeoutRunnable();
-    private static final MessageQueue.IdleHandler IDLE_HANDLER = new MessageQueue.IdleHandler() {
-        @Override
-        public boolean queueIdle() {
-            final Message msg = Message.obtain(HANDLER);
-            msg.obj = HANDLER;
-            HANDLER.sendMessageAtFrontOfQueue(msg);
-            return false; // Remove this idle handler.
-        }
-    };
-
     private static RuntimeException unwrapRuntimeException(final Throwable e) {
         final Throwable cause = e.getCause();
         if (cause != null && cause instanceof RuntimeException) {
             return (RuntimeException) cause;
         } else if (e instanceof RuntimeException) {
             return (RuntimeException) e;
         }
 
@@ -86,19 +78,17 @@ public class UiThreadUtils {
      *
      * @param result A {@link GeckoResult} instance.
      * @param <T> The type of the value held by the {@link GeckoResult}
      * @return The value of the completed {@link GeckoResult}.
      */
     public static <T> T waitForResult(@NonNull GeckoResult<T> result, long timeout) throws Throwable {
         final ResultHolder<T> holder = new ResultHolder<>(result);
 
-        while (!holder.isComplete) {
-            loopUntilIdle(timeout);
-        }
+        waitForCondition(() -> holder.isComplete, timeout);
 
         if (holder.error != null) {
             throw holder.error;
         }
 
         return holder.value;
     }
 
@@ -117,16 +107,37 @@ public class UiThreadUtils {
             });
         }
     }
 
     public interface Condition {
         boolean test();
     }
 
+    public static void loopUntilIdle(final long timeout) {
+        AtomicBoolean idle = new AtomicBoolean(false);
+
+        MessageQueue.IdleHandler handler = null;
+        try {
+            handler = () -> {
+                idle.set(true);
+                // Remove handler
+                return false;
+            };
+
+            HANDLER.getLooper().getQueue().addIdleHandler(handler);
+
+            waitForCondition(() -> idle.get(), timeout);
+        } finally {
+            if (handler != null) {
+                HANDLER.getLooper().getQueue().removeIdleHandler(handler);
+            }
+        }
+    }
+
     public static void waitForCondition(Condition condition, final long timeout) {
          // Adapted from GeckoThread.pumpMessageLoop.
         final MessageQueue queue = HANDLER.getLooper().getQueue();
 
         TIMEOUT_RUNNABLE.set(timeout);
 
         try {
             while (!condition.test()) {
@@ -141,54 +152,9 @@ public class UiThreadUtils {
                     return;
                 }
                 msg.getTarget().dispatchMessage(msg);
             }
         } finally {
             TIMEOUT_RUNNABLE.cancel();
         }
     }
-
-    public static void loopUntilIdle(final long timeout) {
-        // Adapted from GeckoThread.pumpMessageLoop.
-        final MessageQueue queue = HANDLER.getLooper().getQueue();
-        if (timeout > 0) {
-            TIMEOUT_RUNNABLE.set(timeout);
-        } else {
-            queue.addIdleHandler(IDLE_HANDLER);
-        }
-
-        final long startTime = SystemClock.uptimeMillis();
-        try {
-            while (true) {
-                final Message msg;
-                try {
-                    msg = (Message) sGetNextMessage.invoke(queue);
-                } catch (final IllegalAccessException | InvocationTargetException e) {
-                    throw unwrapRuntimeException(e);
-                }
-                if (msg.getTarget() == HANDLER && msg.obj == HANDLER) {
-                    // Our idle signal.
-                    break;
-                } else if (msg.getTarget() == null) {
-                    HANDLER.getLooper().quit();
-                    return;
-                }
-                msg.getTarget().dispatchMessage(msg);
-
-                if (timeout > 0) {
-                    TIMEOUT_RUNNABLE.cancel();
-                    queue.addIdleHandler(IDLE_HANDLER);
-                }
-            }
-
-            final long waitDuration = SystemClock.uptimeMillis() - startTime;
-            if (waitDuration > sLongestWait) {
-                sLongestWait = waitDuration;
-                Log.i(LOGTAG, "New longest wait: " + waitDuration + "ms");
-            }
-        } finally {
-            if (timeout > 0) {
-                TIMEOUT_RUNNABLE.cancel();
-            }
-        }
-    }
 }