-
Notifications
You must be signed in to change notification settings - Fork 24.1k
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 Memory Leak in LogBoxModule #45261
Conversation
Hi @fannnnzhang! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
@fannnzhang thank you for the PR! I've red the summary, and the attached issue, but I still need some clarification. |
Thank you for your feedback! Let me clarify the existing implementation and the memory leak issue. In the current implementation, the reference chain is approximately as follows:
The issue arises because the current Activity reference is passed to When the ReactActivity exits and returns to another native Activity, LeakCanary (the memory leak detection tool) captures this memory leak. If you need additional details, such as reproduction steps or screenshots from the memory leak detection tool, feel free to ask! 😊 |
@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
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.
Thanks for sending this over @fannnzhang
Code looks good to me, but I'd like to do more testing around it before we can merge it
/** | ||
* LogBoxModule can be rendered in different surface. By default, it will use LogBoxDialog to wrap | ||
* the content of logs. In other platform (for example VR), a surfaceDelegate can be provided so | ||
* that the content can be wrapped in custom surface. | ||
*/ |
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.
Can we not lose this comment?
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.
Of course, I've committed the changes to restore the deleted comment. Please review the comment to ensure it's correct.
@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@cortinico merged this pull request in 0847384. |
This pull request was successfully merged by @fannnzhang in 0847384. When will my fix make it into a release? | How to file a pick request? |
@fannnzhang is the issues similar to this? #45217 |
Summary:
To fix The Memory Leak Issue This change modifies the timing of view creation in the LogModule. The motivation behind this update is to address a potential memory leak issue. Previously, views were being created and held onto, which could lead to references to the Activity being retained even when they were no longer needed. By creating the view only when the show method is called and ensuring it is removed in the hide method, we can prevent these memory leaks and improve the overall memory management and stability of the LogModule.
Fixes #45080
show
method is called.hide
method.These changes improve memory management and stability within the LogModule.
Modify the timing of view creation in LogModule. The view is now created when the show method is called, and it can be removed in the hide method. This change resolves potential memory leaks caused by the view holding a reference to the Activity.
Changelog:
[ANDROID] [FIXED] - Fix LogModule to create view when show is called