Leak detected in standard AIDL service pattern

[jpd236]

LeakCanary is fairly consistently reporting a leak due to the Binder object created in an AIDL Service implementation after the Service is destroyed. From some digging, it seems like this is expected platform behavior, and that there was previously a PR that was meant to resolve this (or something that sounds a lot like it):

#351

but it doesn't appear to be working, at least the way it's wired up.

I've attached a sample app which demonstrates the problem: LeakTest.zip. This consists of an AIDL service which is pretty much exactly what is documented at https://developer.android.com/guide/components/aidl#Implement, and an Activity which binds to the service upon clicking a button. Clicking the button fairly reliably produces the below leak, and I can't see anything wrong with what the app is doing. The leak monitoring in the Service is done according to the example at https://square.github.io/leakcanary/recipes/#watching-objects-with-a-lifecycle.

Is this indeed a regression from that PR, or something else?

LeakTrace information

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

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

1132 bytes retained by leaking objects
Signature: 9a265ac2c81e62a8947aa84ddba98dd79dc3258e
┬───
│ GC Root: Global variable in native code
│
├─ com.example.leaktest.MainService$1 instance
│    Leaking: UNKNOWN
│    Anonymous subclass of com.example.leaktest.IMainService$Stub
│    ↓ MainService$1.this$0
│                    ~~~~~~
╰→ com.example.leaktest.MainService instance
​     Leaking: YES (ObjectWatcher was watching this because MainService received Service#onDestroy callback)
​     key = 9ce278da-0f73-4d6f-93fe-dd57582dfec1
​     watchDurationMillis = 11411
​     retainedDurationMillis = 6409
====================================
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
====================================
METADATA

Please include this in bug reports and Stack Overflow questions.

Build.VERSION.SDK_INT: 29
Build.MANUFACTURER: Google
LeakCanary version: 2.4
App process name: com.example.leaktest
Analysis duration: 3037 ms
Heap dump file path: /data/user/0/com.example.leaktest/files/leakcanary/2020-08-12_12-53-46_586.hprof
Heap dump timestamp: 1597262030944
====================================

[jpd236]

Ah, that commit seems to have been reverted in #598, so perhaps the underlying problem was just never fixed.

I guess maybe the "right" thing to do here is to just never make a Binder that depends on the outer service (using the Application context if a context is necessary)? In which case maybe the issue is with the Android documentation suggesting a bad pattern?

[pyricau]

Thanks for this @jpd236 !

You're absolutely right: this is a "bad" pattern that unfortunately happens too often. AFAIK binder instances are held until the other processes finalizes the object on its side. This can happen long after the service is unbound. What that means is that binders should always always clear their references to the components they talk to after they stop being used.

Unfortunately this has created many leaks in AOSP and apps.

Would you mind filing an issue on AOSP for this? The doc should be updated.

[jpd236]

Thanks - filed as https://issuetracker.google.com/167281719.