TRANSCLUDE MEMORY LEAK: fix($compile): connect transclude scopes to their containing scope to prevent memory leaks #9281
Conversation
This enables us to place transclude scopes more accurately in the scope hierarchy.
… prevent memory leaks Transcluded scopes are now connected to the scope in which they are created via their `$parent` property. This means that they will be automatically destroyed when their "containing" scope is destroyed, without having to resort to listening for a `$destroy` event on various DOM elements or other scopes. Previously, transclude scope not only inherited prototypically from the scope from which they were transcluded but they were also still owned by that "outer" scope. This meant that there were scenarios where the "real" container scope/element was destroyed but the transclude scope was not, leading to memory leaks. The original strategy for dealing with this was to attach a `$destroy` event handler to the DOM elements in the transcluded content, so that if the elements were removed from the DOM then their associated transcluded scope would be destroyed. This didn't work for transclude contents that didn't contain any elements - most importantly in the case of the transclude content containing an element transclude directive at its root, since the compiler swaps out this element for a comment before a destroy handler could be attached. BREAKING CHANGE: `$transclude` functions no longer attach `$destroy` event handlers to the transcluded content, and so the associated transclude scope will not automatically be destroyed if you remove a transcluded element from the DOM using direct DOM manipulation such as the jquery `remove()` method. If you want to explicitly remove DOM elements inside your directive that have been compiled, and so potentially contain child (and transcluded) scopes, then it is your responsibility to get hold of the scope and destroy it at the same time. The suggested approach is to create a new child scope of your own around any DOM elements that you wish to manipulate in this way and destroy those scopes if you remove their contents - any child scopes will then be destroyed and cleaned up automatically. Note that all the built-in directives that manipulate the DOM (ngIf, ngRepeat, ngSwitch, etc) already follow this best practice, so if you only use these for manipulating the DOM then you do not have to worry about this change. Closes angular#9095
| @@ -4551,47 +4555,6 @@ describe('$compile', function() { | |||
| } | |||
|
|
|||
|
|
|||
| it('should remove transclusion scope, when the DOM is destroyed', function() { | |||
There was a problem hiding this comment.
This test was testing something that is really bad practice. We should not be arbitrarily removing DOM elements without thinking about their associated scope.
Even before the changes in this commit, doing this could have severe consequences: Since we were attaching $destroy handlers to each top level element in some transcluded DOM, deleting any one of those elements would cause the entire transcluded scope to be destroyed, even though there would still be transcluded elements in the DOM.
|
generally looks good to me, will definitely want someone else to have a look at this. I don't have a major problem with the change if this does address leaks (were the test cases that were added broken without these changes? if yes, then awesome!) So, in addition to the breaking change, this is going to add a bit to the API surface, and that's really the only issue with it (that this could end up abused in some way by third party code, and become very difficult to remove without breaking people). That's an area that you'll definitely want some other eyes on. But since it does fix the problems and the breaking change is at least documented, it doesn't seem bad to me. |
|
The new tests are broken without this fix; except for the text node one, which I think is because the compiler will wrap it in a |
|
LGTM, but please squash the last 3 commits into the 2nd, so that we only have 2 commits: 1 that adds the feature to |
|
@tbosch yes I can squash when merging. I think it would be good if we could get feedback from some internal Google projects on this before merging. Is this possible? |
|
looks good at first sight. |
Prevent isolated scopes from having listeners that get called multiple times when on `$destroy`
petebacondarwin
changed the title
|
@petebacondarwin We need to update the docs with the info that you put into the breaking change note. otherwise people not reading the change log will not now about their responsibility to destroy the scope when dom disappears. it should likely go into ngTransclude and $transclude doc pages. |
This Pull Request fixes memory leaks caused by transcluded scopes not being destroyed when their DOM container is destroyed. See #9095
Transcluded scopes are now connected to the scope in which they are created via their
$parentproperty. This means that they will be automatically destroyed when their "containing" scope is
destroyed, without having to resort to listening for a
$destroyevent on various DOM elementsor other scopes.
Previously, transclude scope not only inherited prototypically from the scope from which they were
transcluded but they were also still owned by that "outer" scope. This meant that there were
scenarios where the "real" container scope/element was destroyed but the transclude scope was not,
leading to memory leaks.
The original strategy for dealing with this was to attach a
$destroyevent handler to the DOMelements in the transcluded content, so that if the elements were removed from the DOM then their
associated transcluded scope would be destroyed.
This didn't work for transclude contents that didn't contain any elements - most importantly in
the case of the transclude content containing an element transclude directive at its root, since
the compiler swaps out this element for a comment before a destroy handler could be attached.
BREAKING CHANGE:
$transcludefunctions no longer attach$destroyevent handlers to the transcluded content,and so the associated transclude scope will not automatically be destroyed if you remove a
transcluded element from the DOM using direct DOM manipulation such as the jquery
remove()method.
If you want to explicitly remove DOM elements inside your directive that have been compiled, and so
potentially contain child (and transcluded) scopes, then it is your responsibility to get hold of
the scope and destroy it at the same time.
The suggested approach is to create a new child scope of your own around any DOM elements that you
wish to manipulate in this way and destroy those scopes if you remove their contents - any child
scopes will then be destroyed and cleaned up automatically.
Note that all the built-in directives that manipulate the DOM (ngIf, ngRepeat, ngSwitch, etc)
already follow this best practice, so if you only use these for manipulating the DOM then you
do not have to worry about this change.
Closes #9095