From 7c06c447af2674f1ea794c7cc10ff3a9d573a62a Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 24 Jun 2014 11:47:11 +0100 Subject: [PATCH 1/4] test($compile): check for memory leaks in nested transclusion --- test/ng/compileSpec.js | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 5de1728a262b..0009b7c6e852 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -4409,6 +4409,35 @@ describe('$compile', function() { $rootScope.$digest(); expect(element.text()).toEqual('transcluded content'); })); + + + it('should not leak memory with nested transclusion', function() { + var calcCacheSize = function() { + var count = 0; + for (var k in jqLite.cache) { ++count; } + return count; + }; + + inject(function($compile, $rootScope) { + var size; + + expect(calcCacheSize()).toEqual(0); + + element = jqLite('
'); + $compile(element)($rootScope.$new()); + + $rootScope.nums = [0,1,2]; + $rootScope.$apply(); + size = calcCacheSize(); + + $rootScope.nums = [3,4,5]; + $rootScope.$apply(); + expect(calcCacheSize()).toEqual(size); + + element.remove(); + expect(calcCacheSize()).toEqual(0); + }); + }); }); From 1a879046868aadd84728edf60781701852d5e558 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 24 Jun 2014 12:08:45 +0100 Subject: [PATCH 2/4] test($compile): check no memory leak with coexisting element transcludes If an element contains two "element" transcludes then the initial clone consists of only comment nodes. The concern was that this meant that the transclude scopes would not be cleaned up. But it turns out that in the case that there are only comments then the scope is never attached to anything so we don't need to worry about cleaning it up. Later if a concrete element is created as part of the transclude then these elements will have destroy handlers. --- test/ng/compileSpec.js | 60 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 0009b7c6e852..f72c1fd8001e 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -3928,6 +3928,66 @@ describe('$compile', function() { }); + + it('should not leak if two "element" transclusions are on the same element', function() { + var calcCacheSize = function() { + var size = 0; + forEach(jqLite.cache, function(item, key) { size++; }); + return size; + }; + + inject(function($compile, $rootScope) { + expect(calcCacheSize()).toEqual(0); + + element = $compile('
{{x}}
')($rootScope); + expect(calcCacheSize()).toEqual(1); + + $rootScope.$apply('xs = [0,1]'); + expect(calcCacheSize()).toEqual(2); + + $rootScope.$apply('xs = [0]'); + expect(calcCacheSize()).toEqual(1); + + $rootScope.$apply('xs = []'); + expect(calcCacheSize()).toEqual(1); + + element.remove(); + expect(calcCacheSize()).toEqual(0); + }); + }); + + + it('should not leak if two "element" transclusions are on the same element', function() { + var calcCacheSize = function() { + var size = 0; + forEach(jqLite.cache, function(item, key) { size++; }); + return size; + }; + inject(function($compile, $rootScope) { + expect(calcCacheSize()).toEqual(0); + element = $compile('
{{x}}
')($rootScope); + + $rootScope.$apply('xs = [0,1]'); + // At this point we have a bunch of comment placeholders but no real transcluded elements + // So the cache only contains the root element's data + expect(calcCacheSize()).toEqual(1); + + $rootScope.$apply('val = true'); + // Now we have two concrete transcluded elements plus some comments so two more cache items + expect(calcCacheSize()).toEqual(3); + + $rootScope.$apply('val = false'); + // Once again we only have comments so no transcluded elements and the cache is back to just + // the root element + expect(calcCacheSize()).toEqual(1); + + element.remove(); + // Now we've even removed the root element along with its cache + expect(calcCacheSize()).toEqual(0); + }); + }); + + it('should remove transclusion scope, when the DOM is destroyed', function() { module(function() { directive('box', valueFn({ From a3d0964748045d9c1da774f1d027cd6f9a40394e Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 24 Jun 2014 11:47:11 +0100 Subject: [PATCH 3/4] fix(jqLite): don't attach event handlers to comments or text nodes We were attaching handlers to comment nodes when setting up bound transclusion functions. But we don't clean up comments and text nodes when deallocating so there was a memory leak. Closes #7913 --- src/jqLite.js | 13 ++++++++++++- test/jqLiteSpec.js | 10 ++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/jqLite.js b/src/jqLite.js index 3eee01f507d1..e81ecb755677 100644 --- a/src/jqLite.js +++ b/src/jqLite.js @@ -161,6 +161,12 @@ function jqLiteIsTextNode(html) { return !HTML_REGEXP.test(html); } +function jqLiteAcceptsData(node) { + // The window object can accept data but has no nodeType + // Otherwise we are only interested in elements (1) and documents (9) + return !node.nodeType || node.nodeType === 1 || node.nodeType === 9; +} + function jqLiteBuildFragment(html, context) { var elem, tmp, tag, wrap, fragment = context.createDocumentFragment(), @@ -319,7 +325,7 @@ function jqLiteData(element, key, value) { if (isSetter) { // set data only on Elements and Documents - if (element.nodeType === 1 || element.nodeType === 9) { + if (jqLiteAcceptsData(element)) { data[key] = value; } } else { @@ -750,6 +756,11 @@ forEach({ on: function onFn(element, type, fn, unsupported){ if (isDefined(unsupported)) throw jqLiteMinErr('onargs', 'jqLite#on() does not support the `selector` or `eventData` parameters'); + // Do not add event handlers to non-elements because they will not be cleaned up. + if (!jqLiteAcceptsData(element)) { + return; + } + var events = jqLiteExpandoStore(element, 'events'), handle = jqLiteExpandoStore(element, 'handle'); diff --git a/test/jqLiteSpec.js b/test/jqLiteSpec.js index ffe02570f59d..da7200fce934 100644 --- a/test/jqLiteSpec.js +++ b/test/jqLiteSpec.js @@ -993,6 +993,16 @@ describe('jqLite', function() { expect(log).toEqual('click on: A;click on: B;'); }); + it('should not bind to comment or text nodes', function() { + var nodes = jqLite('Some text'); + var someEventHandler = jasmine.createSpy('someEventHandler'); + + nodes.on('someEvent', someEventHandler); + nodes.triggerHandler('someEvent'); + + expect(someEventHandler).not.toHaveBeenCalled(); + }); + it('should bind to all events separated by space', function() { var elm = jqLite(a), callback = jasmine.createSpy('callback'); From ec5c56b982ea98bafa61d49bff8b34d466dcea53 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 24 Jun 2014 12:25:25 +0100 Subject: [PATCH 4/4] fix(jqLite): never add to the cache for non-element/document nodes Calling `jqLite.data()` on a disallowed node type caused an empty object to be added to the cache. This could lead to memory leaks since we no longer clean up such node types when they are removed from the DOM. --- src/jqLite.js | 37 ++++++++++++++++++------------------- test/jqLiteSpec.js | 17 +++++++++++++++++ 2 files changed, 35 insertions(+), 19 deletions(-) diff --git a/src/jqLite.js b/src/jqLite.js index e81ecb755677..1fa0f1d64765 100644 --- a/src/jqLite.js +++ b/src/jqLite.js @@ -314,30 +314,29 @@ function jqLiteExpandoStore(element, key, value) { } function jqLiteData(element, key, value) { - var data = jqLiteExpandoStore(element, 'data'), - isSetter = isDefined(value), - keyDefined = !isSetter && isDefined(key), - isSimpleGetter = keyDefined && !isObject(key); + if (jqLiteAcceptsData(element)) { + var data = jqLiteExpandoStore(element, 'data'), + isSetter = isDefined(value), + keyDefined = !isSetter && isDefined(key), + isSimpleGetter = keyDefined && !isObject(key); - if (!data && !isSimpleGetter) { - jqLiteExpandoStore(element, 'data', data = {}); - } + if (!data && !isSimpleGetter) { + jqLiteExpandoStore(element, 'data', data = {}); + } - if (isSetter) { - // set data only on Elements and Documents - if (jqLiteAcceptsData(element)) { + if (isSetter) { data[key] = value; - } - } else { - if (keyDefined) { - if (isSimpleGetter) { - // don't create data in this case. - return data && data[key]; + } else { + if (keyDefined) { + if (isSimpleGetter) { + // don't create data in this case. + return data && data[key]; + } else { + extend(data, key); + } } else { - extend(data, key); + return data; } - } else { - return data; } } } diff --git a/test/jqLiteSpec.js b/test/jqLiteSpec.js index da7200fce934..cf880aef0382 100644 --- a/test/jqLiteSpec.js +++ b/test/jqLiteSpec.js @@ -386,6 +386,23 @@ describe('jqLite', function() { selected.removeData('prop2'); }); + + it('should not add to the cache if the node is a comment or text node', function() { + var calcCacheSize = function() { + var count = 0; + for (var k in jqLite.cache) { ++count; } + return count; + }; + + var nodes = jqLite(' and some text'); + expect(calcCacheSize()).toEqual(0); + nodes.data('someKey'); + expect(calcCacheSize()).toEqual(0); + nodes.data('someKey', 'someValue'); + expect(calcCacheSize()).toEqual(0); + }); + + it('should emit $destroy event if element removed via remove()', function() { var log = ''; var element = jqLite(a);