1416169 - MediaControlService keeps running even when notification is not active

Status: VERIFIED FIXED

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

Description

[Jan Henning [:JanH]]

Once started, the MediaControlService never stops running, even when no media is playing and the notification isn't displayed, which means that while Firefox is in background, according to the process list from Android's developer settings Firefox continues appearing in the Running Services list instead of as a Cached Background Process.

If I remember correctly, the service is started as not sticky, so if it is ever killed because Android runs out of memory and has to start killing services as well, or because the user swipes us away in the recent tasks list, at least it is not restarted again.

Although keeping the service around has the (unintentional) side effect of making us a little less likely to be killed while in background, I'm still not quite convinced this is the right thing to do when we're neither playing some media nor showing the corresponding notification.

Comment 1

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

I wonder if the service is really necessary for showing 'now playing' notifications. It seems to me that the current behavior could be achieved with a global manager object in browser app (or GeckoView?) without dedicated service.

Comment 2

[Jan Henning [:JanH]]

Don't we need a service to prevent Android from randomly OOM-killing us while in background? Doing that while we're actually playing something audible would be a legitimate use case for using a service, and maybe because Gecko startup is non-trivial also if playback has been paused via the media control, but the notification is still showing, so resuming playback works without problems.

Comment 3

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

Yes, you're right. The service is indeed needed to keep browser alive in the background during media playing.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Anecdotally, if I use fennec (with a large profile and sync), my entire phone starts slowing down. Killing the MediaControlService will speed my phone up again. There are other possible explanations:
- something else is running in the process and killing the MediaControlService kills the whole process
- FF is using up a lot of memory

Note that I have an older phone, a GS5, but the entire device shouldn't slow down because the fennec process is still alive.

[triage] This might only affect me but if it doesn't this could basically make fennec unusuable for users on older devices (or maybe users on older devices with larger profiles). Action item: investigate!

Comment 5

[:sdaswani]

Not a critical P1 until we have more data on impact.

Comment 6

[Barbara Bermes [:barbara] - NI please!]

Mike, can you estimate if that's a big eng effort? If not, let's keep it as a P1 and get somebody to fix it asap.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Action items:
1) Research the service: what does it do? What should its lifecycle be?
2) Why might it be causing the device to slowdown? Is it just that it occupies the CPU (as a running service) while the device is doing other things?
3) Implement

Assuming we don't rabbit-hole on 2 and focus on stopping the service, I'd still think this is more likely to be a big eng effort than a small one: it's more likely that this service is doing quite a few things that need to be considered carefully rather than doing something simple that we can just stop when the notification is not shown. It's especially hard because the core author of the service (:alwu) is no longer at Mozilla though Sebastian was the primary reviewer.

Sebastian, do you have any thoughts on how much effort this would be to fix? Please NI me when you make your comment so I can retriage.

Comment 8

[Jan Henning [:JanH]]

2) I wouldn't rule it out completely, but I think it's more likely that some other component in Firefox went haywire, or possible some web site or add-on script running out of control. The thing is that because the MediaControlService keeps running, and running, and running, it's a very easy target for Android to blame things on for anything untoward that might happen within our process.
When going into the background, what used to happen previously was that Firefox would appear as a "cached background process" and our process would continue running (and potentially consuming computing and network resources) until Android decided it needed our memory for more important stuff. What happens now is that the MediaControlService is technically still running, so Android now surfaces us more prominently as a "Running service" and will be somewhat more reluctant to kill our process.
As long as your phone is not running short on memory, I don't think there's any immediate difference between the two scenarios - the Firefox process remains alive in background and that's it. The difference is that if you launch other memory-intensive applications, then Android will be more likely to keep us alive if we've got a service running.

1) I stared a little at the MediaControlService and I think it should be possible to arrange for it to remain active only while the playback notification is being shown.
Getting it to stop when the notification is being dismissed should be as easy as calling shutdown() here [1][2] and verifying that the various lifecycle events are set up correctly to release any resources held by the service.

As for starting it again: At the moment, the service is started via the AudioFocusAgent when Firefox starts. Then, if I understand things correctly, we register a TabEvent listener [4] that is actually responsible for triggering the notification if we learn about media playback happening in any tab.
So if we don't want the service running all the time, this TabEvent listener logic and mTabReference need to be moved somewhere else (maybe the AudioFocusAgent?) where they can remain alive while Firefox is running, monitor tabs for media playback and then start the MediaControlService only as necessary when we actually want to show the notification. This looks doable, although probably not quite just a five-minute job, either.

I most probably won't have the time to look at this further in time for the next merge day, though, if you want to keep this as a P1.

[1] https://dxr.mozilla.org/mozilla-central/rev/165474ef313b106f0364cf1941a0030c41855876/mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java#379
[2] The state changes to STOPPED both when Gecko tells us it's no longer playing media (and we cancel the notification ourselves) as well as when the user swipes the notification away [3].
[3] https://dxr.mozilla.org/mozilla-central/rev/165474ef313b106f0364cf1941a0030c41855876/mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java#416,465-469
[4] https://dxr.mozilla.org/mozilla-central/rev/165474ef313b106f0364cf1941a0030c41855876/mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java#118-156

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

> Sebastian, do you have any thoughts on how much effort this would be to fix? Please NI me when you make your comment so I can retriage.

I think Alastor had some reasons why he wanted to keep the service running in some cases. But unfortunately I do not remember the details here and Alastor doesn't work at Mozilla anymore.

Comment 10

[Jan Henning [:JanH]]

Bug 1290467 comment 42 has some info on the history of that design, but doesn't really answer the question why something like I described above wasn't chosen.
The basic problem is that the MediaControlService needs to listen to some tab events, which of course doesn't work if the service isn't even running.

According to bug 1290467 comment 42 it was considered to either leave things as they were previously (the AudioFocusAgent is responsible for starting the MediaControlService and Tabs then sends its tab events), but this was rejected because with the new design both the AudioFocusAgent and Tabs were in fact partially relying on the same notification from Gecko, so it wouldn't be guaranteed which one of them would be called first - if the AudioFocusAgent lost that race, the MediaControlService would consequently be started too late to still receive the tab event. Alternatively it was considered to let Tabs start the service directly, but that was rejected because coupling Tabs that tightly with the MediaControlService seems rather awkward - we've already got the tab event mechanism for that purpose.
So in the end a design was chosen were the MediaControlService simply remains alive all the time so it can receive all necessary tab events.

What's missing from the above choice of alternatives is however the proposal I've outlined above - let someone else (i.e. most likely the AudioFocusAgent, since it's already around and involved with the MediaControlService) listen for those tab events and then start and notify the MediaControlService accordingly.

Comment 11

[Jan Henning [:JanH]]

Attachment: Bug 1416169 - Part 0 - Cleanups.

Fix some style inconsistencies, plus things Android Studio is complaining about.

Review commit: https://reviewboard.mozilla.org/r/226852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226852/

Comment 12

[Jan Henning [:JanH]]

Attachment: Bug 1416169 - Part 1 - Silence StaticFieldLeak linter.

In practice we've already been using the application context anyway, but this
way makes it more explicit.
With that out of the way, we can also silence the linter here, since the Static-
FieldLeak check is only really relevant when holding onto non-Application
contexts.

Review commit: https://reviewboard.mozilla.org/r/226854/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226854/

Comment 13

[Jan Henning [:JanH]]

Attachment: Bug 1416169 - Part 2 - Move monitoring of Tab media events out of the MediaControlService.

At the moment the MediaControlService is structured such that it is started
shortly after Firefox has launched. Then, it registers a Tab event listener and
starts listening for various, mainly media-related tab events, in response to
which the playback notification is then shown or hidden as appropriate.
This setup has the drawback that the service itself has to remain running all
the time, even when we're not actually showing the notification, so Firefox now
keeps showing up as a "Running service" even when in background and not doing
anything interesting.

It is however relatively easily possible to move the tab event tracking into a
different class that won't require a service to stay alive, and set the Media-
ControlService's state (and at the same time start it if necessary) through a
set of special intent actions.

As the AudioFocusAgent is
a) alive for the whole lifetime of the application and
b) already interacting with the MediaControlService,
we designate it as the class responsible for this.

Review commit: https://reviewboard.mozilla.org/r/226856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226856/

Comment 14

[Jan Henning [:JanH]]

Attachment: Bug 1416169 - Part 4 - Switch the MediaControlService to running only on demand.

Now that the AudioFocusAgent keeps monitoring all relevant tab events and
notifies the MediaControlService as necessary, we can revert to only running the
service when actually required to show the notification and keep us alive during
(background) media playback.

Review commit: https://reviewboard.mozilla.org/r/226858/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226858/

Comment 15

[Jan Henning [:JanH]]

Comment on attachment 8957902 [details]
Bug 1416169 - Part 4 - Switch the MediaControlService to running only on demand.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226858/diff/1-2/

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8957899 [details]
Bug 1416169 - Part 0 - Cleanups.

https://reviewboard.mozilla.org/r/226852/#review232994

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8957900 [details]
Bug 1416169 - Part 1 - Silence StaticFieldLeak linter.

https://reviewboard.mozilla.org/r/226854/#review232996

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8957901 [details]
Bug 1416169 - Part 2 - Move monitoring of Tab media events out of the MediaControlService.

https://reviewboard.mozilla.org/r/226856/#review233000

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8957902 [details]
Bug 1416169 - Part 4 - Switch the MediaControlService to running only on demand.

https://reviewboard.mozilla.org/r/226858/#review233002

Much nicer!

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Jan Henning [:JanH] from comment #8)
> 2) I wouldn't rule it out completely, but I think it's more likely that some
> other component in Firefox went haywire, or possible some web site or add-on
> script running out of control. The thing is that because the
> MediaControlService keeps running, and running, and running, it's a very
> easy target for Android to blame things on for anything untoward that might
> happen within our process.

I didn't consider this (and it was actually a flaw in my Android knowledge). Snorp, do you know what might continue running in threads after fennec is closed that could cause a device to slow down?

We should re-evaluate after this patch lands to see if we need more investigation (i.e. my question to Snorp). Using the debugger might be an easy way to do this.

Comment 21

[Jan Henning [:JanH]]

Attachment: Bug 1416169 - Part 3 - Resolve some lifecycle issues.

We're not a bound service and the MediaSession is initialised during onCreate(),
so its release conversely needs to happen during onDestroy().
For consistency, we also move unregistering of the HeadSetStateReceiver into the
shutdown helper method.

Additionally, while things seem to have worked anyway in practice (possibly be-
cause we're only using the MediaSession within the same process?), technically
you're supposed to explicitly activate the session before using it.

Review commit: https://reviewboard.mozilla.org/r/227462/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227462/

Comment 22

[Jan Henning [:JanH]]

Comment on attachment 8957902 [details]
Bug 1416169 - Part 4 - Switch the MediaControlService to running only on demand.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226858/diff/2-3/

Comment 23

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8958567 [details]
Bug 1416169 - Part 3 - Resolve some lifecycle issues.

https://reviewboard.mozilla.org/r/227462/#review233316

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c909f553340c
Part 0 - Cleanups. r=snorp
https://hg.mozilla.org/integration/autoland/rev/1a1f19183f38
Part 1 - Silence StaticFieldLeak linter. r=snorp
https://hg.mozilla.org/integration/autoland/rev/6b1099c4b838
Part 2 - Move monitoring of Tab media events out of the MediaControlService. r=snorp
https://hg.mozilla.org/integration/autoland/rev/3e917ebfc7ba
Part 3 - Resolve some lifecycle issues. r=snorp
https://hg.mozilla.org/integration/autoland/rev/5dc1d09ffedf
Part 4 - Switch the MediaControlService to running only on demand. r=snorp

Comment 25

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/c909f553340c
https://hg.mozilla.org/mozilla-central/rev/1a1f19183f38
https://hg.mozilla.org/mozilla-central/rev/6b1099c4b838
https://hg.mozilla.org/mozilla-central/rev/3e917ebfc7ba
https://hg.mozilla.org/mozilla-central/rev/5dc1d09ffedf

Comment 26

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Michael Comella (:mcomella) [needinfo or I won't see it] from comment #20)
> (In reply to Jan Henning [:JanH] from comment #8)
> > 2) I wouldn't rule it out completely, but I think it's more likely that some
> > other component in Firefox went haywire, or possible some web site or add-on
> > script running out of control. The thing is that because the
> > MediaControlService keeps running, and running, and running, it's a very
> > easy target for Android to blame things on for anything untoward that might
> > happen within our process.
> 
> I didn't consider this (and it was actually a flaw in my Android knowledge).
> Snorp, do you know what might continue running in threads after fennec is
> closed that could cause a device to slow down?

Not really, but with Gecko still running it could be a lot of different things.

> 
> We should re-evaluate after this patch lands to see if we need more
> investigation (i.e. my question to Snorp). Using the debugger might be an
> easy way to do this.

Yeah, we'll need to take a close look at Gecok's background activity here at some point.

Comment 27

[Levente Sacal]

Verified as fixed in 61.0b15. Have ran a logcat in Android Studio and MediaControlService stops running after dismissing the Media playback notifications while running a Youtube video in the background. Should it need more test and another test scenario here?