Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Releasing session to switch from custom tab to browser does not work anymore #7647

Closed
pocmo opened this issue Jul 8, 2020 · 3 comments
Closed
Assignees
Labels
🐞 bug Something isn't working 💥 crash <session> Component: browser-session
Milestone

Comments

@pocmo
Copy link
Contributor

pocmo commented Jul 8, 2020

Fenix issue: mozilla-mobile/fenix#12311

This looks like it is a regression from the refactoring to use BrowserStore in the SessionFeature.

┆Issue is synchronized with this Jira Task

@pocmo pocmo added 🐞 bug Something isn't working <session> Component: browser-session 💥 crash labels Jul 8, 2020
@pocmo pocmo self-assigned this Jul 8, 2020
@pocmo
Copy link
Contributor Author

pocmo commented Jul 8, 2020

After adding a similar switch action to sample browser I can reproduce it there too:

         AndroidRuntime  E  FATAL EXCEPTION: main
                         E  Process: org.mozilla.samples.browser, PID: 515
                         E  java.lang.IllegalStateException: SET SESSION: Current activity: BrowserActivity Other activity: ExternalAppBrowserActivity
                         E      at mozilla.components.browser.engine.gecko.GeckoEngineView.render(GeckoEngineView.kt:124)
                         E      at mozilla.components.feature.session.engine.EngineViewPresenter.onEngineSession(EngineViewPresenter.kt:53)
                         E      at mozilla.components.feature.session.engine.EngineViewPresenter.access$onEngineSession(EngineViewPresenter.kt:22)
                         E      at mozilla.components.feature.session.engine.EngineViewPresenter$start$1$invokeSuspend$$inlined$collect$1.emit(Collect.kt:137)
                         E      at mozilla.components.support.ktx.kotlinx.coroutines.flow.FlowKt$ifChanged$$inlined$filter$1$2.emit(Collect.kt:147)
                         E      at mozilla.components.feature.session.engine.EngineViewPresenter$start$1$invokeSuspend$$inlined$map$2$2.emit(Collect.kt:139)
                         E      at mozilla.components.feature.session.engine.EngineViewPresenter$start$1$invokeSuspend$$inlined$map$1$2.emit(Collect.kt:139)
                         E      at kotlinx.coroutines.flow.FlowKt__ChannelsKt.emitAllImpl$FlowKt__ChannelsKt(Channels.kt:59)
                         E      at kotlinx.coroutines.flow.FlowKt__ChannelsKt$emitAllImpl$1.invokeSuspend(Unknown Source:11)
                         E      at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
                         E      at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56)
                         E      at android.os.Handler.handleCallback(Handler.java:907)
                         E      at android.os.Handler.dispatchMessage(Handler.java:99)
                         E      at android.os.Looper.loop(Looper.java:216)
                         E      at android.app.ActivityThread.main(ActivityThread.java:7464)
                         E      at java.lang.reflect.Method.invoke(Native Method)
                         E      at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:549)
                         E      at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:955)
                         E  Caused by: java.lang.IllegalStateException: Display already acquired
                         E      at org.mozilla.geckoview.GeckoSession.acquireDisplay(GeckoSession.java:2398)
                         E      at org.mozilla.geckoview.GeckoView.setSession(GeckoView.java:441)
                         E      at mozilla.components.browser.engine.gecko.GeckoEngineView.render(GeckoEngineView.kt:115)
                         E      ... 17 more

This almost looks like we didn't actually release the session? Or maybe too late?

@pocmo
Copy link
Contributor Author

pocmo commented Jul 8, 2020

What's happening in the store is that we are removing the TabSessionState and adding a CustomTabSessionState. Since that new state has the same ID as before the SessionFeature will find it and render it again. Since that happens asynchronously just reordering the calls may not be enough. 🤔

pocmo added a commit to pocmo/android-components that referenced this issue Jul 8, 2020
…to stop rendering engine sessions and release the current one.
pocmo added a commit to pocmo/android-components that referenced this issue Jul 9, 2020
…to stop rendering engine sessions and release the current one.
bors bot pushed a commit that referenced this issue Jul 9, 2020
7650: Issue #7647: Add SessionFeature.release() to allow app to stop rendering engine sessions and release the current one. r=Amejia481,csadilek a=pocmo

Previously Fenix just called `engineView.release()`. However since `EnginePresenter` is now listening to `BrowserStore` it may re-render the session that we just released. This new methods stops the presenter from updating and releases the current session.

Co-authored-by: Sebastian Kaspari <s.kaspari@gmail.com>
@pocmo
Copy link
Contributor Author

pocmo commented Jul 10, 2020

Patch landed in AC and also in Fenix.

@pocmo pocmo closed this as completed Jul 10, 2020
@pocmo pocmo added this to the 50.0.0 milestone Jul 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐞 bug Something isn't working 💥 crash <session> Component: browser-session
Projects
None yet
Development

No branches or pull requests

1 participant