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

gkalpak · gkalpak · commit 85ef70f42835 · 2016-02-22T15:57:10.000+02:00
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 angular#14094 Closes angular#14098
diff --git a/src/ngMock/angular-mocks.js b/src/ngMock/angular-mocks.js
@@ -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();
@@ -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;
+  }];
 };
 
 /**
@@ -2577,6 +2579,7 @@ if (window.jasmine || window.mocha) {
 
 
   (window.beforeEach || window.setup)(function() {
+    originalRootElement = null;
     annotatedFunctions = [];
     currentSpec = this;
   });
@@ -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();
     }
 
diff --git a/test/helpers/testabilityPatch.js b/test/helpers/testabilityPatch.js
@@ -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
diff --git a/test/ngMock/angular-mocksSpec.js b/test/ngMock/angular-mocksSpec.js
@@ -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);
+    }));
   });
 
 
@@ -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() {});
+    });
+  });
+});
