From cf9539a846bad1ee5dbef94c18c80eb8871dcdd0 Mon Sep 17 00:00:00 2001 From: Aaron Klotz Date: Wed, 5 Feb 2020 06:52:11 +0000 Subject: [PATCH] Bug 1607537: Fix timeouts in ParentCrashTest.crashParent and re-enable the test; r=geckoview-reviewers,esawin The fundamental issue here is that `ParentCrashTest` starts a second `GeckoRuntime` via `RemoteGeckoService` within the same logical Android Application. This patch adds `RuntimeCreator.shutdownRuntime()` so that the test can shut down the existing `GeckoRuntime` before starting up its own. Differential Revision: https://phabricator.services.mozilla.com/D61249 --HG-- extra : moz-landing-system : lando --- .../geckoview/test/crash/ParentCrashTest.kt | 7 +- .../geckoview/test/util/RuntimeCreator.java | 88 ++++++++++++++++++- 2 files changed, 93 insertions(+), 2 deletions(-) diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/crash/ParentCrashTest.kt b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/crash/ParentCrashTest.kt index 40de67d8afa7..751f5e71a3f0 100644 --- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/crash/ParentCrashTest.kt +++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/crash/ParentCrashTest.kt @@ -23,6 +23,7 @@ import org.mozilla.geckoview.BuildConfig import org.mozilla.geckoview.GeckoRuntime import org.mozilla.geckoview.test.TestCrashHandler import org.mozilla.geckoview.test.util.Environment +import org.mozilla.geckoview.test.util.RuntimeCreator import java.io.File import java.util.concurrent.TimeUnit @@ -36,13 +37,17 @@ class ParentCrashTest { @Before fun setup() { + // Since this test starts up its own GeckoRuntime via + // RemoteGeckoService, we need to shutdown any runtime already running + // in the RuntimeCreator. + RuntimeCreator.shutdownRuntime() + val context = InstrumentationRegistry.getInstrumentation().targetContext val binder = rule.bindService(Intent(context, RemoteGeckoService::class.java)) messenger = Messenger(binder) assertThat("messenger should not be null", binder, notNullValue()) } - @Ignore // Bug 1607537 disabled due to frequent timeouts @Test @UiThreadTest fun crashParent() { diff --git 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 index 4f4979c7a9b9..5939e0fbd299 100644 --- 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 @@ -6,6 +6,7 @@ import org.mozilla.geckoview.RuntimeTelemetry; import org.mozilla.geckoview.WebExtension; import org.mozilla.geckoview.test.TestCrashHandler; +import android.os.Looper; import android.os.Process; import android.support.annotation.NonNull; import android.support.annotation.Nullable; @@ -14,6 +15,8 @@ import androidx.test.platform.app.InstrumentationRegistry; import android.util.Log; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.FutureTask; +import java.util.concurrent.TimeUnit; public class RuntimeCreator { public static final int TEST_SUPPORT_INITIAL = 0; @@ -123,6 +126,20 @@ public class RuntimeCreator { sPortDelegate = portDelegate; } + private static GeckoRuntime.Delegate sShutdownDelegate; + + private static GeckoRuntime.Delegate sWrapperShutdownDelegate = new GeckoRuntime.Delegate() { + @Override + public void onShutdown() { + if (sShutdownDelegate != null) { + sShutdownDelegate.onShutdown(); + return; + } + + Process.killProcess(Process.myPid()); + } + }; + @UiThread public static GeckoRuntime getRuntime() { if (sRuntime != null) { @@ -144,8 +161,77 @@ public class RuntimeCreator { registerTestSupport(); - sRuntime.setDelegate(() -> Process.killProcess(Process.myPid())); + sRuntime.setDelegate(sWrapperShutdownDelegate); return sRuntime; } + + private static final class ShutdownCompleteIndicator implements GeckoRuntime.Delegate { + private boolean mDone = false; + + @Override + public void onShutdown() { + mDone = true; + } + + public boolean isDone() { + return mDone; + } + } + + @UiThread + private static void shutdownRuntimeInternal(final long timeoutMillis) { + if (sRuntime == null) { + return; + } + + final ShutdownCompleteIndicator indicator = new ShutdownCompleteIndicator(); + sShutdownDelegate = indicator; + + sRuntime.shutdown(); + + UiThreadUtils.waitForCondition(() -> indicator.isDone(), timeoutMillis); + if (!indicator.isDone()) { + throw new RuntimeException("Timed out waiting for GeckoRuntime shutdown to complete"); + } + + sRuntime = null; + sShutdownDelegate = null; + } + + /** + * ParentCrashTest needs to start a GeckoRuntime inside a separate service in a separate + * process from this one. Unfortunately that does not play well with the GeckoRuntime in this + * process, since as far as Android is concerned, they are both running inside the same + * Application. + * + * Any test that starts its own GeckoRuntime should call this method during its setup to shut + * down any extant GeckoRuntime, thus ensuring only one GeckoRuntime is active at once. + */ + public static void shutdownRuntime() { + final Environment env = new Environment(); + // It takes a while to shutdown an existing runtime in debug builds, so + // we double the timeout for this method. + final long timeoutMillis = 2 * env.getDefaultTimeoutMillis(); + + if (Looper.myLooper() == Looper.getMainLooper()) { + shutdownRuntimeInternal(timeoutMillis); + return; + } + + final Runnable runnable = new Runnable() { + @Override + public void run() { + RuntimeCreator.shutdownRuntimeInternal(timeoutMillis); + } + }; + + FutureTask task = new FutureTask<>(runnable, null); + InstrumentationRegistry.getInstrumentation().runOnMainSync(task); + try { + task.get(timeoutMillis, TimeUnit.MILLISECONDS); + } catch (Throwable e) { + throw new RuntimeException(e.toString()); + } + } }