fix($rootScope): fix potential memory leak when removing scope listeners (v2)

docs/content/error/$rootScope/inevt.ngdoc

@@ -0,0 +1,22 @@
+@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');
+});
+```

src/ng/rootScope.js

@@ -1181,10 +1181,14 @@ function $RootScopeProvider() {
 
         var self = this;
         return function() {
-          var indexOfListener = namedListeners.indexOf(listener);
-          if (indexOfListener !== -1) {
-            namedListeners[indexOfListener] = null;
+          var index = arrayRemove(namedListeners, listener);
+          if (index >= 0) {
             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--;
+            }
           }
         };
       },
@@ -1213,9 +1217,7 @@ function $RootScopeProvider() {
        * @return {Object} Event object (see {@link ng.$rootScope.Scope#$on}).
        */
       $emit: function(name, args) {
-        var empty = [],
-            namedListeners,
-            scope = this,
+        var scope = this,
             stopPropagation = false,
             event = {
               name: name,
@@ -1226,28 +1228,11 @@ function $RootScopeProvider() {
               },
               defaultPrevented: false
             },
-            listenerArgs = concat([event], arguments, 1),
-            i, length;
+            listenerArgs = concat([event], arguments, 1);
 
         do {
-          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);
-            }
-          }
+          invokeListeners(scope, event, listenerArgs, name);
+
           //if any listener on the current scope stops propagation, prevent bubbling
           if (stopPropagation) {
             event.currentScope = null;
@@ -1299,28 +1284,11 @@ function $RootScopeProvider() {
 
         if (!target.$$listenerCount[name]) return event;
 
-        var listenerArgs = concat([event], arguments, 1),
-            listeners, i, length;
+        var listenerArgs = concat([event], arguments, 1);
 
         //down while you can, then up and next sibling or up and next sibling until back at root
         while ((current = next)) {
-          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);
-            }
-          }
+          invokeListeners(current, event, listenerArgs, name);
 
           // Insanity Warning: scope depth-first traversal
           // yes, this code is a bit crazy, but it works and we have tests to prove it!
@@ -1350,6 +1318,27 @@ 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) {

test/ng/rootScopeSpec.js

@@ -2316,6 +2316,19 @@ 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);
@@ -2448,6 +2461,107 @@ 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();
+          });
+        });
       });
     });
 
@@ -2537,7 +2651,7 @@ describe('Scope', function() {
         expect(spy1).toHaveBeenCalledOnce();
         expect(spy2).toHaveBeenCalledOnce();
         expect(spy3).toHaveBeenCalledOnce();
-        expect(child.$$listeners['evt'].length).toBe(3); // cleanup will happen on next $emit
+        expect(child.$$listeners['evt'].length).toBe(2);
 
         spy1.calls.reset();
         spy2.calls.reset();
@@ -2571,7 +2685,7 @@ describe('Scope', function() {
         expect(spy1).toHaveBeenCalledOnce();
         expect(spy2).toHaveBeenCalledOnce();
         expect(spy3).toHaveBeenCalledOnce();
-        expect(child.$$listeners['evt'].length).toBe(3); //cleanup will happen on next $broadcast
+        expect(child.$$listeners['evt'].length).toBe(2);
 
         spy1.calls.reset();
         spy2.calls.reset();