[Bug]Tabs Tray context menu: kotlin.KotlinNullPointerException crash

[AndiAJ]

Prerequisites

Don't have any open tabs

Steps to reproduce

1. Open Wikipedia
2. Long tap the tabs counter
3. Select "Close tab" from the context menu
4. Undo the deletion
5. Long tap the tabs counter
6. Select "New tab"

Expected behavior

A new tab should be opened

Actual behavior

Crash

Device information

• Android device: all
• Fenix version: Fenix Nightly200709 from 7/9

Notes

❗ The original issue: #11515

► Video

► Crash report: https://crash-stats.mozilla.org/report/index/3dbf84ee-67c1-4a33-88e3-c31840200709

kotlin.KotlinNullPointerException

* Sentry: https://sentry.prod.mozaws.net/operations/fenix-nightly/?query=22cd1a2727a44ee1b4f587541e87139f

kotlin.KotlinNullPointerException
at kotlin.jvm.internal.ArrayIteratorKt.throwNpe(ArrayIterator.kt:1)
at org.mozilla.fenix.browser.BaseBrowserFragment.getBrowserToolbarView(BaseBrowserFragment.kt:1)
at org.mozilla.fenix.browser.BaseBrowserFragment.fullScreenChanged(BaseBrowserFragment.kt:24)
at org.mozilla.fenix.browser.BaseBrowserFragment.access$fullScreenChanged(BaseBrowserFragment.kt:1)
at org.mozilla.fenix.browser.BaseBrowserFragment$initializeUI$2$14.invoke(BaseBrowserFragment.kt:3)
at mozilla.components.feature.session.FullScreenFeature.onChange(FullScreenFeature.kt:2)
at mozilla.components.feature.session.FullScreenFeature.access$onChange(FullScreenFeature.kt:1)
at mozilla.components.feature.session.FullScreenFeature$start$1$invokeSuspend$$inlined$collect$1.emit(Collect.kt:2)
at mozilla.components.support.ktx.kotlinx.coroutines.flow.FlowKt$ifChanged$$inlined$filter$1$2.emit(Collect.kt:7)
at mozilla.components.feature.session.FullScreenFeature$start$1$invokeSuspend$$inlined$map$2$2.emit(Collect.kt:4)
at mozilla.components.feature.session.FullScreenFeature$start$1$invokeSuspend$$inlined$map$1$2.emit(Collect.kt:3)
at kotlinx.coroutines.flow.FlowKt.emitAllImpl$FlowKt__ChannelsKt(Unknown Source)
at kotlinx.coroutines.flow.FlowKt__ChannelsKt$emitAllImpl$1.invokeSuspend(Channels.kt)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:3)
at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:21)
at android.os.Handler.handleCallback(Handler.java:739)
at android.os.Handler.dispatchMessage(Handler.java:95)
at android.os.Looper.loop(Looper.java:152)
at android.app.ActivityThread.main(ActivityThread.java:5507)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)

┆Issue is synchronized with this Jira Task

[ekager]

It looks like there are a few issues at play here:

1. With these STR it looks like after step 6, the code to remove the singular session (we just restored) in HomeFragment#onResume is triggered again because the args.sessionToDelete is not cleared out. AFAICT this isn't what is causing the crash, but just some weirdness and is definitely a bug.
2. Something with the FullscreenFeature onChange is going wrong and causing the actual crash. I think this is another fallout from Issue #7480: Migrate feature-session to use BrowserStore. android-components#7481 because this timing lines up with the first Sentry report for this trace https://sentry.prod.mozaws.net/operations/fenix-nightly/issues/9004565/?query=is%3Aunresolved%20KotlinNullPointerException

cc @pocmo for thoughts here

[pocmo]

Moved this into the AC sprint to investigate today.

[pocmo]

(Also: I didn't know that you can long-press the tabs counter, that is cool!)

[kbrosnan]

• bz-1651438
• https://crash-stats.mozilla.org/signature/?product=Fenix&signature=kotlin.KotlinNullPointerException%3A%20at%20kotlin.jvm.internal.ArrayIteratorKt.throwNpe%28ArrayIterator.kt%3A1%29&version=0.0a1

[pocmo]

2. Something with the FullscreenFeature onChange is going wrong and causing the actual crash.

One difference with the new implementation is that we may see a call to fullScreenChanged and viewportFitChanged happen with false and 0 on start. I'll take a closer look today.

[pocmo]

Yeah, when that crash happens fullScreenChanged(false) gets called and at that time browserToolbarView is null. But the getter is overriden with:

    protected val browserToolbarView: BrowserToolbarView
        get() = _browserToolbarView!!

(Which may be better expressed as lateinit var?)

Normally that wouldn't be a problem since FullscreenFeature is created after browserToolbarView and then browserToolbarView is not null. But we are clearing the reference in onDestroyView() - and in that situation fullScreenChanged gets called after that.

All of this is happening because FullscreenFeature is observing the currently selected tab. In that flow the new opened tab is the new selected tab and so it starts observing this, meanwhile the fragment (view) gets destroyed to display the input fragment, and the feature receives the initial state (we are not in fullscreen!) and tries to apply that.

One way to mitigate this would be to not invoke the callbacks for the initial state ("We are not in fullscreen mode!") from FullscreenFeature (like it was the case before the refactoring) since we are not interested in doing anything in that case anyways. I'll look at patching this in AC.

[pocmo]

Okay, I have a patch for this now and will open a PR in AC.

However this seems to have uncovered a different problem, that I think is unrelated and probably in Fenix: Using the STR above I end up in the "url input" screen, but the original tab (that I restored with "undo") is no longer around? 🤔 - I'll file a separate issue for that.

[pocmo]

This should be fixed in Nightly now.

[Diana-Rus]

Hi, verified as fixed, no crash, with Samsung Galaxy S9 (Android 8) and Google Pixel 3 XL (Android 9) on latest Nightly 07/22

Samsung GIF

Note:

• No new tab is opened - the app's home screen is present, instead of having the navigation toolbar focused and ready to make a search

Should a new ticket be created for this?

[ekager]

We have an issue for that, thank you! #11955