session: Avoid ForegroundServiceDidNotStartInTimeException...

[nift4]

...and throw more useful exceptions instead.

This will however not throw exceptions where foreground service
start is not absolutely required, and keep logging warnings in
these cases instead.

Issue: #2591
Issue: #2622

(This PR includes PR #2256 because they conflict. It would probably make sense to merge #2256 first.)

[nift4]

As requested earlier, a brief reasoning of the design choices:
Throwing and giving up with a clear error message seems to be the best we can do, because:

1. there's no way to start the service from a broadcast reciever without the system enforcing a foreground service will start after we tell it to (bindService hence MediaController isn't supported from a broadcast reciever, and startService without foreground will - if I understand correctly - not transfer the foreground allowlist to our service)
2. if the system enforces the service will start after we tell it to, shouldStartForegroundService() should be overwritten for every app that supports logging out / empty playlists / etc, but it's not very discoverable
3. the consequence of not doing that is a cryptic ForegroundServiceDidNotStartInTimeException

1 is a hard fact and we can't do anything about it, 2 is more of a documentation problem and it seems external contributors can't edit those anyway, but 3 is possible to address - and it will give a concrete improvement for everyone suffering from ForegroundServiceDidNotStartInTimeException without understanding the reason, as it makes it practically impossible for the library to generate one now. I myself had ForegroundServiceDidNotStartInTimeException in Play Console because I didn't realize shouldStartForegroundService() is required, but with this patch there'd be a useful explainer instead.

I also don't see any disadvantage of this patch for SDK>=31, and I consider the added sanity check on <31 an advantage because developers that don't always test on Android 12+ will still get notified about the fact their code is wrong.

[jackyhieu1211]

@marcbaechinger

Hi mr @marcbaechinger

I’m currently facing the ForegroundServiceDidNotStartInTimeException crash in production, and this PR seems to address it by handling the exception and reporting it to Firebase without crashing the app.

This issue is still happening frequently on Firebase Crashlytics.

I would like to ask:

1. Is this PR considered ready from your side?
2. Is there any plan or timeline to merge it into the main branch?
3. If any additional changes are required before merge, could you share the current status?

At minimum, what I need is that when this exception occurs, it should not be reported as a crash on Firebase Crashlytics. This affects real users, so any update would be very helpful.

Thank you!

[nift4]

this PR seems to address it by handling the exception and reporting it to Firebase without crashing the app

No, that is sadly not possible. The root cause would have to be addressed for this, and it is often different for each individual app, there's no general library bug that causes this in 1.9.x AFAIK (previous versions did have bug #2768). What this PR does is instead make the crash clearly tell what the immediate reason for the crash (ie, no media items returned) is, so it is easier to investigate.

[marcbaechinger]

Thanks for the insight Nick!

@jackyhieu1211

this PR seems to address it by handling the exception and reporting it to Firebase without crashing the app

May I ask you a file a bug and explain in a bit more detail what you are seeing? LIke a stack trace, bug report or similar and what your idea is how it's caused?

If this is about a crash in onPlaybackResumption() or when adding an empty list from there (as mentioned in the description of this change), then I think we can (want) address this with a less intrusive change. Need to be evaluated, but I think it would make sense to file a bug and add a bit more of information.

[jackyhieu1211]

@nift4 @marcbaechinger

Hi, thanks for the suggestion!

1. I cannot reproduce the issue locally. I only receive this crash from Firebase Crashlytics, so I don’t have a reliable way to trigger it manually.

2. To explain more: recently Google Play has introduced stricter App Quality checks. For apps in the “Play Video” category, the maximum allowed crash rate is around 1.09%.
   However, this specific crash alone accounts for 2–5% of all crashes, which means the app is flagged as a low-quality app on Play Store.
   Because of that, my intention is simply: if the user hits this issue, the app should return an error instead of crashing completely.

3. From my perspective, this PR solves exactly that: it prevents the hard crash. The logic still reports an error, but the app stays alive.

4. I’ll continue investigating. If I discover anything more—such as steps to reproduce or additional insights—I’ll file a more detailed bug report.

Thanks again!