Memory Leak with Toasts: Use applicationContext instead of Activity#810
Conversation
…cumentation on Toasts to see that ApplicationContext is what should be used. https://developer.android.com/guide/topics/ui/notifiers/toasts
| } | ||
|
|
||
| private fun exportTransactions(fileName: String, block: suspend (List<HttpTransaction>) -> Sharable) { | ||
| val applicationContext = this.applicationContext |
There was a problem hiding this comment.
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.
|
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 |
cortinico
left a comment
There was a problem hiding this comment.
Also, the link you shared mentions application context, but if we open docs for
makeTextit 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 👍
|
@vbuberen @cortinico Sorry for the delay. Here was the leak that was caught (with some information redacted): |
| 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 | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Same feeling that this is the place.
vbuberen
left a comment
There was a problem hiding this comment.
Alright, let's land this one.
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 passedapplicationContext.📎 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: