Skip to content
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

fix: Remove click listeners to remove memory leaks #1766

Merged
merged 1 commit into from
May 19, 2019
Merged

fix: Remove click listeners to remove memory leaks #1766

merged 1 commit into from
May 19, 2019

Conversation

anhanh11001
Copy link
Contributor

@anhanh11001 anhanh11001 commented May 15, 2019

Detail:

  • The cause of the memory leak is because of the adapter of RecyclerView. If the adapter lives any longer than the RecyclerView does, the adapter is going to hold a reference to the RecyclerView which should have already gone out of memory.

Further description: https://stackoverflow.com/questions/35520946/leak-canary-recyclerview-leaking-madapter.

To be honest, I don't completely understand the problem and the memory leak in here, but this fix does really improve the app as it is faster in my device and leak canary doesn't find any leaks. So please correct me if there is anything wrong or give me further explanations.

Fixes: #1780

@auto-label auto-label bot added the fix label May 15, 2019
@iamareebjamal
Copy link
Member

iamareebjamal commented May 15, 2019

Rather than doing this, just remove koin scoping please

These changes are too much and are not required in other apps

@anhanh11001
Copy link
Contributor Author

I will remove Koin Scope. But should I keep the part where setting recyclerView adapter equals to null in other fragments in onDestroyView()? Leak Canary also find leaks in other fragments with the same adapter-recyclerView problem mentioned above.

@iamareebjamal
Copy link
Member

Honestly, that's a workaround, not a fix. We should try to find out the root cause of the leaks and fix it for the non koin adapters where leak canary is showing leaks. It may be because of nested viewholders or click listeners in adapters

@anhanh11001
Copy link
Contributor Author

anhanh11001 commented May 17, 2019

@iamareebjamal please take the look at this article as it faces the same problem as we are and please give me some advice on which solution should I go for

https://medium.com/@yfujiki/tracing-simple-memory-leak-around-recyclerview-using-leakcanary-927460532d53

Medium
Figuring out the object references under the hood and the importance of LeakCanary

@iamareebjamal
Copy link
Member

I'll take a look over the weekend.

mariobehling
mariobehling previously approved these changes May 18, 2019
@anhanh11001
Copy link
Contributor Author

@iamareebjamal Have you taken a look at the blog post?

@iamareebjamal
Copy link
Member

Yes, the blog post is simply telling the problem. I'll analyse the app today to tell what can be the fix

@iamareebjamal
Copy link
Member

Memory leaks are due to click listeners. Please remove them onDestroyView

Detail:
- Clear click listener inside adapter

Fixes: #1705
@anhanh11001
Copy link
Contributor Author

updated

@iamareebjamal iamareebjamal changed the title fix: Fix memory leak fix: Remove click listeners to remove memory leaks May 19, 2019
@iamareebjamal iamareebjamal merged commit 686ecc6 into fossasia:development May 19, 2019
@anhanh11001 anhanh11001 deleted the 1705_memory_leak branch June 20, 2019 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leaks when setting up Recycler/Adapter
4 participants