IssueTracker

Can you please confirm whether this is still reproducible on API 33?

And if you happen to have access, also on the Developer Preview of API 34?

Final thing, is this a dup of https://issuetracker.google.com/220795151?

Thanks, I believe it's a dup at this point.

The other issue was marked as fixed a year ago, but is still as reproducible as ever, I had just tested it on API 33 and UpsideDownCake DP2.

I suppose I cannot say for sure that the two issues have the same root cause, there may be more than one cause for MediaBrowserService(Compat) leaks.

I just tested this issue here once more on my production app, on API 33 (emulator), and got a leak trace (with current LeakCanary) that closely resembles the original one I posted years ago. I let the app sit idle for another half hour, occasionally forcing a garbage collection via Android Studio Profiler, but my destroyed MediaBrowserServiceCompat (CW) instance just would not release. (I validated non-release by logging from finalize() method, which looks very reliable.)

====================================
HEAP ANALYSIS RESULT
====================================
1 APPLICATION LEAKS

References underlined with "~~~" are likely causes.
Learn more at https://squ.re/leaks.

Signature: 881c4404fc048402f456764f134ed07390863849
┬───
│ GC Root: Global variable in native code
│
├─ android.os.Handler$MessengerImpl instance
│    Leaking: UNKNOWN
│    ↓ Handler$MessengerImpl.this$0
│                            ~~~~~~
├─ androidx.media.MediaBrowserServiceCompat$ServiceHandler instance
│    Leaking: UNKNOWN
│    this$0 instance of com.bubenheimer.rucksack.d.CW
│    ↓ MediaBrowserServiceCompat$ServiceHandler.this$0
│                                               ~~~~~~
╰→ com.bubenheimer.rucksack.d.CW instance
​     Leaking: YES (ObjectWatcher was watching this because com.bubenheimer.rucksack.d.CW received Service#onDestroy()
​     callback and Service not held by ActivityThread)
​     key = 0cda35fa-fa79-4035-89ca-6ab496bd1ddc
​     watchDurationMillis = 5400
​     retainedDurationMillis = 392
​     mApplication instance of com.bubenheimer.rucksack.d.D
​     mBase instance of android.app.ContextImpl
====================================
0 LIBRARY LEAKS

A Library Leak is a leak caused by a known bug in 3rd party code that you do not have control over.
See https://square.github.io/leakcanary/fundamentals-how-leakcanary-works/#4-categorizing-leaks
====================================
0 UNREACHABLE OBJECTS

An unreachable object is still in memory but LeakCanary could not find a strong reference path
from GC roots.
====================================
METADATA

Please include this in bug reports and Stack Overflow questions.

Build.VERSION.SDK_INT: 33
Build.MANUFACTURER: Google
LeakCanary version: 2.10
App process name: com.bubenheimer.rucksack
Class count: 28324
Instance count: 179717
Primitive array count: 132390
Object array count: 25025
Thread count: 39
Heap total bytes: 27055417
Bitmap count: 0
Bitmap total bytes: 0
Large bitmap count: 0
Large bitmap total bytes: 0
Db 1: open /data/user/0/com.bubenheimer.rucksack/databases/com.google.android.datatransport.events
Db 2: open /data/user/0/com.bubenheimer.rucksack/databases/rucksack-5031356898991293088.db
Stats: LruCache[maxSize=3000,hits=32348,misses=41728,hitRate=43%]
RandomAccess[bytes=1991236,reads=41728,travel=35597905162,range=32212305,size=40115227]
Heap dump reason: 1 retained objects, app is visible
Analysis duration: 16991 ms
Heap dump file path: /storage/emulated/0/Download/leakcanary-com.bubenheimer.rucksack/2023-04-25_11-53-35_598.hprof
Heap dump timestamp: 1682438042701
Heap dump duration: 4546 ms
====================================

I ended up letting the app sit idle for an hour, but the service still would not finalize.

I also tried this, but still no finalization:

adb shell am send-trim-memory com.bubenheimer.rucksack:appexternal RUNNING_CRITICAL

I've dug into this further, and identified a leak in AndroidX code that I was using (https://issuetracker.google.com/issues/279625765).

Eliminating this leak lets the MediaBrowserService(Compat) call its finalizer eventually. I assume that the leak-related fixes in API 33 are also essential for reaching this stage.

However, there is still some unidentified holdup between onDestroy() and finalize() in my production app triggering various LeakCanary traces, before finalization after about 30 seconds. (LeakCanary triggers garbage collections, but those do not force finalization in this case.)

I assume the holdup is related to more general problems with service/binder/messenger cleanup. It's an issue, because it makes it very difficult to spot more real problems.

Can anything be done to eliminate these delays?

I am attaching the various leak traces I got from excluding each, one at a time (this is how I eventually found the AndroidX leak).

I think the issue can be closed, I am no longer able to reproduce the delayed finalization issue, and I am hoping to switch to media3 MediaSessionService anyway. I have not tried the minimal repro again, though. Ultimately I think the API 33 fixes took care of what was broken here, in addition to avoiding the referenced AndroidX leak.

Thank you for your effort to reopen this.

PY Ricau, the guy behind LeakCanary, suggested that there is more to this, and that there are several actual leaks here. I will update this issue with more info as soon as I find some time, I'm in a bit of a crunch right now.

The 3 LeakCanary leak reports that I attached above point at 2 distinct leaks that are still real, according to PY's analysis from the Github issue here: https://github.com/square/leakcanary/issues/2502#issuecomment-1550455263

Can these leaks be addressed, please?

MediaBrowserServiceCompat$ServiceHandler is an inner class of MediaBrowserServiceCompat (super class of CW) and it's there so that your service can respond to IPC callbacks. It extends Handler and is passed to a Messenger instance created by MediaBrowserServiceCompat in onCreate(), and that Messenger essentially pulls a Binder out of the Handler and passes that binder back to anything calling your service. In other words, every time something binds to your service, the thing that binds to your service is getting an indirect reference to the ServiceHandler which allows it to post messages to that handler which the service will eventually process. However, for this to work, the native framework code keeps a strong native reference locally to the binder until the calling service finalizes the ownership of the binder on its side.

This is actually a common leak pattern in the Android framework: binders are held until the other side runs garbage collection and finalizes the binder on its end, no matter what we're doing with service lifecycle (that's because binders are used beyond services, the only way to be certain the other side won't call is when it doesn't have that pointer). The proper way to handler this is for the service code to finish its handler in onDestroy, or set the reference from the handler to the service to null in onDestroy.

The first and second leak traces are identical, the problem is that MediaBrowserServiceCompat$ServiceHandler has several direct and indirect references to the service.

The last leak is the exact same issue elsewhere, ExtraSession is a stub that won't be GCed until the other side has run its own GC https://github.com/aosp-mirror/platform_frameworks_support/blob/a9ac247af2afd4115c3eb6d16c05bc92737d6305/media/src/main/java/android/support/v4/media/session/MediaSessionCompat.java#L3413

So these 2 leaks are binder related leaks, and the leaks stay in place until a GC runs in the calling process, and you have no control over that. They really should be fixed.

I've not been able to reproduce the leak on a device running Android 14 (UDC / API 34).

For unimportant reasons I had to build my own toy app from scratch (using https://github.com/bubenheimer/mediabrowserservicecompatleak as a base). I will now try said app on older Android versions and, if unable to repro, I'll share the app back with you for input on what step I might be missing.