From 7b5b8d66813f2fa06214c2bb557bcf39b53da6e1 Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Mon, 4 Dec 2017 22:12:53 -0800 Subject: [PATCH 1/3] revert: fix($rootScope): fix potential memory leak when removing scope listeners This reverts commit 817ac56719505680ac4c9997972e8f39eb40a6d0. --- docs/content/error/$rootScope/inevt.ngdoc | 22 ---- src/ng/rootScope.js | 79 ++++++++------- test/ng/rootScopeSpec.js | 118 +--------------------- 3 files changed, 47 insertions(+), 172 deletions(-) delete mode 100644 docs/content/error/$rootScope/inevt.ngdoc diff --git a/docs/content/error/$rootScope/inevt.ngdoc b/docs/content/error/$rootScope/inevt.ngdoc deleted file mode 100644 index a06eeba18627..000000000000 --- a/docs/content/error/$rootScope/inevt.ngdoc +++ /dev/null @@ -1,22 +0,0 @@ -@ngdoc error -@name $rootScope:inevt -@fullName Recursive $emit/$broadcast event -@description - -This error occurs when the an event is `$emit`ed or `$broadcast`ed recursively on a scope. - -For example, when an event listener fires the same event being listened to. - -``` -$scope.$on('foo', function() { - $scope.$emit('foo'); -}); -``` - -Or when a parent element causes indirect recursion. - -``` -$scope.$on('foo', function() { - $rootScope.$broadcast('foo'); -}); -``` \ No newline at end of file diff --git a/src/ng/rootScope.js b/src/ng/rootScope.js index 964ba7bceaf5..07c692e9101b 100644 --- a/src/ng/rootScope.js +++ b/src/ng/rootScope.js @@ -1271,14 +1271,10 @@ function $RootScopeProvider() { var self = this; return function() { - var index = arrayRemove(namedListeners, listener); - if (index >= 0) { + var indexOfListener = namedListeners.indexOf(listener); + if (indexOfListener !== -1) { + namedListeners[indexOfListener] = null; decrementListenerCount(self, 1, name); - // We are removing a listener while iterating over the list of listeners. - // Update the current $$index if necessary to ensure no listener is skipped. - if (index <= namedListeners.$$index) { - namedListeners.$$index--; - } } }; }, @@ -1307,7 +1303,9 @@ function $RootScopeProvider() { * @return {Object} Event object (see {@link ng.$rootScope.Scope#$on}). */ $emit: function(name, args) { - var scope = this, + var empty = [], + namedListeners, + scope = this, stopPropagation = false, event = { name: name, @@ -1318,11 +1316,28 @@ function $RootScopeProvider() { }, defaultPrevented: false }, - listenerArgs = concat([event], arguments, 1); + listenerArgs = concat([event], arguments, 1), + i, length; do { - invokeListeners(scope, event, listenerArgs, name); - + namedListeners = scope.$$listeners[name] || empty; + event.currentScope = scope; + for (i = 0, length = namedListeners.length; i < length; i++) { + + // if listeners were deregistered, defragment the array + if (!namedListeners[i]) { + namedListeners.splice(i, 1); + i--; + length--; + continue; + } + try { + //allow all listeners attached to the current scope to run + namedListeners[i].apply(null, listenerArgs); + } catch (e) { + $exceptionHandler(e); + } + } //if any listener on the current scope stops propagation, prevent bubbling if (stopPropagation) { break; @@ -1373,11 +1388,28 @@ function $RootScopeProvider() { if (!target.$$listenerCount[name]) return event; - var listenerArgs = concat([event], arguments, 1); + var listenerArgs = concat([event], arguments, 1), + listeners, i, length; //down while you can, then up and next sibling or up and next sibling until back at root while ((current = next)) { - invokeListeners(current, event, listenerArgs, name); + event.currentScope = current; + listeners = current.$$listeners[name] || []; + for (i = 0, length = listeners.length; i < length; i++) { + // if listeners were deregistered, defragment the array + if (!listeners[i]) { + listeners.splice(i, 1); + i--; + length--; + continue; + } + + try { + listeners[i].apply(null, listenerArgs); + } catch (e) { + $exceptionHandler(e); + } + } // Insanity Warning: scope depth-first traversal // yes, this code is a bit crazy, but it works and we have tests to prove it! @@ -1408,27 +1440,6 @@ function $RootScopeProvider() { return $rootScope; - function invokeListeners(scope, event, listenerArgs, name) { - var listeners = scope.$$listeners[name]; - if (listeners) { - if (listeners.$$index !== undefined) { - throw $rootScopeMinErr('inevt', '{0} already $emit/$broadcast-ing on scope ({1})', name, scope.$id); - } - event.currentScope = scope; - try { - for (listeners.$$index = 0; listeners.$$index < listeners.length; listeners.$$index++) { - try { - //allow all listeners attached to the current scope to run - listeners[listeners.$$index].apply(null, listenerArgs); - } catch (e) { - $exceptionHandler(e); - } - } - } finally { - listeners.$$index = undefined; - } - } - } function beginPhase(phase) { if ($rootScope.$$phase) { diff --git a/test/ng/rootScopeSpec.js b/test/ng/rootScopeSpec.js index 90a79625e62c..6f683221742e 100644 --- a/test/ng/rootScopeSpec.js +++ b/test/ng/rootScopeSpec.js @@ -2438,19 +2438,6 @@ 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); - - remove1(); - - expect($rootScope.$$listeners['abc'].length).toBe(1); - })); - - 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); @@ -2583,107 +2570,6 @@ describe('Scope', function() { expect($rootScope.$$listenerCount).toEqual({abc: 1}); expect(child.$$listenerCount).toEqual({abc: 1}); })); - - - it('should throw on recursive $broadcast', inject(function($rootScope) { - $rootScope.$on('e', function() { $rootScope.$broadcast('e'); }); - - expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)'); - expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)'); - })); - - - it('should throw on nested recursive $broadcast', inject(function($rootScope) { - $rootScope.$on('e2', function() { $rootScope.$broadcast('e'); }); - $rootScope.$on('e', function() { $rootScope.$broadcast('e2'); }); - - expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)'); - expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)'); - })); - - - it('should throw on recursive $emit', inject(function($rootScope) { - $rootScope.$on('e', function() { $rootScope.$emit('e'); }); - - expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)'); - expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)'); - })); - - - it('should throw on nested recursive $emit', inject(function($rootScope) { - $rootScope.$on('e2', function() { $rootScope.$emit('e'); }); - $rootScope.$on('e', function() { $rootScope.$emit('e2'); }); - - expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)'); - expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)'); - })); - - - it('should throw on recursive $broadcast on child listener', inject(function($rootScope) { - var child = $rootScope.$new(); - child.$on('e', function() { $rootScope.$broadcast('e'); }); - - expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (2)'); - expect(function() { child.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (2)'); - })); - - - it('should throw on nested recursive $broadcast on child listener', inject(function($rootScope) { - var child = $rootScope.$new(); - child.$on('e2', function() { $rootScope.$broadcast('e'); }); - child.$on('e', function() { $rootScope.$broadcast('e2'); }); - - expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (2)'); - expect(function() { child.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (2)'); - })); - - - it('should throw on recursive $emit parent listener', inject(function($rootScope) { - var child = $rootScope.$new(); - $rootScope.$on('e', function() { child.$emit('e'); }); - - expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)'); - expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)'); - expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)'); - })); - - - it('should throw on nested recursive $emit parent listener', inject(function($rootScope) { - var child = $rootScope.$new(); - $rootScope.$on('e2', function() { child.$emit('e'); }); - $rootScope.$on('e', function() { child.$emit('e2'); }); - - expect(function() { child.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)'); - expect(function() { $rootScope.$emit('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)'); - expect(function() { $rootScope.$broadcast('e', 5); }).toThrowMinErr('$rootScope', 'inevt', 'e already $emit/$broadcast-ing on scope (1)'); - })); - - - it('should clear recursive state of $broadcast if $exceptionHandler rethrows', function() { - module(function($exceptionHandlerProvider) { - $exceptionHandlerProvider.mode('rethrow'); - }); - inject(function($rootScope) { - var throwingListener = jasmine.createSpy('thrower').and.callFake(function() { - throw new Error('Listener Error!'); - }); - var secondListener = jasmine.createSpy('second'); - - $rootScope.$on('e', throwingListener); - $rootScope.$on('e', secondListener); - - expect(function() { $rootScope.$broadcast('e'); }).toThrowError('Listener Error!'); - expect(throwingListener).toHaveBeenCalled(); - expect(secondListener).not.toHaveBeenCalled(); - - throwingListener.calls.reset(); - secondListener.calls.reset(); - - expect(function() { $rootScope.$broadcast('e'); }).toThrowError('Listener Error!'); - expect(throwingListener).toHaveBeenCalled(); - expect(secondListener).not.toHaveBeenCalled(); - }); - }); }); }); @@ -2773,7 +2659,7 @@ describe('Scope', function() { expect(spy1).toHaveBeenCalledOnce(); expect(spy2).toHaveBeenCalledOnce(); expect(spy3).toHaveBeenCalledOnce(); - expect(child.$$listeners['evt'].length).toBe(2); + expect(child.$$listeners['evt'].length).toBe(3); // cleanup will happen on next $emit spy1.calls.reset(); spy2.calls.reset(); @@ -2807,7 +2693,7 @@ describe('Scope', function() { expect(spy1).toHaveBeenCalledOnce(); expect(spy2).toHaveBeenCalledOnce(); expect(spy3).toHaveBeenCalledOnce(); - expect(child.$$listeners['evt'].length).toBe(2); + expect(child.$$listeners['evt'].length).toBe(3); //cleanup will happen on next $broadcast spy1.calls.reset(); spy2.calls.reset(); From ffb075f04b715a6a91eb2631071362bd71b001b4 Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Tue, 8 Aug 2017 23:24:17 -0700 Subject: [PATCH 2/3] 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 07c692e9101b..7f37338e6636 100644 --- a/src/ng/rootScope.js +++ b/src/ng/rootScope.js @@ -1273,7 +1273,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 6f683221742e..06a20f6963c5 100644 --- a/test/ng/rootScopeSpec.js +++ b/test/ng/rootScopeSpec.js @@ -2438,6 +2438,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); From 916f3cbc7fcf25410f040d291ca943b53b15554b Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Tue, 5 Dec 2017 00:49:16 -0800 Subject: [PATCH 3/3] test($rootScope): test recursive event broadcast and emit --- test/ng/rootScopeSpec.js | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/test/ng/rootScopeSpec.js b/test/ng/rootScopeSpec.js index 06a20f6963c5..eea9e824ae66 100644 --- a/test/ng/rootScopeSpec.js +++ b/test/ng/rootScopeSpec.js @@ -2925,6 +2925,45 @@ describe('Scope', function() { })); }); }); + + + it('should allow recursive $emit/$broadcast', inject(function($rootScope) { + var callCount = 0; + $rootScope.$on('evt', function($event, arg0) { + callCount++; + if (arg0 !== 1234) { + $rootScope.$emit('evt', 1234); + $rootScope.$broadcast('evt', 1234); + } + }); + + $rootScope.$emit('evt'); + $rootScope.$broadcast('evt'); + expect(callCount).toBe(6); + })); + + + it('should allow recursive $emit/$broadcast between parent/child', inject(function($rootScope) { + var child = $rootScope.$new(); + var calls = ''; + + $rootScope.$on('evt', function($event, arg0) { + calls += 'r'; // For "root". + if (arg0 === 'fromChild') { + $rootScope.$broadcast('evt', 'fromRoot2'); + } + }); + + child.$on('evt', function($event, arg0) { + calls += 'c'; // For "child". + if (arg0 === 'fromRoot1') { + child.$emit('evt', 'fromChild'); + } + }); + + $rootScope.$broadcast('evt', 'fromRoot1'); + expect(calls).toBe('rccrrc'); + })); }); describe('doc examples', function() {