fix(WatchGroup): don't call reaction functions on removed WatchGroups

Author: mhevery

lib/change_detection/watch_group.dart

@@ -94,7 +94,7 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList {
   /// Pointer for creating tree of [WatchGroup]s.
   WatchGroup _watchGroupHead, _watchGroupTail, _previousWatchGroup,
       _nextWatchGroup;
-  final WatchGroup _parentWatchGroup;
+  WatchGroup _parentWatchGroup;
 
   WatchGroup._child(_parentWatchGroup, this._changeDetector, this.context,
                     this._cache, this._rootGroup)
@@ -115,6 +115,18 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList {
     _evalWatchTail = _evalWatchHead = _marker;
   }
 
+  get isAttached {
+    var group = this;
+    var root = _rootGroup;
+    while(group != null) {
+      if (group == root){
+        return true;
+      }
+      group = group._parentWatchGroup;
+    }
+    return false;
+  }
+
   Watch watch(AST expression, ReactionFn reactionFn) {
     WatchRecord<_Handler> watchRecord =
         _cache.putIfAbsent(expression.expression,
@@ -268,6 +280,8 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList {
 
     _WatchGroupList._remove(_parentWatchGroup, this);
     _changeDetector.remove();
+    _rootGroup._removeCount++;
+    _parentWatchGroup = null;
 
     // Unlink the _watchRecord
     _EvalWatchRecord firstEvalWatch = _evalWatchHead;
@@ -318,6 +332,16 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList {
 class RootWatchGroup extends WatchGroup {
   Watch _dirtyWatchHead, _dirtyWatchTail;
 
+  /**
+   * Every time a [WatchGroup] is destroyed we increment the counter. During
+   * [detectChanges] we reset the count. Before calling the reaction function,
+   * we check [_removeCount] and if it is unchanged we can safely call the
+   * reaction function. If it is changed we only call the reaction function
+   * if the [WatchGroup] is still attached.
+   */
+  int _removeCount = 0;
+
+
   RootWatchGroup(ChangeDetector changeDetector, Object context):
       super._root(changeDetector, context);
 
@@ -369,10 +393,14 @@ class RootWatchGroup extends WatchGroup {
     // We need to call reaction functions asynchronously. This processes the
     // asynchronous reaction function queue.
     Watch dirtyWatch = _dirtyWatchHead;
+    RootWatchGroup root = _rootGroup;
+    root._removeCount = 0;
     while(dirtyWatch != null) {
       count++;
       try {
-        dirtyWatch.invoke();
+        if (root._removeCount == 0 || dirtyWatch._watchGroup.isAttached) {
+          dirtyWatch.invoke();
+        }
       } catch (e, s) {
         if (exceptionHandler == null) rethrow; else exceptionHandler(e, s);
       }
@@ -408,15 +436,15 @@ class Watch {
 
   final Record<_Handler> _record;
   final ReactionFn reactionFn;
+  final WatchGroup _watchGroup;
 
   bool _dirty = false;
   bool _deleted = false;
   Watch _nextDirtyWatch;
 
-  Watch(this._record, this.reactionFn);
+  Watch(this._watchGroup, this._record, this.reactionFn);
 
   get expression => _record.handler.expression;
-
   void invoke() {
     if (_deleted || !_dirty) return;
     _dirty = false;
@@ -468,7 +496,7 @@ abstract class _Handler implements _LinkedList, _LinkedListItem, _WatchList {
   Watch addReactionFn(ReactionFn reactionFn) {
     assert(_next != this); // verify we are not detached
     return watchGrp._rootGroup._addDirtyWatch(_WatchList._add(this,
-        new Watch(watchRecord, reactionFn)));
+        new Watch(watchGrp, watchRecord, reactionFn)));
   }
 
   void addForwardHandler(_Handler forwardToHandler) {

lib/core/scope.dart

@@ -150,7 +150,27 @@ class Scope {
    * The parent [Scope].
    */
   Scope get parentScope => _parentScope;
-  bool get isAttached => _parentScope == null ? false : _parentScope.isAttached;
+
+  /**
+   * Return `true` if the scope has been destroyed. Once scope is destroyed
+   * No operations are allowed on it.
+   */
+  bool get isDestroyed {
+    var scope = this;
+    var root = rootScope;
+    while(scope != null) {
+      if (scope == root) {
+        return false;
+      }
+      scope = scope._parentScope;
+    }
+    return true;
+  }
+
+  /**
+   * Returns true if the scope is still attached to the [RootScope].
+   */
+  bool get isAttached => !isDestroyed;
 
   // TODO(misko): WatchGroup should be private.
   // Instead we should expose performance stats about the watches
@@ -204,6 +224,7 @@ class Scope {
   }
 
   dynamic eval(expression, [Map locals]) {
+    assert(isAttached);
     _assertInternalStateConsistency();
     assert(expression == null ||
            expression is String ||
@@ -236,19 +257,23 @@ class Scope {
   }
 
   ScopeEvent emit(String name, [data]) {
+    assert(isAttached);
     _assertInternalStateConsistency();
     return _Streams.emit(this, name, data);
   }
   ScopeEvent broadcast(String name, [data]) {
+    assert(isAttached);
     _assertInternalStateConsistency();
     return _Streams.broadcast(this, name, data);
   }
   ScopeStream on(String name) {
+    assert(isAttached);
     _assertInternalStateConsistency();
     return _Streams.on(this, rootScope._exceptionHandler, name);
   }
 
   Scope createChild(Object childContext) {
+    assert(isAttached);
     _assertInternalStateConsistency();
     var child = new Scope(childContext, rootScope, this,
                           _readWriteGroup.newGroup(childContext),
@@ -263,6 +288,7 @@ class Scope {
   }
 
   void destroy() {
+    assert(isAttached);
     _assertInternalStateConsistency();
     broadcast(ScopeEvent.DESTROY);
     _Streams.destroy(this);
@@ -359,10 +385,7 @@ class RootScope extends Scope {
             new RootWatchGroup(new DirtyCheckingChangeDetector(cacheGetter), context),
             new RootWatchGroup(new DirtyCheckingChangeDetector(cacheGetter), context))
   {
-    _zone.onTurnDone = () {
-      digest();
-      flush();
-    };
+    _zone.onTurnDone = apply;
   }
 
   RootScope get rootScope => this;

test/change_detection/watch_group_spec.dart

@@ -597,6 +597,28 @@ main() => describe('WatchGroup', () {
     });
 
 
+    it('should not call reaction function on removed group', () {
+      var log = [];
+      context['name'] = 'misko';
+      var child = watchGrp.newGroup(context);
+      watchGrp.watch(parse('name'), (v, _) {
+        log.add('root $v');
+        if (v == 'destroy') {
+          child.remove();
+        }
+      });
+      child.watch(parse('name'), (v, _) => log.add('child $v'));
+      watchGrp.detectChanges();
+      expect(log).toEqual(['root misko', 'child misko']);
+      log.clear();
+
+      context['name'] = 'destroy';
+      watchGrp.detectChanges();
+      expect(log).toEqual(['root destroy']);
+    });
+
+
+
     it('should watch children', () {
       var childContext = new PrototypeMap(context);
       context['a'] = 'OK';

test/core/scope_spec.dart

@@ -556,6 +556,27 @@ main() => describe('scope', () {
       first.destroy();
       expect(log).toEqual(['first', 'first-child']);
     }));
+
+
+    it('should not call reaction function on destroyed scope', inject((RootScope rootScope, Logger log) {
+      rootScope.context['name'] = 'misko';
+      var child = rootScope.createChild(rootScope.context);
+      rootScope.watch('name', (v, _) {
+        log('root $v');
+        if (v == 'destroy') {
+          child.destroy();
+        }
+      });
+      rootScope.watch('name', (v, _) => log('root2 $v'));
+      child.watch('name', (v, _) => log('child $v'));
+      rootScope.apply();
+      expect(log).toEqual(['root misko', 'root2 misko', 'child misko']);
+      log.clear();
+
+      rootScope.context['name'] = 'destroy';
+      rootScope.apply();
+      expect(log).toEqual(['root destroy', 'root2 destroy']);
+    }));
   });
 
 

test/directive/ng_if_spec.dart

@@ -195,4 +195,31 @@ main() {
       expect(logger.result()).toEqual('ALWAYS; JAMES');
     }
   );
+
+  they('should prevent other directives from running when disabled',
+  [
+    '<div><div ng-if="a"><div ng-if="b">content</div></div></div>',
+    '<div><div ng-unless="!a"><div ng-unless="!b">content</div></div></div>'],
+    (html) {
+      compile(html);
+      expect(element.find('span').html()).toEqual('');
+
+      expect(() {
+        rootScope.apply(() {
+          rootScope.context['a'] = true;
+          rootScope.context['b'] = false;
+        });
+      }).not.toThrow();
+      expect(element.find('span').html()).toEqual('');
+
+
+      expect(() {
+        rootScope.apply(() {
+          rootScope.context['a'] = false;
+          rootScope.context['b'] = true;
+        });
+      }).not.toThrow();
+      expect(element.find('span').html()).toEqual('');
+    }
+  );
 }