antoniolg / androidmvp

MVP Android Example
5.94k stars 1.58k forks source link

Memory Leak in Activity? #9

Closed kaedea closed 8 years ago

kaedea commented 8 years ago

The PresenterCompl will hold reference of IView(Activity), so that the Activity's heap can not be recyled when it was onDestroyed.

kaedea commented 8 years ago

I have detect this by calling GC manually. The MainActivity will actually leak memory when I go back to LoginActivity while the presenter of the MainActivity is fetching data in background. In my case, I will remove reference of IView in the Presenter when the Activity is onDestroy.

gengjiawen commented 8 years ago

I track the memory leak using leakcanary, and MainActivity do leaked.

* com.antonioleiva.mvpexample.app.main.MainActivity has leaked:
* GC ROOT android.widget.Toast$TN.mNextView
* references android.widget.LinearLayout.mContext
* leaks com.antonioleiva.mvpexample.app.main.MainActivity instance
antoniolg commented 8 years ago

Thanks! Feel free to PR a solution, otherwise I'll try to find some time to fix this at some point.

raghunandankavi2010 commented 8 years ago

@kaedea so you would do loginview = null when activity is destroyed?. Or may be using a weakreference might help??

raghunandankavi2010 commented 8 years ago

@kaedea i did take a look at your implementation of mvp @ https://github.com/kaedea/android-mvp-pattern/blob/master/app/src/main/java/me/kaede/mvp/loginoptimized/presenter/LoginPresenterCompl.java where in you have iLoginView = null .

antoniolg commented 8 years ago

I understand the point behind this issue, because it could lead to problems that the view keeps attached after the activity has been destroyed, but I don't see how to reproduce the leak you're mentioning.

The MainActivity will actually leak memory when I go back to LoginActivity while the presenter of the MainActivity is fetching data in background

This can't happen afaik, because you can't go back from MainActivity to LoginActivity, it will close the App. The activity will probably be leaked while the background task is being done, but it'll be released afterwards.

I track the memory leak using leakcanary, and MainActivity do leaked.

I added LeakCanary to the project, and was navigating for a while, but couldn't find a way to leak it. The message @gengjiawen is telling that is being leaked by a toast. I'm not very used to LeakCanary, so could be a reason why I don't find it. If you could please create a PR with LeakCanary included and a way to reproduce it, it'd be helpful.

Apart from that, it's true that the example is not perfect. The listener shouldn't be called if the activity is destroyed, or at least the activity shouldn't be updated. But background tasks shouldn't use handlers either, this example is just a simplification. Anyway, I'll look for the simplest possible way to fix this issue.

raghunandankavi2010 commented 8 years ago

@antoniolg you are right. Thanks for the update.

gengjiawen commented 8 years ago

@antoniolg In MainActivity just rotate the screen it will reproduce the bug if I remember correctly. From the experience I deal with memory leak, "leaked by a toast" is mostly indicated leak an activity instance, which is very common. I will test you fix on this issue when I got time :)

openproject commented 8 years ago

@gengjiawen android.widget.Toast$TN.mNextView leak solution is using Toast.makeText(context.getApplicationContext(), message, Toast.LENGTH_SHORT).show(); instead of Toast.makeText(context, message, Toast.LENGTH_SHORT).show();

gengjiawen commented 8 years ago

@openproject you are wrong, this is not the case.You leak a context, it will show leak log like that, it's has nothing to do with the Toast. @antoniolg I tested on new code,the leak no longer exists:)

antoniolg commented 8 years ago

Thanks for testing it @gengjiawen !!

HarisA92 commented 5 years ago

can someone post a solution for this problem? Thanks