1846306 - IllegalStateException "Unable to find session for null or selected session" from mozilla.components.feature.sitepermissions.SitePermissionsFeature.onContentPermissionRequested$feature_sitepermissions_release

Status: RESOLVED FIXED

(Firefox for Android :: General, defect, P1)

Description

[[:skhan]]

Steps to reproduce

java.lang.IllegalStateException: Unable to find session for null or selected session
at mozilla.components.feature.sitepermissions.SitePermissionsFeature.onContentPermissionRequested$feature_sitepermissions_release(SitePermissionsFeature.kt:831)
at mozilla.components.feature.sitepermissions.SitePermissionsFeature$setupPermissionRequestsCollector$1$3.emit(SitePermissionsFeature.kt:144)
at kotlinx.coroutines.flow.FlowKt__BuildersKt$asFlow$$inlined$unsafeFlow$3.collect(SafeCollector.common.kt:79)
at kotlinx.coroutines.flow.FlowKt__BuildersKt$asFlow$$inlined$unsafeFlow$3$1.invokeSuspend(SafeCollector.common.kt:13)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:9)
at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:112)
at android.os.Handler.handleCallback(Handler.java:751)
at android.os.Handler.dispatchMessage(Handler.java:95)
at android.os.Looper.loop(Looper.java:241)
at android.app.ActivityThread.main(ActivityThread.java:6274)
at java.lang.reflect.Method.invoke(Method.java)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:886)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:776)

Expected behavior

Actual behavior

Device information

• Firefox version:
• Android device model:
• Android OS version:

Any additional information?

Comment 1

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1846306 - Catch NullPointerException while printStackTrace (#3056)

Comment 2

[Chris Peterson [:cpeterson]]

I suspect this crash is responsible for the spike in [@ EMPTY: no frame data available] crashes in bug 1847429. I looked in Sentry for top crash signatures that aren't in Socorro and found https://mozilla.sentry.io/issues/4341097224/, which is this bug. It's the top Sentry crash signature over the last 30 days, by both number of crash events and number of affected users.

The crash volume spike started August ~16, which happened to be the release date for the Fenix 116.0.3 dot release with this bug's printStackTrace crash reporter fix.

Comment 3

[Chris Peterson [:cpeterson]]

Looks like a regression starting in 116.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The bug is linked to a topcrash signature, which matches the following criteria:

• Top 10 AArch64 and ARM crashes on beta
• Top 10 AArch64 and ARM crashes on release

For more information, please visit BugBot documentation.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

[Tracking Requested - why for this release]:

Comment 6

[Arturo Mejia [:amejia]]

Maybe we could change our behaviour when not finding the right session for prompt request, instead of throwing an exception we could just cancel the prompt request.

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

This comes as a note from Jonathan - We should also investigate why this started in 116 and understand the changes that caused the crash before we go with cancelling the prompt without crashing.

Comment 8

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1846306 - Address crash n at onContentPermissionRequested (#3608)

Comment 9

[Arturo Mejia [:amejia]]

This comes as a note from Jonathan - We should also investigate why this started in 116 and understand the changes that caused the crash before we go with cancelling the prompt without crashing.

It's really odd that we start to see a spike in crashes starting in 116 and up. Either way, I think we were too aggressive throwing
IllegalStateException which causes crashes. This edge case could happens for multiple reasons, and rejecting the request is a sense behaviour. For further research, we already have a lot of information on Sentry like breadcrums that could give us some hints about the root cause, if it's needed we can add extra info. I think we should stop the crashes and investigate further without affecting users.

What do yo think?

Comment 10

[Arturo Mejia [:amejia]]

Cases that I noticed from looking at Sentry breadcrums:

1. Common case: A tab requested a prompt but it was killed before we could handle the request, making the session not longer available. I think this happens when we get the sessionId as part of the stack trace for example "Unable to find session for f2751595-64c0-442c-9824-df18f9bb3a81 or selected session".
2. Unknown case: When the sessionId, and the selectedTab are null , when we get "Unable to find session for null or selected session".

Noticed most of the bread bread crums point to ExternalAppBrowserFragment.

Comment 11

[Jonathan Almeida [:jonalmeida]]

(In reply to Arturo Mejia [:amejia] from comment #9)

This comes as a note from Jonathan - We should also investigate why this started in 116 and understand the changes that caused the crash before we go with cancelling the prompt without crashing.

It's really odd that we start to see a spike in crashes starting in 116 and up. Either way, I think we were too aggressive throwing
IllegalStateException which causes crashes. This edge case could happens for multiple reasons, and rejecting the request is a sense behaviour. For further research, we already have a lot of information on Sentry like breadcrums that could give us some hints about the root cause, if it's needed we can add extra info. I think we should stop the crashes and investigate further without affecting users.

What do yo think?

Continuing this conversation on the linked PR that started there.

Comment 12

[BMO Github Automation]

Authored by https://github.com/Amejia481
https://github.com/mozilla-mobile/firefox-android/commit/66f97a9278fdbfc69ae6fbdee34949bdc7126a2d
[main] Bug 1846306 - Do not throw IllegalStateException when unable to find a session for given prompt request in onContentPermissionRequested

Comment 13

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1846306 - Do not throw IllegalStateException when unable to find a session for a given prompt request in onContentPermissionRequested (backport #3608) (#3688)

Comment 14

[Arturo Mejia [:amejia]]

Comment on attachment 9353640 [details] [review]
[mozilla-mobile/firefox-android] Bug 1846306 - Do not throw IllegalStateException when unable to find a session for a given prompt request in onContentPermissionRequested (backport #3608) (#3688)

Beta/Release Uplift Approval Request

• User impact if declined: Users will continue having crashes
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We are preventing the crash, and dismissing the permission request for the session that is not longer existing in AC.
• String changes made/needed:
• Is Android affected?: Yes

Comment 15

[Pascal Chevrel:pascalc]

Comment on attachment 9353640 [details] [review]
[mozilla-mobile/firefox-android] Bug 1846306 - Do not throw IllegalStateException when unable to find a session for a given prompt request in onContentPermissionRequested (backport #3608) (#3688)

Approved for our RC build, thanks

Comment 16

[BMO Github Automation]

Authored by https://github.com/Amejia481
https://github.com/mozilla-mobile/firefox-android/commit/b4d6ab4bb9e140d95b3edeb66626509e4b025d32
[releases_v118] Bug 1846306 - Do not throw IllegalStateException when unable to find a session for given prompt request in onContentPermissionRequested

Authored by https://github.com/pascalchevrel
https://github.com/mozilla-mobile/firefox-android/commit/6992b6b23aa40664f2bd83422342d19ff1ced024
[releases_v118] Merge pull request #3688 from mozilla-mobile/mergify/bp/releases_v118/pr-3608