IssueTracker

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.

In conclusion, I was able to reproduce on S (Android 12), but unable to reproduce the issue on T (Android 13) or U (Android 14). I'm attaching the project I used for testing in case you find anything wrong.

Can you use that project to reproduce the issue? If yes, we need to figure out what we are doing differently. Also please kindly include a bugreport (running adb bugreport shortly after the leak is detected) if you are able to repro.

Please assign back to me once you've replied (if you are not able to, ignore).

I've beefed up your example to make it repro the leaks that I attached in #12. I'll attach the updated example. The relevant changes were:

1. Run service & receiver in separate process from Activity.
2. Customize LeakCanary to support multiple processes.
3. Add some additional media-related code to trigger a leak.

Even when not running the service in a separate process there is a leak in the Activity. I'll attach that one, too. May be a similar root cause. I have not investigated this one.

I am attaching the bugreport, too. I took it right after LeakCanary GC'ed and detected a leak. (About 5 seconds after onDestroy().) But it looks easy to reproduce now.

(I did the repro on API 33)

Also, I don't think non-Googlers are able to reassign an issue, so please take it from here.

Thanks. Will provide an update in the next few days.

Correct me if I'm wrong but the only issue we are aware that still exists is in MediaBrowserServiceCompat, right? (not in the platform MediaBrowserService)

I've sent https://android-review.googlesource.com/c/platform/frameworks/support/+/2611832 for review. Feel free to leave any comments there, or here if you don't have access to make comments in the patch.

Once merged, you should just need to update your androidx.media dependency to pick up the fix.

Thanks for the fix.

I'd think that MediaSessionCompat needs a similar fix, based on the leak traces (MediaBrowserServiceCompatLeak4.txt & MediaBrowserActivityLeak.txt) and PY's comments. You can repro MediaBrowserActivityLeak.txt by running the service in the same process as the Activity.

I'll ping PY about possibly chiming in on the code review.

The change has now been merged. I'll now look into the media session compat issue, which seems to persist.

Sorry for the late review. Not sure if you get notifications there, I left a comment: https://android-review.googlesource.com/c/platform/frameworks/support/+/2611832

TL;DR is that using weak refs isn't ideal, you could stick to a strong ref set to null from Service.onDestroy()

Ack. Will implement the proposed fix.

Есть ли утечка информации?

Есть ли утечка информации?

According to translate: Is there any information leak?

No. This is a resources leak, no security issues here.

Another fix for MediaSessionCompat...ExtraSession here: https://android-review.googlesource.com/c/platform/frameworks/support/+/2646722

Only one pending fix after this change is MediaSessionCompat...MediaSessionStub, after which I plan to mark this as fixed. Still, people should migrate to media3, but at least you can upgrade your MediaSessionCompat dep to get a quick fix of the leaks. Feel free to leave any comments here or in the patch.

https://android-review.googlesource.com/c/platform/frameworks/support/+/2646824 contains the last change. The leak fixes will be available in the next release of androidx media.

If there are more leaks to fix in androidx, please kindly file a fresh issue.

The following release(s) address this bug.It is possible this bug has only been partially addressed:

• androidx.media:media:1.7.0-beta01

Hi im wondering if the leak happening in this issue on media3's issue tracker

https://github.com/androidx/media/issues/1059#issuecomment-2031345189

is related to this.

By taking UAMP and having the MusicServiceConnection be a Dagger @Singleton, the MusicService is leaked with a very similar stack trace that is exposed here, which contains only stacktraces from androidx.media (until the MusicService, that is):

                                                                  ====================================
                                                                  HEAP ANALYSIS RESULT
                                                                  ====================================
                                                                  1 APPLICATION LEAKS
                                                                  
                                                                  References underlined with "~~~" are likely causes.
                                                                  Learn more at https://squ.re/leaks.
                                                                  
                                                                  43381 bytes retained by leaking objects
                                                                  Signature: 80f093d135adc2b278b02ca2f66fbd46aaa36bea
                                                                  ┬───
                                                                  │ GC Root: Global variable in native code
                                                                  │
                                                                  ├─ android.support.v4.media.session.MediaSessionCompat$MediaSessionImplApi21$ExtraSession instance
                                                                  │    Leaking: UNKNOWN
                                                                  │    Retaining 6,7 MB in 1826 objects
                                                                  │    ↓ MediaSessionCompat$MediaSessionImplApi21$ExtraSession.this$0
                                                                  │                                                            ~~~~~~
                                                                  ├─ android.support.v4.media.session.MediaSessionCompat$MediaSessionImplApi29 instance
                                                                  │    Leaking: UNKNOWN
                                                                  │    Retaining 6,7 MB in 1825 objects
                                                                  │    ↓ MediaSessionCompat$MediaSessionImplApi21.mSessionFwk
                                                                  │                                               ~~~~~~~~~~~
                                                                  ├─ android.media.session.MediaSession instance
                                                                  │    Leaking: UNKNOWN
                                                                  │    Retaining 1,8 kB in 12 objects
                                                                  │    mContext instance of com.example.android.uamp.media.MusicService
                                                                  │    ↓ MediaSession.mContext
                                                                  │                   ~~~~~~~~
                                                                  ╰→ com.example.android.uamp.media.MusicService instance
                                                                  ​     Leaking: YES (ObjectWatcher was watching this because com.example.android.uamp.media.MusicService received
                                                                  ​     Service#onDestroy() callback and Service not held by ActivityThread)
                                                                  ​     Retaining 43,4 kB in 896 objects
                                                                  ​     key = 11c50fab-5b0c-4267-8c51-ad235a51d861
                                                                  ​     watchDurationMillis = 5391
                                                                  ​     retainedDurationMillis = 390
                                                                  ​     mApplication instance of com.example.android.uamp.UampApplication
                                                                  ​     mBase instance of android.app.ContextImpl
                                                                  ====================================

`

I really dont see how using @Singleton is any different from the default:

companion object {
    // For Singleton instantiation.
    @Volatile
    private var instance: MusicServiceConnection? = null

    fun getInstance(context: Context, serviceComponent: ComponentName) =
        instance ?: synchronized(this) {
            instance ?: MusicServiceConnection(context, serviceComponent)
                .also { instance = it }
        }
}

The issue can be reproduced on this branch of my fork of UAMP:

https://github.com/kelmer44/uamp/tree/dagger-nocast

Which uses Dagger Hilt and removes Cast for simplification.

To reproduce:

1. Open UAMP
2. Play something
3. Swipe the app away in the task manager
4. Music Service is leaked.

From this point restarting UAMP also reveals the issue because nothing is loaded as there will be two MusicService instances.

Also note that the example uses media 1.7.0 so fixes above should be included already.

@ke...@gmail.com might be an issue with MediaSessionCompat, but we'll need a fresh ticket to look into this.

Should I create that one then?