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

Commit

Permalink
fix(ngMock): prevent memory leak due to data attached to $rootElement
Browse files Browse the repository at this point in the history
Starting with 88bb551, `ngMock` will attach the `$injector` to the `$rootElement`, but will never
clean it up, resulting in a memory leak. Since a new `$rootElement` is created for every test,
this leak causes Karma to crash on large test-suites.
The problem was not detected by our internal tests, because we do our own clean-up in
`testabilityPatch.js`.

88bb551 was revert with 1b8590a.
This commit incorporates the changes from 88bb551 and prevents the memory leak, by cleaning up all
data attached to `$rootElement` after each test.

Fixes #14094

Closes #14098
  • Loading branch information
gkalpak committed Feb 22, 2016
1 parent a7244fd commit 85ef70f
Show file tree
Hide file tree
Showing 3 changed files with 140 additions and 16 deletions.
31 changes: 21 additions & 10 deletions src/ngMock/angular-mocks.js
Expand Up @@ -127,12 +127,12 @@ angular.mock.$Browser = function() {
};
angular.mock.$Browser.prototype = {

/**
* @name $browser#poll
*
* @description
* run all fns in pollFns
*/
/**
* @name $browser#poll
*
* @description
* run all fns in pollFns
*/
poll: function poll() {
angular.forEach(this.pollFns, function(pollFn) {
pollFn();
Expand Down Expand Up @@ -2089,10 +2089,12 @@ angular.mock.$RAFDecorator = ['$delegate', function($delegate) {
/**
*
*/
var originalRootElement;
angular.mock.$RootElementProvider = function() {
this.$get = function() {
return angular.element('<div ng-app></div>');
};
this.$get = ['$injector', function($injector) {
originalRootElement = angular.element('<div ng-app></div>').data('$injector', $injector);
return originalRootElement;
}];
};

/**
Expand Down Expand Up @@ -2577,6 +2579,7 @@ if (window.jasmine || window.mocha) {


(window.beforeEach || window.setup)(function() {
originalRootElement = null;
annotatedFunctions = [];
currentSpec = this;
});
Expand All @@ -2600,7 +2603,15 @@ if (window.jasmine || window.mocha) {
currentSpec = null;

if (injector) {
injector.get('$rootElement').off();
// Ensure `$rootElement` is instantiated, before checking `originalRootElement`
var $rootElement = injector.get('$rootElement');
var rootNode = $rootElement && $rootElement[0];
var cleanUpNodes = !originalRootElement ? [] : [originalRootElement[0]];
if (rootNode && (!originalRootElement || rootNode !== originalRootElement[0])) {
cleanUpNodes.push(rootNode);
}
angular.element.cleanData(cleanUpNodes);

injector.get('$rootScope').$destroy();
}

Expand Down
8 changes: 2 additions & 6 deletions test/helpers/testabilityPatch.js
Expand Up @@ -139,12 +139,8 @@ function dealoc(obj) {
}

function cleanup(element) {
element.off().removeData();
if (window.jQuery) {
// jQuery 2.x doesn't expose the cache storage; ensure all element data
// is removed during its cleanup.
jQuery.cleanData([element]);
}
angular.element.cleanData(element);

// Note: We aren't using element.contents() here. Under jQuery, element.contents() can fail
// for IFRAME elements. jQuery explicitly uses (element.contentDocument ||
// element.contentWindow.document) and both properties are null for IFRAMES that aren't attached
Expand Down
117 changes: 117 additions & 0 deletions test/ngMock/angular-mocksSpec.js
Expand Up @@ -1667,6 +1667,10 @@ describe('ngMock', function() {
it('should create mock application root', inject(function($rootElement) {
expect($rootElement.text()).toEqual('');
}));

it('should attach the `$injector` to `$rootElement`', inject(function($injector, $rootElement) {
expect($rootElement.injector()).toBe($injector);
}));
});


Expand Down Expand Up @@ -2404,9 +2408,122 @@ describe('ngMockE2E', function() {
});
});


describe('make sure that we can create an injector outside of tests', function() {
//since some libraries create custom injectors outside of tests,
//we want to make sure that this is not breaking the internals of
//how we manage annotated function cleanup during tests. See #10967
angular.injector([function($injector) {}]);
});


describe('`afterEach` clean-up', function() {
describe('undecorated `$rootElement`', function() {
var prevRootElement;
var prevCleanDataSpy;


it('should set up spies so the next test can verify `$rootElement` was cleaned up', function() {
module(function($provide) {
$provide.decorator('$rootElement', function($delegate) {
prevRootElement = $delegate;

// Spy on `angular.element.cleanData()`, so the next test can verify
// that it has been called as necessary
prevCleanDataSpy = spyOn(angular.element, 'cleanData').andCallThrough();

return $delegate;
});
});

// Inject the `$rootElement` to ensure it has been created
inject(function($rootElement) {
expect($rootElement.injector()).toBeDefined();
});
});


it('should clean up `$rootElement` after each test', function() {
// One call is made by `testabilityPatch`'s `dealoc()`
// We want to verify the subsequent call, made by `angular-mocks`
expect(prevCleanDataSpy.callCount).toBe(2);

var cleanUpNodes = prevCleanDataSpy.calls[1].args[0];
expect(cleanUpNodes.length).toBe(1);
expect(cleanUpNodes[0]).toBe(prevRootElement[0]);
});
});


describe('decorated `$rootElement`', function() {
var prevOriginalRootElement;
var prevRootElement;
var prevCleanDataSpy;


it('should set up spies so the next text can verify `$rootElement` was cleaned up', function() {
module(function($provide) {
$provide.decorator('$rootElement', function($delegate) {
prevOriginalRootElement = $delegate;

// Mock `$rootElement` to be able to verify that the correct object is cleaned up
prevRootElement = angular.element('<div></div>');

// Spy on `angular.element.cleanData()`, so the next test can verify
// that it has been called as necessary
prevCleanDataSpy = spyOn(angular.element, 'cleanData').andCallThrough();

return prevRootElement;
});
});

// Inject the `$rootElement` to ensure it has been created
inject(function($rootElement) {
expect($rootElement).toBe(prevRootElement);
expect(prevOriginalRootElement.injector()).toBeDefined();
expect(prevRootElement.injector()).toBeUndefined();

// If we don't clean up `prevOriginalRootElement`-related data now, `testabilityPatch` will
// complain about a memory leak, because it doesn't clean up after the original
// `$rootElement`
// This is a false alarm, because `angular-mocks` would have cleaned up in a subsequent
// `afterEach` block
prevOriginalRootElement.removeData();
});
});


it('should clean up `$rootElement` (both original and decorated) after each test', function() {
// One call is made by `testabilityPatch`'s `dealoc()`
// We want to verify the subsequent call, made by `angular-mocks`
expect(prevCleanDataSpy.callCount).toBe(2);

var cleanUpNodes = prevCleanDataSpy.calls[1].args[0];
expect(cleanUpNodes.length).toBe(2);
expect(cleanUpNodes[0]).toBe(prevOriginalRootElement[0]);
expect(cleanUpNodes[1]).toBe(prevRootElement[0]);
});
});


describe('uninstantiated or falsy `$rootElement`', function() {
it('should not break if `$rootElement` was never instantiated', function() {
// Just an empty test to verify that `angular-mocks` doesn't break,
// when trying to clean up `$rootElement`, if `$rootElement` was never injected in the test
// (and thus never instantiated/created)

// Ensure the `$injector` is created - if there is no `$injector`, no clean-up takes places
inject(function() {});
});


it('should not break if the decorated `$rootElement` is falsy (e.g. `null`)', function() {
module(function($provide) {
$provide.value('$rootElement', null);
});

// Ensure the `$injector` is created - if there is no `$injector`, no clean-up takes places
inject(function() {});
});
});
});

0 comments on commit 85ef70f

Please sign in to comment.