1649494 - Crash in [@ mozilla::widget::AndroidAlerts::ShowPersistentNotification]

Status: VERIFIED FIXED

(GeckoView :: General, defect, P1)

Description

[Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him]

First report for this bug is 6/15, so either something we or AC did recently.

This bug is for crash report bp-140bf268-ae89-4d3b-b210-5eafa0200630.

Top 10 frames of crashing thread:

0 libxul.so mozilla::widget::AndroidAlerts::ShowPersistentNotification widget/android/AndroidAlerts.cpp:86
1 libxul.so nsAlertsService::ShowPersistentNotification toolkit/components/alerts/nsAlertsService.cpp:222
2 libxul.so nsAlertsService::ShowAlertNotification toolkit/components/alerts/nsAlertsService.cpp:193
3 libxul.so NS_InvokeByIndex xpcom/reflect/xptcall/md/unix/xptcinvoke_arm.cpp:167
4 libxul.so XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1140
5 libxul.so XPC_WN_CallMethod js/xpconnect/src/XPCWrappedNativeJSOps.cpp:946
6 libxul.so js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:576
7 libxul.so Interpret js/src/vm/Interpreter.cpp:639
8 libxul.so js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:611
9 libxul.so js::jit::DoCallFallback js/src/jit/BaselineIC.cpp:2992

Comment 2

[Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him]

Attachment: Bug 1649494 - Don't assume a notification has a URL. a=RyanVM

Comment 3

[Rob Wu [:robwu]]

Agi and I chatted on Matrix about this, and we evaluated the option of explicitly specifying the extension URL at https://searchfox.org/mozilla-central/rev/73a14f1b367948faa571ed2fe5d7eb29460787c1/toolkit/components/extensions/parent/ext-notifications.js#47-50 .We decided against that in favor of the above patch, because adding the moz-extension://UUID URL to the notification would cause "UUID" to appear in the notification, in the spot where the domain name would usually appear.

If we ever want to add the URL, we need to add a way to get the extension name (or somewhere similarly human-readable) to appear instead.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4f5d187f2bc
Don't assume a notification has a URL. r=aklotz

Comment 5

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/a4f5d187f2bc

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 7

[Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him]

Comment on attachment 9171958 [details]
Bug 1649494 - Don't assume a notification has a URL. a=RyanVM

Beta/Release Uplift Approval Request

• User impact if declined: Crash when extensions create notifications (e.g. search by image).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• 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): The fix is basically a null check.
• String changes made/needed:

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9171958 [details]
Bug 1649494 - Don't assume a notification has a URL. a=RyanVM

Approved for 81.0b4.

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

There's a conflict on mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md when trying to uplift this to beta.

Agi, could, you, please, take a look?

Comment 10

[Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him]

This patch should now apply to Beta. I could swear I had access to push to beta directly but looks like I don't? https://phabricator.services.mozilla.com/D88165

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/23428999fb81

Comment 12

[Oana Horvath]

We can still reproduce this on Fenix Beta 81.1.0 with the steps from https://github.com/mozilla-mobile/fenix/issues/12910:
Fixed on Nightly 9/2.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

The uplift in comment 11 didn't land in time for the 81.1.0 build.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

FYI, the 81.1.0-beta.2 build should contain this fix.

Comment 15

[Oana Horvath]

Thanks Ryan, we have verified the fix on the 81.1.0-beta.2 build, no longer crashing.

Comment 16

[Theraten]

It seems, as a result a new bug appeared: https://github.com/mozilla-mobile/fenix/issues/14993