From d12578cf6a675e54424535d228b861af112f2c18 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Fri, 27 Jul 2018 17:26:04 +0300 Subject: [PATCH 1/2] fix($animate): avoid memory leak with `$animate.enabled(element, enabled)` When disabling/enabling animations on a specific element (via `$animate.enabled(element, enabled)`), the element is added in a map to track its state. Previously, the element was never removed from the map, causing AngularJS to hold on to the element even after it is removed from the DOM, thus preventing it from being garbage collected. This commit fixes it by removing the element from the map on `$destroy`. Fixes #16637. --- src/apis.js | 12 ++++++++---- src/ngAnimate/animateQueue.js | 9 +++++++++ test/ApiSpecs.js | 28 ++++++++++++++++++++++++++++ test/ngAnimate/animateSpec.js | 35 +++++++++++++++++++++++++++++++++++ 4 files changed, 80 insertions(+), 4 deletions(-) diff --git a/src/apis.js b/src/apis.js index 856b86e8b709..1767b9134f4b 100644 --- a/src/apis.js +++ b/src/apis.js @@ -45,11 +45,10 @@ function NgMapShim() { } NgMapShim.prototype = { _idx: function(key) { - if (key === this._lastKey) { - return this._lastIndex; + if (key !== this._lastKey) { + this._lastKey = key; + this._lastIndex = this._keys.indexOf(key); } - this._lastKey = key; - this._lastIndex = this._keys.indexOf(key); return this._lastIndex; }, _transformKey: function(key) { @@ -62,6 +61,11 @@ NgMapShim.prototype = { return this._values[idx]; } }, + has: function(key) { + key = this._transformKey(key); + var idx = this._idx(key); + return idx !== -1; + }, set: function(key, value) { key = this._transformKey(key); var idx = this._idx(key); diff --git a/src/ngAnimate/animateQueue.js b/src/ngAnimate/animateQueue.js index 9183abc11ef5..6cff2ac2343c 100644 --- a/src/ngAnimate/animateQueue.js +++ b/src/ngAnimate/animateQueue.js @@ -120,6 +120,10 @@ var $$AnimateQueueProvider = ['$animateProvider', /** @this */ function($animate var disabledElementsLookup = new $$Map(); var animationsEnabled = null; + function removeFromDisabledElementsLookup(evt) { + disabledElementsLookup.delete(evt.target); + } + function postDigestTaskFactory() { var postDigestCalled = false; return function(fn) { @@ -303,6 +307,11 @@ var $$AnimateQueueProvider = ['$animateProvider', /** @this */ function($animate bool = !disabledElementsLookup.get(node); } else { // (element, bool) - Element setter + if (!disabledElementsLookup.has(node)) { + // The element is added to the map for the first time. + // Create a listener to remove it on `$destroy` (to avoid memory leak). + $$jqLite(element).on('$destroy', removeFromDisabledElementsLookup); + } disabledElementsLookup.set(node, !bool); } } diff --git a/test/ApiSpecs.js b/test/ApiSpecs.js index bc87caa07105..79a4d850c762 100644 --- a/test/ApiSpecs.js +++ b/test/ApiSpecs.js @@ -89,6 +89,34 @@ describe('api', function() { expect(map.get(keys[2])).toBe(values[2]); }); + it('should return if a key exists or not', function() { + var map = new NgMapShim(); + var keys = ['foo', {}]; + + expect(map.has(keys[0])).toBe(false); + expect(map.has(keys[1])).toBe(false); + + map.set(keys[0], 'bar'); + expect(map.has(keys[0])).toBe(true); + expect(map.has(keys[1])).toBe(false); + + map.set(keys[1], 'baz'); + expect(map.has(keys[0])).toBe(true); + expect(map.has(keys[1])).toBe(true); + + map.delete(keys[0]); + expect(map.has(keys[0])).toBe(false); + expect(map.has(keys[1])).toBe(true); + + map.delete(keys[1]); + expect(map.has(keys[0])).toBe(false); + expect(map.has(keys[1])).toBe(false); + + map.set(keys[1], 'qux'); + expect(map.has(keys[0])).toBe(false); + expect(map.has(keys[1])).toBe(true); + }); + it('should be able to deal with `NaN` keys', function() { var map = new NgMapShim(); diff --git a/test/ngAnimate/animateSpec.js b/test/ngAnimate/animateSpec.js index c8d70b5a32fe..b584d0a1c8b1 100644 --- a/test/ngAnimate/animateSpec.js +++ b/test/ngAnimate/animateSpec.js @@ -577,6 +577,41 @@ describe('animations', function() { $rootScope.$digest(); expect(capturedAnimation).toBeTruthy(); })); + + it('should remove the element from the `disabledElementsLookup` map on `$destroy`', + inject(function($$Map, $animate, $rootScope) { + + var setSpy = spyOn($$Map.prototype, 'set').and.callThrough(); + var deleteSpy = spyOn($$Map.prototype, 'delete').and.callThrough(); + + parent.append(element); + + $animate.enabled(element, false); + $animate.enabled(element, true); + $animate.enabled(element, false); + expect(setSpy).toHaveBeenCalledWith(element[0], jasmine.any(Boolean)); + expect(deleteSpy).not.toHaveBeenCalledWith(element[0]); + expect($animate.enabled(element)).toBe(false); + + // No clean-up on `detach` (no `$destroy` event). + element.detach(); + expect(deleteSpy).not.toHaveBeenCalledWith(element[0]); + expect($animate.enabled(element)).toBe(false); + + // Clean-up on `remove` (causes `$destroy` event). + element.remove(); + expect(deleteSpy).toHaveBeenCalledOnceWith(element[0]); + expect($animate.enabled(element)).toBe(true); + + deleteSpy.calls.reset(); + + element.triggerHandler('$destroy'); + expect(deleteSpy).not.toHaveBeenCalledWith(element[0]); + + $animate.enabled(element, true); + element.triggerHandler('$destroy'); + expect(deleteSpy).toHaveBeenCalledOnceWith(element[0]); + })); }); it('should strip all comment nodes from the animation and not issue an animation if not real elements are found', From d13fbf36872c08b84d24eea1a4101f0c9f745f8f Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Fri, 27 Jul 2018 20:00:47 +0300 Subject: [PATCH 2/2] fixup! fix($animate): avoid memory leak with `$animate.enabled(element, enabled)` --- src/ngAnimate/animateQueue.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ngAnimate/animateQueue.js b/src/ngAnimate/animateQueue.js index 6cff2ac2343c..f8f9e4b9389d 100644 --- a/src/ngAnimate/animateQueue.js +++ b/src/ngAnimate/animateQueue.js @@ -310,7 +310,7 @@ var $$AnimateQueueProvider = ['$animateProvider', /** @this */ function($animate if (!disabledElementsLookup.has(node)) { // The element is added to the map for the first time. // Create a listener to remove it on `$destroy` (to avoid memory leak). - $$jqLite(element).on('$destroy', removeFromDisabledElementsLookup); + jqLite(element).on('$destroy', removeFromDisabledElementsLookup); } disabledElementsLookup.set(node, !bool); }