-
-
Notifications
You must be signed in to change notification settings - Fork 339
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
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
@@ -136,12 +136,11 @@ internal class MainActivity : | |||
} | |||
|
|||
private fun exportTransactions(fileName: String, block: suspend (List<HttpTransaction>) -> Sharable) { | |||
val applicationContext = this.applicationContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same feeling that this is the place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: