From ce44df6debd14f2406fb24d76495534f6cb17d1e Mon Sep 17 00:00:00 2001 From: pollymce Date: Fri, 19 Apr 2024 13:28:06 +0000 Subject: [PATCH] Bug 1645114 - for screenshot capture, include exception in failure result rather than throwing. r=geckoview-reviewers,tthibaud,twhite Also remove exception handling at call site in Android components - no need to catch exceptions for method that no longer throws. Differential Revision: https://phabricator.services.mozilla.com/D206488 --- .../browser/engine/gecko/GeckoEngineView.kt | 33 +++++++------------ .../engine/gecko/GeckoEngineViewTest.kt | 9 ----- .../android-components/docs/changelog.md | 3 ++ .../mozilla/geckoview/test/ScreenshotTest.kt | 26 +++++++++++++-- .../org/mozilla/geckoview/GeckoDisplay.java | 6 ++-- 5 files changed, 42 insertions(+), 35 deletions(-) diff --git a/mobile/android/android-components/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt b/mobile/android/android-components/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt index d5d77b307347..b65b2f08aac8 100644 --- a/mobile/android/android-components/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt +++ b/mobile/android/android-components/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt @@ -205,29 +205,18 @@ class GeckoEngineView @JvmOverloads constructor( geckoView.activityContextDelegate = GeckoViewActivityContextDelegate(WeakReference(context)) } - @Suppress("TooGenericExceptionCaught") override fun captureThumbnail(onFinish: (Bitmap?) -> Unit) { - try { - val geckoResult = geckoView.capturePixels() - geckoResult.then( - { bitmap -> - onFinish(bitmap) - GeckoResult() - }, - { - onFinish(null) - GeckoResult() - }, - ) - } catch (e: Exception) { - // There's currently no reliable way for consumers of GeckoView to - // know whether or not the compositor is ready. So we have to add - // a catch-all here. In the future, GeckoView will invoke our error - // callback instead and this block can be removed: - // https://bugzilla.mozilla.org/show_bug.cgi?id=1645114 - // https://github.com/mozilla-mobile/android-components/issues/6680 - onFinish(null) - } + val geckoResult = geckoView.capturePixels() + geckoResult.then( + { bitmap -> + onFinish(bitmap) + GeckoResult() + }, + { + onFinish(null) + GeckoResult() + }, + ) } override fun clearSelection() { diff --git a/mobile/android/android-components/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt b/mobile/android/android-components/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt index 7056187e0924..13fd98348389 100644 --- a/mobile/android/android-components/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt +++ b/mobile/android/android-components/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt @@ -90,15 +90,6 @@ class GeckoEngineViewTest { shadowOf(getMainLooper()).idle() assertNull(thumbnail) - - // Test GeckoView throwing an exception - whenever(mockGeckoView.capturePixels()).thenThrow(IllegalStateException("Compositor not ready")) - - thumbnail = mock() - engineView.captureThumbnail { - thumbnail = it - } - assertNull(thumbnail) } @Test diff --git a/mobile/android/android-components/docs/changelog.md b/mobile/android/android-components/docs/changelog.md index e73e897552cf..b1be1fe0271e 100644 --- a/mobile/android/android-components/docs/changelog.md +++ b/mobile/android/android-components/docs/changelog.md @@ -10,6 +10,9 @@ permalink: /changelog/ * Added `StartForegroundService` to safely start a foreground service, see [Bug 1839039](https://bugzilla.mozilla.org/show_bug.cgi?id=1839039) for crash reference. * Added `ProcessInfoProvider` and `BuildVersionProvider` to get information about the app process and the build version. +* **browser-engine-gecko** + * For screenshot capture, include exception in failure result rather than throwing. + # 126.0 * **browser-menu** diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ScreenshotTest.kt b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ScreenshotTest.kt index f3141c661c45..301995c95c09 100644 --- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ScreenshotTest.kt +++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ScreenshotTest.kt @@ -12,7 +12,7 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.filters.MediumTest import androidx.test.platform.app.InstrumentationRegistry import org.hamcrest.Matchers.* // ktlint-disable no-wildcard-imports -import org.junit.Assert +import org.junit.Assert.* import org.junit.Assume.assumeThat import org.junit.Test import org.junit.runner.RunWith @@ -25,6 +25,7 @@ import org.mozilla.geckoview.GeckoSession.ContentDelegate import org.mozilla.geckoview.GeckoSession.ProgressDelegate import org.mozilla.geckoview.test.rule.GeckoSessionTestRule.AssertCalled import org.mozilla.geckoview.test.rule.GeckoSessionTestRule.WithDisplay +import org.mozilla.geckoview.test.util.UiThreadUtils import java.lang.IllegalStateException import kotlin.math.absoluteValue import kotlin.math.max @@ -142,6 +143,27 @@ class ScreenshotTest : BaseSessionTest() { } } + @WithDisplay(height = SCREEN_HEIGHT, width = SCREEN_WIDTH) + @Test + fun capturePixelsFailsWhenCompositorNotReady() { + sessionRule.display?.let { display -> + mainSession.close() + var exceptionListenerCalled = false + val result = display.capturePixels() + result.exceptionally { error: Throwable -> + assertTrue(error is IllegalStateException) + exceptionListenerCalled = true + result + }.accept { + fail("screenshot shouldn't complete successfully after session is closed") + } + UiThreadUtils.waitForCondition( + { exceptionListenerCalled }, + sessionRule.env.defaultTimeoutMillis, + ) + } ?: run { fail("no display found") } + } + // This tests tries to catch problems like Bug 1644561. @WithDisplay(height = SCREEN_HEIGHT, width = SCREEN_WIDTH) @Test @@ -430,7 +452,7 @@ class ScreenshotTest : BaseSessionTest() { .capture() .exceptionally( OnExceptionListener { error: Throwable -> - Assert.assertTrue(error is OutOfMemoryError) + assertTrue(error is OutOfMemoryError) fromException(error) }, ) diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoDisplay.java b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoDisplay.java index 1fc34cb8bba5..e0c16d66cc2f 100644 --- a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoDisplay.java +++ b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoDisplay.java @@ -453,13 +453,15 @@ public class GeckoDisplay { *

This function must be called on the UI thread. * * @return A {@link GeckoResult} that completes with a {@link Bitmap} containing the pixels and - * size information of the requested portion of the visible web page. + * size information of the requested portion of the visible web page, or returns a failure + * {@link GeckoResult} including the reason why in an {@link Exception} */ @UiThread public @NonNull GeckoResult capture() { ThreadUtils.assertOnUiThread(); if (!mSession.isCompositorReady()) { - throw new IllegalStateException("Compositor must be ready before pixels can be captured"); + return GeckoResult.fromException( + new IllegalStateException("Compositor must be ready before pixels can be captured")); } final GeckoResult result = new GeckoResult<>();