Skip to content
This repository has been archived by the owner on Dec 5, 2024. It is now read-only.

Memory leak when Reference children are removed from a page #312

Closed
linusthe3rd opened this issue Oct 1, 2019 · 4 comments · Fixed by #313
Closed

Memory leak when Reference children are removed from a page #312

linusthe3rd opened this issue Oct 1, 2019 · 4 comments · Fixed by #313

Comments

@linusthe3rd
Copy link
Contributor

Reproduction demo

https://codesandbox.io/s/react-popper-v1x-issue-template-3hcp9

Steps to reproduce the problem

  1. Replace the demo code in the repo's demo/index.js file with the code in index.js of the codesandbox's index.js file
  2. Start the demo app and load it in Chrome
  3. Open Chrome devtools and take a heap snapshot
  4. Click on "Toggle Popout" in the demo page
  5. Take another heap snapshot
  6. In the second heap snapshot look for "Detached HTMLDivElement" with the shortest distance and find in the retainer panel that referenceNode is retaining the div element

What is the expected behavior?

referenceNode in Manager is set to null when the Reference element is removed from the page.

What went wrong?

When the Reference element is removed from the page, the ref value passed to the Manager context is retained. This creates a memory leak in situations where the reference element is regularly removed from a page.

Any other comments?

  • This is most easily seen in a production build of React since the dev build adds a lot of extra utilities that prevents a clear exposure of the leak.

Packages versions

  • Popper.js: whatever version react-popper 1.3.4 depends on
  • react-popper: 1.3.4
@linusthe3rd
Copy link
Contributor Author

It looks like the reason this is happening is because of this line in Manager.js: https://github.com/FezVrasta/react-popper/blob/master/src/Manager.js#L35

The Reference component sets its ref attribute to null when it is unmounting, but Manager no-ops when a null value is sent.

@FezVrasta
Copy link
Member

Thanks for the report! Would you mind if I asked you to send a PR with a proposed fix?

@linusthe3rd
Copy link
Contributor Author

Already working on it :)

I should have one up later this morning

@linusthe3rd linusthe3rd mentioned this issue Oct 2, 2019
Merged
@linusthe3rd
Copy link
Contributor Author

@FezVrasta Got a PR up for this. LMK if you want anything changed on it.

FezVrasta pushed a commit that referenced this issue Nov 6, 2019
@linusthe3rd @FezVrasta
fix: referenceNode memory leak in Manager (#312) (#313)
e995a1a 
* fix: referenceNode memory leak

Instead of storing referenceNode in state, store it in a class property. This is done to enable the component to cleanup referenceNode’s value during unmounting.

I also added unit tests to account for this change, but that resulted in me having to update some dependencies and snapshots to get unit tests to pass.

* CR: Create separate setter and getter contexts

In order to help reduce re-renders of the Reference component, this commit creates separate context objects for the setters and getters of the referenceNode attribute.

This also removes the need for a context object, so it simplifies the management of the referenceNode attribute in the Manager class.

* CR Feedback: remove need for componentWillUnmount

* FIx flow

* Update size snapshot
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: referenceNode memory leak in Manager (#312) linusthe3rd/react-popper
2 participants
@linusthe3rd @FezVrasta