From d87a4e4af755b22423048412b3ae83d5221954e2 Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Wed, 2 Aug 2017 21:44:35 -0700 Subject: [PATCH 1/2] test($rootScope): test removal of event listeners during event broadcast --- test/ng/rootScopeSpec.js | 84 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/test/ng/rootScopeSpec.js b/test/ng/rootScopeSpec.js index 3f5a444f4a58..dd1a23f20625 100644 --- a/test/ng/rootScopeSpec.js +++ b/test/ng/rootScopeSpec.js @@ -1903,6 +1903,90 @@ describe('Scope', function() { })); + it('should call next listener after removing the current listener via its own handler', inject(function($rootScope) { + var listener1 = jasmine.createSpy('listener1').and.callFake(function() { remove1(); }); + var remove1 = $rootScope.$on('abc', listener1); + + var listener2 = jasmine.createSpy('listener2'); + var remove2 = $rootScope.$on('abc', listener2); + + var listener3 = jasmine.createSpy('listener3'); + var remove3 = $rootScope.$on('abc', listener3); + + $rootScope.$broadcast('abc'); + expect(listener1).toHaveBeenCalled(); + expect(listener2).toHaveBeenCalled(); + expect(listener3).toHaveBeenCalled(); + + listener1.calls.reset(); + listener2.calls.reset(); + listener3.calls.reset(); + + $rootScope.$broadcast('abc'); + expect(listener1).not.toHaveBeenCalled(); + expect(listener2).toHaveBeenCalled(); + expect(listener3).toHaveBeenCalled(); + })); + + + it('should call all subsequent listeners when a previous listener is removed via a handler', inject(function($rootScope) { + var listener1 = jasmine.createSpy(); + var remove1 = $rootScope.$on('abc', listener1); + + var listener2 = jasmine.createSpy().and.callFake(remove1); + var remove2 = $rootScope.$on('abc', listener2); + + var listener3 = jasmine.createSpy(); + var remove3 = $rootScope.$on('abc', listener3); + + $rootScope.$broadcast('abc'); + expect(listener1).toHaveBeenCalled(); + expect(listener2).toHaveBeenCalled(); + expect(listener3).toHaveBeenCalled(); + + listener1.calls.reset(); + listener2.calls.reset(); + listener3.calls.reset(); + + $rootScope.$broadcast('abc'); + expect(listener1).not.toHaveBeenCalled(); + expect(listener2).toHaveBeenCalled(); + expect(listener3).toHaveBeenCalled(); + })); + + + it('should not call listener when removed by previous', inject(function($rootScope) { + var listener1 = jasmine.createSpy('listener1'); + var remove1 = $rootScope.$on('abc', listener1); + + var listener2 = jasmine.createSpy('listener2').and.callFake(function() { remove3(); }); + var remove2 = $rootScope.$on('abc', listener2); + + var listener3 = jasmine.createSpy('listener3'); + var remove3 = $rootScope.$on('abc', listener3); + + var listener4 = jasmine.createSpy('listener4'); + var remove4 = $rootScope.$on('abc', listener4); + + $rootScope.$broadcast('abc'); + expect(listener1).toHaveBeenCalled(); + expect(listener2).toHaveBeenCalled(); + expect(listener3).not.toHaveBeenCalled(); + expect(listener4).toHaveBeenCalled(); + + listener1.calls.reset(); + listener2.calls.reset(); + listener3.calls.reset(); + listener4.calls.reset(); + + $rootScope.$broadcast('abc'); + expect(listener1).toHaveBeenCalled(); + expect(listener2).toHaveBeenCalled(); + expect(listener3).not.toHaveBeenCalled(); + expect(listener4).toHaveBeenCalled(); + })); + + it('should decrement ancestor $$listenerCount entries', inject(function($rootScope) { var child1 = $rootScope.$new(), child2 = child1.$new(), From 2f2fa7a198784374735ca40ae7cc6fdff9216fb7 Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Tue, 8 Aug 2017 23:24:17 -0700 Subject: [PATCH 2/2] fix($rootScope): fix potential memory leak when removing scope listeners When removing listeners they are removed from the array but the array size is not changed until the event is fired again. If the event is never fired but listeners are added/removed then the array will continue growing. By changing the listener removal to `delete` the array entry instead of setting it to `null` browsers can potentially deallocate the memory for the entry. Fixes #16135 Closes #16161 --- src/ng/rootScope.js | 5 ++++- test/ng/rootScopeSpec.js | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/ng/rootScope.js b/src/ng/rootScope.js index 85c04376b2f5..f2fbec1a1c61 100644 --- a/src/ng/rootScope.js +++ b/src/ng/rootScope.js @@ -1207,7 +1207,10 @@ function $RootScopeProvider() { return function() { var indexOfListener = namedListeners.indexOf(listener); if (indexOfListener !== -1) { - namedListeners[indexOfListener] = null; + // Use delete in the hope of the browser deallocating the memory for the array entry, + // while not shifting the array indexes of other listeners. + // See issue https://github.com/angular/angular.js/issues/16135 + delete namedListeners[indexOfListener]; decrementListenerCount(self, 1, name); } }; diff --git a/test/ng/rootScopeSpec.js b/test/ng/rootScopeSpec.js index dd1a23f20625..83005e674cbc 100644 --- a/test/ng/rootScopeSpec.js +++ b/test/ng/rootScopeSpec.js @@ -1903,6 +1903,21 @@ describe('Scope', function() { })); + // See issue https://github.com/angular/angular.js/issues/16135 + it('should deallocate the listener array entry', inject(function($rootScope) { + var remove1 = $rootScope.$on('abc', noop); + $rootScope.$on('abc', noop); + + expect($rootScope.$$listeners['abc'].length).toBe(2); + expect(0 in $rootScope.$$listeners['abc']).toBe(true); + + remove1(); + + expect($rootScope.$$listeners['abc'].length).toBe(2); + expect(0 in $rootScope.$$listeners['abc']).toBe(false); + })); + + it('should call next listener after removing the current listener via its own handler', inject(function($rootScope) { var listener1 = jasmine.createSpy('listener1').and.callFake(function() { remove1(); }); var remove1 = $rootScope.$on('abc', listener1);