1282391 - Cleanup the media controls when the Fennec gets killed

Status: RESOLVED FIXED

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

Description

[Paul Adenot (:padenot)]

STR:
- Have an HTMLMediaElement playing in fennec
- Have Firefox killed by the system

Expected:
- The media controls have been cleaned up.

Actual:
- The media controls are still present, but do nothing.

Comment 1

[Alastor Wu [:alwu]]

Attachment: Bug 1282391 - make MediaControlService to foreground service when we're playing media.

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

Comment 2

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6015db0a114

Comment 3

[Alastor Wu [:alwu]]

Hi, Sebastian,
Could you help me review this patch?
In this patch, the MediaControlSerivce would remove all notifications in its inialization in order to ensure there isn't any redundant notification.
Thanks!

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8769605 [details]
Bug 1282391 - make MediaControlService to foreground service when we're playing media.

https://reviewboard.mozilla.org/r/63430/#review60722

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:80
(Diff revision 1)
>      }
>  
>      @Override
>      public int onStartCommand(Intent intent, int flags, int startId) {
>          handleIntent(intent);
> +        flags = START_STICKY;

What was your intention here? Usually this flag is returned from this method.

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:228
(Diff revision 1)
>                  mActionState = ACTION_STOP;
>                  stopSelf();
>              }
>          });
>          mIsInitMediaSession = true;
> +        notifyControlInterfaceChanged(ACTION_REMOVE_CONTROL);

Doesn't this just remove the notification on init? Isn't this bug about the opposite: The notification remains after the app is gone?

Comment 5

[Alastor Wu [:alwu]]

(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> What was your intention here? Usually this flag is returned from this method.

You're right, I would remove this flag.

> Doesn't this just remove the notification on init? Isn't this bug about the
> opposite: The notification remains after the app is gone?

Yes, that is what I want. When the MediaControlService is initialized, there shouldn't have any notification before calling controller::onPlay().

When Fennec was killed by system, we don't have any way to aware about that, the app would just disapper immediately.

However, since the MediaControlService is Android's service, Android's ActivityManager would restart it again. 

What I do is to make sure MediaControlService would clear all un-needed notification when it restarts.

Comment 6

[Alastor Wu [:alwu]]

Comment on attachment 8769605 [details]
Bug 1282391 - make MediaControlService to foreground service when we're playing media.

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

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Do we have the exact steps to reproduce this? I'm unable to get into this broken state by manually force stopping the app (adb force-stop or swiping away in recent apps or killing from the app settings).


(In reply to Alastor Wu [:alwu][PTO 7/20-7/28] from comment #5)
> However, since the MediaControlService is Android's service, Android's
> ActivityManager would restart it again. 

Well, the MediaControlService is our service running in our process. It feels like the actual problem is that this service shouldn't be restarted.

The return value from onStartCommand()[1] signals Android the mode we want to run in:

- START_STICKY: If the process is killed then the service will be re-created later.
https://developer.android.com/reference/android/app/Service.html#START_STICKY

- START_NOT_STICKY: If the process is killed then this service will not be re-created by the system until explicitly started again.
https://developer.android.com/reference/android/app/Service.html#START_NOT_STICKY

- START_REDELIVER_INTENT: After the process is killed this service will be re-created and the previous Intent will be delivered to it again.
https://developer.android.com/reference/android/app/Service.html#START_REDELIVER_INTENT

Currently we are just calling super.onStartCommand():
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java?q=path%3AMediaControlService.java&redirect_type=single#80

And the default implementation will return START_STICKY:
https://github.com/android/platform_frameworks_base/blob/master/core/java/android/app/Service.java#L460

My assumption is that we actually want to return START_NOT_STICKY here. The service should not be restarted if the process is killed (There's no media session anymore).

Additionally I think we want to change the foreground/background state depending on whether we are playing audio or not here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java#284-285

If we are currently playing audio then we want to call startForeground()[2] with the notification. This will prevent the system from just killing our service whenever it is convenient and will show the notification as long as the service is running. While we are playing music in the background we do not want the system to kill us. For all other states we want to call stopForeground(false)[3] and update the notification normally with notify() like we do right now. In those situations the system is allowed to kill us.

[1] https://developer.android.com/reference/android/app/Service.html#onStartCommand(android.content.Intent,%20int,%20int)
[2] https://developer.android.com/reference/android/app/Service.html#startForeground(int,%20android.app.Notification)
[3] https://developer.android.com/reference/android/app/Service.html#stopForeground(boolean)

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #7)
> Do we have the exact steps to reproduce this? I'm unable to get into this
> broken state by manually force stopping the app (adb force-stop or swiping
> away in recent apps or killing from the app settings).

I was able to reproduce this by backgrounding the application and using "adb shell am kill <package>" instead of "force-stop".

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8769605 [details]
Bug 1282391 - make MediaControlService to foreground service when we're playing media.

Clearing flag. See comments above. Please re-flag me if needed. :)

Comment 10

[Alastor Wu [:alwu]]

Comment on attachment 8769605 [details]
Bug 1282391 - make MediaControlService to foreground service when we're playing media.

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

Comment 11

[Alastor Wu [:alwu]]

Hi, Sebastian,
I modified my patch according comment8, could you help me review it again?
Thanks :)

Comment 12

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4da47c4a11a6

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8769605 [details]
Bug 1282391 - make MediaControlService to foreground service when we're playing media.

https://reviewboard.mozilla.org/r/63430/#review62218

r+ with the changes. :)

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:270
(Diff revision 3)
>          ThreadUtils.assertNotOnUiThread();
>  
>          final Notification.MediaStyle style = new Notification.MediaStyle();
>          style.setShowActionsInCompactView(0);
>  
> +        boolean isMediaPlaying = action.equals(ACTION_PAUSE);

nit: final

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:291
(Diff revision 3)
>          NotificationManagerCompat.from(this)
>                  .notify(MEDIA_CONTROL_ID, notification);

You can move this to the else block, we do not need it if we call startForeground(). Android will take care of showing the notification in this case.

Comment 14

[Alastor Wu [:alwu]]

Comment on attachment 8769605 [details]
Bug 1282391 - make MediaControlService to foreground service when we're playing media.

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fdce84af42d2
make MediaControlService to foreground service when we're playing media. r=sebastian

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

I believe this caused the android build times to increase slightly:
odd, this seemed to cause build times to increase:
https://treeherder.mozilla.org/perf.html#/alerts?id=1940

possibly the data is misleading as the graphs include data in a larger range an the shift is overlapping.

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

I guess we should request uplift to 49, right?

Comment 19

[Alastor Wu [:alwu]]

Yes, but since I'm in the PTO, I don't have computer now.
Could you help me to check whether this patch can be applied to FF49 without any merge conflict?
Thanks!

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Alastor Wu [:alwu][PTO 7/20-7/28] from comment #19)
> Yes, but since I'm in the PTO, I don't have computer now.
> Could you help me to check whether this patch can be applied to FF49 without
> any merge conflict?

Sure! Enjoy your PTO! :)

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8769605 [details]
Bug 1282391 - make MediaControlService to foreground service when we're playing media.

Patch applies cleanly.

Approval Request Comment

[Feature/regressing bug #]: Media controls, meta bug: 1264901

[User impact if declined]: The media control notification is not correctly removed if the app is getting killed while it is in the background. Additionally this patch prevents the app from getting killed while playing audio.

[Describe test coverage new/current, TreeHerder]: Local testing.

[Risks and why]: Low - This patch only modifies the background service lifecycle - This shouldn't have an impact on other components.

[String/UUID change made/needed]: -

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8769605 [details]
Bug 1282391 - make MediaControlService to foreground service when we're playing media.

This patch cleans up the media control notification correctly when app is killed. Take it in 49 aurora.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/23b42c01eba2

Comment 24

[Sorina Florean [:sflorean]]

Verified as fixed in build 50.0a1 (2016-07-27);
Device: Nexus 5 (Android 6.0.1).