Commit 03bb8b7f authored by Christian Sadilek's avatar Christian Sadilek
Browse files

Closes #6680: Handle exceptions thrown by capturePixels

Checking firstContenfulPaint is not reliable to determine whether
or not the compositor is ready. There's no other way for us right
now to handle this but to catch all and return a null bitmap.
parent 2c423b5e
......@@ -72,10 +72,6 @@ class GeckoEngineSession(
internal var currentUrl: String? = null
internal var scrollY: Int = 0
// This is set once the first content paint has occurred and can be used to
// decide if it's safe to call capturePixels on the view.
internal var firstContentfulPaint = false
internal var job: Job = Job()
private var lastSessionState: GeckoSession.SessionState? = null
private var stateBeforeCrash: GeckoSession.SessionState? = null
......@@ -328,7 +324,6 @@ class GeckoEngineSession(
super.close()
job.cancel()
geckoSession.close()
firstContentfulPaint = false
}
/**
......@@ -616,7 +611,6 @@ class GeckoEngineSession(
override fun onFirstComposite(session: GeckoSession) = Unit
override fun onFirstContentfulPaint(session: GeckoSession) {
firstContentfulPaint = true
notifyObservers { onFirstContentfulPaint() }
}
......
......@@ -189,10 +189,9 @@ class GeckoEngineView @JvmOverloads constructor(
currentGeckoView.setDynamicToolbarMaxHeight(height)
}
@Suppress("TooGenericExceptionCaught")
override fun captureThumbnail(onFinish: (Bitmap?) -> Unit) {
// Do not capture pixels if no content has been rendered for this session yet. GeckoView
// might otherwise throw an IllegalStateException if the compositor isn't ready.
if (currentSession?.firstContentfulPaint == true) {
try {
val geckoResult = currentGeckoView.capturePixels()
geckoResult.then({ bitmap ->
onFinish(bitmap)
......@@ -201,7 +200,13 @@ class GeckoEngineView @JvmOverloads constructor(
onFinish(null)
GeckoResult<Void>()
})
} else {
} 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)
}
}
......
......@@ -1914,6 +1914,25 @@ class GeckoEngineSessionTest {
assertEquals(state, actualState)
}
@Test
fun `onFirstContentfulPaint notifies observers`() {
val engineSession = GeckoEngineSession(mock(),
geckoSessionProvider = geckoSessionProvider)
captureDelegates()
var observed = false
engineSession.register(object : EngineSession.Observer {
override fun onFirstContentfulPaint() {
observed = true
}
})
contentDelegate.value.onFirstContentfulPaint(mock())
assertTrue(observed)
}
@Test
fun `onCrash notifies observers about crash`() {
val engineSession = GeckoEngineSession(mock(),
......@@ -2390,21 +2409,6 @@ class GeckoEngineSessionTest {
assertEquals(WindowRequest.Type.CLOSE, receivedWindowRequest!!.type)
}
@Test
fun managesStateOfFirstContentfulPaint() {
val engineSession = GeckoEngineSession(mock(),
geckoSessionProvider = geckoSessionProvider)
captureDelegates()
assertFalse(engineSession.firstContentfulPaint)
contentDelegate.value.onFirstContentfulPaint(geckoSession)
assertTrue(engineSession.firstContentfulPaint)
engineSession.close()
assertFalse(engineSession.firstContentfulPaint)
}
class MockSecurityInformation(
origin: String? = null,
certificate: X509Certificate? = null
......
......@@ -28,6 +28,7 @@ import org.mockito.Mockito.verify
import org.mozilla.geckoview.GeckoResult
import org.mozilla.geckoview.GeckoSession
import org.robolectric.Robolectric.buildActivity
import java.lang.IllegalStateException
@RunWith(AndroidJUnit4::class)
class GeckoEngineViewTest {
......@@ -55,7 +56,6 @@ class GeckoEngineViewTest {
@Test
fun captureThumbnail() {
val geckoSession: GeckoEngineSession = mock()
val engineView = GeckoEngineView(context)
val mockGeckoView = mock<NestedGeckoView>()
var thumbnail: Bitmap? = null
......@@ -64,18 +64,7 @@ class GeckoEngineViewTest {
whenever(mockGeckoView.capturePixels()).thenReturn(geckoResult)
engineView.currentGeckoView = mockGeckoView
engineView.captureThumbnail {
thumbnail = it
}
verify(mockGeckoView, never()).capturePixels()
engineView.currentSession = geckoSession
engineView.captureThumbnail {
thumbnail = it
}
verify(mockGeckoView, never()).capturePixels()
whenever(geckoSession.firstContentfulPaint).thenReturn(true)
// Test GeckoResult resolves successfuly
engineView.captureThumbnail {
thumbnail = it
}
......@@ -86,24 +75,20 @@ class GeckoEngineViewTest {
geckoResult = GeckoResult()
whenever(mockGeckoView.capturePixels()).thenReturn(geckoResult)
// Test GeckoResult resolves in error
engineView.captureThumbnail {
thumbnail = it
}
geckoResult.completeExceptionally(mock())
assertNull(thumbnail)
// Verify that with `firstContentfulPaint` set to false, capturePixels returns a null bitmap
geckoResult = GeckoResult()
// Test GeckoView throwing an exception
whenever(mockGeckoView.capturePixels()).thenThrow(IllegalStateException("Compositor not ready"))
thumbnail = mock()
whenever(geckoSession.firstContentfulPaint).thenReturn(false)
engineView.captureThumbnail {
thumbnail = it
}
// capturePixels should not have been called again because `firstContentfulPaint` is false
verify(mockGeckoView, times(2)).capturePixels()
geckoResult.complete(mock())
assertNull(thumbnail)
}
......
......@@ -72,10 +72,6 @@ class GeckoEngineSession(
internal var currentUrl: String? = null
internal var scrollY: Int = 0
// This is set once the first content paint has occurred and can be used to
// decide if it's safe to call capturePixels on the view.
internal var firstContentfulPaint = false
internal var job: Job = Job()
private var lastSessionState: GeckoSession.SessionState? = null
private var stateBeforeCrash: GeckoSession.SessionState? = null
......@@ -328,7 +324,6 @@ class GeckoEngineSession(
super.close()
job.cancel()
geckoSession.close()
firstContentfulPaint = false
}
/**
......@@ -616,7 +611,6 @@ class GeckoEngineSession(
override fun onFirstComposite(session: GeckoSession) = Unit
override fun onFirstContentfulPaint(session: GeckoSession) {
firstContentfulPaint = true
notifyObservers { onFirstContentfulPaint() }
}
......
......@@ -189,10 +189,9 @@ class GeckoEngineView @JvmOverloads constructor(
currentGeckoView.setDynamicToolbarMaxHeight(height)
}
@Suppress("TooGenericExceptionCaught")
override fun captureThumbnail(onFinish: (Bitmap?) -> Unit) {
// Do not capture pixels if no content has been rendered for this session yet. GeckoView
// might otherwise throw an IllegalStateException if the compositor isn't ready.
if (currentSession?.firstContentfulPaint == true) {
try {
val geckoResult = currentGeckoView.capturePixels()
geckoResult.then({ bitmap ->
onFinish(bitmap)
......@@ -201,7 +200,13 @@ class GeckoEngineView @JvmOverloads constructor(
onFinish(null)
GeckoResult<Void>()
})
} else {
} 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)
}
}
......
......@@ -1914,6 +1914,25 @@ class GeckoEngineSessionTest {
assertEquals(state, actualState)
}
@Test
fun `onFirstContentfulPaint notifies observers`() {
val engineSession = GeckoEngineSession(mock(),
geckoSessionProvider = geckoSessionProvider)
captureDelegates()
var observed = false
engineSession.register(object : EngineSession.Observer {
override fun onFirstContentfulPaint() {
observed = true
}
})
contentDelegate.value.onFirstContentfulPaint(mock())
assertTrue(observed)
}
@Test
fun `onCrash notifies observers about crash`() {
val engineSession = GeckoEngineSession(mock(),
......@@ -2390,21 +2409,6 @@ class GeckoEngineSessionTest {
assertEquals(WindowRequest.Type.CLOSE, receivedWindowRequest!!.type)
}
@Test
fun managesStateOfFirstContentfulPaint() {
val engineSession = GeckoEngineSession(mock(),
geckoSessionProvider = geckoSessionProvider)
captureDelegates()
assertFalse(engineSession.firstContentfulPaint)
contentDelegate.value.onFirstContentfulPaint(geckoSession)
assertTrue(engineSession.firstContentfulPaint)
engineSession.close()
assertFalse(engineSession.firstContentfulPaint)
}
class MockSecurityInformation(
origin: String? = null,
certificate: X509Certificate? = null
......
......@@ -28,6 +28,7 @@ import org.mockito.Mockito.verify
import org.mozilla.geckoview.GeckoResult
import org.mozilla.geckoview.GeckoSession
import org.robolectric.Robolectric.buildActivity
import java.lang.IllegalStateException
@RunWith(AndroidJUnit4::class)
class GeckoEngineViewTest {
......@@ -55,7 +56,6 @@ class GeckoEngineViewTest {
@Test
fun captureThumbnail() {
val geckoSession: GeckoEngineSession = mock()
val engineView = GeckoEngineView(context)
val mockGeckoView = mock<NestedGeckoView>()
var thumbnail: Bitmap? = null
......@@ -64,18 +64,7 @@ class GeckoEngineViewTest {
whenever(mockGeckoView.capturePixels()).thenReturn(geckoResult)
engineView.currentGeckoView = mockGeckoView
engineView.captureThumbnail {
thumbnail = it
}
verify(mockGeckoView, never()).capturePixels()
engineView.currentSession = geckoSession
engineView.captureThumbnail {
thumbnail = it
}
verify(mockGeckoView, never()).capturePixels()
whenever(geckoSession.firstContentfulPaint).thenReturn(true)
// Test GeckoResult resolves successfuly
engineView.captureThumbnail {
thumbnail = it
}
......@@ -86,24 +75,20 @@ class GeckoEngineViewTest {
geckoResult = GeckoResult()
whenever(mockGeckoView.capturePixels()).thenReturn(geckoResult)
// Test GeckoResult resolves in error
engineView.captureThumbnail {
thumbnail = it
}
geckoResult.completeExceptionally(mock())
assertNull(thumbnail)
// Verify that with `firstContentfulPaint` set to false, capturePixels returns a null bitmap
geckoResult = GeckoResult()
// Test GeckoView throwing an exception
whenever(mockGeckoView.capturePixels()).thenThrow(IllegalStateException("Compositor not ready"))
thumbnail = mock()
whenever(geckoSession.firstContentfulPaint).thenReturn(false)
engineView.captureThumbnail {
thumbnail = it
}
// capturePixels should not have been called again because `firstContentfulPaint` is false
verify(mockGeckoView, times(2)).capturePixels()
geckoResult.complete(mock())
assertNull(thumbnail)
}
......
......@@ -71,10 +71,6 @@ class GeckoEngineSession(
internal var currentUrl: String? = null
internal var scrollY: Int = 0
// This is set once the first content paint has occurred and can be used to
// decide if it's safe to call capturePixels on the view.
internal var firstContentfulPaint = false
internal var job: Job = Job()
private var lastSessionState: GeckoSession.SessionState? = null
private var stateBeforeCrash: GeckoSession.SessionState? = null
......@@ -327,7 +323,6 @@ class GeckoEngineSession(
super.close()
job.cancel()
geckoSession.close()
firstContentfulPaint = false
}
/**
......@@ -583,7 +578,7 @@ class GeckoEngineSession(
override fun onFirstComposite(session: GeckoSession) = Unit
override fun onFirstContentfulPaint(session: GeckoSession) {
firstContentfulPaint = true
notifyObservers { onFirstContentfulPaint() }
}
override fun onContextMenu(
......
......@@ -189,10 +189,9 @@ class GeckoEngineView @JvmOverloads constructor(
currentGeckoView.setDynamicToolbarMaxHeight(height)
}
@Suppress("TooGenericExceptionCaught")
override fun captureThumbnail(onFinish: (Bitmap?) -> Unit) {
// Do not capture pixels if no content has been rendered for this session yet. GeckoView
// might otherwise throw an IllegalStateException if the compositor isn't ready.
if (currentSession?.firstContentfulPaint == true) {
try {
val geckoResult = currentGeckoView.capturePixels()
geckoResult.then({ bitmap ->
onFinish(bitmap)
......@@ -201,7 +200,13 @@ class GeckoEngineView @JvmOverloads constructor(
onFinish(null)
GeckoResult<Void>()
})
} else {
} 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)
}
}
......
......@@ -1883,6 +1883,25 @@ class GeckoEngineSessionTest {
assertEquals(state, actualState)
}
@Test
fun `onFirstContentfulPaint notifies observers`() {
val engineSession = GeckoEngineSession(mock(),
geckoSessionProvider = geckoSessionProvider)
captureDelegates()
var observed = false
engineSession.register(object : EngineSession.Observer {
override fun onFirstContentfulPaint() {
observed = true
}
})
contentDelegate.value.onFirstContentfulPaint(mock())
assertTrue(observed)
}
@Test
fun `onCrash notifies observers about crash`() {
val engineSession = GeckoEngineSession(mock(),
......@@ -2310,21 +2329,6 @@ class GeckoEngineSessionTest {
assertEquals(WindowRequest.Type.CLOSE, receivedWindowRequest!!.type)
}
@Test
fun managesStateOfFirstContentfulPaint() {
val engineSession = GeckoEngineSession(mock(),
geckoSessionProvider = geckoSessionProvider)
captureDelegates()
assertFalse(engineSession.firstContentfulPaint)
contentDelegate.value.onFirstContentfulPaint(geckoSession)
assertTrue(engineSession.firstContentfulPaint)
engineSession.close()
assertFalse(engineSession.firstContentfulPaint)
}
class MockSecurityInformation(
origin: String? = null,
certificate: X509Certificate? = null
......
......@@ -28,6 +28,7 @@ import org.mockito.Mockito.verify
import org.mozilla.geckoview.GeckoResult
import org.mozilla.geckoview.GeckoSession
import org.robolectric.Robolectric.buildActivity
import java.lang.IllegalStateException
@RunWith(AndroidJUnit4::class)
class GeckoEngineViewTest {
......@@ -55,7 +56,6 @@ class GeckoEngineViewTest {
@Test
fun captureThumbnail() {
val geckoSession: GeckoEngineSession = mock()
val engineView = GeckoEngineView(context)
val mockGeckoView = mock<NestedGeckoView>()
var thumbnail: Bitmap? = null
......@@ -64,18 +64,7 @@ class GeckoEngineViewTest {
whenever(mockGeckoView.capturePixels()).thenReturn(geckoResult)
engineView.currentGeckoView = mockGeckoView
engineView.captureThumbnail {
thumbnail = it
}
verify(mockGeckoView, never()).capturePixels()
engineView.currentSession = geckoSession
engineView.captureThumbnail {
thumbnail = it
}
verify(mockGeckoView, never()).capturePixels()
whenever(geckoSession.firstContentfulPaint).thenReturn(true)
// Test GeckoResult resolves successfuly
engineView.captureThumbnail {
thumbnail = it
}
......@@ -86,24 +75,20 @@ class GeckoEngineViewTest {
geckoResult = GeckoResult()
whenever(mockGeckoView.capturePixels()).thenReturn(geckoResult)
// Test GeckoResult resolves in error
engineView.captureThumbnail {
thumbnail = it
}
geckoResult.completeExceptionally(mock())
assertNull(thumbnail)
// Verify that with `firstContentfulPaint` set to false, capturePixels returns a null bitmap
geckoResult = GeckoResult()
// Test GeckoView throwing an exception
whenever(mockGeckoView.capturePixels()).thenThrow(IllegalStateException("Compositor not ready"))
thumbnail = mock()
whenever(geckoSession.firstContentfulPaint).thenReturn(false)
engineView.captureThumbnail {
thumbnail = it
}
// capturePixels should not have been called again because `firstContentfulPaint` is false
verify(mockGeckoView, times(2)).capturePixels()
geckoResult.complete(mock())
assertNull(thumbnail)
}
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment