1621190 - ephemeral silent "Firefox is running" notification some time after every start on android 10

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Audio/Video, defect, P1)

Description

[bughit]

Attachment: notification

• stock android 10 (March 2020)
• Firefox 68.5.0
• ublock origin

At some point after every start, possibly triggered by something in content, the attached notification is briefly shown.

Title: Firefox is running
Body: Tap for more information or to stop the app.

The category of this notification, which can be found by swiping right on it and tapping its gear icon, is "Sound and video", which means it can't be disabled (by category) without disabling legitimate "Sound and video" notification.

It's a silent notification so it doesn't auto pop on screen. It appears in the android status bar as a dark circle.

Since nothing useful is being communicated by it, and it only creates confusion, it should be suppressed.

Comment 1

[BugBot [:suhaib / :marco]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Alex Chronopoulos [:achronop]]

John, can you please help me triage this one?

Comment 3

[John Lin [:jhlin][:jolin]]

From what the reporter described, it looks like a notification with empty title and text in "MEDIA" channel. The only code I could find posting this kind of notification is [1].

[1] https://searchfox.org/mozilla-esr68/source/mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java#32

Comment 4

[Petru-Mugurel Lingurar [:petru]]

Will investigate this a bit on Fennec side.

Comment 5

[Petru-Mugurel Lingurar [:petru]]

STRs to easily reproduce this:

• Android 10 device (issue seem to only be reproducible on Android 10)
• cold start
• close the AwesomeScreen tab

There very probably are other scenarios for this issue but the underlying problem is the same and indeed located in MediaControlService where John indicated - MediaControlService#onCreate().

Android helps us here and posts the issue in logcat

V/ActivityManager: Attempted to start a foreground service (org.mozilla.fennec_petru/org.mozilla.gecko.media.MediaControlService) with a broken notification (no icon: Notification(channel=media-notification-channel pri=0 contentView=null vibrate=null sound=null defaults=0x0 flags=0x40 color=0x00000000 vis=PRIVATE))

and then posts a generic "App is running" notification for us instead of even maybe crashing the app.

Seems like this service could get into some edgecases - "being started with the stutdown action", so it will be called to stop before being actually started. In this case, because MediaControlService is a foreground service Android Oreo+ imposes that it should always post a notification.
In this usecase it's clear that it's notification has a very limited lifespan though so I think that at this point we should go with the easy solution and just add the notification icon Android is complaining about.

Comment 6

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1621190 - Use Fenix icon in the stub media notification; r?AndreiLazar

Not providing the smallIcon would result in Android showing a generic
notification on Android 10 devices.

Comment 7

[Petru-Mugurel Lingurar [:petru]]

esr68 try build with the above patch - https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1ccdef0e9656357fb58736d25cabd69f3ad9c66

Comment 8

[Oana Horvath [:ohorvath - out of office]]

Attachment: Screenshot_20200327-112214.png

Verified on the try build on a Pixel 4, Android 10, and the notification will still appear but without content. See screenshot.

Comment 9

[Petru-Mugurel Lingurar [:petru]]

Thanks Oana!

Indeed, my patch comes to just restore to the old behaviour - show for a brief moment a "blank notification", instead of the generic one posted by Android on devices running Q, something that was added in bug 1477041 - https://hg.mozilla.org/mozreview/gecko/rev/eb3011d304daf31650f5fc05a1ead4cdd0b63c49#l3.30

Comment 10

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 9135086 [details]
Bug 1621190 - Use Fenix icon in the stub media notification; r?AndreiLazar

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Prevent a generic, potentially confusing notification.
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Not risky - small change, verified by QA
• String or UUID changes made by this patch:

Comment 11

[bughit]

Indeed, my patch comes to just restore to the old behaviour - show for a brief moment a "blank notification"

That is not a fix for this bug. AFAICT from the screenshot, all you did is change the title("Firefox is running" => "Firefox Nightly") and remove the body. Had I seen this "improved" notification I would still have filed the bug, because the issue here is that it's confusing and pointless and should not be shown.

Comment 12

[Petru-Mugurel Lingurar [:petru]]

(In reply to bughit from comment #11)

Indeed, my patch comes to just restore to the old behaviour - show for a brief moment a "blank notification"

That is not a fix for this bug. AFAICT from the screenshot, all you did is change the title("Firefox is running" => "Firefox Nightly") and remove the body. Had I seen this "improved" notification I would still have filed the bug, because the issue here is that it's confusing and pointless and should not be shown.

I understand your concerns but as I said in my previous comment the "blank notification" is something that was already in the application for good reasons - to prevent a crash.

To offer a bit more context - Android 8 introduced new limitations - https://developer.android.com/about/versions/oreo/background.
This for us meant that the MediaControlService which is a background service must show a notification, otherwise Android would kill the application, as seen in the ticket which introduced this notification - bug 1477041 .

MediaControlService is tied to media playing in the background and there are cases in which it might be interacted with but no actual media is playing. In this cases it would start, show a notification because it must and then immediately stop itself.
I think you can confirm that this notification is non-intrusive and shown for just a brief moment.
At that time this was the best solution we could use and now we can only maintain it and ensure no new problems are discovered.

My patch comes to unify the UX and use the same notification from Android 8 and 9 devices on Android 10 devices also.
It fixes an otherwise invalid notification and continue to prevent a crash from known scenarios.

Comment 13

[bughit]

This notification can be turned of using android notification controls, by its category ("Sound and video") . Is it still performing its required function when it's thus suppressed?

If it's not, then it's not a good solution to depend on an annoying notification that a user can disable.

If it is, then can you give its own category? Presumably turning off "Sound and video" will disable other possibly useful notifications. If it had its own category it could be turned off specifically.

Comment 14

[Petru-Mugurel Lingurar [:petru]]

Thanks bughit!

Tested locally the scenario in which the notifications from a specific channel are turned off and I only saw in logs

E/NotificationService: Suppressing notification from package by user request.

No other errors, no problems for the app.

Having a new notification channel, maybe "Debug" in which to post this notifications would indeed allow users to suppress all such notifications but I'm not sure this is a behavior we want to promote.
Indeed the best solution would be to not post this notifications in the first place but after skimming through bug 1477041 which introduced this and the related code finding such a solution would not be easy and might involve an important refactoring.

Asking Stefan and Ashley how we should proceed

• bring the same behaviour as on Android 8 and 9 on Android 10 - a blank notification shown for very little time, in some edge cases scenarios.
• create a new notifications channel (with what name?) in which to post the above blank notification which users could then suppress.
• investigate and try to find an alternative solution to posting the notification in the first place.

Comment 15

[Petru-Mugurel Lingurar [:petru]]

In the meantime, let's wait on the uplift.

Comment 16

[bughit]

Having a new notification channel, maybe "Debug" in which to post this notifications would indeed allow users to suppress all such notifications but I'm not sure this is a behavior we want to promote.

You are showing a blank notification that is both useless and confusing to the user, but don't want to make it possible to suppress it? So you actually want to annoy users? If not, you should add a new channel, I would consider that a fix for this issue, the current change is not.

Comment 17

[Petru-Mugurel Lingurar [:petru]]

After discussing with Stefan I've added a new Debug Messages notifications channel that will be used for notifications only useful in debugging and not for the general userbase.
Although they should be non-intrusive they can be freely blocked by users without any loss in the normal app functionality.

Video showing the result - https://drive.google.com/file/d/1JIjSkILzv6ptaqX2UfuF8cMyT4RuhZqm/view?usp=sharing

Before uplifting this please help verify on this new try build that:

• users have the same experience on Android 8, 9 and 10 related to the above "blank" notification.
• the above "blank" notification can be blocked by blocking all notifications from the "Debug Messages" notification channel.
• the application will work as before, no loose in functionality / new crashes related to the above STRs if the notification is blocked.

Comment 18

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I'm not sure yet if this is a good improvement. I'm afraid it will cause similar confusion.

I'm also hesitant to introduce a new notification channel. I think we would definitely not call it "Debug Messages" but I don't know what else it could be.

Comment 19

[athomas]

Closing as this is expected behavior. We haven't identified a better alternative