Skip to content

Commit 43a3b4e

Browse files
petebacondarwinpetebacondarwin
committed
fix($compile): connect transclude scopes to their containing scope to 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
1 parent d58ae61 commit 43a3b4e

File tree

2 files changed

+138
-61
lines changed

2 files changed

+138
-61
lines changed

‎src/ng/compile.js

+17-15
Original file line numberDiff line numberDiff line change
@@ -294,13 +294,20 @@
294294
* compile the content of the element and make it available to the directive.
295295
* Typically used with {@link ng.directive:ngTransclude
296296
* ngTransclude}. The advantage of transclusion is that the linking function receives a
297-
* transclusion function which is pre-bound to the correct scope. In a typical setup the widget
298-
* creates an `isolate` scope, but the transclusion is not a child, but a sibling of the `isolate`
299-
* scope. This makes it possible for the widget to have private state, and the transclusion to
300-
* be bound to the parent (pre-`isolate`) scope.
297+
* transclusion function which is pre-bound to the scope of the position in the DOM from where
298+
* it was taken.
301299
*
302-
* * `true` - transclude the content of the directive.
303-
* * `'element'` - transclude the whole element including any directives defined at lower priority.
300+
* In a typical setup the widget creates an `isolate` scope, but the transcluded
301+
* content has its own **transclusion scope**. While the **transclusion scope** is owned as a child,
302+
* by the **isolate scope**, it prototypically inherits from the original scope from where the
303+
* transcluded content was taken.
304+
*
305+
* This makes it possible for the widget to have private state, and the transclusion to
306+
* be bound to the original (pre-`isolate`) scope.
307+
*
308+
* * `true` - transclude the content (i.e. the child nodes) of the directive's element.
309+
* * `'element'` - transclude the whole of the directive's element including any directives on this
310+
* element that defined at a lower priority than this directive.
304311
*
305312
* <div class="alert alert-warning">
306313
* **Note:** When testing an element transclude directive you must not place the directive at the root of the
@@ -1166,20 +1173,15 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
11661173

11671174
function createBoundTranscludeFn(scope, transcludeFn, previousBoundTranscludeFn, elementTransclusion) {
11681175

1169-
var boundTranscludeFn = function(transcludedScope, cloneFn, controllers, futureParentElement) {
1176+
var boundTranscludeFn = function(transcludedScope, cloneFn, controllers, futureParentElement, containingScope) {
11701177
var scopeCreated = false;
11711178

11721179
if (!transcludedScope) {
1173-
transcludedScope = scope.$new();
1180+
transcludedScope = scope.$new(false, containingScope);
11741181
transcludedScope.$$transcluded = true;
1175-
scopeCreated = true;
11761182
}
11771183

1178-
var clone = transcludeFn(transcludedScope, cloneFn, controllers, previousBoundTranscludeFn, futureParentElement);
1179-
if (scopeCreated && !elementTransclusion) {
1180-
clone.on('$destroy', function() { transcludedScope.$destroy(); });
1181-
}
1182-
return clone;
1184+
return transcludeFn(transcludedScope, cloneFn, controllers, previousBoundTranscludeFn, futureParentElement);
11831185
};
11841186

11851187
return boundTranscludeFn;
@@ -1810,7 +1812,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
18101812
if (!futureParentElement) {
18111813
futureParentElement = hasElementTranscludeDirective ? $element.parent() : $element;
18121814
}
1813-
return boundTranscludeFn(scope, cloneAttachFn, transcludeControllers, futureParentElement);
1815+
return boundTranscludeFn(scope, cloneAttachFn, transcludeControllers, futureParentElement, scopeToChild);
18141816
}
18151817
}
18161818
}

‎test/ng/compileSpec.js

+121-46
Original file line numberDiff line numberDiff line change
@@ -4331,17 +4331,21 @@ describe('$compile', function() {
43314331
return {
43324332
transclude: 'content',
43334333
replace: true,
4334-
scope: true,
4335-
template: '<ul><li>W:{{$parent.$id}}-{{$id}};</li><li ng-transclude></li></ul>'
4334+
scope: {},
4335+
link: function(scope) {
4336+
scope.x='iso';
4337+
},
4338+
template: '<ul><li>W:{{x}}-{{$parent.$id}}-{{$id}};</li><li ng-transclude></li></ul>'
43364339
};
43374340
});
43384341
});
43394342
inject(function(log, $rootScope, $compile) {
4340-
element = $compile('<div><div trans>T:{{$parent.$id}}-{{$id}}<span>;</span></div></div>')
4343+
element = $compile('<div><div trans>T:{{x}}-{{$parent.$id}}-{{$id}}<span>;</span></div></div>')
43414344
($rootScope);
4345+
$rootScope.x = 'root';
43424346
$rootScope.$apply();
4343-
expect(element.text()).toEqual('W:1-2;T:1-3;');
4344-
expect(jqLite(element.find('span')[0]).text()).toEqual('T:1-3');
4347+
expect(element.text()).toEqual('W:iso-1-2;T:root-2-3;');
4348+
expect(jqLite(element.find('span')[0]).text()).toEqual('T:root-2-3');
43454349
expect(jqLite(element.find('span')[1]).text()).toEqual(';');
43464350
});
43474351
});
@@ -4551,47 +4555,6 @@ describe('$compile', function() {
45514555
}
45524556

45534557

4554-
it('should remove transclusion scope, when the DOM is destroyed', function() {
4555-
module(function() {
4556-
directive('box', valueFn({
4557-
transclude: true,
4558-
scope: { name: '=', show: '=' },
4559-
template: '<div><h1>Hello: {{name}}!</h1><div ng-transclude></div></div>',
4560-
link: function(scope, element) {
4561-
scope.$watch(
4562-
'show',
4563-
function(show) {
4564-
if (!show) {
4565-
element.find('div').find('div').remove();
4566-
}
4567-
}
4568-
);
4569-
}
4570-
}));
4571-
});
4572-
inject(function($compile, $rootScope) {
4573-
$rootScope.username = 'Misko';
4574-
$rootScope.select = true;
4575-
element = $compile(
4576-
'<div><div box name="username" show="select">user: {{username}}</div></div>')
4577-
($rootScope);
4578-
$rootScope.$apply();
4579-
expect(element.text()).toEqual('Hello: Misko!user: Misko');
4580-
4581-
var widgetScope = $rootScope.$$childHead;
4582-
var transcludeScope = widgetScope.$$nextSibling;
4583-
expect(widgetScope.name).toEqual('Misko');
4584-
expect(widgetScope.$parent).toEqual($rootScope);
4585-
expect(transcludeScope.$parent).toEqual($rootScope);
4586-
4587-
$rootScope.select = false;
4588-
$rootScope.$apply();
4589-
expect(element.text()).toEqual('Hello: Misko!');
4590-
expect(widgetScope.$$nextSibling).toEqual(null);
4591-
});
4592-
});
4593-
4594-
45954558
it('should add a $$transcluded property onto the transcluded scope', function() {
45964559
module(function() {
45974560
directive('trans', function() {
@@ -4964,6 +4927,118 @@ describe('$compile', function() {
49644927
});
49654928

49664929

4930+
// see issue https://github.com/angular/angular.js/issues/9095
4931+
describe('removing a transcluded element', function() {
4932+
4933+
function countScopes($rootScope) {
4934+
return [$rootScope].concat(
4935+
getChildScopes($rootScope)
4936+
).length;
4937+
4938+
function getChildScopes(scope) {
4939+
var children = [];
4940+
if (!scope.$$childHead) { return children; }
4941+
var childScope = scope.$$childHead;
4942+
do {
4943+
children.push(childScope);
4944+
children = children.concat(getChildScopes(childScope));
4945+
} while ((childScope = childScope.$$nextSibling));
4946+
return children;
4947+
}
4948+
}
4949+
4950+
beforeEach(module(function() {
4951+
directive('toggle', function() {
4952+
return {
4953+
transclude: true,
4954+
template: '<div ng:if="t"><div ng:transclude></div></div>'
4955+
};
4956+
});
4957+
}));
4958+
4959+
4960+
it('should not leak the transclude scope when the transcluded content is an element transclusion directive',
4961+
inject(function($compile, $rootScope) {
4962+
4963+
element = $compile(
4964+
'<div toggle>' +
4965+
'<div ng:repeat="msg in [\'msg-1\']">{{ msg }}</div>' +
4966+
'</div>'
4967+
)($rootScope);
4968+
4969+
$rootScope.$apply('t = true');
4970+
expect(element.text()).toContain('msg-1');
4971+
expect(countScopes($rootScope)).toEqual(4);
4972+
4973+
$rootScope.$apply('t = false');
4974+
expect(element.text()).not.toContain('msg-1');
4975+
expect(countScopes($rootScope)).toEqual(1);
4976+
4977+
$rootScope.$apply('t = true');
4978+
expect(element.text()).toContain('msg-1');
4979+
expect(countScopes($rootScope)).toEqual(4);
4980+
4981+
$rootScope.$apply('t = false');
4982+
expect(element.text()).not.toContain('msg-1');
4983+
expect(countScopes($rootScope)).toEqual(1);
4984+
}));
4985+
4986+
4987+
it('should not leak the transclude scope if the transcluded contains only comments',
4988+
inject(function($compile, $rootScope) {
4989+
4990+
element = $compile(
4991+
'<div toggle>' +
4992+
'<!-- some comment -->' +
4993+
'</div>'
4994+
)($rootScope);
4995+
4996+
$rootScope.$apply('t = true');
4997+
expect(element.html()).toContain('some comment');
4998+
expect(countScopes($rootScope)).toEqual(3);
4999+
5000+
$rootScope.$apply('t = false');
5001+
expect(element.html()).not.toContain('some comment');
5002+
expect(countScopes($rootScope)).toEqual(1);
5003+
5004+
$rootScope.$apply('t = true');
5005+
expect(element.html()).toContain('some comment');
5006+
expect(countScopes($rootScope)).toEqual(3);
5007+
5008+
$rootScope.$apply('t = false');
5009+
expect(element.html()).not.toContain('some comment');
5010+
expect(countScopes($rootScope)).toEqual(1);
5011+
}));
5012+
5013+
it('should not leak the transclude scope if the transcluded contains only text nodes',
5014+
inject(function($compile, $rootScope) {
5015+
5016+
element = $compile(
5017+
'<div toggle>' +
5018+
'some text' +
5019+
'</div>'
5020+
)($rootScope);
5021+
5022+
$rootScope.$apply('t = true');
5023+
expect(element.html()).toContain('some text');
5024+
expect(countScopes($rootScope)).toEqual(3);
5025+
5026+
$rootScope.$apply('t = false');
5027+
expect(element.html()).not.toContain('some text');
5028+
expect(countScopes($rootScope)).toEqual(1);
5029+
5030+
$rootScope.$apply('t = true');
5031+
expect(element.html()).toContain('some text');
5032+
expect(countScopes($rootScope)).toEqual(3);
5033+
5034+
$rootScope.$apply('t = false');
5035+
expect(element.html()).not.toContain('some text');
5036+
expect(countScopes($rootScope)).toEqual(1);
5037+
}));
5038+
5039+
});
5040+
5041+
49675042
describe('nested transcludes', function() {
49685043

49695044
beforeEach(module(function($compileProvider) {

0 commit comments

Comments
 (0)