facebook
Fix Memory Leak in LogBoxModule
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 https://github.com/facebook/react-native/issues/45080
- Adjusted the timing of view creation to occur when the
showmethod is called. - Ensured that the created view can be removed in the
hidemethod. - This update addresses potential memory leaks by preventing the view from holding a reference to the Activity.
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
Hi @fannnnzhang!
Thank you for your pull request and welcome to our community.
Action Required
In 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.
Process
In 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 CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at [email protected]. Thanks!
| Warnings | |
|---|---|
| :warning: | :clipboard: Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested. |
Generated by :no_entry_sign: dangerJS against 7519b018a9e1af82a718dccc27fb2e2afd98feef
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
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. What exactly is leaking in the existing implementation?
@fannnzhang thank you for the PR! I've red the summary, and the attached issue, but I still need some clarification. What exactly is leaking in the existing implementation?
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:
LogBoxModule -> LogBoxDialogSurfaceDelegate -> mReactRootView -> ReactSurfaceView -> context (current Activity)
The issue arises because the current Activity reference is passed to ReactSurfaceImpl via the constructor during the creation of the View. However, the reference chain is only broken when the hide() method is called in LogBoxDialogSurfaceDelegate. Unfortunately, hide() is not always guaranteed to be called, which can lead to a memory leak.
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.
@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 facebook/react-native@0847384a4598d9bbc8987788047bc58d7596881d.
This pull request was successfully merged by @fannnzhang in 0847384a4598d9bbc8987788047bc58d7596881d.
When will my fix make it into a release? | How to file a pick request?
@fannnzhang is the issues similar to this? https://github.com/facebook/react-native/issues/45217
@fannnzhang is the issues similar to this? #45217
I think it doesn't seem to be the case. There are quite a few memory leaks in the current new version. So far, I have discovered at least two. One is related to this PR, and the other is in the call chain of FabricUIManager, where a reference to the Activity is held and not released. I will take some time to organize this issue further and might raise an issue and a PR for it later.
@fannnzhang Did you get a chance to look at the memory leak when opening as activity
@fannnzhang Did you get a chance to look at the memory leak when opening as activity
Sure, I will investigate this issue over this weekend. If progress is smooth, I will update you with the relevant details.
@fannnzhang Just checking again if anything to related to this was found ?
