From 35fe396559c959c1f3f439f6676606e49ec9f3a0 Mon Sep 17 00:00:00 2001 From: Sebastian Kaspari Date: Mon, 31 Aug 2020 18:14:53 +0200 Subject: [PATCH] Issue #8255: Lazily restore engine sessions after content process kill or crash. * Instead of keeping the EngineSessionState inside EngineSession, we now always attach it to EngineState and also do not clear it anymore. * If the content process gets killed we now just suspend affected EngineSession instances. They will automatically and lazily get restored from the last EngineSessionState once needed. * On a content process crash we now mark the EngineState as crashed and suspend the EngineSession. We will not restore the EngineSession until explicitly restored by the application. --- .../engine/gecko/GeckoEngineSession.kt | 42 +------ .../browser/engine/gecko/GeckoEngineView.kt | 90 +++------------ .../engine/gecko/GeckoEngineSessionTest.kt | 84 +------------- .../engine/gecko/GeckoEngineViewTest.kt | 70 +++--------- .../engine/gecko/GeckoEngineSession.kt | 46 +------- .../browser/engine/gecko/GeckoEngineView.kt | 82 +++----------- .../engine/gecko/GeckoEngineSessionTest.kt | 80 +++----------- .../engine/gecko/GeckoEngineViewTest.kt | 72 +++--------- .../engine/gecko/GeckoEngineSession.kt | 42 +------ .../browser/engine/gecko/GeckoEngineView.kt | 78 ++++--------- .../engine/gecko/GeckoEngineSessionTest.kt | 104 ------------------ .../engine/gecko/GeckoEngineViewTest.kt | 70 +++--------- .../engine/system/SystemEngineSession.kt | 25 ----- .../engine/system/SystemEngineSessionTest.kt | 53 +-------- .../session/engine/EngineMiddleware.kt | 14 ++- .../browser/session/engine/EngineObserver.kt | 14 +++ .../engine/middleware/CrashMiddleware.kt | 44 +++----- .../engine/middleware/SuspendMiddleware.kt | 15 +-- .../session/storage/BrowserStateSerializer.kt | 2 +- .../session/storage/SnapshotSerializer.kt | 2 +- .../browser/state/reducer/CrashReducer.kt | 12 +- .../state/reducer/EngineStateReducer.kt | 14 ++- .../state/state/CustomTabSessionState.kt | 9 +- .../browser/state/state/EngineState.kt | 3 +- .../browser/state/state/SessionState.kt | 4 +- .../browser/state/state/TabSessionState.kt | 7 +- .../concept/engine/EngineSession.kt | 27 ++--- .../feature/session/SessionUseCases.kt | 2 +- .../session/engine/EngineViewPresenter.kt | 24 +++- 29 files changed, 222 insertions(+), 909 deletions(-) diff --git a/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt b/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt index 11abf85e87b..a5288b9fbf5 100644 --- a/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt +++ b/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt @@ -77,8 +77,6 @@ class GeckoEngineSession( internal var scrollY: Int = 0 internal var job: Job = Job() - private var lastSessionState: GeckoSession.SessionState? = null - private var stateBeforeCrash: GeckoSession.SessionState? = null private var canGoBack: Boolean = false /** @@ -189,13 +187,6 @@ class GeckoEngineSession( geckoSession.gotoHistoryIndex(index) } - /** - * See [EngineSession.saveState] - */ - override fun saveState(): EngineSessionState { - return GeckoEngineSessionState(lastSessionState) - } - /** * See [EngineSession.restoreState] */ @@ -371,22 +362,6 @@ class GeckoEngineSession( geckoSession.exitFullScreen() } - /** - * See [EngineSession.recoverFromCrash] - */ - @Synchronized - override fun recoverFromCrash(): Boolean { - val state = stateBeforeCrash - - return if (state != null) { - geckoSession.restoreState(state) - stateBeforeCrash = null - true - } else { - false - } - } - /** * See [EngineSession.markActiveForWebExtensions]. */ @@ -601,7 +576,9 @@ class GeckoEngineSession( } override fun onSessionStateChange(session: GeckoSession, sessionState: GeckoSession.SessionState) { - lastSessionState = sessionState + notifyObservers { + onState(GeckoEngineSessionState(sessionState)) + } } } @@ -726,23 +703,10 @@ class GeckoEngineSession( } override fun onCrash(session: GeckoSession) { - stateBeforeCrash = lastSessionState - - recoverGeckoSession() - notifyObservers { onCrash() } } override fun onKill(session: GeckoSession) { - // The content process of this session got killed (resources reclaimed by Android). - // Let's recover and restore the last known state. - - val state = lastSessionState - - recoverGeckoSession() - - state?.let { geckoSession.restoreState(it) } - notifyObservers { onProcessKilled() } } diff --git a/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt b/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt index c8041609bd4..c8a503d99d2 100644 --- a/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt +++ b/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt @@ -7,14 +7,12 @@ package mozilla.components.browser.engine.gecko import android.content.Context import android.graphics.Bitmap import android.util.AttributeSet -import android.view.View import android.widget.FrameLayout import androidx.annotation.VisibleForTesting import androidx.core.view.ViewCompat import mozilla.components.browser.engine.gecko.selection.GeckoSelectionActionDelegate import mozilla.components.concept.engine.EngineSession import mozilla.components.concept.engine.EngineView -import mozilla.components.concept.engine.permission.PermissionRequest import mozilla.components.concept.engine.selection.SelectionActionDelegate import org.mozilla.geckoview.BasicSelectionActionDelegate import org.mozilla.geckoview.GeckoResult @@ -30,7 +28,7 @@ class GeckoEngineView @JvmOverloads constructor( defStyleAttr: Int = 0 ) : FrameLayout(context, attrs, defStyleAttr), EngineView { @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) - internal var currentGeckoView = object : NestedGeckoView(context) { + internal var geckoView = object : NestedGeckoView(context) { override fun onAttachedToWindow() { try { @@ -40,12 +38,12 @@ class GeckoEngineView @JvmOverloads constructor( val otherActivityClassName = this.session?.accessibility?.view?.context?.javaClass?.simpleName val otherActivityClassHashcode = - this.session?.accessibility?.view?.context?.hashCode() + this.session?.accessibility?.view?.context?.hashCode() val activityClassName = context.javaClass.simpleName val activityClassHashCode = context.hashCode() val msg = "ATTACH VIEW: Current activity: $activityClassName hashcode " + - "$activityClassHashCode Other activity: $otherActivityClassName " + - "hashcode $otherActivityClassHashcode" + "$activityClassHashCode Other activity: $otherActivityClassName " + + "hashcode $otherActivityClassHashcode" throw IllegalStateException(msg, e) } } @@ -63,24 +61,6 @@ class GeckoEngineView @JvmOverloads constructor( ViewCompat.setImportantForAutofill(this, ViewCompat.IMPORTANT_FOR_ACCESSIBILITY_YES) } - @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) - internal val observer = object : EngineSession.Observer { - override fun onCrash() { - rebind() - } - - override fun onProcessKilled() { - rebind() - } - - override fun onFirstContentfulPaint() { - visibility = View.VISIBLE - } - - override fun onAppPermissionRequest(permissionRequest: PermissionRequest) = Unit - override fun onContentPermissionRequest(permissionRequest: PermissionRequest) = Unit - } - @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) internal var currentSession: GeckoEngineSession? = null @@ -90,9 +70,7 @@ class GeckoEngineView @JvmOverloads constructor( override var selectionActionDelegate: SelectionActionDelegate? = null init { - // Currently this is just a FrameLayout with a single GeckoView instance. Eventually this - // implementation should handle at least two GeckoView so that we can switch between - addView(currentGeckoView) + addView(geckoView) } /** @@ -101,23 +79,18 @@ class GeckoEngineView @JvmOverloads constructor( @Synchronized override fun render(session: EngineSession) { val internalSession = session as GeckoEngineSession + currentSession = session - currentSession?.apply { unregister(observer) } - - currentSession = session.apply { - register(observer) - } - - if (currentGeckoView.session != internalSession.geckoSession) { - currentGeckoView.session?.let { + if (geckoView.session != internalSession.geckoSession) { + geckoView.session?.let { // Release a previously assigned session. Otherwise GeckoView will close it // automatically. detachSelectionActionDelegate(it) - currentGeckoView.releaseSession() + geckoView.releaseSession() } try { - currentGeckoView.setSession(internalSession.geckoSession) + geckoView.setSession(internalSession.geckoSession) attachSelectionActionDelegate(internalSession.geckoSession) } catch (e: IllegalStateException) { // This is to debug "display already acquired" crashes @@ -128,8 +101,8 @@ class GeckoEngineView @JvmOverloads constructor( val activityClassName = context.javaClass.simpleName val activityClassHashCode = context.hashCode() val msg = "SET SESSION: Current activity: $activityClassName hashcode " + - "$activityClassHashCode Other activity: $otherActivityClassName " + - "hashcode $otherActivityClassHashcode" + "$activityClassHashCode Other activity: $otherActivityClassName " + + "hashcode $otherActivityClassHashcode" throw IllegalStateException(msg, e) } } @@ -150,40 +123,13 @@ class GeckoEngineView @JvmOverloads constructor( } } - /** - * Rebinds the current session to this view. This may be required after the session crashed and - * the view needs to notified about a new underlying GeckoSession (created under the hood by - * GeckoEngineSession) - since the previous one is no longer usable. - */ - private fun rebind() { - try { - currentSession?.let { currentGeckoView.setSession(it.geckoSession) } - } catch (e: IllegalArgumentException) { - // This is to debug "Display not attached" crashes - val otherActivityClassName = - currentSession?.geckoSession?.accessibility?.view?.context?.javaClass?.simpleName - val otherActivityClassHashcode = - currentSession?.geckoSession?.accessibility?.view?.context?.hashCode() - val activityClassName = context.javaClass.simpleName - val activityClassHashCode = context.hashCode() - val msg = "REBIND: Current activity: $activityClassName hashcode " + - "$activityClassHashCode Other activity: $otherActivityClassName " + - "hashcode $otherActivityClassHashcode" - throw IllegalArgumentException(msg, e) - } - } - @Synchronized override fun release() { detachSelectionActionDelegate(currentSession?.geckoSession) - currentSession?.apply { - unregister(observer) - } - currentSession = null - currentGeckoView.releaseSession() + geckoView.releaseSession() } override fun onDetachedFromWindow() { @@ -201,21 +147,21 @@ class GeckoEngineView @JvmOverloads constructor( override fun getInputResult(): EngineView.InputResult { // Direct mapping of GeckoView's returned values. // There should be a 1-1 relation. If not fail fast to allow for a quick fix. - return EngineView.InputResult.values().first { it.value == currentGeckoView.inputResult } + return EngineView.InputResult.values().first { it.value == geckoView.inputResult } } override fun setVerticalClipping(clippingHeight: Int) { - currentGeckoView.setVerticalClipping(clippingHeight) + geckoView.setVerticalClipping(clippingHeight) } override fun setDynamicToolbarMaxHeight(height: Int) { - currentGeckoView.setDynamicToolbarMaxHeight(height) + geckoView.setDynamicToolbarMaxHeight(height) } @Suppress("TooGenericExceptionCaught") override fun captureThumbnail(onFinish: (Bitmap?) -> Unit) { try { - val geckoResult = currentGeckoView.capturePixels() + val geckoResult = geckoView.capturePixels() geckoResult.then({ bitmap -> onFinish(bitmap) GeckoResult() @@ -243,7 +189,7 @@ class GeckoEngineView @JvmOverloads constructor( // https://bugzilla.mozilla.org/show_bug.cgi?id=1630775 // We do this to prevent the content from resizing when the view is not visible: // https://github.com/mozilla-mobile/android-components/issues/6664 - currentGeckoView.visibility = visibility + geckoView.visibility = visibility super.setVisibility(visibility) } } diff --git a/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt b/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt index 11fb49c3143..07252026740 100644 --- a/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt +++ b/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt @@ -80,6 +80,7 @@ import org.mozilla.geckoview.WebRequestError.ERROR_MALFORMED_URI import org.mozilla.geckoview.WebRequestError.ERROR_UNKNOWN import java.security.Principal import java.security.cert.X509Certificate + typealias GeckoAntiTracking = ContentBlocking.AntiTracking typealias GeckoSafeBrowsing = ContentBlocking.SafeBrowsing typealias GeckoCookieBehavior = ContentBlocking.CookieBehavior @@ -1934,19 +1935,6 @@ class GeckoEngineSessionTest { verifyZeroInteractions(observer) } - @Test - fun `after onCrash get called geckoSession must be reset`() { - val engineSession = GeckoEngineSession(runtime) - val oldGeckoSession = engineSession.geckoSession - - assertTrue(engineSession.geckoSession.isOpen) - - oldGeckoSession.contentDelegate!!.onCrash(mock()) - - assertFalse(oldGeckoSession.isOpen) - assertTrue(engineSession.geckoSession != oldGeckoSession) - } - @Test fun `Closing engine session should close underlying gecko session`() { val geckoSession = mockGeckoSession() @@ -2157,25 +2145,6 @@ class GeckoEngineSessionTest { assertEquals(false, observedTriggeredByWebContent) } - @Test - fun `State provided through delegate will be returned from saveState`() { - val engineSession = GeckoEngineSession(mock(), - geckoSessionProvider = geckoSessionProvider) - - captureDelegates() - - val state: GeckoSession.SessionState = mock() - - progressDelegate.value.onSessionStateChange(mock(), state) - - val savedState = engineSession.saveState() - assertNotNull(savedState) - assertTrue(savedState is GeckoEngineSessionState) - - val actualState = (savedState as GeckoEngineSessionState).actualState - assertEquals(state, actualState) - } - @Test fun `onFirstContentfulPaint notifies observers`() { val engineSession = GeckoEngineSession(mock(), @@ -2234,52 +2203,6 @@ class GeckoEngineSessionTest { assertEquals(true, crashedState) } - @Test - fun `recoverFromCrash does not restore state if no state has been saved previously`() { - val engineSession = GeckoEngineSession(mock(), - geckoSessionProvider = geckoSessionProvider) - - assertFalse(engineSession.recoverFromCrash()) - verify(engineSession.geckoSession, never()).restoreState(any()) - } - - @Test - fun `recoverFromCrash restores last known state`() { - val engineSession = GeckoEngineSession(mock(), - geckoSessionProvider = geckoSessionProvider) - - captureDelegates() - - val state1: GeckoSession.SessionState = mock() - val state2: GeckoSession.SessionState = mock() - - progressDelegate.value.onSessionStateChange(engineSession.geckoSession, state1) - progressDelegate.value.onSessionStateChange(engineSession.geckoSession, state2) - - contentDelegate.value.onCrash(engineSession.geckoSession) - - assertTrue(engineSession.recoverFromCrash()) - - verify(engineSession.geckoSession).restoreState(state2) - } - - @Test - fun `recoverFromCrash does not restore last known state if no crash occurred`() { - val engineSession = GeckoEngineSession(mock(), - geckoSessionProvider = geckoSessionProvider) - - captureDelegates() - - val state: GeckoSession.SessionState = mock() - - progressDelegate.value.onSessionStateChange(engineSession.geckoSession, state) - - assertFalse(engineSession.recoverFromCrash()) - - verify(engineSession.geckoSession, never()).restoreState(state) - verify(engineSession.geckoSession, never()).restoreState(any()) - } - @Test fun `onLoadRequest will notify onLaunchIntent observers if request was intercepted with app intent`() { val engineSession = GeckoEngineSession(mock(), @@ -2690,7 +2613,7 @@ class GeckoEngineSessionTest { } @Test - fun `onKill will recover, restore state and notify observers`() { + fun `onKill will notify observers`() { val engineSession = GeckoEngineSession(mock(), geckoSessionProvider = geckoSessionProvider) @@ -2707,11 +2630,8 @@ class GeckoEngineSessionTest { val mockedState: GeckoSession.SessionState = mock() progressDelegate.value.onSessionStateChange(geckoSession, mockedState) - verify(geckoSession, never()).restoreState(mockedState) - contentDelegate.value.onKill(geckoSession) - verify(geckoSession).restoreState(mockedState) assertTrue(observerNotified) } diff --git a/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt b/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt index 70242a10948..03c9949ccc4 100644 --- a/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt +++ b/components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt @@ -23,7 +23,6 @@ import org.junit.Assert.assertTrue import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mockito.never -import org.mockito.Mockito.reset import org.mockito.Mockito.times import org.mockito.Mockito.verify import org.mozilla.gecko.util.GeckoBundle @@ -47,7 +46,7 @@ class GeckoEngineViewTest { val geckoView = mock() whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView + engineView.geckoView = geckoView engineView.render(engineSession) verify(geckoView, times(1)).setSession(geckoSession) @@ -65,7 +64,7 @@ class GeckoEngineViewTest { var geckoResult = GeckoResult() whenever(mockGeckoView.capturePixels()).thenReturn(geckoResult) - engineView.currentGeckoView = mockGeckoView + engineView.geckoView = mockGeckoView // Test GeckoResult resolves successfuly engineView.captureThumbnail { @@ -98,7 +97,7 @@ class GeckoEngineViewTest { @Test fun `clearSelection is forwarded to BasicSelectionAction instance`() { val engineView = GeckoEngineView(context) - engineView.currentGeckoView = mock() + engineView.geckoView = mock() engineView.currentSelection = mock() engineView.clearSelection() @@ -109,21 +108,21 @@ class GeckoEngineViewTest { @Test fun `setVerticalClipping is forwarded to GeckoView instance`() { val engineView = GeckoEngineView(context) - engineView.currentGeckoView = mock() + engineView.geckoView = mock() engineView.setVerticalClipping(-42) - verify(engineView.currentGeckoView).setVerticalClipping(-42) + verify(engineView.geckoView).setVerticalClipping(-42) } @Test fun `setDynamicToolbarMaxHeight is forwarded to GeckoView instance`() { val engineView = GeckoEngineView(context) - engineView.currentGeckoView = mock() + engineView.geckoView = mock() engineView.setDynamicToolbarMaxHeight(42) - verify(engineView.currentGeckoView).setDynamicToolbarMaxHeight(42) + verify(engineView.geckoView).setDynamicToolbarMaxHeight(42) } @Test @@ -134,7 +133,7 @@ class GeckoEngineViewTest { val geckoView = mock() whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView + engineView.geckoView = geckoView engineView.render(engineSession) @@ -144,47 +143,6 @@ class GeckoEngineViewTest { engineView.release() verify(geckoView).releaseSession() - verify(engineSession).unregister(any()) - } - - @Test - fun `View will rebind session if process gets killed`() { - val engineView = GeckoEngineView(context) - val engineSession = mock() - val geckoSession = mock() - val geckoView = mock() - - whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView - - engineView.render(engineSession) - - reset(geckoView) - verify(geckoView, never()).setSession(geckoSession) - - engineView.observer.onProcessKilled() - - verify(geckoView).setSession(geckoSession) - } - - @Test - fun `View will rebind session if session crashed`() { - val engineView = GeckoEngineView(context) - val engineSession = mock() - val geckoSession = mock() - val geckoView = mock() - - whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView - - engineView.render(engineSession) - - reset(geckoView) - verify(geckoView, never()).setSession(geckoSession) - - engineView.observer.onCrash() - - verify(geckoView).setSession(geckoSession) } @Test @@ -197,7 +155,7 @@ class GeckoEngineViewTest { val geckoView = mock() whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView + engineView.geckoView = geckoView engineView.render(engineSession) @@ -216,7 +174,7 @@ class GeckoEngineViewTest { val geckoView = mock() whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView + engineView.geckoView = geckoView engineView.render(engineSession) @@ -247,7 +205,7 @@ class GeckoEngineViewTest { val geckoView = mock() whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView + engineView.geckoView = geckoView engineView.render(engineSession) @@ -273,16 +231,16 @@ class GeckoEngineViewTest { @Test fun `setVisibility is propagated to gecko view`() { val engineView = GeckoEngineView(context) - engineView.currentGeckoView = mock() + engineView.geckoView = mock() engineView.visibility = View.GONE - verify(engineView.currentGeckoView)?.visibility = View.GONE + verify(engineView.geckoView)?.visibility = View.GONE } @Test fun `canClearSelection should return false for null selection, null and empty selection text`() { val engineView = GeckoEngineView(context) - engineView.currentGeckoView = mock() + engineView.geckoView = mock() engineView.currentSelection = mock() // null selection returns false diff --git a/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt b/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt index 11abf85e87b..835a23ec188 100644 --- a/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt +++ b/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt @@ -77,8 +77,6 @@ class GeckoEngineSession( internal var scrollY: Int = 0 internal var job: Job = Job() - private var lastSessionState: GeckoSession.SessionState? = null - private var stateBeforeCrash: GeckoSession.SessionState? = null private var canGoBack: Boolean = false /** @@ -189,13 +187,6 @@ class GeckoEngineSession( geckoSession.gotoHistoryIndex(index) } - /** - * See [EngineSession.saveState] - */ - override fun saveState(): EngineSessionState { - return GeckoEngineSessionState(lastSessionState) - } - /** * See [EngineSession.restoreState] */ @@ -371,22 +362,6 @@ class GeckoEngineSession( geckoSession.exitFullScreen() } - /** - * See [EngineSession.recoverFromCrash] - */ - @Synchronized - override fun recoverFromCrash(): Boolean { - val state = stateBeforeCrash - - return if (state != null) { - geckoSession.restoreState(state) - stateBeforeCrash = null - true - } else { - false - } - } - /** * See [EngineSession.markActiveForWebExtensions]. */ @@ -601,7 +576,9 @@ class GeckoEngineSession( } override fun onSessionStateChange(session: GeckoSession, sessionState: GeckoSession.SessionState) { - lastSessionState = sessionState + notifyObservers { + onState(GeckoEngineSessionState(sessionState)) + } } } @@ -726,24 +703,13 @@ class GeckoEngineSession( } override fun onCrash(session: GeckoSession) { - stateBeforeCrash = lastSessionState - - recoverGeckoSession() - notifyObservers { onCrash() } } override fun onKill(session: GeckoSession) { - // The content process of this session got killed (resources reclaimed by Android). - // Let's recover and restore the last known state. - - val state = lastSessionState - - recoverGeckoSession() - - state?.let { geckoSession.restoreState(it) } - - notifyObservers { onProcessKilled() } + notifyObservers { + onProcessKilled() + } } private fun recoverGeckoSession() { diff --git a/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt b/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt index c8041609bd4..41f80552f73 100644 --- a/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt +++ b/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt @@ -4,17 +4,16 @@ package mozilla.components.browser.engine.gecko +import android.annotation.SuppressLint import android.content.Context import android.graphics.Bitmap import android.util.AttributeSet -import android.view.View import android.widget.FrameLayout import androidx.annotation.VisibleForTesting import androidx.core.view.ViewCompat import mozilla.components.browser.engine.gecko.selection.GeckoSelectionActionDelegate import mozilla.components.concept.engine.EngineSession import mozilla.components.concept.engine.EngineView -import mozilla.components.concept.engine.permission.PermissionRequest import mozilla.components.concept.engine.selection.SelectionActionDelegate import org.mozilla.geckoview.BasicSelectionActionDelegate import org.mozilla.geckoview.GeckoResult @@ -23,6 +22,7 @@ import org.mozilla.geckoview.GeckoSession /** * Gecko-based EngineView implementation. */ +@SuppressLint("SetTextI18n") @Suppress("TooManyFunctions") class GeckoEngineView @JvmOverloads constructor( context: Context, @@ -30,7 +30,7 @@ class GeckoEngineView @JvmOverloads constructor( defStyleAttr: Int = 0 ) : FrameLayout(context, attrs, defStyleAttr), EngineView { @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) - internal var currentGeckoView = object : NestedGeckoView(context) { + internal var geckoView = object : NestedGeckoView(context) { override fun onAttachedToWindow() { try { @@ -63,24 +63,6 @@ class GeckoEngineView @JvmOverloads constructor( ViewCompat.setImportantForAutofill(this, ViewCompat.IMPORTANT_FOR_ACCESSIBILITY_YES) } - @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) - internal val observer = object : EngineSession.Observer { - override fun onCrash() { - rebind() - } - - override fun onProcessKilled() { - rebind() - } - - override fun onFirstContentfulPaint() { - visibility = View.VISIBLE - } - - override fun onAppPermissionRequest(permissionRequest: PermissionRequest) = Unit - override fun onContentPermissionRequest(permissionRequest: PermissionRequest) = Unit - } - @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) internal var currentSession: GeckoEngineSession? = null @@ -90,9 +72,7 @@ class GeckoEngineView @JvmOverloads constructor( override var selectionActionDelegate: SelectionActionDelegate? = null init { - // Currently this is just a FrameLayout with a single GeckoView instance. Eventually this - // implementation should handle at least two GeckoView so that we can switch between - addView(currentGeckoView) + addView(geckoView) } /** @@ -101,23 +81,18 @@ class GeckoEngineView @JvmOverloads constructor( @Synchronized override fun render(session: EngineSession) { val internalSession = session as GeckoEngineSession + currentSession = session - currentSession?.apply { unregister(observer) } - - currentSession = session.apply { - register(observer) - } - - if (currentGeckoView.session != internalSession.geckoSession) { - currentGeckoView.session?.let { + if (geckoView.session != internalSession.geckoSession) { + geckoView.session?.let { // Release a previously assigned session. Otherwise GeckoView will close it // automatically. detachSelectionActionDelegate(it) - currentGeckoView.releaseSession() + geckoView.releaseSession() } try { - currentGeckoView.setSession(internalSession.geckoSession) + geckoView.setSession(internalSession.geckoSession) attachSelectionActionDelegate(internalSession.geckoSession) } catch (e: IllegalStateException) { // This is to debug "display already acquired" crashes @@ -150,40 +125,13 @@ class GeckoEngineView @JvmOverloads constructor( } } - /** - * Rebinds the current session to this view. This may be required after the session crashed and - * the view needs to notified about a new underlying GeckoSession (created under the hood by - * GeckoEngineSession) - since the previous one is no longer usable. - */ - private fun rebind() { - try { - currentSession?.let { currentGeckoView.setSession(it.geckoSession) } - } catch (e: IllegalArgumentException) { - // This is to debug "Display not attached" crashes - val otherActivityClassName = - currentSession?.geckoSession?.accessibility?.view?.context?.javaClass?.simpleName - val otherActivityClassHashcode = - currentSession?.geckoSession?.accessibility?.view?.context?.hashCode() - val activityClassName = context.javaClass.simpleName - val activityClassHashCode = context.hashCode() - val msg = "REBIND: Current activity: $activityClassName hashcode " + - "$activityClassHashCode Other activity: $otherActivityClassName " + - "hashcode $otherActivityClassHashcode" - throw IllegalArgumentException(msg, e) - } - } - @Synchronized override fun release() { detachSelectionActionDelegate(currentSession?.geckoSession) - currentSession?.apply { - unregister(observer) - } - currentSession = null - currentGeckoView.releaseSession() + geckoView.releaseSession() } override fun onDetachedFromWindow() { @@ -201,21 +149,21 @@ class GeckoEngineView @JvmOverloads constructor( override fun getInputResult(): EngineView.InputResult { // Direct mapping of GeckoView's returned values. // There should be a 1-1 relation. If not fail fast to allow for a quick fix. - return EngineView.InputResult.values().first { it.value == currentGeckoView.inputResult } + return EngineView.InputResult.values().first { it.value == geckoView.inputResult } } override fun setVerticalClipping(clippingHeight: Int) { - currentGeckoView.setVerticalClipping(clippingHeight) + geckoView.setVerticalClipping(clippingHeight) } override fun setDynamicToolbarMaxHeight(height: Int) { - currentGeckoView.setDynamicToolbarMaxHeight(height) + geckoView.setDynamicToolbarMaxHeight(height) } @Suppress("TooGenericExceptionCaught") override fun captureThumbnail(onFinish: (Bitmap?) -> Unit) { try { - val geckoResult = currentGeckoView.capturePixels() + val geckoResult = geckoView.capturePixels() geckoResult.then({ bitmap -> onFinish(bitmap) GeckoResult() @@ -243,7 +191,7 @@ class GeckoEngineView @JvmOverloads constructor( // https://bugzilla.mozilla.org/show_bug.cgi?id=1630775 // We do this to prevent the content from resizing when the view is not visible: // https://github.com/mozilla-mobile/android-components/issues/6664 - currentGeckoView.visibility = visibility + geckoView.visibility = visibility super.setVisibility(visibility) } } diff --git a/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt b/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt index 77b657f0f40..f425812cf3d 100644 --- a/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt +++ b/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt @@ -19,6 +19,7 @@ import mozilla.components.concept.engine.EngineSession.SafeBrowsingPolicy import mozilla.components.concept.engine.EngineSession.TrackingProtectionPolicy import mozilla.components.concept.engine.EngineSession.TrackingProtectionPolicy.CookiePolicy import mozilla.components.concept.engine.EngineSession.TrackingProtectionPolicy.TrackingCategory +import mozilla.components.concept.engine.EngineSessionState import mozilla.components.concept.engine.HitResult import mozilla.components.concept.engine.UnsupportedSettingException import mozilla.components.concept.engine.content.blocking.Tracker @@ -1936,19 +1937,6 @@ class GeckoEngineSessionTest { verifyZeroInteractions(observer) } - @Test - fun `after onCrash get called geckoSession must be reset`() { - val engineSession = GeckoEngineSession(runtime) - val oldGeckoSession = engineSession.geckoSession - - assertTrue(engineSession.geckoSession.isOpen) - - oldGeckoSession.contentDelegate!!.onCrash(mock()) - - assertFalse(oldGeckoSession.isOpen) - assertTrue(engineSession.geckoSession != oldGeckoSession) - } - @Test fun `Closing engine session should close underlying gecko session`() { val geckoSession = mockGeckoSession() @@ -2168,13 +2156,20 @@ class GeckoEngineSessionTest { val state: GeckoSession.SessionState = mock() + var observedState: EngineSessionState? = null + + engineSession.register(object : EngineSession.Observer { + override fun onState(state: EngineSessionState) { + observedState = state + } + }) + progressDelegate.value.onSessionStateChange(mock(), state) - val savedState = engineSession.saveState() - assertNotNull(savedState) - assertTrue(savedState is GeckoEngineSessionState) + assertNotNull(observedState) + assertTrue(observedState is GeckoEngineSessionState) - val actualState = (savedState as GeckoEngineSessionState).actualState + val actualState = (observedState as GeckoEngineSessionState).actualState assertEquals(state, actualState) } @@ -2236,52 +2231,6 @@ class GeckoEngineSessionTest { assertEquals(true, crashedState) } - @Test - fun `recoverFromCrash does not restore state if no state has been saved previously`() { - val engineSession = GeckoEngineSession(mock(), - geckoSessionProvider = geckoSessionProvider) - - assertFalse(engineSession.recoverFromCrash()) - verify(engineSession.geckoSession, never()).restoreState(any()) - } - - @Test - fun `recoverFromCrash restores last known state`() { - val engineSession = GeckoEngineSession(mock(), - geckoSessionProvider = geckoSessionProvider) - - captureDelegates() - - val state1: GeckoSession.SessionState = mock() - val state2: GeckoSession.SessionState = mock() - - progressDelegate.value.onSessionStateChange(engineSession.geckoSession, state1) - progressDelegate.value.onSessionStateChange(engineSession.geckoSession, state2) - - contentDelegate.value.onCrash(engineSession.geckoSession) - - assertTrue(engineSession.recoverFromCrash()) - - verify(engineSession.geckoSession).restoreState(state2) - } - - @Test - fun `recoverFromCrash does not restore last known state if no crash occurred`() { - val engineSession = GeckoEngineSession(mock(), - geckoSessionProvider = geckoSessionProvider) - - captureDelegates() - - val state: GeckoSession.SessionState = mock() - - progressDelegate.value.onSessionStateChange(engineSession.geckoSession, state) - - assertFalse(engineSession.recoverFromCrash()) - - verify(engineSession.geckoSession, never()).restoreState(state) - verify(engineSession.geckoSession, never()).restoreState(any()) - } - @Test fun `onLoadRequest will notify onLaunchIntent observers if request was intercepted with app intent`() { val engineSession = GeckoEngineSession(mock(), @@ -2692,7 +2641,7 @@ class GeckoEngineSessionTest { } @Test - fun `onKill will recover, restore state and notify observers`() { + fun `onKill will notify observers`() { val engineSession = GeckoEngineSession(mock(), geckoSessionProvider = geckoSessionProvider) @@ -2709,11 +2658,8 @@ class GeckoEngineSessionTest { val mockedState: GeckoSession.SessionState = mock() progressDelegate.value.onSessionStateChange(geckoSession, mockedState) - verify(geckoSession, never()).restoreState(mockedState) - contentDelegate.value.onKill(geckoSession) - verify(geckoSession).restoreState(mockedState) assertTrue(observerNotified) } diff --git a/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt b/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt index 70242a10948..34013761475 100644 --- a/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt +++ b/components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt @@ -11,7 +11,6 @@ import android.view.View import androidx.test.ext.junit.runners.AndroidJUnit4 import mozilla.components.browser.engine.gecko.selection.GeckoSelectionActionDelegate import mozilla.components.concept.engine.selection.SelectionActionDelegate -import mozilla.components.support.test.any import mozilla.components.support.test.argumentCaptor import mozilla.components.support.test.mock import mozilla.components.support.test.whenever @@ -23,7 +22,6 @@ import org.junit.Assert.assertTrue import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mockito.never -import org.mockito.Mockito.reset import org.mockito.Mockito.times import org.mockito.Mockito.verify import org.mozilla.gecko.util.GeckoBundle @@ -47,7 +45,7 @@ class GeckoEngineViewTest { val geckoView = mock() whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView + engineView.geckoView = geckoView engineView.render(engineSession) verify(geckoView, times(1)).setSession(geckoSession) @@ -65,7 +63,7 @@ class GeckoEngineViewTest { var geckoResult = GeckoResult() whenever(mockGeckoView.capturePixels()).thenReturn(geckoResult) - engineView.currentGeckoView = mockGeckoView + engineView.geckoView = mockGeckoView // Test GeckoResult resolves successfuly engineView.captureThumbnail { @@ -98,7 +96,7 @@ class GeckoEngineViewTest { @Test fun `clearSelection is forwarded to BasicSelectionAction instance`() { val engineView = GeckoEngineView(context) - engineView.currentGeckoView = mock() + engineView.geckoView = mock() engineView.currentSelection = mock() engineView.clearSelection() @@ -109,21 +107,21 @@ class GeckoEngineViewTest { @Test fun `setVerticalClipping is forwarded to GeckoView instance`() { val engineView = GeckoEngineView(context) - engineView.currentGeckoView = mock() + engineView.geckoView = mock() engineView.setVerticalClipping(-42) - verify(engineView.currentGeckoView).setVerticalClipping(-42) + verify(engineView.geckoView).setVerticalClipping(-42) } @Test fun `setDynamicToolbarMaxHeight is forwarded to GeckoView instance`() { val engineView = GeckoEngineView(context) - engineView.currentGeckoView = mock() + engineView.geckoView = mock() engineView.setDynamicToolbarMaxHeight(42) - verify(engineView.currentGeckoView).setDynamicToolbarMaxHeight(42) + verify(engineView.geckoView).setDynamicToolbarMaxHeight(42) } @Test @@ -134,57 +132,15 @@ class GeckoEngineViewTest { val geckoView = mock() whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView + engineView.geckoView = geckoView engineView.render(engineSession) verify(geckoView, never()).releaseSession() - verify(engineSession, never()).unregister(any()) engineView.release() verify(geckoView).releaseSession() - verify(engineSession).unregister(any()) - } - - @Test - fun `View will rebind session if process gets killed`() { - val engineView = GeckoEngineView(context) - val engineSession = mock() - val geckoSession = mock() - val geckoView = mock() - - whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView - - engineView.render(engineSession) - - reset(geckoView) - verify(geckoView, never()).setSession(geckoSession) - - engineView.observer.onProcessKilled() - - verify(geckoView).setSession(geckoSession) - } - - @Test - fun `View will rebind session if session crashed`() { - val engineView = GeckoEngineView(context) - val engineSession = mock() - val geckoSession = mock() - val geckoView = mock() - - whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView - - engineView.render(engineSession) - - reset(geckoView) - verify(geckoView, never()).setSession(geckoSession) - - engineView.observer.onCrash() - - verify(geckoView).setSession(geckoSession) } @Test @@ -197,7 +153,7 @@ class GeckoEngineViewTest { val geckoView = mock() whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView + engineView.geckoView = geckoView engineView.render(engineSession) @@ -216,7 +172,7 @@ class GeckoEngineViewTest { val geckoView = mock() whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView + engineView.geckoView = geckoView engineView.render(engineSession) @@ -247,7 +203,7 @@ class GeckoEngineViewTest { val geckoView = mock() whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView + engineView.geckoView = geckoView engineView.render(engineSession) @@ -273,16 +229,16 @@ class GeckoEngineViewTest { @Test fun `setVisibility is propagated to gecko view`() { val engineView = GeckoEngineView(context) - engineView.currentGeckoView = mock() + engineView.geckoView = mock() engineView.visibility = View.GONE - verify(engineView.currentGeckoView)?.visibility = View.GONE + verify(engineView.geckoView)?.visibility = View.GONE } @Test fun `canClearSelection should return false for null selection, null and empty selection text`() { val engineView = GeckoEngineView(context) - engineView.currentGeckoView = mock() + engineView.geckoView = mock() engineView.currentSelection = mock() // null selection returns false diff --git a/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt b/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt index 28273019d90..3593b1b4d22 100644 --- a/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt +++ b/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt @@ -77,8 +77,6 @@ class GeckoEngineSession( internal var scrollY: Int = 0 internal var job: Job = Job() - private var lastSessionState: GeckoSession.SessionState? = null - private var stateBeforeCrash: GeckoSession.SessionState? = null private var canGoBack: Boolean = false /** @@ -189,13 +187,6 @@ class GeckoEngineSession( geckoSession.gotoHistoryIndex(index) } - /** - * See [EngineSession.saveState] - */ - override fun saveState(): EngineSessionState { - return GeckoEngineSessionState(lastSessionState) - } - /** * See [EngineSession.restoreState] */ @@ -371,22 +362,6 @@ class GeckoEngineSession( geckoSession.exitFullScreen() } - /** - * See [EngineSession.recoverFromCrash] - */ - @Synchronized - override fun recoverFromCrash(): Boolean { - val state = stateBeforeCrash - - return if (state != null) { - geckoSession.restoreState(state) - stateBeforeCrash = null - true - } else { - false - } - } - /** * See [EngineSession.markActiveForWebExtensions]. */ @@ -601,7 +576,9 @@ class GeckoEngineSession( } override fun onSessionStateChange(session: GeckoSession, sessionState: GeckoSession.SessionState) { - lastSessionState = sessionState + notifyObservers { + onState(GeckoEngineSessionState(sessionState)) + } } } @@ -722,23 +699,10 @@ class GeckoEngineSession( } override fun onCrash(session: GeckoSession) { - stateBeforeCrash = lastSessionState - - recoverGeckoSession() - notifyObservers { onCrash() } } override fun onKill(session: GeckoSession) { - // The content process of this session got killed (resources reclaimed by Android). - // Let's recover and restore the last known state. - - val state = lastSessionState - - recoverGeckoSession() - - state?.let { geckoSession.restoreState(it) } - notifyObservers { onProcessKilled() } } diff --git a/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt b/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt index 27b5aa05799..c8a503d99d2 100644 --- a/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt +++ b/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt @@ -7,14 +7,12 @@ package mozilla.components.browser.engine.gecko import android.content.Context import android.graphics.Bitmap import android.util.AttributeSet -import android.view.View import android.widget.FrameLayout import androidx.annotation.VisibleForTesting import androidx.core.view.ViewCompat import mozilla.components.browser.engine.gecko.selection.GeckoSelectionActionDelegate import mozilla.components.concept.engine.EngineSession import mozilla.components.concept.engine.EngineView -import mozilla.components.concept.engine.permission.PermissionRequest import mozilla.components.concept.engine.selection.SelectionActionDelegate import org.mozilla.geckoview.BasicSelectionActionDelegate import org.mozilla.geckoview.GeckoResult @@ -30,7 +28,7 @@ class GeckoEngineView @JvmOverloads constructor( defStyleAttr: Int = 0 ) : FrameLayout(context, attrs, defStyleAttr), EngineView { @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) - internal var currentGeckoView = object : NestedGeckoView(context) { + internal var geckoView = object : NestedGeckoView(context) { override fun onAttachedToWindow() { try { @@ -38,14 +36,14 @@ class GeckoEngineView @JvmOverloads constructor( } catch (e: IllegalStateException) { // This is to debug "display already acquired" crashes val otherActivityClassName = - this.session?.accessibility?.view?.context?.javaClass?.simpleName + this.session?.accessibility?.view?.context?.javaClass?.simpleName val otherActivityClassHashcode = - this.session?.accessibility?.view?.context?.hashCode() + this.session?.accessibility?.view?.context?.hashCode() val activityClassName = context.javaClass.simpleName val activityClassHashCode = context.hashCode() val msg = "ATTACH VIEW: Current activity: $activityClassName hashcode " + - "$activityClassHashCode Other activity: $otherActivityClassName " + - "hashcode $otherActivityClassHashcode" + "$activityClassHashCode Other activity: $otherActivityClassName " + + "hashcode $otherActivityClassHashcode" throw IllegalStateException(msg, e) } } @@ -63,24 +61,6 @@ class GeckoEngineView @JvmOverloads constructor( ViewCompat.setImportantForAutofill(this, ViewCompat.IMPORTANT_FOR_ACCESSIBILITY_YES) } - @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) - internal val observer = object : EngineSession.Observer { - override fun onCrash() { - rebind() - } - - override fun onProcessKilled() { - rebind() - } - - override fun onFirstContentfulPaint() { - visibility = View.VISIBLE - } - - override fun onAppPermissionRequest(permissionRequest: PermissionRequest) = Unit - override fun onContentPermissionRequest(permissionRequest: PermissionRequest) = Unit - } - @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) internal var currentSession: GeckoEngineSession? = null @@ -90,9 +70,7 @@ class GeckoEngineView @JvmOverloads constructor( override var selectionActionDelegate: SelectionActionDelegate? = null init { - // Currently this is just a FrameLayout with a single GeckoView instance. Eventually this - // implementation should handle at least two GeckoView so that we can switch between - addView(currentGeckoView) + addView(geckoView) } /** @@ -101,23 +79,18 @@ class GeckoEngineView @JvmOverloads constructor( @Synchronized override fun render(session: EngineSession) { val internalSession = session as GeckoEngineSession + currentSession = session - currentSession?.apply { unregister(observer) } - - currentSession = session.apply { - register(observer) - } - - if (currentGeckoView.session != internalSession.geckoSession) { - currentGeckoView.session?.let { + if (geckoView.session != internalSession.geckoSession) { + geckoView.session?.let { // Release a previously assigned session. Otherwise GeckoView will close it // automatically. detachSelectionActionDelegate(it) - currentGeckoView.releaseSession() + geckoView.releaseSession() } try { - currentGeckoView.setSession(internalSession.geckoSession) + geckoView.setSession(internalSession.geckoSession) attachSelectionActionDelegate(internalSession.geckoSession) } catch (e: IllegalStateException) { // This is to debug "display already acquired" crashes @@ -128,8 +101,8 @@ class GeckoEngineView @JvmOverloads constructor( val activityClassName = context.javaClass.simpleName val activityClassHashCode = context.hashCode() val msg = "SET SESSION: Current activity: $activityClassName hashcode " + - "$activityClassHashCode Other activity: $otherActivityClassName " + - "hashcode $otherActivityClassHashcode" + "$activityClassHashCode Other activity: $otherActivityClassName " + + "hashcode $otherActivityClassHashcode" throw IllegalStateException(msg, e) } } @@ -150,26 +123,13 @@ class GeckoEngineView @JvmOverloads constructor( } } - /** - * Rebinds the current session to this view. This may be required after the session crashed and - * the view needs to notified about a new underlying GeckoSession (created under the hood by - * GeckoEngineSession) - since the previous one is no longer usable. - */ - private fun rebind() { - currentSession?.let { currentGeckoView.setSession(it.geckoSession) } - } - @Synchronized override fun release() { detachSelectionActionDelegate(currentSession?.geckoSession) - currentSession?.apply { - unregister(observer) - } - currentSession = null - currentGeckoView.releaseSession() + geckoView.releaseSession() } override fun onDetachedFromWindow() { @@ -187,21 +147,21 @@ class GeckoEngineView @JvmOverloads constructor( override fun getInputResult(): EngineView.InputResult { // Direct mapping of GeckoView's returned values. // There should be a 1-1 relation. If not fail fast to allow for a quick fix. - return EngineView.InputResult.values().first { it.value == currentGeckoView.inputResult } + return EngineView.InputResult.values().first { it.value == geckoView.inputResult } } override fun setVerticalClipping(clippingHeight: Int) { - currentGeckoView.setVerticalClipping(clippingHeight) + geckoView.setVerticalClipping(clippingHeight) } override fun setDynamicToolbarMaxHeight(height: Int) { - currentGeckoView.setDynamicToolbarMaxHeight(height) + geckoView.setDynamicToolbarMaxHeight(height) } @Suppress("TooGenericExceptionCaught") override fun captureThumbnail(onFinish: (Bitmap?) -> Unit) { try { - val geckoResult = currentGeckoView.capturePixels() + val geckoResult = geckoView.capturePixels() geckoResult.then({ bitmap -> onFinish(bitmap) GeckoResult() @@ -229,7 +189,7 @@ class GeckoEngineView @JvmOverloads constructor( // https://bugzilla.mozilla.org/show_bug.cgi?id=1630775 // We do this to prevent the content from resizing when the view is not visible: // https://github.com/mozilla-mobile/android-components/issues/6664 - currentGeckoView.visibility = visibility + geckoView.visibility = visibility super.setVisibility(visibility) } } diff --git a/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt b/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt index 67ebde5cdb1..1ec25d54b99 100644 --- a/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt +++ b/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt @@ -1933,19 +1933,6 @@ class GeckoEngineSessionTest { verifyZeroInteractions(observer) } - @Test - fun `after onCrash get called geckoSession must be reset`() { - val engineSession = GeckoEngineSession(runtime) - val oldGeckoSession = engineSession.geckoSession - - assertTrue(engineSession.geckoSession.isOpen) - - oldGeckoSession.contentDelegate!!.onCrash(mock()) - - assertFalse(oldGeckoSession.isOpen) - assertTrue(engineSession.geckoSession != oldGeckoSession) - } - @Test fun `Closing engine session should close underlying gecko session`() { val geckoSession = mockGeckoSession() @@ -2156,25 +2143,6 @@ class GeckoEngineSessionTest { assertEquals(false, observedTriggeredByWebContent) } - @Test - fun `State provided through delegate will be returned from saveState`() { - val engineSession = GeckoEngineSession(mock(), - geckoSessionProvider = geckoSessionProvider) - - captureDelegates() - - val state: GeckoSession.SessionState = mock() - - progressDelegate.value.onSessionStateChange(mock(), state) - - val savedState = engineSession.saveState() - assertNotNull(savedState) - assertTrue(savedState is GeckoEngineSessionState) - - val actualState = (savedState as GeckoEngineSessionState).actualState - assertEquals(state, actualState) - } - @Test fun `onFirstContentfulPaint notifies observers`() { val engineSession = GeckoEngineSession(mock(), @@ -2214,52 +2182,6 @@ class GeckoEngineSessionTest { assertEquals(true, crashedState) } - @Test - fun `recoverFromCrash does not restore state if no state has been saved previously`() { - val engineSession = GeckoEngineSession(mock(), - geckoSessionProvider = geckoSessionProvider) - - assertFalse(engineSession.recoverFromCrash()) - verify(engineSession.geckoSession, never()).restoreState(any()) - } - - @Test - fun `recoverFromCrash restores last known state`() { - val engineSession = GeckoEngineSession(mock(), - geckoSessionProvider = geckoSessionProvider) - - captureDelegates() - - val state1: GeckoSession.SessionState = mock() - val state2: GeckoSession.SessionState = mock() - - progressDelegate.value.onSessionStateChange(engineSession.geckoSession, state1) - progressDelegate.value.onSessionStateChange(engineSession.geckoSession, state2) - - contentDelegate.value.onCrash(engineSession.geckoSession) - - assertTrue(engineSession.recoverFromCrash()) - - verify(engineSession.geckoSession).restoreState(state2) - } - - @Test - fun `recoverFromCrash does not restore last known state if no crash occurred`() { - val engineSession = GeckoEngineSession(mock(), - geckoSessionProvider = geckoSessionProvider) - - captureDelegates() - - val state: GeckoSession.SessionState = mock() - - progressDelegate.value.onSessionStateChange(engineSession.geckoSession, state) - - assertFalse(engineSession.recoverFromCrash()) - - verify(engineSession.geckoSession, never()).restoreState(state) - verify(engineSession.geckoSession, never()).restoreState(any()) - } - @Test fun `onLoadRequest will notify onLaunchIntent observers if request was intercepted with app intent`() { val engineSession = GeckoEngineSession(mock(), @@ -2669,32 +2591,6 @@ class GeckoEngineSessionTest { assertEquals(LoadUrlFlags.LOAD_FLAGS_REPLACE_HISTORY, GeckoSession.LOAD_FLAGS_REPLACE_HISTORY) } - @Test - fun `onKill will recover, restore state and notify observers`() { - val engineSession = GeckoEngineSession(mock(), - geckoSessionProvider = geckoSessionProvider) - - captureDelegates() - - var observerNotified = false - - engineSession.register(object : EngineSession.Observer { - override fun onProcessKilled() { - observerNotified = true - } - }) - - val mockedState: GeckoSession.SessionState = mock() - progressDelegate.value.onSessionStateChange(geckoSession, mockedState) - - verify(geckoSession, never()).restoreState(mockedState) - - contentDelegate.value.onKill(geckoSession) - - verify(geckoSession).restoreState(mockedState) - assertTrue(observerNotified) - } - @Test fun `onNewSession creates window request`() { val engineSession = GeckoEngineSession(mock(), geckoSessionProvider = geckoSessionProvider) diff --git a/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt b/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt index 70242a10948..03c9949ccc4 100644 --- a/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt +++ b/components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineViewTest.kt @@ -23,7 +23,6 @@ import org.junit.Assert.assertTrue import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mockito.never -import org.mockito.Mockito.reset import org.mockito.Mockito.times import org.mockito.Mockito.verify import org.mozilla.gecko.util.GeckoBundle @@ -47,7 +46,7 @@ class GeckoEngineViewTest { val geckoView = mock() whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView + engineView.geckoView = geckoView engineView.render(engineSession) verify(geckoView, times(1)).setSession(geckoSession) @@ -65,7 +64,7 @@ class GeckoEngineViewTest { var geckoResult = GeckoResult() whenever(mockGeckoView.capturePixels()).thenReturn(geckoResult) - engineView.currentGeckoView = mockGeckoView + engineView.geckoView = mockGeckoView // Test GeckoResult resolves successfuly engineView.captureThumbnail { @@ -98,7 +97,7 @@ class GeckoEngineViewTest { @Test fun `clearSelection is forwarded to BasicSelectionAction instance`() { val engineView = GeckoEngineView(context) - engineView.currentGeckoView = mock() + engineView.geckoView = mock() engineView.currentSelection = mock() engineView.clearSelection() @@ -109,21 +108,21 @@ class GeckoEngineViewTest { @Test fun `setVerticalClipping is forwarded to GeckoView instance`() { val engineView = GeckoEngineView(context) - engineView.currentGeckoView = mock() + engineView.geckoView = mock() engineView.setVerticalClipping(-42) - verify(engineView.currentGeckoView).setVerticalClipping(-42) + verify(engineView.geckoView).setVerticalClipping(-42) } @Test fun `setDynamicToolbarMaxHeight is forwarded to GeckoView instance`() { val engineView = GeckoEngineView(context) - engineView.currentGeckoView = mock() + engineView.geckoView = mock() engineView.setDynamicToolbarMaxHeight(42) - verify(engineView.currentGeckoView).setDynamicToolbarMaxHeight(42) + verify(engineView.geckoView).setDynamicToolbarMaxHeight(42) } @Test @@ -134,7 +133,7 @@ class GeckoEngineViewTest { val geckoView = mock() whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView + engineView.geckoView = geckoView engineView.render(engineSession) @@ -144,47 +143,6 @@ class GeckoEngineViewTest { engineView.release() verify(geckoView).releaseSession() - verify(engineSession).unregister(any()) - } - - @Test - fun `View will rebind session if process gets killed`() { - val engineView = GeckoEngineView(context) - val engineSession = mock() - val geckoSession = mock() - val geckoView = mock() - - whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView - - engineView.render(engineSession) - - reset(geckoView) - verify(geckoView, never()).setSession(geckoSession) - - engineView.observer.onProcessKilled() - - verify(geckoView).setSession(geckoSession) - } - - @Test - fun `View will rebind session if session crashed`() { - val engineView = GeckoEngineView(context) - val engineSession = mock() - val geckoSession = mock() - val geckoView = mock() - - whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView - - engineView.render(engineSession) - - reset(geckoView) - verify(geckoView, never()).setSession(geckoSession) - - engineView.observer.onCrash() - - verify(geckoView).setSession(geckoSession) } @Test @@ -197,7 +155,7 @@ class GeckoEngineViewTest { val geckoView = mock() whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView + engineView.geckoView = geckoView engineView.render(engineSession) @@ -216,7 +174,7 @@ class GeckoEngineViewTest { val geckoView = mock() whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView + engineView.geckoView = geckoView engineView.render(engineSession) @@ -247,7 +205,7 @@ class GeckoEngineViewTest { val geckoView = mock() whenever(engineSession.geckoSession).thenReturn(geckoSession) - engineView.currentGeckoView = geckoView + engineView.geckoView = geckoView engineView.render(engineSession) @@ -273,16 +231,16 @@ class GeckoEngineViewTest { @Test fun `setVisibility is propagated to gecko view`() { val engineView = GeckoEngineView(context) - engineView.currentGeckoView = mock() + engineView.geckoView = mock() engineView.visibility = View.GONE - verify(engineView.currentGeckoView)?.visibility = View.GONE + verify(engineView.geckoView)?.visibility = View.GONE } @Test fun `canClearSelection should return false for null selection, null and empty selection text`() { val engineView = GeckoEngineView(context) - engineView.currentGeckoView = mock() + engineView.geckoView = mock() engineView.currentSelection = mock() // null selection returns false diff --git a/components/browser/engine-system/src/main/java/mozilla/components/browser/engine/system/SystemEngineSession.kt b/components/browser/engine-system/src/main/java/mozilla/components/browser/engine/system/SystemEngineSession.kt index ce4b60ee9a6..f0f3af9433a 100644 --- a/components/browser/engine-system/src/main/java/mozilla/components/browser/engine/system/SystemEngineSession.kt +++ b/components/browser/engine-system/src/main/java/mozilla/components/browser/engine/system/SystemEngineSession.kt @@ -5,7 +5,6 @@ package mozilla.components.browser.engine.system import android.content.Context -import android.os.Bundle import android.webkit.CookieManager import android.webkit.WebChromeClient import android.webkit.WebSettings @@ -16,11 +15,9 @@ import android.webkit.WebViewDatabase import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch -import kotlinx.coroutines.runBlocking import mozilla.components.browser.errorpages.ErrorType import mozilla.components.concept.engine.Engine.BrowsingData import mozilla.components.concept.engine.EngineSession -import mozilla.components.concept.engine.EngineSession.LoadUrlFlags import mozilla.components.concept.engine.EngineSessionState import mozilla.components.concept.engine.Settings import mozilla.components.concept.engine.history.HistoryTrackingDelegate @@ -134,18 +131,6 @@ class SystemEngineSession( webView.goBackOrForward(index - historyList.currentIndex) } - /** - * See [EngineSession.saveState] - */ - override fun saveState(): EngineSessionState { - return runBlocking(Dispatchers.Main) { - val state = Bundle() - webView.saveState(state) - - SystemEngineSessionState(state) - } - } - /** * See [EngineSession.restoreState] */ @@ -247,16 +232,6 @@ class SystemEngineSession( webView.clearMatches() } - /** - * This method is a no-op. - */ - override fun recoverFromCrash(): Boolean { - // Do nothing. - // Technically we could remember saved states and restore the last one we saw. But for that to be useful we - // would need to implement and handle onRenderProcessGone() first. - return false - } - /** * See [EngineSession.settings] */ diff --git a/components/browser/engine-system/src/test/java/mozilla/components/browser/engine/system/SystemEngineSessionTest.kt b/components/browser/engine-system/src/test/java/mozilla/components/browser/engine/system/SystemEngineSessionTest.kt index 12cb7fb769c..385aa6c6326 100644 --- a/components/browser/engine-system/src/test/java/mozilla/components/browser/engine/system/SystemEngineSessionTest.kt +++ b/components/browser/engine-system/src/test/java/mozilla/components/browser/engine/system/SystemEngineSessionTest.kt @@ -7,8 +7,6 @@ package mozilla.components.browser.engine.system import android.content.Context import android.net.Uri import android.os.Bundle -import android.os.Looper -import android.webkit.WebBackForwardList import android.webkit.WebChromeClient import android.webkit.WebResourceRequest import android.webkit.WebSettings @@ -193,39 +191,6 @@ class SystemEngineSessionTest { verify(webView).goBackOrForward(0) } - @Test - fun saveState() { - val engineSession = spy(SystemEngineSession(testContext)) - val webView = mock() - - engineSession.saveState() - verify(webView, never()).saveState(any(Bundle::class.java)) - - engineSession.webView = webView - - engineSession.saveState() - verify(webView).saveState(any(Bundle::class.java)) - } - - @Test - fun saveStateRunsOnMainThread() { - val engineSession = spy(SystemEngineSession(testContext)) - var calledOnMainThread = false - var saveStateCalled = false - val webView = object : WebView(testContext) { - override fun saveState(outState: Bundle?): WebBackForwardList? { - saveStateCalled = true - calledOnMainThread = Looper.getMainLooper().isCurrentThread - return null - } - } - engineSession.webView = webView - - engineSession.saveState() - assertTrue(saveStateCalled) - assertTrue(calledOnMainThread) - } - @Test fun restoreState() { val engineSession = spy(SystemEngineSession(testContext)) @@ -240,10 +205,13 @@ class SystemEngineSessionTest { engineSession.webView = webView engineSession.webView.loadUrl("http://example.com") - val state = engineSession.saveState() + + val bundle = Bundle() + webView.saveState(bundle) + val state = SystemEngineSessionState(bundle) assertTrue(engineSession.restoreState(state)) - verify(webView).restoreState(any(Bundle::class.java)) + verify(webView).restoreState(bundle) } @Test @@ -989,17 +957,6 @@ class SystemEngineSessionTest { verify(webView).destroy() } - @Test - fun `recoverFromCrash does not restore state`() { - val engineSession = SystemEngineSession(testContext) - val webView = mock() - engineSession.webView = webView - - assertFalse(engineSession.recoverFromCrash()) - - verify(webView, never()).restoreState(any()) - } - @Test fun `GIVEN webView_canGoBack() true WHEN goBack() is called THEN verify EngineObserver onNavigateBack() is triggered`() { var observedOnNavigateBack = false diff --git a/components/browser/session/src/main/java/mozilla/components/browser/session/engine/EngineMiddleware.kt b/components/browser/session/src/main/java/mozilla/components/browser/session/engine/EngineMiddleware.kt index 03853799023..1f435a3a285 100644 --- a/components/browser/session/src/main/java/mozilla/components/browser/session/engine/EngineMiddleware.kt +++ b/components/browser/session/src/main/java/mozilla/components/browser/session/engine/EngineMiddleware.kt @@ -59,16 +59,13 @@ object EngineMiddleware { WebExtensionMiddleware(), TrimMemoryMiddleware(), LastAccessMiddleware(), - CrashMiddleware( - engine, - sessionLookup, - scope - ) + CrashMiddleware() ) } } @MainThread +@Suppress("ReturnCount") internal fun getOrCreateEngineSession( engine: Engine, logger: Logger, @@ -82,8 +79,13 @@ internal fun getOrCreateEngineSession( return null } + if (tab.engineState.crashed) { + logger.warn("Not creating engine session, since tab is crashed. Waiting for restore.") + return null + } + tab.engineState.engineSession?.let { - logger.debug("Engine Session already exists for tab $tabId") + logger.debug("Engine session already exists for tab $tabId") return it } diff --git a/components/browser/session/src/main/java/mozilla/components/browser/session/engine/EngineObserver.kt b/components/browser/session/src/main/java/mozilla/components/browser/session/engine/EngineObserver.kt index 1794afa1426..409331a0901 100644 --- a/components/browser/session/src/main/java/mozilla/components/browser/session/engine/EngineObserver.kt +++ b/components/browser/session/src/main/java/mozilla/components/browser/session/engine/EngineObserver.kt @@ -17,6 +17,7 @@ import mozilla.components.browser.session.ext.toElement import mozilla.components.browser.state.action.BrowserAction import mozilla.components.browser.state.action.ContentAction import mozilla.components.browser.state.action.CrashAction +import mozilla.components.browser.state.action.EngineAction import mozilla.components.browser.state.action.MediaAction import mozilla.components.browser.state.action.TrackingProtectionAction import mozilla.components.browser.state.state.BrowserState @@ -24,6 +25,7 @@ import mozilla.components.browser.state.state.content.DownloadState import mozilla.components.browser.state.state.content.DownloadState.Status.INITIATED import mozilla.components.browser.state.state.content.FindResultState import mozilla.components.concept.engine.EngineSession +import mozilla.components.concept.engine.EngineSessionState import mozilla.components.concept.engine.HitResult import mozilla.components.concept.engine.content.blocking.Tracker import mozilla.components.concept.engine.history.HistoryItem @@ -311,6 +313,18 @@ internal class EngineObserver( )) } + override fun onProcessKilled() { + store?.dispatch(EngineAction.SuspendEngineSessionAction( + session.id + )) + } + + override fun onState(state: EngineSessionState) { + store?.dispatch(EngineAction.UpdateEngineSessionStateAction( + session.id, state + )) + } + override fun onRecordingStateChanged(devices: List) { session.recordingDevices = devices } diff --git a/components/browser/session/src/main/java/mozilla/components/browser/session/engine/middleware/CrashMiddleware.kt b/components/browser/session/src/main/java/mozilla/components/browser/session/engine/middleware/CrashMiddleware.kt index 03aed3ca945..27ac4d16051 100644 --- a/components/browser/session/src/main/java/mozilla/components/browser/session/engine/middleware/CrashMiddleware.kt +++ b/components/browser/session/src/main/java/mozilla/components/browser/session/engine/middleware/CrashMiddleware.kt @@ -4,55 +4,39 @@ package mozilla.components.browser.session.engine.middleware -import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.launch -import mozilla.components.browser.session.Session -import mozilla.components.browser.session.engine.getOrCreateEngineSession import mozilla.components.browser.state.action.BrowserAction import mozilla.components.browser.state.action.CrashAction -import mozilla.components.browser.state.selector.findTabOrCustomTab +import mozilla.components.browser.state.action.EngineAction import mozilla.components.browser.state.state.BrowserState -import mozilla.components.concept.engine.Engine import mozilla.components.concept.engine.EngineSession import mozilla.components.lib.state.Middleware import mozilla.components.lib.state.MiddlewareContext -import mozilla.components.lib.state.Store -import mozilla.components.support.base.log.logger.Logger /** * [Middleware] responsible for recovering crashed [EngineSession] instances. */ -internal class CrashMiddleware( - private val engine: Engine, - private val sessionLookup: (String) -> Session?, - private val scope: CoroutineScope -) : Middleware { - private val logger = Logger("CrashMiddleware") - +internal class CrashMiddleware : Middleware { override fun invoke( context: MiddlewareContext, next: (BrowserAction) -> Unit, action: BrowserAction ) { - if (action is CrashAction.RestoreCrashedSessionAction) { - restore(context.store, action) + if (action is CrashAction.SessionCrashedAction) { + onCrash(context, action) } next(action) } - private fun restore( - store: Store, - action: CrashAction.RestoreCrashedSessionAction - ) = scope.launch { - val tab = store.state.findTabOrCustomTab(action.tabId) ?: return@launch - - // Currently we are forcing the creation of an engine session here. This is mimicing - // the previous behavior. But it is questionable if this is the right approach: - // - How did this tab crash if it does not have an engine session? - // - Instead of creating the engine session, could we turn it into a suspended - // session with the "crash state" as the last state? - val engineSession = getOrCreateEngineSession(engine, logger, sessionLookup, store, tab.id) - engineSession?.recoverFromCrash() + private fun onCrash( + context: MiddlewareContext, + action: CrashAction.SessionCrashedAction + ) { + // We suspend the crashed session here. After that the reducer will mark it as "crashed". + // That will prevent it from getting recreated until explicitly handling the crash by + // restoring. + context.dispatch( + EngineAction.SuspendEngineSessionAction(action.tabId) + ) } } diff --git a/components/browser/session/src/main/java/mozilla/components/browser/session/engine/middleware/SuspendMiddleware.kt b/components/browser/session/src/main/java/mozilla/components/browser/session/engine/middleware/SuspendMiddleware.kt index ff011ca3b9c..4fcb245fb57 100644 --- a/components/browser/session/src/main/java/mozilla/components/browser/session/engine/middleware/SuspendMiddleware.kt +++ b/components/browser/session/src/main/java/mozilla/components/browser/session/engine/middleware/SuspendMiddleware.kt @@ -43,26 +43,13 @@ internal class SuspendMiddleware( context: MiddlewareContext, action: EngineAction.SuspendEngineSessionAction ) { - val tab = context.state.findTab(action.sessionId) - val state = tab?.engineState?.engineSession?.saveState() - - if (tab == null || state == null) { - // If we can't find this tab or if there's no state for this tab then there's nothing - // to do here. - return - } + val tab = context.state.findTab(action.sessionId) ?: return // First we unlink (which clearsEngineSession and state) context.dispatch(EngineAction.UnlinkEngineSessionAction( tab.id )) - // Then we attach the saved state to it. - context.dispatch(EngineAction.UpdateEngineSessionStateAction( - tab.id, - state - )) - // Now we can close the unlinked EngineSession (on the main thread). scope.launch { tab.engineState.engineSession?.close() diff --git a/components/browser/session/src/main/java/mozilla/components/browser/session/storage/BrowserStateSerializer.kt b/components/browser/session/src/main/java/mozilla/components/browser/session/storage/BrowserStateSerializer.kt index 690f7eea355..c0e9b92f2d9 100644 --- a/components/browser/session/src/main/java/mozilla/components/browser/session/storage/BrowserStateSerializer.kt +++ b/components/browser/session/src/main/java/mozilla/components/browser/session/storage/BrowserStateSerializer.kt @@ -58,7 +58,7 @@ class BrowserStateSerializer { val engineSessionState = tab.engineState.engineSessionState val engineSessionStateJson = engineSessionState?.toJSON() - ?: (tab.engineState.engineSession?.saveState()?.toJSON() + ?: (tab.engineState.engineSessionState?.toJSON() ?: JSONObject()) itemJson.put(Keys.ENGINE_SESSION_KEY, engineSessionStateJson) diff --git a/components/browser/session/src/main/java/mozilla/components/browser/session/storage/SnapshotSerializer.kt b/components/browser/session/src/main/java/mozilla/components/browser/session/storage/SnapshotSerializer.kt index d49dce8f7bc..ad6680bd632 100644 --- a/components/browser/session/src/main/java/mozilla/components/browser/session/storage/SnapshotSerializer.kt +++ b/components/browser/session/src/main/java/mozilla/components/browser/session/storage/SnapshotSerializer.kt @@ -62,7 +62,7 @@ class SnapshotSerializer( val engineSessionState = if (item.engineSessionState != null) { item.engineSessionState.toJSON() } else { - item.engineSession?.saveState()?.toJSON() ?: JSONObject() + null } itemJson.put(Keys.ENGINE_SESSION_KEY, engineSessionState) return itemJson diff --git a/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/CrashReducer.kt b/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/CrashReducer.kt index ed9c299a589..62e52ca4c07 100644 --- a/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/CrashReducer.kt +++ b/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/CrashReducer.kt @@ -13,12 +13,16 @@ internal object CrashReducer { */ fun reduce(state: BrowserState, action: CrashAction): BrowserState = when (action) { is CrashAction.SessionCrashedAction -> state.updateTabState(action.tabId) { tab -> - tab.createCopy(crashed = true) + tab.createCopy(engineState = tab.engineState.copy( + crashed = true + )) } is CrashAction.RestoreCrashedSessionAction -> state.updateTabState(action.tabId) { tab -> - // We only update the flag in the reducer. A middleware is responsible for actually - // performing a restoring side effect. - tab.createCopy(crashed = false) + // We only update the flag in the reducer. With that the next action trying to get + // the engine session will automatically restore this tab lazily. + tab.createCopy(engineState = tab.engineState.copy( + crashed = false + )) } } } diff --git a/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/EngineStateReducer.kt b/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/EngineStateReducer.kt index 828f97e01c4..61d99b934a3 100644 --- a/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/EngineStateReducer.kt +++ b/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/EngineStateReducer.kt @@ -17,22 +17,26 @@ internal object EngineStateReducer { fun reduce(state: BrowserState, action: EngineAction): BrowserState = when (action) { is EngineAction.LinkEngineSessionAction -> state.copyWithEngineState(action.sessionId) { it.copy( - engineSession = action.engineSession, - engineSessionState = null + engineSession = action.engineSession ) } is EngineAction.UnlinkEngineSessionAction -> state.copyWithEngineState(action.sessionId) { it.copy( engineSession = null, - engineSessionState = null, engineObserver = null ) } is EngineAction.UpdateEngineSessionObserverAction -> state.copyWithEngineState(action.sessionId) { it.copy(engineObserver = action.engineSessionObserver) } - is EngineAction.UpdateEngineSessionStateAction -> state.copyWithEngineState(action.sessionId) { - it.copy(engineSessionState = action.engineSessionState) + is EngineAction.UpdateEngineSessionStateAction -> state.copyWithEngineState(action.sessionId) { engineState -> + if (engineState.crashed) { + // We ignore state updates for a crashed engine session. We want to keep the last state until + // this tab gets restored (or closed). + engineState + } else { + engineState.copy(engineSessionState = action.engineSessionState) + } } is EngineAction.SuspendEngineSessionAction, is EngineAction.CreateEngineSessionAction, diff --git a/components/browser/state/src/main/java/mozilla/components/browser/state/state/CustomTabSessionState.kt b/components/browser/state/src/main/java/mozilla/components/browser/state/state/CustomTabSessionState.kt index 1cf800a26a2..e2e86635fc1 100644 --- a/components/browser/state/src/main/java/mozilla/components/browser/state/state/CustomTabSessionState.kt +++ b/components/browser/state/src/main/java/mozilla/components/browser/state/state/CustomTabSessionState.kt @@ -25,8 +25,7 @@ data class CustomTabSessionState( override val engineState: EngineState = EngineState(), override val extensionState: Map = emptyMap(), override val contextId: String? = null, - override val source: SessionState.Source = SessionState.Source.CUSTOM_TAB, - override val crashed: Boolean = false + override val source: SessionState.Source = SessionState.Source.CUSTOM_TAB ) : SessionState { override fun createCopy( @@ -35,16 +34,14 @@ data class CustomTabSessionState( trackingProtection: TrackingProtectionState, engineState: EngineState, extensionState: Map, - contextId: String?, - crashed: Boolean + contextId: String? ) = copy( id = id, content = content, trackingProtection = trackingProtection, engineState = engineState, extensionState = extensionState, - contextId = contextId, - crashed = crashed + contextId = contextId ) } diff --git a/components/browser/state/src/main/java/mozilla/components/browser/state/state/EngineState.kt b/components/browser/state/src/main/java/mozilla/components/browser/state/state/EngineState.kt index fb7ff60e8f8..15e0bb8f232 100644 --- a/components/browser/state/src/main/java/mozilla/components/browser/state/state/EngineState.kt +++ b/components/browser/state/src/main/java/mozilla/components/browser/state/state/EngineState.kt @@ -21,5 +21,6 @@ import mozilla.components.concept.engine.EngineSessionState data class EngineState( val engineSession: EngineSession? = null, val engineSessionState: EngineSessionState? = null, - val engineObserver: EngineSession.Observer? = null + val engineObserver: EngineSession.Observer? = null, + val crashed: Boolean = false ) diff --git a/components/browser/state/src/main/java/mozilla/components/browser/state/state/SessionState.kt b/components/browser/state/src/main/java/mozilla/components/browser/state/state/SessionState.kt index f25bd3dd267..7d7ce0034fd 100644 --- a/components/browser/state/src/main/java/mozilla/components/browser/state/state/SessionState.kt +++ b/components/browser/state/src/main/java/mozilla/components/browser/state/state/SessionState.kt @@ -31,7 +31,6 @@ interface SessionState { val extensionState: Map val contextId: String? val source: Source - val crashed: Boolean /** * Copy the class and override some parameters. @@ -43,8 +42,7 @@ interface SessionState { trackingProtection: TrackingProtectionState = this.trackingProtection, engineState: EngineState = this.engineState, extensionState: Map = this.extensionState, - contextId: String? = this.contextId, - crashed: Boolean = this.crashed + contextId: String? = this.contextId ): SessionState /** diff --git a/components/browser/state/src/main/java/mozilla/components/browser/state/state/TabSessionState.kt b/components/browser/state/src/main/java/mozilla/components/browser/state/state/TabSessionState.kt index 4969f5ef84a..40d5a9ce022 100644 --- a/components/browser/state/src/main/java/mozilla/components/browser/state/state/TabSessionState.kt +++ b/components/browser/state/src/main/java/mozilla/components/browser/state/state/TabSessionState.kt @@ -30,7 +30,6 @@ data class TabSessionState( override val extensionState: Map = emptyMap(), override val contextId: String? = null, override val source: SessionState.Source = SessionState.Source.NONE, - override val crashed: Boolean = false, val parentId: String? = null, val lastAccess: Long = 0L, val readerState: ReaderState = ReaderState() @@ -42,16 +41,14 @@ data class TabSessionState( trackingProtection: TrackingProtectionState, engineState: EngineState, extensionState: Map, - contextId: String?, - crashed: Boolean + contextId: String? ): SessionState = copy( id = id, content = content, trackingProtection = trackingProtection, engineState = engineState, extensionState = extensionState, - contextId = contextId, - crashed = crashed + contextId = contextId ) } diff --git a/components/concept/engine/src/main/java/mozilla/components/concept/engine/EngineSession.kt b/components/concept/engine/src/main/java/mozilla/components/concept/engine/EngineSession.kt index 766fd4d1bec..1fddda9b0ee 100644 --- a/components/concept/engine/src/main/java/mozilla/components/concept/engine/EngineSession.kt +++ b/components/concept/engine/src/main/java/mozilla/components/concept/engine/EngineSession.kt @@ -88,6 +88,11 @@ abstract class EngineSession( fun onProcessKilled() = Unit fun onRecordingStateChanged(devices: List) = Unit + /** + * Event to indicate that a new saved [EngineSessionState] is available. + */ + fun onState(state: EngineSessionState) = Unit + /** * The engine received a request to load a request. * @@ -508,19 +513,10 @@ abstract class EngineSession( abstract fun goToHistoryIndex(index: Int) /** - * Saves and returns the engine state. Engine implementations are not required - * to persist the state anywhere else than in the returned map. Engines that - * already provide a serialized state can use a single entry in this map to - * provide this state. The only requirement is that the same map can be used - * to restore the original state. See [restoreState] and the specific - * engine implementation for details. - */ - abstract fun saveState(): EngineSessionState - - /** - * Restores the engine state as provided by [saveState]. + * Restore a saved state; only data that is saved (history, scroll position, zoom, and form data) + * will be restored. * - * @param state state retrieved from [saveState] + * @param state A saved session state. * @return true if the engine session has successfully been restored with the provided state, * false otherwise. */ @@ -568,13 +564,6 @@ abstract class EngineSession( */ abstract fun exitFullScreenMode() - /** - * Tries to recover from a crash by restoring the last know state. - * - * Returns true if a last known state was restored, otherwise false. - */ - abstract fun recoverFromCrash(): Boolean - /** * Marks this session active/inactive for web extensions to support * tabs.query({active: true}). diff --git a/components/feature/session/src/main/java/mozilla/components/feature/session/SessionUseCases.kt b/components/feature/session/src/main/java/mozilla/components/feature/session/SessionUseCases.kt index dc2ca106b47..4a81fd639e0 100644 --- a/components/feature/session/src/main/java/mozilla/components/feature/session/SessionUseCases.kt +++ b/components/feature/session/src/main/java/mozilla/components/feature/session/SessionUseCases.kt @@ -330,7 +330,7 @@ class SessionUseCases( val tabIds = store.state.let { it.tabs + it.customTabs }.filter { - it.crashed + it.engineState.crashed }.map { it.id } diff --git a/components/feature/session/src/main/java/mozilla/components/feature/session/engine/EngineViewPresenter.kt b/components/feature/session/src/main/java/mozilla/components/feature/session/engine/EngineViewPresenter.kt index 453e9032f08..7c2bf3991ba 100644 --- a/components/feature/session/src/main/java/mozilla/components/feature/session/engine/EngineViewPresenter.kt +++ b/components/feature/session/src/main/java/mozilla/components/feature/session/engine/EngineViewPresenter.kt @@ -4,6 +4,7 @@ package mozilla.components.feature.session.engine +import android.view.View import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.cancel import kotlinx.coroutines.flow.collect @@ -33,7 +34,14 @@ internal class EngineViewPresenter( scope = store.flowScoped { flow -> flow.map { state -> state.findTabOrCustomTabOrSelectedTab(tabId) } // Render if the tab itself changed and when an engine session is linked - .ifAnyChanged { tab -> arrayOf(tab?.id, tab?.engineState?.engineSession) } + .ifAnyChanged { tab -> + arrayOf( + tab?.id, + tab?.engineState?.engineSession, + tab?.engineState?.crashed, + tab?.content?.firstContentfulPaint + ) + } .collect { tab -> onTabToRender(tab) } } } @@ -56,6 +64,20 @@ internal class EngineViewPresenter( private fun renderTab(tab: SessionState) { val engineSession = tab.engineState.engineSession + val actualView = engineView.asView() + + if (tab.engineState.crashed) { + engineView.release() + actualView.visibility = View.GONE + return + } else { + actualView.visibility = View.VISIBLE + } + + if (tab.content.firstContentfulPaint) { + actualView.visibility = View.VISIBLE + } + if (engineSession == null) { // This tab does not have an EngineSession that we can render yet. Let's dispatch an // action to request creating one. Once one was created and linked to this session, this