Memory Leak with Toasts: Use applicationContext instead of Activity #810

handstandsam · 2022-04-26T12:52:55Z

Saw a memory leak pop up in LeakCanary. May be related to Toasts using Activity Context.

Android Docs suggest using ApplicationContext for Toasts: https://developer.android.com/guide/topics/ui/notifiers/toasts

📷 Screenshots

📄 Context

Issue found in Leak Canary.

📝 Changes

Found all instances of .makeText( and ensured it's being passed applicationContext.

📎 Related PR

🚫 Breaking

No.

🛠️ How to test

Use these features of the library and ensure that the toasts are still properly shown.

⏱️ Next steps

This is it, just merging.

Leak Trace from Leak Canary:

┬───
│ GC Root: Thread object
│
├─ ...RealLocalThumbManager$Thumbnailer instance
│    Leaking: NO (...Application↓ is not leaking)
│    Thread name: 'LocalThumbManager$Thumbnailer'
│    mContext instance of ...Application
│    ↓ RealLocalThumbManager$Thumbnailer.mContext
├─ ...Application instance
│    Leaking: NO (Application is a singleton)
│    mBase instance of android.app.ContextImpl
│    ↓ Application.mLoadedApk
│                  ~~~~~~~~~~
├─ android.app.LoadedApk instance
│    Leaking: UNKNOWN
│    Retaining 1.0 MB in 14258 objects
│    mApplication instance of ...Application
│    Receivers
│    ..HyperionService@333835072
│    ....HyperionService$OpenMenuReceiver@333837384
│    .....Application@325642976
│    ....LockReceiver@330442336
│    ....SystemBroadcastReceiver@329566192
│    ....K60@333852472
│    ....AppStateInitializerKt$registerReceivers$1@333852536
│    ....ak@333852600
│    ....f@333852656
│    ....ProxyChangeListener$ProxyReceiver@333852720
│    ....CancelUploadsIntentReceiver@333852784
│    ....ny0@327172256
│    ....w50@333852880
│    ....z6@333852936
│    ....VisibilityTracker@333853000
│    ....dl@333853072
│    ....Dispatcher$NetworkBroadcastReceiver@330534912
│    ....eo0@333853136
│    ....D6@333853200
│    ↓ LoadedApk.mServices
│                ~~~~~~~~~
├─ android.util.ArrayMap instance
│    Leaking: UNKNOWN
│    Retaining 999.3 kB in 14190 objects
│    ↓ ArrayMap.mArray
│               ~~~~~~
├─ java.lang.Object[] array
│    Leaking: UNKNOWN
│    Retaining 999.3 kB in 14188 objects
│    ↓ Object[4]
│            ~~~
╰→ com.chuckerteam.chucker.internal.ui.MainActivity instance
     Leaking: YES (ObjectWatcher was watching this because com.chuckerteam.chucker.internal.ui.MainActivity received Activity#onDestroy() callback and Activity#mDestroyed is true)
     Retaining 997.1 kB in 14153 objects
     key = 0f6874e8-79a2-493d-b176-ab5db9ebdff6
     watchDurationMillis = 6171
     retainedDurationMillis = 1171
     mApplication instance of ...Application
     mBase instance of androidx.appcompat.view.ContextThemeWrapper

…cumentation on Toasts to see that ApplicationContext is what should be used. https://developer.android.com/guide/topics/ui/notifiers/toasts

handstandsam · 2022-04-26T12:53:50Z

library/src/main/kotlin/com/chuckerteam/chucker/internal/ui/MainActivity.kt

@@ -136,12 +136,11 @@ internal class MainActivity :
 }

 private fun exportTransactions(fileName: String, block: suspend (List<HttpTransaction>) -> Sharable) {
+ val applicationContext = this.applicationContext


Because a coroutine is being launched below that tries to access the Activity, we would be holding an instance. By pulling out applicationContext into a local shadow val, we don't need to retain an instance of the Activity.

vbuberen · 2022-04-27T19:41:16Z

The PR looks good, but could you share the LeakCanary log/screenshot with this leak?

Also, the link you shared mentions application context, but if we open docs for makeText it mentions that both application and activity are both valid options: https://developer.android.com/reference/android/widget/Toast#makeText(android.content.Context,%20java.lang.CharSequence,%20int)

cortinico

Also, the link you shared mentions application context, but if we open docs for makeText it mentions that both application and activity are both valid options: developer.android.com/reference/android/widget/Toast#makeText(android.content.Context,%20java.lang.CharSequence,%20int)

+1 on this.
I think this is safe to land, and I'm fine merging it regardless of this being the root cause of the leak. Curious to see the leakcanary output if we have it though 👍

handstandsam · 2022-05-03T01:41:36Z

@vbuberen @cortinico Sorry for the delay. Here was the leak that was caught (with some information redacted):

┬───
│ GC Root: Thread object
│
├─ ...RealLocalThumbManager$Thumbnailer instance
│    Leaking: NO (...Application↓ is not leaking)
│    Thread name: 'LocalThumbManager$Thumbnailer'
│    mContext instance of ...Application
│    ↓ RealLocalThumbManager$Thumbnailer.mContext
├─ ...Application instance
│    Leaking: NO (Application is a singleton)
│    mBase instance of android.app.ContextImpl
│    ↓ Application.mLoadedApk
│                  ~~~~~~~~~~
├─ android.app.LoadedApk instance
│    Leaking: UNKNOWN
│    Retaining 1.0 MB in 14258 objects
│    mApplication instance of ...Application
│    Receivers
│    ..HyperionService@333835072
│    ....HyperionService$OpenMenuReceiver@333837384
│    .....Application@325642976
│    ....LockReceiver@330442336
│    ....SystemBroadcastReceiver@329566192
│    ....K60@333852472
│    ....AppStateInitializerKt$registerReceivers$1@333852536
│    ....ak@333852600
│    ....f@333852656
│    ....ProxyChangeListener$ProxyReceiver@333852720
│    ....CancelUploadsIntentReceiver@333852784
│    ....ny0@327172256
│    ....w50@333852880
│    ....z6@333852936
│    ....VisibilityTracker@333853000
│    ....dl@333853072
│    ....Dispatcher$NetworkBroadcastReceiver@330534912
│    ....eo0@333853136
│    ....D6@333853200
│    ↓ LoadedApk.mServices
│                ~~~~~~~~~
├─ android.util.ArrayMap instance
│    Leaking: UNKNOWN
│    Retaining 999.3 kB in 14190 objects
│    ↓ ArrayMap.mArray
│               ~~~~~~
├─ java.lang.Object[] array
│    Leaking: UNKNOWN
│    Retaining 999.3 kB in 14188 objects
│    ↓ Object[4]
│            ~~~
╰→ com.chuckerteam.chucker.internal.ui.MainActivity instance
     Leaking: YES (ObjectWatcher was watching this because com.chuckerteam.chucker.internal.ui.MainActivity received Activity#onDestroy() callback and Activity#mDestroyed is true)
     Retaining 997.1 kB in 14153 objects
     key = 0f6874e8-79a2-493d-b176-ab5db9ebdff6
     watchDurationMillis = 6171
     retainedDurationMillis = 1171
     mApplication instance of ...Application
     mBase instance of androidx.appcompat.view.ContextThemeWrapper

handstandsam · 2022-05-03T01:45:22Z

library/src/main/kotlin/com/chuckerteam/chucker/internal/ui/MainActivity.kt

 lifecycleScope.launch {
 val transactions = viewModel.getAllTransactions()
 if (transactions.isNullOrEmpty()) {
- Toast
- .makeText(this@MainActivity, R.string.chucker_export_empty_text, Toast.LENGTH_SHORT)
- .show()
+ showToast(applicationContext.getString(R.string.chucker_export_empty_text))
 return@launch
 }


I have a feeling it was this which caused the leak since it references the activity after launching a coroutine.

The other updates aren't required, and activity can be fine as you mentioned. I like to always try and use application Context just as a best practice to avoid these sorts of leaks.

Same feeling that this is the place.

vbuberen

Alright, let's land this one.

Use applicationContext to avoid potential memory leak. See Android Do…

ce50f21

…cumentation on Toasts to see that ApplicationContext is what should be used. https://developer.android.com/guide/topics/ui/notifiers/toasts

handstandsam commented Apr 26, 2022

View reviewed changes

Small cleanup. Also wanted to re-trigger PR checks.

66e3141

cortinico approved these changes May 2, 2022

View reviewed changes

handstandsam commented May 3, 2022

View reviewed changes

vbuberen approved these changes May 3, 2022

View reviewed changes

vbuberen merged commit 2046d6c into ChuckerTeam:develop May 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory Leak with Toasts: Use applicationContext instead of Activity #810

Memory Leak with Toasts: Use applicationContext instead of Activity #810

handstandsam commented Apr 26, 2022 •

edited

Loading

handstandsam Apr 26, 2022

vbuberen commented Apr 27, 2022 •

edited

Loading

cortinico left a comment

handstandsam commented May 3, 2022

handstandsam May 3, 2022

vbuberen May 3, 2022

vbuberen left a comment

Memory Leak with Toasts: Use applicationContext instead of Activity #810

Memory Leak with Toasts: Use applicationContext instead of Activity #810

Conversation

handstandsam commented Apr 26, 2022 • edited Loading

📷 Screenshots

📄 Context

📝 Changes

📎 Related PR

🚫 Breaking

🛠️ How to test

⏱️ Next steps

handstandsam Apr 26, 2022

Choose a reason for hiding this comment

vbuberen commented Apr 27, 2022 • edited Loading

cortinico left a comment

Choose a reason for hiding this comment

handstandsam commented May 3, 2022

handstandsam May 3, 2022

Choose a reason for hiding this comment

vbuberen May 3, 2022

Choose a reason for hiding this comment

vbuberen left a comment

Choose a reason for hiding this comment

handstandsam commented Apr 26, 2022 •

edited

Loading

vbuberen commented Apr 27, 2022 •

edited

Loading