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

fix(ngMock): prevent memory leak due to data attached to $rootElement #14098

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 21 additions & 10 deletions src/ngMock/angular-mocks.js
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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]);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to do anything to tidy up the spies that we are holding on to?

Copy link
Member Author

Choose a reason for hiding this comment

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

Theoretically, no. Jasmine will restore the original objects after each test. We hold on to the spies, but the original object is restored. When the describe blocks are done, the references to those spies are useless (and should be garbage collected), but don't affect the environment in any way.



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() {});
});
});
});