Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(jqLite): don't attach event handler to comments (memory leak) #7942

Closed
wants to merge 4 commits into from
Closed

fix(jqLite): don't attach event handler to comments (memory leak) #7942

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7c06c44
test($compile): check for memory leaks in nested transclusion
petebacondarwin Jun 24, 2014
1a87904
test($compile): check no memory leak with coexisting element transcludes
petebacondarwin Jun 24, 2014
a3d0964
fix(jqLite): don't attach event handlers to comments or text nodes
petebacondarwin Jun 24, 2014
eca016d
test(compile): check transclusion/scopes work with multi-element dire…
petebacondarwin Jun 24, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/jqLite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also please add a separate commit to move this up and prevent the empty expando object created?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already done at #7966

data[key] = value;
}
} else {
Expand Down Expand Up @@ -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');

Expand Down
10 changes: 10 additions & 0 deletions test/jqLiteSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 comment -->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');
Expand Down
277 changes: 269 additions & 8 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
'use strict';


function calcCacheSize() {
var size = 0;
for(var key in jqLite.cache) { size++; }
return size;
}


describe('$compile', function() {
var element, directive, $compile, $rootScope;

Expand Down Expand Up @@ -158,12 +166,6 @@ describe('$compile', function() {
});

it('should not leak memory when there are top level empty text nodes', function() {
var calcCacheSize = function() {
var size = 0;
forEach(jqLite.cache, function(item, key) { size++; });
return size;
};

// We compile the contents of element (i.e. not element itself)
// Then delete these contents and check the cache has been reset to zero

Expand Down Expand Up @@ -3928,6 +3930,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('<div><div ng-repeat="x in xs" ng-if="x==1">{{x}}</div></div>')($rootScope);
expect(calcCacheSize()).toEqual(1);

$rootScope.$apply('xs = [0,1]');
expect(calcCacheSize()).toEqual(2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw wouldn't the global after each that checks the state of the cache catch the leaks?

These tests are much more readable, so keep things as they are. I'm just curious.


$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('<div><div ng-repeat="x in xs" ng-if="val">{{x}}</div></div>')($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({
Expand Down Expand Up @@ -4409,6 +4471,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('<div><ul><li ng-repeat="n in nums">{{n}} => <i ng-if="0 === n%2">Even</i><i ng-if="1 === n%2">Odd</i></li></ul></div>');
$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);
});
});
});


Expand Down Expand Up @@ -5380,7 +5471,7 @@ describe('$compile', function() {
}));


it('should group on nested groups of same directive', inject(function($compile, $rootScope) {
it('should group on nested groups', inject(function($compile, $rootScope) {
$rootScope.show = false;
element = $compile(
'<div></div>' +
Expand All @@ -5395,7 +5486,7 @@ describe('$compile', function() {
}));


it('should group on nested groups', inject(function($compile, $rootScope) {
it('should group on nested groups of same directive', inject(function($compile, $rootScope) {
$rootScope.show = false;
element = $compile(
'<div></div>' +
Expand All @@ -5410,6 +5501,176 @@ describe('$compile', function() {
}));


it('should set up and destroy the transclusion scopes correctly',
inject(function($compile, $rootScope) {
element = $compile(
'<div>' +
'<div ng-if-start="val0"><span ng-if="val1"></span></div>' +
'<div ng-if-end><span ng-if="val2"></span></div>' +
'</div>'
)($rootScope);
$rootScope.$apply('val0 = true; val1 = true; val2 = true');

// At this point we should have something like:
//
// <div class="ng-scope">
//
// <!-- ngIf: val0 -->
//
// <div ng-if-start="val0" class="ng-scope">
// <!-- ngIf: val1 -->
// <span ng-if="val1" class="ng-scope"></span>
// <!-- end ngIf: val1 -->
// </div>
//
// <div ng-if-end="" class="ng-scope">
// <!-- ngIf: val2 -->
// <span ng-if="val2" class="ng-scope"></span>
// <!-- end ngIf: val2 -->
// </div>
//
// <!-- end ngIf: val0 -->
// </div>
var ngIfStartScope = element.find('div').eq(0).scope();
var ngIfEndScope = element.find('div').eq(1).scope();

expect(ngIfStartScope.$id).toEqual(ngIfEndScope.$id);

var ngIf1Scope = element.find('span').eq(0).scope();
var ngIf2Scope = element.find('span').eq(1).scope();

expect(ngIf1Scope.$id).not.toEqual(ngIf2Scope.$id);
expect(ngIf1Scope.$parent.$id).toEqual(ngIf2Scope.$parent.$id);

$rootScope.$apply('val1 = false');

// Now we should have something like:
//
// <div class="ng-scope">
// <!-- ngIf: val0 -->
// <div ng-if-start="val0" class="ng-scope">
// <!-- ngIf: val1 -->
// </div>
// <div ng-if-end="" class="ng-scope">
// <!-- ngIf: val2 -->
// <span ng-if="val2" class="ng-scope"></span>
// <!-- end ngIf: val2 -->
// </div>
// <!-- end ngIf: val0 -->
// </div>

expect(ngIfStartScope.$$destroyed).not.toEqual(true);
expect(ngIf1Scope.$$destroyed).toEqual(true);
expect(ngIf2Scope.$$destroyed).not.toEqual(true);

$rootScope.$apply('val0 = false');

// Now we should have something like:
//
// <div class="ng-scope">
// <!-- ngIf: val0 -->
// </div>

expect(ngIfStartScope.$$destroyed).toEqual(true);
expect(ngIf1Scope.$$destroyed).toEqual(true);
expect(ngIf2Scope.$$destroyed).toEqual(true);
}));


it('should set up and destroy the transclusion scopes correctly',
inject(function($compile, $rootScope) {
element = $compile(
'<div>' +
'<div ng-repeat-start="val in val0" ng-if="val1"></div>' +
'<div ng-repeat-end ng-if="val2"></div>' +
'</div>'
)($rootScope);

// To begin with there is (almost) nothing:
// <div class="ng-scope">
// <!-- ngRepeat: val in val0 -->
// </div>

expect(element.scope().$id).toEqual($rootScope.$id);

// Now we create all the elements
$rootScope.$apply('val0 = [1]; val1 = true; val2 = true');

// At this point we have:
//
// <div class="ng-scope">
//
// <!-- ngRepeat: val in val0 -->
// <!-- ngIf: val1 -->
// <div ng-repeat-start="val in val0" class="ng-scope">
// </div>
// <!-- end ngIf: val1 -->
//
// <!-- ngIf: val2 -->
// <div ng-repeat-end="" class="ng-scope">
// </div>
// <!-- end ngIf: val2 -->
// <!-- end ngRepeat: val in val0 -->
// </div>
var ngIf1Scope = element.find('div').eq(0).scope();
var ngIf2Scope = element.find('div').eq(1).scope();
var ngRepeatScope = ngIf1Scope.$parent;

expect(ngIf1Scope.$id).not.toEqual(ngIf2Scope.$id);
expect(ngIf1Scope.$parent.$id).toEqual(ngRepeatScope.$id);
expect(ngIf2Scope.$parent.$id).toEqual(ngRepeatScope.$id);

// What is happening here??
// We seem to have a repeater scope which doesn't actually match to any element
expect(ngRepeatScope.$parent.$id).toEqual($rootScope.$id);


// Now remove the first ngIf element from the first item in the repeater
$rootScope.$apply('val1 = false');

// At this point we should have:
//
// <div class="ng-scope">
// <!-- ngRepeat: val in val0 -->
//
// <!-- ngIf: val1 -->
//
// <!-- ngIf: val2 -->
// <div ng-repeat-end="" ng-if="val2" class="ng-scope"></div>
// <!-- end ngIf: val2 -->
//
// <!-- end ngRepeat: val in val0 -->
// </div>
//
expect(ngRepeatScope.$$destroyed).toEqual(false);
expect(ngIf1Scope.$$destroyed).toEqual(true);
expect(ngIf2Scope.$$destroyed).toEqual(false);

// Now remove the second ngIf element from the first item in the repeater
$rootScope.$apply('val2 = false');

// We are mostly back to where we started
//
// <div class="ng-scope">
// <!-- ngRepeat: val in val0 -->
// <!-- ngIf: val1 -->
// <!-- ngIf: val2 -->
// <!-- end ngRepeat: val in val0 -->
// </div>

expect(ngRepeatScope.$$destroyed).toEqual(false);
expect(ngIf1Scope.$$destroyed).toEqual(true);
expect(ngIf2Scope.$$destroyed).toEqual(true);

// Finally remove the repeat items
$rootScope.$apply('val0 = []');

// Somehow this ngRepeat scope knows how to destroy itself...
expect(ngRepeatScope.$$destroyed).toEqual(true);
expect(ngIf1Scope.$$destroyed).toEqual(true);
expect(ngIf2Scope.$$destroyed).toEqual(true);
}));

it('should throw error if unterminated', function () {
module(function($compileProvider) {
$compileProvider.directive('foo', function() {
Expand Down