-
Notifications
You must be signed in to change notification settings - Fork 45.6k
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
Memory leak when unmounting component containing user components #781
Comments
(I took the liberty to edit your comment to add syntax highlighting) |
I can't reproduce this. With this diff https://gist.github.com/spicyj/747c79473b86bbefae0c applied, I loaded http://127.0.0.1:8000/examples/ballmer-peak/ in the console and checked Am I misunderstanding? |
Yes. You've missed one more user defined component in there.. Here you go: $(function() {
var TestComponent = React.createClass({
render: function() {
return <input/>;
}
});
var TestContainer = React.createClass({
render: function() {
return (
<div>
<label>label1</label>
<label>label2</label>
<TestComponent/>
</div>
);
}
});
var el = document.getElementById('somediv');
React.renderComponent(
<TestContainer/>,
el,
function() {
React.unmountComponentAtNode(el);
console.log(React.__internals.Mount._nodeCache);
}
);
}); |
Thanks, fixed in #786. |
Thank you for fast fix :) |
Fix potential memory leak when unmounting
Thanks for the find/fix! |
When unmounting components like this:
a memory leak happens because ReactMount.getID(node) puts DOM nodes back to nodeCache if they are not there.. just right after they were purged from cache.
To fix the leak there is a need for some read-only getID()..
Here is what's happing when unmounting that example component:
which contains this.getDOMNode() which iterates through the children of the Form element calling getID() on each of them.. which puts these elements back into the cache.
So first they were purged and here they are all put back in cache causing a memory leak :(
I'm playing with React just 3rd day so not sure of a way to correctly fix this issue
The text was updated successfully, but these errors were encountered: