fix(upgrade): fix memory leaks when the component is destroyed #39921
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jessicajaniuk
added
the
area: upgrade
arturovt
changed the title
$$$angularInjectorController
Currently we're storing the real injector on the element inside the `ParentInjectorPromise` and its `resolve()` method. This causes a memory leak if we don't remove the reference when the component is destroyed. We also setup the `$destroy` listener and never release it, the problem is that this listener captures `this`, as long as there is a reference, `this` will not be GC'd. PR Close #39911
|
@arturovt, is this PR ready for review (I've noticed you have been pushing new commits recently)? |
Yep, you can start reviewing. |
petebacondarwin
approved these changes
Dec 2, 2020
There was a problem hiding this comment.
LGTM but it has been a while since I look at ngUpgrade code.
@gkalpak will probably have more to say :-)
|
@petebacondarwin @gkalpak gentlemen just for your information:
this is AFTER
Basically, the |
gkalpak
added a commit
to gkalpak/angular
that referenced
this pull request
Dec 3, 2020
Previously, due to the way the AngularJS and Angular clean-up processes interfere with each other when removing an AngularJS element that contains a downgraded Angular component, the data associated with the host element of the downgraded component was not removed. This data was kept in an internal AngularJS cache, which prevented the element and component instance from being garbage-collected, leading to memory leaks. This commit fixes this by ensuring the element data is explicitly removed when cleaning up a downgraded component. NOTE: This is essentially the equivalent of angular#26209 but for downgraded (instead of upgraded) components. Fixes angular#39911 Closes angular#39921
gkalpak
added a commit
to gkalpak/angular
that referenced
this pull request
Dec 3, 2020
Previously, due to the way the AngularJS and Angular clean-up processes interfere with each other when removing an AngularJS element that contains a downgraded Angular component, the data associated with the host element of the downgraded component was not removed. This data was kept in an internal AngularJS cache, which prevented the element and component instance from being garbage-collected, leading to memory leaks. This commit fixes this by ensuring the element data is explicitly removed when cleaning up a downgraded component. NOTE: This is essentially the equivalent of angular#26209 but for downgraded (instead of upgraded) components. Fixes angular#39911 Closes angular#39921
gkalpak
added a commit
to gkalpak/angular
that referenced
this pull request
Dec 3, 2020
Previously, due to the way the AngularJS and Angular clean-up processes interfere with each other when removing an AngularJS element that contains a downgraded Angular component, the data associated with the host element of the downgraded component was not removed. This data was kept in an internal AngularJS cache, which prevented the element and component instance from being garbage-collected, leading to memory leaks. This commit fixes this by ensuring the element data is explicitly removed when cleaning up a downgraded component. NOTE: This is essentially the equivalent of angular#26209 but for downgraded (instead of upgraded) components. Fixes angular#39911 Closes angular#39921
gkalpak
added a commit
to gkalpak/angular
that referenced
this pull request
Dec 3, 2020
Previously, due to the way the AngularJS and Angular clean-up processes interfere with each other when removing an AngularJS element that contains a downgraded Angular component, the data associated with the host element of the downgraded component was not removed. This data was kept in an internal AngularJS cache, which prevented the element and component instance from being garbage-collected, leading to memory leaks. This commit fixes this by ensuring the element data is explicitly removed when cleaning up a downgraded component. NOTE: This is essentially the equivalent of angular#26209 but for downgraded (instead of upgraded) components. Fixes angular#39911 Closes angular#39921
gkalpak
added a commit
to gkalpak/angular
that referenced
this pull request
Dec 3, 2020
Previously, due to the way the AngularJS and Angular clean-up processes interfere with each other when removing an AngularJS element that contains a downgraded Angular component, the data associated with the host element of the downgraded component was not removed. This data was kept in an internal AngularJS cache, which prevented the element and component instance from being garbage-collected, leading to memory leaks. This commit fixes this by ensuring the element data is explicitly removed when cleaning up a downgraded component. NOTE: This is essentially the equivalent of angular#26209 but for downgraded (instead of upgraded) components. Fixes angular#39911 Closes angular#39921
|
Thx for working on this, @arturovt 👍 While reviewing your PR, it thought it was "suspicious" that we had to clean up stuff that would normally be cleaned up automatically. So I dug deeper to understand why it wasn't being clean up. In a nutshell, it happens because (under some circumstances) Angular will remove the component host element from the DOM before AngularJS has a chance to clean it up. See here for more details. Based on the above, I have created #39965 with an alternative fix, which I think is simpler and more consistent with how things would work in a pure AngularJS app. I would appreciate it if you could take a look and let me know what you think? BTW, you can also grab the build artifacts from my PR (see here for more info) and use them in a sample app in order to verify the fix. |
|
I've played with your changes and it seems to work (the adapter is released successfully). Nice job! |
|
Awesome! Thx for checking, @arturovt ❤️ |
gkalpak
added a commit
to gkalpak/angular
that referenced
this pull request
Dec 7, 2020
Previously, due to the way the AngularJS and Angular clean-up processes interfere with each other when removing an AngularJS element that contains a downgraded Angular component, the data associated with the host element of the downgraded component was not removed. This data was kept in an internal AngularJS cache, which prevented the element and component instance from being garbage-collected, leading to memory leaks. This commit fixes this by ensuring the element data is explicitly removed when cleaning up a downgraded component. NOTE: This is essentially the equivalent of angular#26209 but for downgraded (instead of upgraded) components. Fixes angular#39911 Closes angular#39921
mhevery
pushed a commit
that referenced
this pull request
Dec 8, 2020
…39965) Previously, due to the way the AngularJS and Angular clean-up processes interfere with each other when removing an AngularJS element that contains a downgraded Angular component, the data associated with the host element of the downgraded component was not removed. This data was kept in an internal AngularJS cache, which prevented the element and component instance from being garbage-collected, leading to memory leaks. This commit fixes this by ensuring the element data is explicitly removed when cleaning up a downgraded component. NOTE: This is essentially the equivalent of #26209 but for downgraded (instead of upgraded) components. Fixes #39911 Closes #39921 PR Close #39965
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Checklist
PR Type
What is the current behavior?
Issue Number: #39911
Does this PR introduce a breaking change?