leandroBorgesFerreira / LoadingButtonAndroid

A button to substitute the ProgressDialog
MIT License
1.95k stars 214 forks source link

Memory leak on button press #4

Closed jbmlaird closed 7 years ago

jbmlaird commented 7 years ago

Great library. Looks great.

I got a 49KB memory leak when having two of these buttons in an Activity. I clicked on both of them and they did the full item animation.

btnCollection.startAnimation();

...

btnCollection.revertAnimation(() ->
{
    if (inCollection)
        btnCollection.setText("Remove from Collection");
    else
        btnCollection.setText("Add to Collection");
    });

I thought it might be due to assigning the OnAnimationEndListener but when I tried with:

btnCollection.revertAnimation();
btnCollection.setText("Remove from Collection");

I also got the 49KB leak.

EDIT: The leak does not occur if I do not press the button

Any ideas?

LeakCanary log:

loadingbuttonmemoryleak.txt

leandroBorgesFerreira commented 7 years ago

I am going to study this problem to see what can be causing this leak.

jbmlaird commented 7 years ago

I've got a leak of 2MB a couple of times. It seems to only leak 1 out of 10 times, I'm still trying to see what exactly is causing the issue. It may be my implementation. I have two of these buttons in a RecyclerView.

loadingbuttonmemoryleak.txt

jbmlaird commented 7 years ago

I'm going to close this for now because I think it may have been due to me not disposing my Observables properly. I have been unable to reproduce since proper GC. LeakCanary must have been falsely pointing to this library as I started the Observables on button press,

jbmlaird commented 7 years ago

@leandroBorgesFerreira me again.

I've created a fork to show when this leaks: https://github.com/jbmlaird/LoadingButtonAndroidMemoryLeak

It seems to leak when you do any action on another thread, regardless of whether you call revertAnimation() on the UI Thread or not.

If you go into the SecondActivity press both buttons and then return to the first Activity you should see the LeakCanary prompt appear and then about a minute later you'll get a notification.

secondactivityleak.txt

EDIT:

I believe I have fixed the memory leak by exposing a dispose() method in the AnimatedButton interface. I'll create a pull request when finished so you can decide whether you want to use my implementation.

leandroBorgesFerreira commented 7 years ago

Thanks for the contribution jbmlaird. I will wait for your pull request

jbmlaird commented 7 years ago

I've created the pull request: #9

This is just a quick fix that I implemented to stop it leaking but you might want to call it internally somewhere. Facebook has an equivalent private method here but theirs still leaks unless you call it yourself

Thanks again for the great library!

leandroBorgesFerreira commented 7 years ago

I merged your pull request. You can use the 1.7.0 version to use the dispose method. I take a look at the leaks those days

jbmlaird commented 7 years ago

Nice one, thank you :)