search

Wootric / WootricSDK-Android

Android SDK for Wootric Survey Platform supporting NPS, CSAT and CES surveys
https://wootric.com
MIT License
15 stars 16 forks source link

Fix activity related leaks. #98

Closed Yannshu closed 3 years ago

Yannshu commented 3 years ago

Hello again 👋

Had a bit of time this afternoon, so I investigated and solved the leak reported by @astamato in issue https://github.com/Wootric/WootricSDK-Android/issues/97. This leak is can be easily spotted by Leak Canary and reproduced by displaying the survey in one activity, closing the survey, then finishing this activity and opening another one. You can view the heap analysis result in the following gist: link.

This PR fixes 2 different leaks related to holding hard references to activity objects. It will be easier to review it commit per commit.

Changes:

  1. Stop keeping a hard reference to activities related object in SurveyManager. Looking at the code structure, this isn't needed. This was causing a memory leak when navigating to another activity, as the hard references were preventing the previous activity to be garbage collected.
  2. Set FragmentManager.currentEvent attribute to null when the survey is finished (or doesn't load). The CurrentEvent class holds hard references to activity objects, and as for 1., when navigating to another activity, these references were preventing the previous activity to be garbage collected.

How to test it:

To observe the memory leak:

  1. On a branch starting from master (not from this PR's branch), apply this patch in your repo. To do so, download the gist to a file and use the following git command in your repo: git am -3 -k spot-leak-setup.patch.
  2. Compile the app, launch it, get the survey displayed, close it, and press the new Navigate to another activity button added by the patch.
  3. If this is tested before merging my other PR, Leak Canary will report 3 leaks. 1 fixed by that previous PR, and this PR is fixing the other 2.

To test this PR

  1. Checkout changes from this PR.
  2. Apply that same patch again.
  3. Compile the app, launch it, get the survey displayed, close it, and press the new Navigate to another activity button added by the patch.
  4. Leak Canary will no longer report 3 leaks. If this is tested before merging my other PR, it will still report one though. Nothing to worry about, it'll get fixed when merging the other one.
  5. Make sure only this branch gets merged into master, I don't think my patch to spot the leak should be added to the repo.

Same thing than for my other PR. I'm obviously not the most familiar person with this codebase, I just spent a bit of time fixing these memory leaks. While all the unit tests still pass, I would advise to fully test the different behaviours of the survey before publishing a new version with this change, to make sure it did not create any regression.

diegoserranoa commented 3 years ago

hi @Yannshu ! Thanks for this other PR 🙏 appreciate the contribution.

I've tested both fixes and they work fine , the leaks have disappeared. I'm putting everything together in another branch with both fixes.

I'll update you once it's ready!

Yannshu commented 3 years ago

That's good to hear, thanks for having a look into this @diegoserranoa. Looking forward to the coming release with these fixes!

diegoserranoa commented 3 years ago

Hi @Yannshu , I've created a new PR merging the two solutions and also added a fix for another leak I was able to find. Can you please test and let me know if it's working correctly for you? 🙏

Thanks!

Yannshu commented 3 years ago

Thanks @diegoserranoa. Let's continue the discussion there 😄

diegoserranoa commented 3 years ago

Released version 2.18.1 with this fix. Thanks for the help!

Closing this PR.