Fix for race condition bug in the "traverse the scopes" loop.

[scardine]

Calls to Scope.$watch returns a deregistration function for this listener.

Sometimes a listener may be removed by this function between the call length = watchers.length; on line 517 and watch = watchers[length]; on line 520; in this case, watch will be undefined and the call to watch.get at line 523 will fail.

This is a real bug and I was bitten when writing a huge directive (using d3.js). The case must be obvious enough and the fix too small to bother creating a reproducible test case - reproducing race conditions is hard and testing for the existence of watch before calling watch.get will not hurt, anyway.

[IgorMinar]

the issue is slightly more generic though and we do need tests for it since it should be easy to reproduce.

the problem is that if from within a watch action we register or deregister a watch then the length counter will either miss a watch or go negative and try to dereference watchers[-1] just like in the case you mentioned.

your change fixes the issue of deregistering a watch from a watch action but doesn't address the other case - registering a new watch from a watch action.

can you please add tests for both of these cases and update this PR?

[scardine]

Good catch, Igor. I'm not sure if my javascript is up to the task (pythonist here) but will try my best.

[scardine]

@IgorMinar : I have a better understanding of the issue now. It is not really a race condition, it is the case of a loop over a list that modifies the very list it is iterating over - a classic anti-pattern.

An easy and safe solution would be a clone:

    if ((watchers = current.$$watchers.slice(0)))

I don't know enough of javascript to have a clue, but I guess it could be expensive even for an array of callbacks where the copy is made by reference.

The other solution I came up is changing $watch and its return value (deregistration function) to use entrance/exit queues instead of modifying Scope.$$watchers directly. Looking at the code, I infer assignment is less expensive than attribute look-up, and by the level of nested loops I guess function calls should be expensive as well - I tried to follow the pattern.

          do { // "traverse the scopes" loop
            watchers = current.$$watchers;
            if((exitQueue = current.$$watchers_del)) {
              length = exitQueue.length;
              while (length--) {
                arrayRemove(watchers, exitQueue.shift());
              }
            }
            if((entranceQueue = current.$$watchers_add)) {
              length = entranceQueue.length;
              while (length--) {
                watchers.unshift(watchers, entranceQueue.pop());
              }
            }
            if (watchers) {

This PR was made against master, should I submit other from a branch instead of update this?

BTW, as a bug fix I would go with the smaller patch and work to solve any performance issues later (as Knut used to say, premature optimization is the root of all evil).

[petebacondarwin]

If we copy the array then even if a watch has been unregistered then it will still be called at least one more time, since it is in the copy. Is this what we want?

[petebacondarwin]

Here are two tests that fail:

  it('should allow a watch to be unregistered while in a digest', inject(function($rootScope) {
      var remove1, remove2;
      $rootScope.$watch('remove', function() {
        remove1();
        remove2();
      });
      remove1 = $rootScope.$watch('thing', function() {});
      remove2 = $rootScope.$watch('thing', function() {});
      expect(function() {
        $rootScope.$apply('remove = true');
      }).not.toThrow();
    }));

    it('should allow a watch to be added while in a digest', inject(function($rootScope) {
      var watch1 = jasmine.createSpy('watch1'),
          watch2 = jasmine.createSpy('watch2');
      $rootScope.$watch('foo', function() {
        $rootScope.$watch('foo', watch1);
        $rootScope.$watch('foo', watch1);
      });
      $rootScope.$apply('foo = true');
      expect(watch1).toHaveBeenCalled();
      expect(watch2).toHaveBeenCalled();
    }));

[petebacondarwin]

The second solution also has the problem of still calling a handler after it has been unregistered.

[petebacondarwin]

OK, so the original fix in this PR actually works. Although registering and unregistering watchers from handlers is modifying the array that we are iterating over, the way that digests works means that it doesn't matter as long as we ignore the current item if it has fallen off the end of the array.
There is a chance that a watch expression may be evaluated more than once in a single iteration of the array but that is OK because they should be fast and have no side-effects anyway.
There is a chance that a new watch is missed in a single iteration of the array but that is also OK since it will sort itself out in the next iteration of the outer loop.

[petebacondarwin]

@scardine - thanks for helping out with this. Can you please make sure you have signed the CLA?

[scardine]

@petebacondarwin: Signed, thanks for the test cases.