fix($rootScope): when adding/removing watchers during $digest

gkalpak · gkalpak · commit a9708de84b50 · 2016-12-09T10:45:32.000+02:00
Previously, adding a watcher during a `$digest` (i.e. from within a watcher), would result in the next watcher getting skipped. Similarly, removing a watcher during a `$digest` could result in the current watcher being run twice (if the removed watcher had not run yet in the current `$digest`). This commit fixes both cases by keeping track of the current watcher index during a digest and properly updating it when adding/removing watchers. Fixes angular#15422 Closes angular#15424
diff --git a/src/ng/rootScope.js b/src/ng/rootScope.js
@@ -413,15 +413,21 @@ function $RootScopeProvider() {
 
         if (!array) {
           array = scope.$$watchers = [];
+          array.$$digestWatchIndex = -1;
         }
         // we use unshift since we use a while loop in $digest for speed.
         // the while loop reads in reverse order.
         array.unshift(watcher);
+        array.$$digestWatchIndex++;
         incrementWatchersCount(this, 1);
 
         return function deregisterWatch() {
-          if (arrayRemove(array, watcher) >= 0) {
+          var index = arrayRemove(array, watcher);
+          if (index >= 0) {
             incrementWatchersCount(scope, -1);
+            if (index < array.$$digestWatchIndex) {
+              array.$$digestWatchIndex--;
+            }
           }
           lastDirtyWatch = null;
         };
@@ -754,7 +760,6 @@ function $RootScopeProvider() {
       $digest: function() {
         var watch, value, last, fn, get,
             watchers,
-            length,
             dirty, ttl = TTL,
             next, current, target = this,
             watchLog = [],
@@ -795,10 +800,10 @@ function $RootScopeProvider() {
           do { // "traverse the scopes" loop
             if ((watchers = current.$$watchers)) {
               // process our watches
-              length = watchers.length;
-              while (length--) {
+              watchers.$$digestWatchIndex = watchers.length;
+              while (watchers.$$digestWatchIndex--) {
                 try {
-                  watch = watchers[length];
+                  watch = watchers[watchers.$$digestWatchIndex];
                   // Most common watches are on primitives, in which case we can short
                   // circuit it with === operator, only when === fails do we use .equals
                   if (watch) {
diff --git a/test/ng/rootScopeSpec.js b/test/ng/rootScopeSpec.js
@@ -498,6 +498,78 @@ describe('Scope', function() {
       expect(watch2).toHaveBeenCalled();
     }));
 
+
+    it('should not skip watchers when adding new watchers during digest',
+      inject(function($rootScope) {
+        var log = [];
+
+        var watchFn1 = function() { log.push(1); };
+        var watchFn2 = function() { log.push(2); };
+        var watchFn3 = function() { log.push(3); };
+        var addWatcherOnce = function(newValue, oldValue) {
+          if (newValue === oldValue) {
+            $rootScope.$watch(watchFn3);
+          }
+        };
+
+        $rootScope.$watch(watchFn1, addWatcherOnce);
+        $rootScope.$watch(watchFn2);
+
+        $rootScope.$digest();
+
+        expect(log).toEqual([1, 2, 3, 1, 2, 3]);
+      })
+    );
+
+
+    it('should not run the current watcher twice when removing a watcher during digest',
+      inject(function($rootScope) {
+        var log = [];
+        var removeWatcher3;
+
+        var watchFn3 = function() { log.push(3); };
+        var watchFn2 = function() { log.push(2); };
+        var watchFn1 = function() { log.push(1); };
+        var removeWatcherOnce = function(newValue, oldValue) {
+          if (newValue === oldValue) {
+            removeWatcher3();
+          }
+        };
+
+        $rootScope.$watch(watchFn1, removeWatcherOnce);
+        $rootScope.$watch(watchFn2);
+        removeWatcher3 = $rootScope.$watch(watchFn3);
+
+        $rootScope.$digest();
+
+        expect(log).toEqual([1, 2, 1, 2]);
+      })
+    );
+
+
+    it('should not skip watchers when removing itself during digest',
+      inject(function($rootScope) {
+        var log = [];
+        var removeWatcher1;
+
+        var watchFn3 = function() { log.push(3); };
+        var watchFn2 = function() { log.push(2); };
+        var watchFn1 = function() { log.push(1); };
+        var removeItself = function() {
+          removeWatcher1();
+        };
+
+        removeWatcher1 = $rootScope.$watch(watchFn1, removeItself);
+        $rootScope.$watch(watchFn2);
+        $rootScope.$watch(watchFn3);
+
+        $rootScope.$digest();
+
+        expect(log).toEqual([1, 2, 3, 2, 3]);
+      })
+    );
+
+
     it('should not infinitely digest when current value is NaN', inject(function($rootScope) {
       $rootScope.$watch(function() { return NaN;});
 
@@ -598,7 +670,7 @@ describe('Scope', function() {
 
         $rootScope.$digest();
 
-        expect(log).toEqual(['watch1', 'watchAction1', 'watch1', 'watch3', 'watchAction3',
+        expect(log).toEqual(['watch1', 'watchAction1', 'watch3', 'watchAction3',
                              'watch1', 'watch3']);
         scope.$destroy();
         log.reset();
@@ -895,8 +967,7 @@ describe('Scope', function() {
           $rootScope.$watch(log.fn('w5'), log.fn('w5action'));
         });
         $rootScope.$digest();
-        expect(log).toEqual(['w1', 'w2', 'w3', 'w4', 'w4action',
-                             'w1', 'w2', 'w3', 'w4', 'w5', 'w5action',
+        expect(log).toEqual(['w1', 'w2', 'w3', 'w4', 'w4action', 'w5', 'w5action',
                              'w1', 'w2', 'w3', 'w4', 'w5']);
       }));
 
