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

fix(animations): leaking detached nodes when parent has a leave transition #34409

Closed

Conversation

@crisbeto
Copy link
Member

crisbeto commented Dec 14, 2019

In the TransitionAnimationEngine we keep track of the existing elements with animations and we clear the cached data when they're removed. We also have some logic where we transition away the child elements when a parent is removed, however in that case we never cleared the cached element data which resulted in a memory leak. The leak is particularly visible in Material where whenever there's an animated overlay with a component inside of it that has an animation, the child component would always be retained in memory.

Fixes #25744.

@googlebot googlebot added the cla: yes label Dec 14, 2019
…ition

In the TransitionAnimationEngine we keep track of the existing elements with animations and we clear the cached data when they're removed. We also have some logic where we transition away the child elements when a parent is removed, however in that case we never cleared the cached element data which resulted in a memory leak. The leak is particularly visible in Material where whenever there's an animated overlay with a component inside of it that has an animation, the child component would always be retained in memory.

Fixes #25744.
@crisbeto crisbeto force-pushed the crisbeto:25744/animations-material-leak branch from 7083000 to 3c9c360 Dec 14, 2019

// If the child elements were removed along with the parent, their animations might not
// have completed. Clear all the elements from the cache so we don't end up with a memory leak.
this._engine.afterFlushAnimationsDone(

This comment has been minimized.

Copy link
@crisbeto

crisbeto Dec 14, 2019

Author Member

Note that I only have a high level understanding of how animations work so it's possible that this introduces some unintended consequences. I assumed that it's ok, because it passes all of the existing tests and I couldn't see any issues while testing it against the Material dev app.

@ngbot ngbot bot modified the milestone: needsTriage Dec 14, 2019
@crisbeto crisbeto marked this pull request as ready for review Dec 14, 2019
@crisbeto crisbeto requested a review from angular/fw-animations as a code owner Dec 14, 2019
@matsko
matsko approved these changes Dec 16, 2019
Copy link
Member

matsko left a comment

LGTM. Thank you Kristiyan.

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Dec 16, 2019

@kara kara closed this in 835ed0f Dec 16, 2019
kara added a commit that referenced this pull request Dec 16, 2019
…ition (#34409)

In the TransitionAnimationEngine we keep track of the existing elements with animations and we clear the cached data when they're removed. We also have some logic where we transition away the child elements when a parent is removed, however in that case we never cleared the cached element data which resulted in a memory leak. The leak is particularly visible in Material where whenever there's an animated overlay with a component inside of it that has an animation, the child component would always be retained in memory.

Fixes #25744.

PR Close #34409
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Jan 16, 2020

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.