From 67e9d0269cfde8d24feca783ce142c6b4e383bb3 Mon Sep 17 00:00:00 2001 From: Lucas Galfaso Date: Sun, 14 Sep 2014 18:47:37 +0200 Subject: [PATCH 1/2] refactor($scope): prevent multiple calls to listener on `$destroy` Prevent isolated scopes from having listeners that get called multiple times when on `$destroy` --- src/ng/rootScope.js | 8 ++++++++ test/ng/compileSpec.js | 37 +++++++++++++++++++++++++++---------- test/ng/rootScopeSpec.js | 7 +++++++ 3 files changed, 42 insertions(+), 10 deletions(-) diff --git a/src/ng/rootScope.js b/src/ng/rootScope.js index b00fe80511b2..72fdc920d9f8 100644 --- a/src/ng/rootScope.js +++ b/src/ng/rootScope.js @@ -222,7 +222,15 @@ function $RootScopeProvider(){ } else { parent.$$childHead = parent.$$childTail = child; } + + // The listener needs to be added after the parent is set + if (isolate || parent != this) child.$on('$destroy', destroyChild); + return child; + + function destroyChild() { + child.$$destroyed = true; + } }, /** diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index ddde992ab454..f7bb09b04905 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -4971,17 +4971,17 @@ describe('$compile', function() { return [$rootScope].concat( getChildScopes($rootScope) ).length; + } - function getChildScopes(scope) { - var children = []; - if (!scope.$$childHead) { return children; } - var childScope = scope.$$childHead; - do { - children.push(childScope); - children = children.concat(getChildScopes(childScope)); - } while ((childScope = childScope.$$nextSibling)); - return children; - } + function getChildScopes(scope) { + var children = []; + if (!scope.$$childHead) { return children; } + var childScope = scope.$$childHead; + do { + children.push(childScope); + children = children.concat(getChildScopes(childScope)); + } while ((childScope = childScope.$$nextSibling)); + return children; } beforeEach(module(function() { @@ -5117,6 +5117,23 @@ describe('$compile', function() { expect(countScopes($rootScope)).toEqual(1); })); + it('should destroy mark as destroyed all sub scopes of the scope being destroyed', + inject(function($compile, $rootScope) { + + element = $compile( + '
' + + '
{{ msg }}
' + + '
' + )($rootScope); + + $rootScope.$apply('t = true'); + var childScopes = getChildScopes($rootScope); + + $rootScope.$apply('t = false'); + for (var i = 0; i < childScopes.length; ++i) { + expect(childScopes[i].$$destroyed).toBe(true); + } + })); }); diff --git a/test/ng/rootScopeSpec.js b/test/ng/rootScopeSpec.js index 48c001489f66..5034ce637261 100644 --- a/test/ng/rootScopeSpec.js +++ b/test/ng/rootScopeSpec.js @@ -1020,6 +1020,13 @@ describe('Scope', function() { expect(log).toBe('123'); })); + it('should broadcast the $destroy only once', inject(function($rootScope, log) { + var isolateScope = first.$new(true); + isolateScope.$on('$destroy', log.fn('event')); + first.$destroy(); + isolateScope.$destroy(); + expect(log).toEqual('event'); + })); it('should decrement ancestor $$listenerCount entries', inject(function($rootScope) { var EVENT = 'fooEvent', From 1598639295967d6180575ab8dd55c182b7300946 Mon Sep 17 00:00:00 2001 From: Lucas Galfaso Date: Mon, 22 Sep 2014 13:08:25 +0200 Subject: [PATCH 2/2] fix($compile): Resolve leak with asynchronous compilation Stop an asynchronous compilation when this is performed on an already destroyed scope Closes #9199 --- src/ng/compile.js | 3 +++ src/ng/rootScope.js | 4 +++ test/ng/compileSpec.js | 61 +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 67 insertions(+), 1 deletion(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index e8f582a385d8..2e252a8a62df 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -2011,6 +2011,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { boundTranscludeFn = linkQueue.shift(), linkNode = $compileNode[0]; + if (scope.$$destroyed) continue; + if (beforeTemplateLinkNode !== beforeTemplateCompileNode) { var oldClasses = beforeTemplateLinkNode.className; @@ -2037,6 +2039,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { return function delayedNodeLinkFn(ignoreChildLinkFn, scope, node, rootElement, boundTranscludeFn) { var childBoundTranscludeFn = boundTranscludeFn; + if (scope.$$destroyed) return; if (linkQueue) { linkQueue.push(scope); linkQueue.push(node); diff --git a/src/ng/rootScope.js b/src/ng/rootScope.js index 72fdc920d9f8..0ec85364c915 100644 --- a/src/ng/rootScope.js +++ b/src/ng/rootScope.js @@ -223,6 +223,10 @@ function $RootScopeProvider(){ parent.$$childHead = parent.$$childTail = child; } + // When the new scope is not isolated or we inherit from `this`, and + // the parent scope is destroyed, the property `$$destroyed` is inherited + // prototypically. In all other cases, this property needs to be set + // when the parent scope is destroyed. // The listener needs to be added after the parent is set if (isolate || parent != this) child.$on('$destroy', destroyChild); diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index f7bb09b04905..cc0203e104b8 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -4537,6 +4537,65 @@ describe('$compile', function() { }); }); + it('should not leak when continuing the compilation of elements on a scope that was destroyed', function() { + if (jQuery) { + // jQuery 2.x doesn't expose the cache storage. + return; + } + + var linkFn = jasmine.createSpy('linkFn'); + + module(function($controllerProvider, $compileProvider) { + $controllerProvider.register('Leak', function ($scope, $timeout) { + $scope.code = 'red'; + $timeout(function () { + $scope.code = 'blue'; + }); + }); + $compileProvider.directive('isolateRed', function() { + return { + restrict: 'A', + scope: {}, + template: '
' + }; + }); + $compileProvider.directive('red', function() { + return { + restrict: 'A', + templateUrl: 'red.html', + scope: {}, + link: linkFn + }; + }); + }); + + inject(function($compile, $rootScope, $httpBackend, $timeout, $templateCache) { + $httpBackend.whenGET('red.html').respond('

red.html

'); + var template = $compile( + '
' + + '
' + + '
' + + '
' + + '
' + + '
' + + '
'); + element = template($rootScope); + $rootScope.$digest(); + $timeout.flush(); + $httpBackend.flush(); + expect(linkFn).not.toHaveBeenCalled(); + expect(jqLiteCacheSize()).toEqual(2); + + $templateCache.removeAll(); + var destroyedScope = $rootScope.$new(); + destroyedScope.$destroy(); + var clone = template(destroyedScope); + $rootScope.$digest(); + $timeout.flush(); + expect(linkFn).not.toHaveBeenCalled(); + }); + }); + if (jQuery) { describe('cleaning up after a replaced element', function () { var $compile, xs; @@ -5117,7 +5176,7 @@ describe('$compile', function() { expect(countScopes($rootScope)).toEqual(1); })); - it('should destroy mark as destroyed all sub scopes of the scope being destroyed', + it('should mark as destroyed all sub scopes of the scope being destroyed', inject(function($compile, $rootScope) { element = $compile(