fix(dirty-checking): fix removal of watch record on disconnected group.

Author: mhevery

lib/change_detection/dirty_checking_change_detector.dart

@@ -68,12 +68,23 @@ class DirtyCheckingChangeDetectorGroup<H> implements ChangeDetectorGroup<H> {
     return tail._recordTail;
   }
 
+  bool get isAttached {
+    DirtyCheckingChangeDetectorGroup current = this;
+    DirtyCheckingChangeDetectorGroup parent;
+    while ((parent = current._parent) != null) {
+      current = parent;
+    }
+    return current is DirtyCheckingChangeDetector
+      ? true
+      : current._prev != null && current._next != null;
+  }
+
 
   DirtyCheckingChangeDetector get _root {
     var root = this;
-    var next;
-    while ((next = root._parent) != null) {
-      root = next;
+    var parent;
+    while ((parent = root._parent) != null) {
+      root = parent;
     }
     return root is DirtyCheckingChangeDetector ? root : null;
   }
@@ -146,16 +157,12 @@ class DirtyCheckingChangeDetectorGroup<H> implements ChangeDetectorGroup<H> {
     assert((root = _root) != null);
     assert(root._assertRecordsOk());
     DirtyCheckingRecord prevRecord = _recordHead._prevRecord;
-    DirtyCheckingRecord nextRecord = _childInclRecordTail._nextRecord;
+    var childInclRecordTail = _childInclRecordTail;
+    DirtyCheckingRecord nextRecord = childInclRecordTail._nextRecord;
 
     if (prevRecord != null) prevRecord._nextRecord = nextRecord;
     if (nextRecord != null) nextRecord._prevRecord = prevRecord;
 
-    var cursor = _recordHead;
-    while (cursor != nextRecord) {
-      cursor = cursor._nextRecord;
-    }
-
     var prevGroup = _prev;
     var nextGroup = _next;
 
@@ -172,8 +179,7 @@ class DirtyCheckingChangeDetectorGroup<H> implements ChangeDetectorGroup<H> {
     _parent = null;
     _prev = _next = null;
     _recordHead._prevRecord = null;
-    _recordTail._nextRecord = null;
-    _recordHead = _recordTail = null;
+    childInclRecordTail._nextRecord = null;
     assert(root._assertRecordsOk());
   }
 
@@ -222,8 +228,8 @@ class DirtyCheckingChangeDetectorGroup<H> implements ChangeDetectorGroup<H> {
       do {
         allRecords.add(record.toString());
         record = record._nextRecord;
-      }
-      while (record != includeChildrenTail);
+      } while (record != includeChildrenTail);
+      allRecords.add(includeChildrenTail);
       lines.add('FIELDS: ${allRecords.join(', ')}');
     }
 
@@ -267,17 +273,42 @@ class DirtyCheckingChangeDetector<H> extends DirtyCheckingChangeDetectorGroup<H>
       }
       var groupRecord = group._recordHead;
       var groupLast = group._recordTail;
-      while (true) {
+      if (record != groupRecord) {
+        throw "Next record is $record expecting $groupRecord";
+      }
+      var done = false;
+      while (!done && groupRecord != null) {
         if (groupRecord == record) {
+          if (record._group != null && record._group != group) {
+            throw "Wrong group: $record "
+                  "Got ${record._group} Expecting: $group";
+          }
           record = record._nextRecord;
         } else {
           throw 'lost: $record found $groupRecord\n$this';
         }
 
-        if (groupRecord == groupLast) break;
+        if (groupRecord._nextRecord != null &&
+            groupRecord._nextRecord._prevRecord != groupRecord) {
+          throw "prev/next pointer missmatch on "
+                "$groupRecord -> ${groupRecord._nextRecord} "
+                "<= ${groupRecord._nextRecord._prevRecord} in $this";
+        }
+        if (groupRecord._prevRecord != null &&
+            groupRecord._prevRecord._nextRecord != groupRecord) {
+              throw "prev/next pointer missmatch on "
+                    "$groupRecord -> ${groupRecord._prevRecord} "
+                    "<= ${groupRecord._prevRecord._nextChange} in $this";
+        }
+        if (groupRecord == groupLast) {
+          done = true;
+        }
         groupRecord = groupRecord._nextRecord;
       }
     }
+    if(record != null) {
+      throw "Extra records at tail: $record on $this";
+    }
     return true;
   }
 

test/change_detection/dirty_checking_change_detector_spec.dart

@@ -4,13 +4,15 @@ import '../_specs.dart';
 import 'package:angular/change_detection/change_detection.dart';
 import 'package:angular/change_detection/dirty_checking_change_detector.dart';
 import 'dart:collection';
+import 'dart:math';
 
 void main() {
   describe('DirtyCheckingChangeDetector', () {
     DirtyCheckingChangeDetector<String> detector;
+    GetterCache getterCache;
 
     beforeEach(() {
-      GetterCache getterCache = new GetterCache({
+      getterCache = new GetterCache({
         "first": (o) => o.first,
         "age": (o) => o.age
       });
@@ -170,6 +172,80 @@ void main() {
         var b = detector.newGroup();
         expect(detector.collectChanges).not.toThrow();
       });
+
+      it('should properly disconnect group in case watch is removed in disconected group', () {
+        var map = {};
+        var detector0 = new DirtyCheckingChangeDetector<String>(getterCache);
+          var detector1 = detector0.newGroup();
+            var detector2 = detector1.newGroup();
+            var watch2 = detector2.watch(map, 'f1', null);
+          var detector3 = detector0.newGroup();
+          detector1.remove();
+            watch2.remove(); // removing a dead record
+          detector3.watch(map, 'f2', null);
+      });
+
+      it('should find random bugs', () {
+        List detectors;
+        List records;
+        List steps;
+        var field = 'someField';
+        step(text) {
+          //print(text);
+          steps.add(text);
+        }
+        Map map = {};
+        var random = new Random();
+        try {
+          for (var i = 0; i < 100000; i++) {
+            if (i % 50 == 0) {
+              //print(steps);
+              //print('===================================');
+              records = [];
+              steps = [];
+              detectors = [new DirtyCheckingChangeDetector<String>(getterCache)];
+            }
+            switch (random.nextInt(4)) {
+              case 0: // new child detector
+                if (detectors.length > 10) break;
+                var index = random.nextInt(detectors.length);
+                ChangeDetectorGroup detector = detectors[index];
+                step('detectors[$index].newGroup()');
+                var child = detector.newGroup();
+                detectors.add(child);
+                break;
+              case 1: // add watch
+                var index = random.nextInt(detectors.length);
+                ChangeDetectorGroup detector = detectors[index];
+                step('detectors[$index].watch(map, field, null)');
+                WatchRecord record = detector.watch(map, field, null);
+                records.add(record);
+                break;
+              case 2: // destroy watch group
+                if (detectors.length == 1) break;
+                var index = random.nextInt(detectors.length - 1) + 1;
+                ChangeDetectorGroup detector = detectors[index];
+                step('detectors[$index].remove()');
+                detector.remove();
+                detectors = detectors
+                    .where((s) => s.isAttached)
+                    .toList();
+                break;
+              case 3: // remove watch on watch group
+                if (records.length == 0) break;
+                var index = random.nextInt(records.length);
+                WatchRecord record = records.removeAt(index);
+                step('records.removeAt($index).remove()');
+                record.remove();
+                break;
+            }
+          }
+        } catch(e) {
+          print(steps);
+          rethrow;
+        }
+      });
+
     });
 
     describe('list watching', () {