App crashes if skip notification action is used while paused

[spun]

Steps to reproduce

1. Launch app and play audio using a MediaLibraryService/MediaSessionService
2. Remove app from recents while playing
3. Pause media from notification
4. Press skip to next from notification
5. Wait

The app will crash with:

android.app.RemoteServiceException: Context.startForegroundService() did not then call Service.startForeground()

Cause

MediaNotificationHandler method createPendingIntent() is used for the creation of its NotificationCompat.Actions. This method decides to call getService or getForegroundService and it correctly checks for PAUSE and STOP actions that won't put the service in the foreground, but there are more actions that should be considered. ACTION_SKIP_TO_PREVIOUS, ACTION_SKIP_TO_NEXT and many more actions won't put the service in the foreground and all of them will crash the app if getForegroundService is used.

Also, I think that media2 has the same createPendingIntent and probably has the same problem.

[marcbaechinger]

I think the behavior of the service looks correct and sensible to me, except that when paused, the MediaNotificationHandler should hide all actions except the play action.

The general approach is to remove the service from the foreground when playWhenReady is false. If playback is not resumed after a while, the system will clean up the idle service and the notification is removed. Once in this non-foreground state, any calls to the service must result in a call to service.startForeground(int, Notification). If not, the exception is thrown that complains about this method not being called.

That's what happens when the skip buttons are pressed in that state. A notification action results in media button action (ACTION_MEDIA_BUTTON) that is handled in the onStartCommand method of the MediaSessionService. When paused, only the play action can possibly make MediaNotificationHandler call startForeground by toggling playWhenReady back to true.

With the current implementation and design, the missing piece when in the described state, is hiding all actions except such actions that put the service into the foreground after begin executed. I look into doing this.

[spun]

I agree, the service foreground/background behavior with playWhenReady is correct and the system removing the service also works fine, the notification will disappear and this won't be an issue. I have no problem with that.

My issue is with the actions you can do when the app is in that period where the service is in the background and the OS hasn't removed it yet. One fix is, like you said, hiding those actions but, wouldn't it be easier if the actions know what is expected from them and do the right call?

I mean, right now the PLAY action is expected to put the service in the foreground and that's why it calls getForegroundService, PAUSE and STOP actions are not expected to put the service in the foreground, they call getService. SKIP_TO_PREVIOUS/SKIP_TO_NEXT could do the same and call just use getService. With this, users could skip media without resuming if they want to.

[christosts]

@spun, thanks for your feedback. We submitted a fix as per your recommendation . I will close this issue once the commit is included in an upcoming release.

[marcbaechinger]

Closing as this is fixed and will land in the next release.