On Android O (when targeting the new SDK in the build!) we can only call startForeground() in a service if it was started with startForegroundService(). Our code calling startForeground(): https://dxr.mozilla.org/mozilla-central/search?q=path%3Amobile+startForeground&redirect=true

Maybe Max knows where it's used

Sebastian: What is the effect of this bug to a user? Or is it a build issue?

I think this might actually be the other way round? I.e. you cannot call startService() when the app is in background and will receive an IllegalStateException instead. You may however call the new startForegroundService() method, but this *must* be followed by a call to startForeground() within a few seconds, otherwise the app will crash. From what I've read, the crash will still happen even if you stop the service again before the timeout expires, though. Alternatively, switching to bindService() instead of startService() instead would work, too - and at least for the MediaControlService being able to directly call methods on a simple local service object might be a better fit than using a shedload of intent actions like we currently do and having to call startService() each time we need to notify it about some relevant state change. The only other place where we use startForeground() is the NotificationService, where the fact that we also use startService to shut the service down again (https://dxr.mozilla.org/mozilla-central/rev/e0eacfc97568bc270e61839c60ae787fb76c4f07/mobile/android/base/java/org/mozilla/gecko/notifications/NotificationService.java#27-30) might potentially be a bit problematic.

I find it concerning that people are still reporting crashes even after abiding by these changes. https://issuetracker.google.com/issues/76112072 While refactoring the MediaControlService to a bound service keeps us clear, we might still see quite a few of these crashes from the NotificationService until Google addresses this unless we find a workaround.

Changes depend on Bug 1450447, so this patch should be landed after the notification channels issue. Submitting it now so we can get going with the review, I'm going to make the changes after that one gets resolved. NI-ing Mike for his opinion on comm 4

Comment on attachment 8980353 [details] Bug 1384866 - refactored MediaControlService, updated NotificationService for Oreo. https://reviewboard.mozilla.org/r/246520/#review252626 General note: It would be nicer to split clearly separable changes into separate changesets to make reviewing easier. At the very least, that would mean separate commits for the MediaControlService and the NotificationService. ::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java (Diff revision 1) > } > > Log.d(LOGTAG, "onStateChanged, state = " + mMediaState); > > if (isNeedToRemoveControlInterface(mMediaState)) { > - stopForeground(false); We still need this so that the service is really considered a foreground service while we're playing music, don't we? ::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:398 (Diff revision 1) > .setShowWhen(false) > .setWhen(0) > .setVisibility(visibility) > .build(); > > - if (isPlaying) { > + NotificationManagerCompat.from(this).notify(R.id.mediaControlNotification, notification); Ditto.

@Jan, I think that we need to use startForeground and subsequently stopForeground in order to flag Android for when our service has priority and shouldn't be killed under memory pressure. This prevents disruptive scenarios such as Android killing the service while playing music to the user which can be noticed. When using bindService, as per docs: * The service will remain running as long as the connection * is established (whether or not the client retains a reference on the * service's IBinder) Isn't this good enough for our case ?

(In reply to Vlad Baicu from comment #8) > @Jan, I think that we need to use startForeground and subsequently > stopForeground in order to flag Android for when our service has priority > and shouldn't be killed under memory pressure. This prevents disruptive > scenarios such as Android killing the service while playing music to the > user which can be noticed. > > When using bindService, as per docs: > * The service will remain running as long as the connection > * is established (whether or not the client retains a reference on the > * service's IBinder) Yes, the service might be kept alive as long as somebody has bound to it, but in practice the service is running in the same process as the rest of Firefox and Firefox is the only one who will actually bind to that service. So if we're considered a background app (and if our UI is actually backgrounded and we're not using startForeground(), we probably will be sooner or later), Android will still likely kill our process under memory pressure, in which case the connection to the MediaControlService will be gone as well. So in order to be considered a foreground app while playing back music with our UI in background, I'm relatively certain that we still need to use start/stopForeground as appropriate.

Although give the existence of the BIND_ALLOW_OOM_MANAGEMENT flag, you might have a point there, although I'm not sure whether a foreground app/service might not still be considered even higher priority than a bound service. Plus as I mentioned we'd want startForeground to get higher CPU scheduling priority as well.

Comment on attachment 8980574 [details] Bug 1384866 - Refactored MediaControlService. https://reviewboard.mozilla.org/r/246720/#review252974 ::: mobile/android/base/java/org/mozilla/gecko/media/AudioFocusAgent.java:218 (Diff revision 1) > // The notification only works from Lollipop onwards (at least until we try using > // the support library version), so there's no point in starting the service. > return; > } > > + if (mServiceBound) { Binding to a service is async, so this can potentially cause problems if you get a second event before the service has fully started, as you'll try binding the service again. Since we also cannot use startService (see other comment), we might have to do some command queueing ourselves: I.e. ```java if (mServiceBound) { mediaControlService.handleAction(action); } else { queueCommand(action); if (!mServiceStarting) { Intent intent = new Intent(mContext, MediaControlService.class); mContext.bindService(intent, this, Context.BIND_AUTO_CREATE); mServiceStarting = true; } } ``` Then, in `onServiceConnected` you reset `mServiceStarting` to false and execute all queued commands. Although again, feel free to suggest a better design if you can think of one. ::: mobile/android/base/java/org/mozilla/gecko/media/AudioFocusAgent.java:223 (Diff revision 1) > + if (mServiceBound) { > + mediaControlService.handleAction(action); > + } else { > - Intent intent = new Intent(mContext, MediaControlService.class); > + Intent intent = new Intent(mContext, MediaControlService.class); > - intent.setAction(action); > + intent.setAction(action); > - mContext.startService(intent); > + mContext.startService(intent); If you're still calling startService unconditionally, then you will still run into trouble on Android O once we raise the target API, won't we? I.e. if music playback starts while we're considered a background app, then calling `startService` will trigger an IllegalStateException once we're targetting Oreo, which was the very problem this refactoring was intended to avoid. I'm not sure if the following proposal is really the best design, so feel free to think of something better, but one thing that might work is: - The AudioFocusAgent only ever binds to the service when required, and never calls `startService`. - Within the MediaControlService, when we want to show a notification and *know* for sure that we're going to use `startForeground`, we additionally start the service, using `startServiceForeground` on Android O and above and then, once "started", show the notification through `startForeground`. - When we use `stopForeground`, e.g. because playback is paused, we call `stopService` as well, although the service will of course still remain alive because the AudioFocusAgent remains bound to it. - Only when we fully hide the notification and want the service to really stop running do we cause the AudioFocusAgent to unbind us as well. ::: mobile/android/base/java/org/mozilla/gecko/media/AudioFocusAgent.java:224 (Diff revision 1) > + mediaControlService.handleAction(action); > + } else { > - Intent intent = new Intent(mContext, MediaControlService.class); > + Intent intent = new Intent(mContext, MediaControlService.class); > - intent.setAction(action); > + intent.setAction(action); > - mContext.startService(intent); > + mContext.startService(intent); > + mContext.bindService(intent, this, Context.BIND_AUTO_CREATE); If we're still using startForeground to get foreground priority while actually playing back music, maybe use also use `BIND_ALLOW_OOM_MANAGEMENT` here, so we're more like a normal started service as long as we don't actually need foreground priority?

Comment on attachment 8980574 [details] Bug 1384866 - Refactored MediaControlService. https://reviewboard.mozilla.org/r/246720/#review252982 ::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java (Diff revision 1) > .setWhen(0) > .setVisibility(visibility) > .build(); > > - if (isPlaying) { > + NotificationManagerCompat.from(this).notify(R.id.mediaControlNotification, notification); > - startForeground(R.id.mediaControlNotification, notification); What I previously said: I still think we want startForeground to make sure that Firefox gets foreground priority when playing back music - both for memory and CPU scheduling.

(In reply to Vlad Baicu from comment #6) > NI-ing Mike for his opinion on comm 4 What's the question here? My understanding is that in order to perform background work, we need to do one of: 1) Call `startForegroundService(); startService();` before `stopService();` 2) Use JobScheduler/WorkManager Bound services seem to run for the lifecycle of another component so aren't really "background work" persay and I don't actually understand why you'd want to use them over threads (maybe because the Service is easier to share between multiple components than an in-progress thread is?). If someone knows the answer, feel free to NI me (no action required) so I learn. :)

Jan, would you want to review this patches for me? You seem pretty informed about the new requirements for Services and how they work. :) If so, feel free to take my r? and NI me when this is r+ for me to glance over and land. If not, please NI me again. Thanks!

@Micheal: The current setup is that the AudioFocusAgent is responsible for gathering all relevant tab- and media-related events and forwarding them to the MediaControlService using startService with a corresponding Intent action, which also has the beneficial side effect of starting the service if it isn't currently running. The problem is that on Android O you can no longer call startService if the app is in background, e.g. if we're in background and then some tab starts playing sound. You *can* instead call startForegroundService, but in that case you *must* also call startForeground from the service (no idea though if that only applies if the service was freshly started, or whether you have to call startForeground again even if the service was *already* running in the foreground...). The problem is that with the current setup the AudioFocusAgent doesn't actually know for sure what the MediaControlService is going to do, i.e. whether a call the startForegroundService will really be followed by a call to startForeground from the service itself. Hence the idea was that the AudioFocusAgent would get the service up and running by binding to it instead (which isn't subject to any background execution restrictions) and then the service would start itself through startForegroundService only if it knew for sure that it was going to show a foreground notification for some currently running media playback. To be honest I don't know how workable my idea is in practice, though - a practical implementation might hit some snags that could make things less straightforward than I'd imagined, especially regarding events that might cause us to bind and unbind/start and stop the service in quick succession.

@Vlad: Since the MediaControlService might prove more tricky, might it make sense to move the NotificationService changes which look more straightforward into their own bug blocking this one?

I think calling bindService after startService even in background will not trigger the illegal state exception. Besides the command queueing, to my understading we want our service to be a bound + foreground service which should work without any issues and allowing us to bypass the usage of startForegroundService. My biggest concern with the new startForegroundService call, is that during my research I came across various reports on Google's tracker where people are complaining that even with immediate calls to startForeground in onCreate/onStartCommand they are having an alarming amount of crash reports - https://issuetracker.google.com/issues/76112072. Apparently Google cannot guarantee that onStartCommand will be called before the ANR interval. Furthermore, Android P seems to be hit by this issue as well. I think that we are going to experience this as well, but I don't know if there is much we can do, besides refactoring where possible such as in this case with the MediaControlService.

(In reply to Vlad Baicu from comment #19) > I think calling bindService after startService even in background will not > trigger the illegal state exception. No, but calling startService in the first place while the app is in background will, once we target Android O or above. See https://developer.android.com/about/versions/oreo/android-8.0-changes#back-all: "The startService() method now throws an IllegalStateException if an app targeting Android 8.0 tries to use that method in a situation when it isn't permitted to create background services." startForegroundService() avoids the IllegalStateException, but introduces the dependency on a timely call to startForeground afterwards. PS.: You can just change the commit message so you don't have to keep manually adjusting the reviewer after the fact. (Or at least manually change the reviewer through Mozreview before publishing the updated review.)

I see no question in comment 17 so clearing NI – please re-add NI if I'm misinterpreting! > My biggest concern with the new startForegroundService call, is that during my research I > came across various reports on Google's tracker where people are complaining that even with > immediate calls to startForeground in onCreate/onStartCommand they are having an alarming amount of crash reports Vlad, in the bug you linked, I think the issue people are experiencing is that they're calling: startForegroundService(); /* maybe do things */ stopService();` in order to stop the service but it appears you need you would need to call: startForegroundService(); /* ... */ startForeground(); /* ... */ stopService(); I'm interpreting this from the WONTFIX Google comment [1]. [1]: https://issuetracker.google.com/issues/76112072#comment36

@Michael: Yes, that NI was only in response to > If someone knows the answer, feel free to NI me (no action required) so I learn. :) (In reply to Michael Comella (:mcomella) [needinfo or I won't see it] from comment #22) > in order to stop the service but it appears you need you would need to call: > startForegroundService(); > /* ... */ > startForeground(); > /* ... */ > stopService(); Which would mean showing a notification for however briefly, though, so with the current design we'd relatively certainly end up with situations where we'd briefly (and unnecessarily) pop up the media control notification, e.g. through bug 1454712. @Vlad: If binding the service turns out too problematic, there's one possible alternative design I could think of: Create a new MediaControlSomething singleton class similar to the AudioFocusAgent (MediaControlAgent, MediaControlNotification, MediaControlLogic, ...?) and then - move all Tab event-related and similar logic that was moved into the AudioFocusAgent in bug 1416169 into this new class - likewise move all logic that's possible to move from the MediaControlService, especially the code handling *when* the notification should be shown and then turn the MediaControlService into a simple shell service similar to the NotificationService that is only responsible for - actually manipulating the notification and calling start/stopForeground as required (i.e. the new MediaControlSomething class sends an intent to the MediaControlService whenever it wants to show/change/hide the notification, if necessary attaching the notification as an intent extra), and - receiving any pending intents from the notification and forwarding them back to the MediaControlSomething class.

I actually like this design alternative a lot better, thank you Jan, you are being very helpful, I'll look into the other bugs as well and see what I can do.

Comment on attachment 8980574 [details] Bug 1384866 - Refactored MediaControlService. Cancelling review for now as per > I actually like this design alternative a lot better, thank you Jan, you are being very helpful, I'll look into the other bugs as well and see what I can do. Sorry for leading you down a dead end with the bound service idea.

I'll look at this, but it'll have to wait until I get back home.

Comment on attachment 8985032 [details] Bug 1384866 - Refactored MediaControlService logic to GeckoMediaControlAgent. https://reviewboard.mozilla.org/r/250756/#review258594 Looks okay ::: mobile/android/base/java/org/mozilla/gecko/media/AudioFocusAgent.java:31 (Diff revision 1) > private AudioManager mAudioManager; > private OnAudioFocusChangeListener mAfChangeListener; > > private WeakReference<Tab> mTabReference = new WeakReference<>(null); > > + private MediaControlAgent mediaControlAgent; `private final MediaControlAgent mMediaControlAgent = MediaControlAgent.getInstance();` ::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlAgent.java:1 (Diff revision 1) > +package org.mozilla.gecko.media; Add license header ::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlAgent.java:37 (Diff revision 1) > +import org.mozilla.gecko.Tab; > +import org.mozilla.gecko.annotation.RobocopTarget; > +import org.mozilla.gecko.util.ThreadUtils; > + > +public class MediaControlAgent { > + private static final String LOGTAG = "MediaControlAgent"; "GeckoMediaControlAgent" ::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlAgent.java:41 (Diff revision 1) > +public class MediaControlAgent { > + private static final String LOGTAG = "MediaControlAgent"; > + > + // We're referencing the *application* context, so this is in fact okay. > + @SuppressLint("StaticFieldLeak") > + private static Context mContext; Since `MediaControlAgent` is a singleton, this should be an instance field instead of static. ::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlAgent.java:69 (Diff revision 1) > + private HeadSetStateReceiver mHeadSetStateReceiver; > + > + private PrefsHelper.PrefHandler mPrefsObserver; > + private final String[] mPrefs = { MEDIA_CONTROL_PREF }; > + > + private boolean mInitialize = false; `mInitialized` ::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlAgent.java:96 (Diff revision 1) > + } > + > + private static class SingletonHolder { > + // We're referencing the *application* context, so this is in fact okay. > + @SuppressLint("StaticFieldLeak") > + private static final MediaControlAgent INSTANCE = new MediaControlAgent(); You can put `INSTANCE` inside `MediaControlAgent`. Using `SingletonHolder` doesn't really have an advantage here. ::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlAgent.java:123 (Diff revision 1) > + > + if (!isAndroidVersionLollipopOrHigher()) { > + return; > + } > + > + Log.d(LOGTAG, "initialize"); Put debug logging inside `if (DEBUG) { ... }`

Comment on attachment 8985032 [details] Bug 1384866 - Refactored MediaControlService logic to GeckoMediaControlAgent. https://reviewboard.mozilla.org/r/250756/#review258594 > Add license header Added the license header also to AudioFocusAgent and MediaControlService

Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/mozilla-inbound/rev/65677fe92b47 Refactored MediaControlService logic to GeckoMediaControlAgent. r=jchen

One drive-by question: Does the logic for showing/hiding the notification require any changes to apply the lessons learnt from bug 1465102 (hoping that the current iteration of that bug actually works without unintended side effects and crashes)?