fix(ngMock): prevent memory leak due to data attached to `$rootElement` by gkalpak · Pull Request #14098 · angular/angular.js

gkalpak · 2016-02-20T22:44:42Z

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.

This commit prevents the memory leak, by cleaning up all data attached to $rootElement after each test.

Fixes #14094

petebacondarwin · 2016-02-21T07:58:26Z

test/ngMock/angular-mocksSpec.js

 });
+
+
+describe('don\'t leak memory between tests', function() {


I prefer describe block descriptions to be nouns of things that are being tested...

But I see you are following the pattern above :-(

I don't like the descriptions either. I'll tweak them.

petebacondarwin · 2016-02-21T08:09:46Z

So the idea of the tests is that you have to run a test beforehand that sets up the spies so that you can check to see that the root element was in fact cleaned up?
I think that the wording of the describe and it blocks could be tweaked so that we remember what they are doing in 12 months time.

petebacondarwin · 2016-02-21T09:58:39Z

test/ngMock/angular-mocksSpec.js

+  });
+});
+
+describe('don\'t leak memory between tests with mocked `$rootScope`', function() {


should this be "with mocked $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`. This commit prevents the memory leak, by cleaning up all data attached to `$rootElement` after each test. Fixes angular#14094

gkalpak · 2016-02-21T21:42:55Z

I has to rebase this on top of master (because the commit this is supposed to fix was reverted).

I incorporated the original commit (attaching $injector to $rootScope) in this PR's 1st commit. (Other than that, I haven't done any other changes in the 1st commit, so you don't have to re-review the other parts.)

I added a 2nd commit, that addressed your feedback (except for #14098 (comment) - I'll play with that next).
I tried to make the test descriptions as clear as possible and add comments explaining parts of the test logic that felt unclear, but I'm open to suggestions for better explanations 😃

- Group into a `describe` block - Tweak test descriptions - Fix typos

gkalpak · 2016-02-21T22:39:33Z

I added a 3rd commit that uses jqLite.cleanData(...).

gkalpak · 2016-02-22T09:36:54Z

And a 4th commit (fixup 3) that fixes a couple of corner cases.
_{(BTW, the commit messages contain more details about the contents of each fixup commit.)}

- Unify the clean-up paths for tests with/without jQuery, using `angular.element.cleanData()`. - Remove redundant calls to `.off().removeData()`. - Fix bug in `testabilityPatch.js`, where `cleanData()` was called with an array of jqLite/jQuery elements (instead of "raw" nodes).

- Reset `originalRootElement` for each test to avoid. - Fix clean-up break, if `$rootElement` was never initialized. - Fix clean-up break, if decorated `$rootElement` was falsy (e.g. `null`). - (Rename `cleanUpElems` to `cleanUpNodes` - since it's "raw" nodes, not jq-wrapped elements.)

petebacondarwin · 2016-02-22T13:48:32Z

LGTM - please squash and merge.

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

gkalpak · 2016-02-22T15:13:52Z

Backported to v1.5.x (75373dd) and v1.4.x (571e323).
_{(For v.1.4.x I had to also backport jqLiteCleanData() from 96288d0.)}

googlebot added the cla: yes label Feb 20, 2016

gkalpak mentioned this pull request Feb 21, 2016

ngMock - injector not attached to $rootElement #14022

Closed

petebacondarwin added this to the 1.5.1 milestone Feb 21, 2016

petebacondarwin added type: bug origin: google component: ngMock severity: memory leak labels Feb 21, 2016

petebacondarwin reviewed Feb 21, 2016
View reviewed changes

gkalpak force-pushed the fix-ngMock-prevent-memory-leak branch from 44684c9 to 393f68b Compare February 21, 2016 20:51

gkalpak force-pushed the fix-ngMock-prevent-memory-leak branch from 393f68b to 739d4c6 Compare February 21, 2016 20:58

gkalpak force-pushed the fix-ngMock-prevent-memory-leak branch 2 times, most recently from fd1891d to a33e753 Compare February 21, 2016 21:46

fixup 1

b972a12

- Group into a `describe` block - Tweak test descriptions - Fix typos

gkalpak force-pushed the fix-ngMock-prevent-memory-leak branch from a33e753 to b972a12 Compare February 21, 2016 21:48

gkalpak force-pushed the fix-ngMock-prevent-memory-leak branch from 9e62160 to d8fca15 Compare February 22, 2016 08:11

fixup 2

0a4fe53

- Unify the clean-up paths for tests with/without jQuery, using `angular.element.cleanData()`. - Remove redundant calls to `.off().removeData()`. - Fix bug in `testabilityPatch.js`, where `cleanData()` was called with an array of jqLite/jQuery elements (instead of "raw" nodes).

gkalpak force-pushed the fix-ngMock-prevent-memory-leak branch from 339468f to 352e2a9 Compare February 22, 2016 10:03

gkalpak force-pushed the fix-ngMock-prevent-memory-leak branch 2 times, most recently from 223fb52 to db92324 Compare February 22, 2016 11:21

petebacondarwin added the needs: merging label Feb 22, 2016

petebacondarwin assigned gkalpak Feb 22, 2016

gkalpak closed this in 85ef70f Feb 22, 2016

gkalpak deleted the fix-ngMock-prevent-memory-leak branch February 22, 2016 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

fix(ngMock): prevent memory leak due to data attached to `$rootElement`#14098
gkalpak wants to merge 4 commits intoangular:masterfrom
gkalpak:fix-ngMock-prevent-memory-leak

gkalpak commented Feb 20, 2016

Uh oh!

petebacondarwin Feb 21, 2016

Uh oh!

gkalpak Feb 21, 2016

Uh oh!

petebacondarwin commented Feb 21, 2016

Uh oh!

petebacondarwin Feb 21, 2016

Uh oh!

gkalpak Feb 21, 2016

Uh oh!

gkalpak commented Feb 21, 2016

Uh oh!

gkalpak commented Feb 21, 2016

Uh oh!

gkalpak commented Feb 22, 2016

Uh oh!

petebacondarwin commented Feb 22, 2016

Uh oh!

gkalpak commented Feb 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		});


		describe('don\'t leak memory between tests', function() {

Conversation

gkalpak commented Feb 20, 2016

Uh oh!

petebacondarwin Feb 21, 2016

Choose a reason for hiding this comment

Uh oh!

gkalpak Feb 21, 2016

Choose a reason for hiding this comment

Uh oh!

petebacondarwin commented Feb 21, 2016

Uh oh!

petebacondarwin Feb 21, 2016

Choose a reason for hiding this comment

Uh oh!

gkalpak Feb 21, 2016

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Feb 21, 2016

Uh oh!

gkalpak commented Feb 21, 2016

Uh oh!

gkalpak commented Feb 22, 2016

Uh oh!

petebacondarwin commented Feb 22, 2016

Uh oh!

gkalpak commented Feb 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants