Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve detection of leaky receivers? #2091

Closed
pyricau opened this issue Mar 18, 2021 · 3 comments · Fixed by #2100
Closed

Improve detection of leaky receivers? #2091

pyricau opened this issue Mar 18, 2021 · 3 comments · Fixed by #2100
Milestone

Comments

@pyricau
Copy link
Member

pyricau commented Mar 18, 2021

This comes from Stack Overflow: https://stackoverflow.com/q/66611127/703646

The leaktrace looked like this:

   ┬───
    │ GC Root: System class
    │
    ├─ android.provider.FontsContract class
    │    Leaking: NO (MyApplication↓ is not leaking and a class is never leaking)
    │    ↓ static FontsContract.sContext
    ├─ com.example.MyApplication instance
    │    Leaking: NO (Application is a singleton)
    │    mBoundService instance of com.example.services.SessionService
    │    mBase instance of android.app.ContextImpl
    │    ↓ Application.mLoadedApk
    │                  ~~~~~~~~~~
    ├─ android.app.LoadedApk instance
    │    Leaking: UNKNOWN
    │    Retaining 302.8 kB in 4641 objects
    │    mApplication instance of com.example.MyApplication
    │    ↓ LoadedApk.mReceivers
    │                ~~~~~~~~~~
    ├─ android.util.ArrayMap instance
    │    Leaking: UNKNOWN
    │    Retaining 301.7 kB in 4615 objects
    │    ↓ ArrayMap.mArray
    │               ~~~~~~
    ├─ java.lang.Object[] array
    │    Leaking: UNKNOWN
    │    Retaining 301.7 kB in 4613 objects
    │    ↓ Object[].[2]
    │               ~~~
    ╰→ com.example.activities.SelectActivity instance
        ​     Leaking: YES (ObjectWatcher was watching this because com.example.activities.SelectActivity received
    ​         Activity#onDestroy() callback and Activity#mDestroyed is true)
    ​         Retaining 298.2 kB in 4579 objects
    ​         key = 68660c30-bc17-4a74-a5e2-c54f6d676c59
    ​         watchDurationMillis = 5185
    ​         retainedDurationMillis = 183
    ​         mApplication instance of com.example.MyApplication
    ​         mBase instance of androidx.appcompat.view.ContextThemeWrapper

Apparently this was caused by a receiver registered twice. Worth trying to repro and then see how we could make leakcanary provide a better report for these.

@pyricau
Copy link
Member Author

pyricau commented Mar 25, 2021

Here's a repro:

class MainActivity : Activity() {
  class NoOpBroadcastReceiver : BroadcastReceiver() {
    override fun onReceive(
      context: Context,
      intent: Intent
    ) = Unit
  }
  override fun onDestroy() {
    super.onDestroy()
    Handler().postDelayed({
      registerReceiver(NoOpBroadcastReceiver(), IntentFilter())
    }, 500)
  }
}

Activity receiver leaks aren’t supposed to happen. Sometimes after Activity.onDestroy(), there’s a “final cleanup” callback that ensures all registered receivers and unregistered, and drops a message in logcat yelling at you because you should have done that already. However, here, we’re using a post delayed so we are registering to the destroyed activity AFTER the cleanup has happened. The proper behavior would likely be for Android to throw an exception (don’t register on a destroyed activity) or at least a log + no-op. However this goes through just fine.

The receiver itself does not hold any reference to the activity, which will make that leak really hard to track, because this bad registration won’t be visible anywhere in the leak trace:

pyricau added a commit that referenced this issue Mar 25, 2021
pyricau added a commit that referenced this issue Mar 26, 2021
pyricau added a commit that referenced this issue Mar 26, 2021
@pyricau pyricau added this to the 2.7 milestone Mar 26, 2021
@vanniktech
Copy link
Contributor

@pyricau can the same be done for services?

┬───
│ GC Root: Global variable in native code
│
├─ com.vanniktech.meditationtimer.MeditationTimerApplication instance
│    Leaking: NO (Application is a singleton)
│    mBase instance of android.app.ContextImpl
│    ↓ Application.mLoadedApk
│                  ~~~~~~~~~~
├─ android.app.LoadedApk instance
│    Leaking: UNKNOWN
│    Retaining 684.6 kB in 11606 objects
│    mApplication instance of com.vanniktech.meditationtimer.MeditationTimerApplication
│    Receivers
│    ..MeditationTimerApplication@318809616
│    ....EC0@323502664
│    ....ProxyChangeListener$ProxyReceiver@323502720
│    ....O80@323502784
│    ....f@323502848
│    ....w6@323502912
│    ....VisibilityTracker@323502976
│    ....x70@323503048
│    ....p@323503104
│    ....dj@321051072
│    ....am@322244752
│    ....lw@322121976
│    ....q@323503240
│    ....bx@321880120
│    ....s6@323503304
│    ....wq0@323503368
│    ↓ LoadedApk.mServices
│                ~~~~~~~~~
├─ android.util.ArrayMap instance
│    Leaking: UNKNOWN
│    Retaining 675.4 kB in 11535 objects
│    ↓ ArrayMap.mArray
│               ~~~~~~
├─ java.lang.Object[] array
│    Leaking: UNKNOWN
│    Retaining 675.3 kB in 11533 objects
│    ↓ Object[].[4]
│               ~~~
╰→ com.vanniktech.meditationtimer.MeditationTimerMainActivity instance
     Leaking: YES (ObjectWatcher was watching this because com.vanniktech.meditationtimer.MeditationTimerMainActivity
     received Activity#onDestroy() callback and Activity#mDestroyed is true)
     Retaining 673.3 kB in 11511 objects
     key = c10fa223-ffa3-4523-ba85-611e5fab414c
     watchDurationMillis = 218555
     retainedDurationMillis = 213555
     mApplication instance of com.vanniktech.meditationtimer.MeditationTimerApplication
     mBase instance of androidx.appcompat.view.ContextThemeWrapper
METADATA
Build.VERSION.SDK_INT: 30
Build.MANUFACTURER: Google
LeakCanary version: 2.7
App process name: com.vanniktech.meditationtimer
Stats: LruCache[maxSize=3000,hits=4700,misses=97445,hitRate=4%]
RandomAccess[bytes=4984938,reads=97445,travel=46095612296,range=28247320,size=34799969]
Heap dump reason: 3 retained objects, app is not visible
Analysis duration: 5907 ms

ghost pushed a commit to shivagowda/leakcanary that referenced this issue Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants