-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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(material/core): resolve memory leak by removing event listeners from the ripple element #24663
Conversation
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.
Are you sure this actually is a memory leak? I was thinking about it when adding the event listeners but it seemed fine in profiling and GC.
Hey Paul, I've double checked it in other browser (Firefox) and the memory leak exists (note I've renamed The object ID in the tree is The capture map shows that the DOM node is being captured by lambda (which is That wouldn't be a memory leak in Chrome (because V8's garbage collector is really smart about reference counting) if the lambda didn't capture |
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.
LGTM overall. I re-checked and was able to reproduce this. Thanks @arturovt
…rom the ripple element
* Map of currently active ripple references. | ||
* The ripple reference is mapped to its element event listeners. | ||
* The reason why `| null` is used is that event listeners are added only | ||
* when the condition is truthy (see the `_startFadeOutTransition` method). |
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.
Not sure if _startFadeOutTransition
is the best reference here, but thanks for the comment 👍
…rom the ripple element (angular#24663)
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
We were adding
transitionend
andtransitioncancel
event listeners, but never removed it. I didn't want to touch theRippleRef
class since that would be a change to the public API.