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(core): remove nested views from registry when root is destroyed #41894
Conversation
This is a follow-up to angular#41358 where my assumption was that `destroyLView` would be called for all views down the tree, but it turns out that it only gets called for the root and we actually call `cleanUpView` for the descendants. The result is that some views might not be removed from the registry after they're destroyed. These changes move the `unregisterLView` call into `cleanUpView` instead.
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, thanks for the fix @crisbeto 👍
These changes combine angular#41358 and angular#41894. Currently we save a reference to an `LView` on most DOM nodes created by Angular either by saving the `LView` directly in the `__ngContext__` or by saving the `LContext` which has a reference to the `LView`. This can be a problem if the DOM node is retained in memory, because the `LView` has references to all of the child nodes of the view, as well as other internal data structures. Previously we tried to resolve the issue by clearing the `__ngContext__` when a node is removed (see angular#36011), but we decided not to proceeed, because it can slow down destruction due to a megamorphic write. These changes aim to address the issue while reducing the performance impact by assigning a unique ID when an `LView` is created and adding it to `__ngContext__`. All active views are tracked in a map where their unique ID is used as the key. We don't need to worry about leaks within that map, because `LView`s are an internal data structure and we have complete control over when they are created and destroyed. Fixes angular#41047.
Closing in favor of #41908. |
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. |
This is a follow-up to #41358 where my assumption was that
destroyLView
would be called for all views down the tree, but it turns out that it only gets called for the root and we actually callcleanUpView
for the descendants. The result is that some views might not be removed from the registry after they're destroyed.These changes move the
unregisterLView
call intocleanUpView
instead.