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

React Example Node Leak #1332

Closed
samccone opened this issue Jun 7, 2015 · 7 comments
Closed

React Example Node Leak #1332

samccone opened this issue Jun 7, 2015 · 7 comments
Labels

Comments

@samccone
Copy link
Member

samccone commented Jun 7, 2015

Steps to reproduce
  • start timeline profiler
    • add three todos
    • delete three todos
    • (repeat above 2 more times)
  • force GC
  • stop timeline profiler

screen shot 2015-06-06 at 8 38 15 pm


It seems like one event listener is not getting unbound, this seems like a leak. ⛲

@samccone
Copy link
Member Author

samccone commented Jun 7, 2015

ping @petehunt @chenglou

@chenglou
Copy link
Contributor

chenglou commented Jun 7, 2015

Event listener? Afaik there are only react event listeners in there and they're all released correctly. Maybe it's something else?

@samccone
Copy link
Member Author

samccone commented Jun 8, 2015

Hi @chenglou thanks for getting back.

One thing that seems suspicious to me is the fact that once I started my test steps an event listener was bound and then never released even after a forced GC. Is this expected?

@samccone
Copy link
Member Author

After a new round of profiling @chenglou and warming the initial render state (adding and removing one todo) before starting my tests I am confident that there is a leak in this example.

Note how the heap size increases and I have a whole bunch of new ReactElements between each test:
screen shot 2015-06-24 at 5 49 19 pm

Also each time I go through the steps of my test I end up with more nodes each run:

screen shot 2015-06-24 at 5 51 13 pm

Based on this information I am quite confident there is a leak in this example (unless there is some caching behavior that can be accounted for)

Let me know if I can be of any more help

@sophiebits
Copy link
Contributor

If you add some todos and remove them before taking the first snapshot, you shouldn't see the listener. This is because React adds a single top-level listener for each event used and some events are not used except by the <TodoItem /> component. I can't repro the HTML elements you're seeing. I get this:

image

The new ReactElements in the heap snapshot are there because each time you call render, new elements are allocated – but the old ones that they replace are freed, so there shouldn't be a real leak.

@samccone
Copy link
Member Author

samccone commented Jul 8, 2015

Hey @spicyj thanks for getting back to me, I agree there is no listener leak that was a mistake on my part. As far as the ReactElement issue, I have codified the process here #1368

and am still seeing a growth over time (even with a forced GC), let me know if this expected or if I have missed something. Thanks again for your time!

@samccone samccone changed the title React Example Memory/Listener Leak React Example Node Leak Jul 8, 2015
@flashdesignory
Copy link
Collaborator

Closing since React was updated #2202

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants