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

Commit 75373dd

Browse files
committed
fix(ngMock): prevent memory leak due to data attached to $rootElement
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
1 parent 2dda75b commit 75373dd

File tree

3 files changed

+140
-16
lines changed

3 files changed

+140
-16
lines changed

src/ngMock/angular-mocks.js

+21-10
Original file line numberDiff line numberDiff line change
@@ -127,12 +127,12 @@ angular.mock.$Browser = function() {
127127
};
128128
angular.mock.$Browser.prototype = {
129129

130-
/**
131-
* @name $browser#poll
132-
*
133-
* @description
134-
* run all fns in pollFns
135-
*/
130+
/**
131+
* @name $browser#poll
132+
*
133+
* @description
134+
* run all fns in pollFns
135+
*/
136136
poll: function poll() {
137137
angular.forEach(this.pollFns, function(pollFn) {
138138
pollFn();
@@ -2089,10 +2089,12 @@ angular.mock.$RAFDecorator = ['$delegate', function($delegate) {
20892089
/**
20902090
*
20912091
*/
2092+
var originalRootElement;
20922093
angular.mock.$RootElementProvider = function() {
2093-
this.$get = function() {
2094-
return angular.element('<div ng-app></div>');
2095-
};
2094+
this.$get = ['$injector', function($injector) {
2095+
originalRootElement = angular.element('<div ng-app></div>').data('$injector', $injector);
2096+
return originalRootElement;
2097+
}];
20962098
};
20972099

20982100
/**
@@ -2577,6 +2579,7 @@ if (window.jasmine || window.mocha) {
25772579

25782580

25792581
(window.beforeEach || window.setup)(function() {
2582+
originalRootElement = null;
25802583
annotatedFunctions = [];
25812584
currentSpec = this;
25822585
});
@@ -2600,7 +2603,15 @@ if (window.jasmine || window.mocha) {
26002603
currentSpec = null;
26012604

26022605
if (injector) {
2603-
injector.get('$rootElement').off();
2606+
// Ensure `$rootElement` is instantiated, before checking `originalRootElement`
2607+
var $rootElement = injector.get('$rootElement');
2608+
var rootNode = $rootElement && $rootElement[0];
2609+
var cleanUpNodes = !originalRootElement ? [] : [originalRootElement[0]];
2610+
if (rootNode && (!originalRootElement || rootNode !== originalRootElement[0])) {
2611+
cleanUpNodes.push(rootNode);
2612+
}
2613+
angular.element.cleanData(cleanUpNodes);
2614+
26042615
injector.get('$rootScope').$destroy();
26052616
}
26062617

test/helpers/testabilityPatch.js

+2-6
Original file line numberDiff line numberDiff line change
@@ -139,12 +139,8 @@ function dealoc(obj) {
139139
}
140140

141141
function cleanup(element) {
142-
element.off().removeData();
143-
if (window.jQuery) {
144-
// jQuery 2.x doesn't expose the cache storage; ensure all element data
145-
// is removed during its cleanup.
146-
jQuery.cleanData([element]);
147-
}
142+
angular.element.cleanData(element);
143+
148144
// Note: We aren't using element.contents() here. Under jQuery, element.contents() can fail
149145
// for IFRAME elements. jQuery explicitly uses (element.contentDocument ||
150146
// element.contentWindow.document) and both properties are null for IFRAMES that aren't attached

test/ngMock/angular-mocksSpec.js

+117
Original file line numberDiff line numberDiff line change
@@ -1667,6 +1667,10 @@ describe('ngMock', function() {
16671667
it('should create mock application root', inject(function($rootElement) {
16681668
expect($rootElement.text()).toEqual('');
16691669
}));
1670+
1671+
it('should attach the `$injector` to `$rootElement`', inject(function($injector, $rootElement) {
1672+
expect($rootElement.injector()).toBe($injector);
1673+
}));
16701674
});
16711675

16721676

@@ -2404,9 +2408,122 @@ describe('ngMockE2E', function() {
24042408
});
24052409
});
24062410

2411+
24072412
describe('make sure that we can create an injector outside of tests', function() {
24082413
//since some libraries create custom injectors outside of tests,
24092414
//we want to make sure that this is not breaking the internals of
24102415
//how we manage annotated function cleanup during tests. See #10967
24112416
angular.injector([function($injector) {}]);
24122417
});
2418+
2419+
2420+
describe('`afterEach` clean-up', function() {
2421+
describe('undecorated `$rootElement`', function() {
2422+
var prevRootElement;
2423+
var prevCleanDataSpy;
2424+
2425+
2426+
it('should set up spies so the next test can verify `$rootElement` was cleaned up', function() {
2427+
module(function($provide) {
2428+
$provide.decorator('$rootElement', function($delegate) {
2429+
prevRootElement = $delegate;
2430+
2431+
// Spy on `angular.element.cleanData()`, so the next test can verify
2432+
// that it has been called as necessary
2433+
prevCleanDataSpy = spyOn(angular.element, 'cleanData').andCallThrough();
2434+
2435+
return $delegate;
2436+
});
2437+
});
2438+
2439+
// Inject the `$rootElement` to ensure it has been created
2440+
inject(function($rootElement) {
2441+
expect($rootElement.injector()).toBeDefined();
2442+
});
2443+
});
2444+
2445+
2446+
it('should clean up `$rootElement` after each test', function() {
2447+
// One call is made by `testabilityPatch`'s `dealoc()`
2448+
// We want to verify the subsequent call, made by `angular-mocks`
2449+
expect(prevCleanDataSpy.callCount).toBe(2);
2450+
2451+
var cleanUpNodes = prevCleanDataSpy.calls[1].args[0];
2452+
expect(cleanUpNodes.length).toBe(1);
2453+
expect(cleanUpNodes[0]).toBe(prevRootElement[0]);
2454+
});
2455+
});
2456+
2457+
2458+
describe('decorated `$rootElement`', function() {
2459+
var prevOriginalRootElement;
2460+
var prevRootElement;
2461+
var prevCleanDataSpy;
2462+
2463+
2464+
it('should set up spies so the next text can verify `$rootElement` was cleaned up', function() {
2465+
module(function($provide) {
2466+
$provide.decorator('$rootElement', function($delegate) {
2467+
prevOriginalRootElement = $delegate;
2468+
2469+
// Mock `$rootElement` to be able to verify that the correct object is cleaned up
2470+
prevRootElement = angular.element('<div></div>');
2471+
2472+
// Spy on `angular.element.cleanData()`, so the next test can verify
2473+
// that it has been called as necessary
2474+
prevCleanDataSpy = spyOn(angular.element, 'cleanData').andCallThrough();
2475+
2476+
return prevRootElement;
2477+
});
2478+
});
2479+
2480+
// Inject the `$rootElement` to ensure it has been created
2481+
inject(function($rootElement) {
2482+
expect($rootElement).toBe(prevRootElement);
2483+
expect(prevOriginalRootElement.injector()).toBeDefined();
2484+
expect(prevRootElement.injector()).toBeUndefined();
2485+
2486+
// If we don't clean up `prevOriginalRootElement`-related data now, `testabilityPatch` will
2487+
// complain about a memory leak, because it doesn't clean up after the original
2488+
// `$rootElement`
2489+
// This is a false alarm, because `angular-mocks` would have cleaned up in a subsequent
2490+
// `afterEach` block
2491+
prevOriginalRootElement.removeData();
2492+
});
2493+
});
2494+
2495+
2496+
it('should clean up `$rootElement` (both original and decorated) after each test', function() {
2497+
// One call is made by `testabilityPatch`'s `dealoc()`
2498+
// We want to verify the subsequent call, made by `angular-mocks`
2499+
expect(prevCleanDataSpy.callCount).toBe(2);
2500+
2501+
var cleanUpNodes = prevCleanDataSpy.calls[1].args[0];
2502+
expect(cleanUpNodes.length).toBe(2);
2503+
expect(cleanUpNodes[0]).toBe(prevOriginalRootElement[0]);
2504+
expect(cleanUpNodes[1]).toBe(prevRootElement[0]);
2505+
});
2506+
});
2507+
2508+
2509+
describe('uninstantiated or falsy `$rootElement`', function() {
2510+
it('should not break if `$rootElement` was never instantiated', function() {
2511+
// Just an empty test to verify that `angular-mocks` doesn't break,
2512+
// when trying to clean up `$rootElement`, if `$rootElement` was never injected in the test
2513+
// (and thus never instantiated/created)
2514+
2515+
// Ensure the `$injector` is created - if there is no `$injector`, no clean-up takes places
2516+
inject(function() {});
2517+
});
2518+
2519+
2520+
it('should not break if the decorated `$rootElement` is falsy (e.g. `null`)', function() {
2521+
module(function($provide) {
2522+
$provide.value('$rootElement', null);
2523+
});
2524+
2525+
// Ensure the `$injector` is created - if there is no `$injector`, no clean-up takes places
2526+
inject(function() {});
2527+
});
2528+
});
2529+
});

0 commit comments

Comments
 (0)