Crash when tapping the search widget while in PIP

[Mugurell]

Prerequisites:
Have the search widget added to homescreen

STRs:

• open a new tab for a video, enter fullscreen, enter PIP
• tap the search widget

Expected result:

• seeing the search fragment

Actual result:

• intermittent crash (maybe a race)

java.lang.IllegalStateException: Fragment BrowserFragment{81f5054} (45fbba67-a5b5-46ca-bbf1-3b5d7fa38b73) not attached to a context.
        at androidx.fragment.app.Fragment.requireContext(Fragment.java:2)
        at org.mozilla.fenix.ext.FragmentKt.getRequireComponents(Fragment.kt:1)
        at org.mozilla.fenix.browser.BaseBrowserFragment.getCurrentTab$app_nightly(BaseBrowserFragment.kt:1)
        at org.mozilla.fenix.browser.BaseBrowserFragment.removeSessionIfNeeded(BaseBrowserFragment.kt:1)
        at org.mozilla.fenix.browser.BaseBrowserFragment.onBackPressed(BaseBrowserFragment.kt:5)
        at org.mozilla.fenix.browser.BrowserFragment.onBackPressed(BrowserFragment.kt:2)
        at org.mozilla.fenix.browser.BaseBrowserFragment$initializeUI$22$invokeSuspend$$inlined$collect$1.emit(Collect.kt:8)
        at mozilla.components.support.ktx.kotlinx.coroutines.flow.FlowKt$ifChanged$$inlined$filter$1$2.emit(Collect.kt:9)
        at org.mozilla.fenix.browser.BaseBrowserFragment$initializeUI$22$invokeSuspend$$inlined$mapNotNull$1$2.emit(Collect.kt:8)

PipToSearchCrash.mp4

┆Issue is synchronized with this Jira Task

[indurs]

@Mugurell , can I explore this issue?

[Mugurell]

@Mugurell , can I explore this issue?

Would be great to but the issue here might be more complex.
A bit more context here - #22471 (comment)

[indurs]

@Mugurell , Thank You !! I'll be sure to delve deep into the given context.

[indurs]

@Mugurell , it's interesting and I'm getting to know more about the main core of the app.

Code snippet from BaseBrowserFragment

store.flowScoped(viewLifecycleOwner) { flow ->

                flow.mapNotNull { state -> state.findTabOrCustomTabOrSelectedTab(customTabSessionId) }
                    .ifChanged { tab -> tab.content.pictureInPictureEnabled }
                    .collect { tab -> pipModeChanged(tab)}
        }

The following are the two scenarios I tried.

We come to the browser((BrowserFragment) either from the search widget or from the launcher

1. select a video
2. enter into PIP mode
3. enter full screen

We are still in BrowserFragment when we reach 3. The flow and the method pipModeChanged() has the BrowserFragement context

But in the following scenario

1. select a video
2. enter into PIP mode
3. search from the widget

We are in the HomeFragment when we search from the search widget. Meanwhile if the flow and the method pipModeChanged() has access to the BrowserFragment context before its stopped and destroyed, the code will run fine. Otherwise we will get the "java.lang.IllegalStateException: Fragment BrowserFragment{15d201b} (8cc9275a-45e9-4fbe-9a4f-9947a4e10347) not attached to a context."

Need to explore more...

[Mugurell]

Maybe we could just add a check for if the BrowserFragment isAdded here?
And then ensure we are exiting immersive mode from HomeFragment as BrowserFragment would try to do here?

But ideally we should try avoiding the race between these two fragments (actually between StartSearchIntentProcessor#process and BaseBrowserFragment#pipModeChanged ) and this seems a bit more trickier.

[indurs]

Thank You @Mugurell , I've already tried with isAdded and the exception is not thrown. If we catch the exception pipModeChanged() might not be executed when onPictureInPictureModeChanged(false) is invoked. When the search is from search widget, I'm not sure whether we need to execute pipModeChanged() when onPictureInPictureModeChanged(false) is invoked

[indurs]

@Mugurell , I'm not sure whether the pipModeChanged() method might be useful in the following scenario

1. select a video
2. enter into PIP mode
3. search from the widget

private fun pipModeChanged(session: SessionState) {
        if (!session.content.pictureInPictureEnabled && session.content.fullScreen) {
            onBackPressed()
            fullScreenChanged(false)
        }
    }

As you mentioned if the race between StartSearchIntentProcessor#process and BaseBrowserFragment#pipModeChanged is avoided we can prevent the issue from happening.

For now I'm modifying pipModeChanged method as

private fun pipModeChanged(session: SessionState) {
        if (!session.content.pictureInPictureEnabled && session.content.fullScreen && isAdded) {
            onBackPressed()
            fullScreenChanged(false)
        }
    }

The issue is gone but the screen is locked to landscape orientation.

There are few issues in orientation

#22471 (comment)
#24678 (comment)

I'm interested to work on this orientation issue as well. Will debug and explore

[indurs]

@Mugurell , I think the purpose of MediaSessionFullscreenFeature is to enable/disable full screen mode when a full screen event is dispatched from Gecko engine. This feature is used only when a video is played in the browser.

https://github.com/mozilla-mobile/android-components/blob/01012c16f9e3aab667aaa1ad9065d87bf1bad9ce/components/feature/media/src/main/java/mozilla/components/feature/media/fullscreen/MediaSessionFullscreenFeature.kt#L42 is called more than once even after the screen is set to full screen mode. Probably the flow is emitting any browser state updates after the fullscreen mode is set. I also noted that couple of times the video is played in portrait mode.

Another possible issue that I noticed..
When the video is playing in PIP mode and if we click the full screen button in the PIP window, the screen initially goes to the landscape mode but immediately goes back to the portrait mode (to the browser page itself). But I think that when we click the full screen button in PIP mode, we need to display the video in full screen and in immersive mode.

[indurs]

@Mugurell , to avoid the crash when the search widget is tapped and to open the search page in portrait mode, I've modified the StartSearchIntentProcessor and added a method in BrowserStore. Just wanted to make sure I'm going in the right direction. Suggestions would be helpful. I've created a draft PR #24951. Still few modifications need to be done.. for instance onBackPressed() is called more than once..

[delia-pop]

Verified as fixed in the latest Nightly from 05/30 with Google Pixel 6 (Android 12). The app no longer crashes after tapping the search widget while in Pip mode.