fix(core): Add an error state for ChangeDetectors that is set when bi…

…ndings or lifecycle events throw exceptions and prevents further detection.

- Changes the `alreadyChecked` flag of AbstractChangeDetector to a new `state` flag.
- Changes all checks of alreadyChecked to check that the state is NeverChecked.
- Set state to Errored if an error is thrown during detection.
- Skip change detection for a detector and its children when the state is Errored.
- Add a test to validate this fixes issue angular#4323.

modules/angular2/src/core/change_detection/abstract_change_detector.ts

@@ -13,7 +13,7 @@ import {
 } from './exceptions';
 import {BindingTarget} from './binding_record';
 import {Locals} from './parser/locals';
-import {ChangeDetectionStrategy} from './constants';
+import {ChangeDetectionStrategy, ChangeDetectorState} from './constants';
 import {wtfCreateScope, wtfLeave, WtfScopeFn} from '../profile/profile';
 import {isObservable} from './observable_facade';
 
@@ -33,7 +33,7 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
 
   // The names of the below fields must be kept in sync with codegen_name_util.ts or
   // change detection will fail.
-  alreadyChecked: any = false;
+  state: ChangeDetectorState = ChangeDetectorState.NeverChecked;
   context: T;
   locals: Locals = null;
   mode: ChangeDetectionStrategy = null;
@@ -80,7 +80,7 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
 
   runDetectChanges(throwOnChange: boolean): void {
     if (this.mode === ChangeDetectionStrategy.Detached ||
-        this.mode === ChangeDetectionStrategy.Checked)
+        this.mode === ChangeDetectionStrategy.Checked || this.state === ChangeDetectorState.Errored)
       return;
     var s = _scope_check(this.id, throwOnChange);
 
@@ -95,7 +95,7 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
     if (this.mode === ChangeDetectionStrategy.CheckOnce)
       this.mode = ChangeDetectionStrategy.Checked;
 
-    this.alreadyChecked = true;
+    this.state = ChangeDetectorState.CheckedBefore;
     wtfLeave(s);
   }
 
@@ -112,6 +112,10 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
     try {
       this.detectChangesInRecordsInternal(throwOnChange);
     } catch (e) {
+      // throwOnChange errors aren't counted as fatal errors.
+      if (!(e instanceof ExpressionChangedAfterItHasBeenCheckedException)) {
+        this.state = ChangeDetectorState.Errored;
+      }
       this._throwError(e, e.stack);
     }
   }
@@ -137,7 +141,7 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
     this.locals = locals;
     this.pipes = pipes;
     this.hydrateDirectives(directives);
-    this.alreadyChecked = false;
+    this.state = ChangeDetectorState.NeverChecked;
   }
 
   // Subclasses should override this method to hydrate any directives.

modules/angular2/src/core/change_detection/change_detection_jit_generator.dart

@@ -9,7 +9,7 @@ library change_detection.change_detection_jit_generator;
 class ChangeDetectorJITGenerator {
   String typeName;
   ChangeDetectorJITGenerator(
-      definition, changeDetectionUtilVarName, abstractChangeDetectorVarName) {}
+      definition, changeDetectionUtilVarName, abstractChangeDetectorVarName, changeDetectorStateVarName) {}
 
   generate() {
     throw "Jit Change Detection is not supported in Dart";

modules/angular2/src/core/change_detection/change_detection_jit_generator.ts

@@ -13,7 +13,7 @@ import {codify} from './codegen_facade';
 import {EventBinding} from './event_binding';
 import {BindingTarget} from './binding_record';
 import {ChangeDetectorGenConfig, ChangeDetectorDefinition} from './interfaces';
-import {ChangeDetectionStrategy} from './constants';
+import {ChangeDetectionStrategy, ChangeDetectorState} from './constants';
 import {createPropertyRecords, createEventRecords} from './proto_change_detector';
 
 /**
@@ -41,7 +41,8 @@ export class ChangeDetectorJITGenerator {
   typeName: string;
 
   constructor(definition: ChangeDetectorDefinition, private changeDetectionUtilVarName: string,
-              private abstractChangeDetectorVarName: string) {
+              private abstractChangeDetectorVarName: string,
+              private changeDetectorStateVarName: string) {
     var propertyBindingRecords = createPropertyRecords(definition);
     var eventBindingRecords = createEventRecords(definition);
     var propertyBindingTargets = definition.bindingRecords.map(b => b.target);
@@ -55,8 +56,9 @@ export class ChangeDetectorJITGenerator {
     this.directiveRecords = definition.directiveRecords;
     this._names = new CodegenNameUtil(this.records, this.eventBindings, this.directiveRecords,
                                       this.changeDetectionUtilVarName);
-    this._logic = new CodegenLogicUtil(this._names, this.changeDetectionUtilVarName,
-                                       this.changeDetectionStrategy);
+    this._logic =
+        new CodegenLogicUtil(this._names, this.changeDetectionUtilVarName,
+                             this.changeDetectorStateVarName, this.changeDetectionStrategy);
     this.typeName = sanitizeName(`ChangeDetector_${this.id}`);
   }
 
@@ -68,7 +70,8 @@ export class ChangeDetectorJITGenerator {
       }
     `;
     return new Function(this.abstractChangeDetectorVarName, this.changeDetectionUtilVarName,
-                        factorySource)(AbstractChangeDetector, ChangeDetectionUtil);
+                        this.changeDetectorStateVarName, factorySource)(
+        AbstractChangeDetector, ChangeDetectionUtil, ChangeDetectorState);
   }
 
   generateSource(): string {
@@ -423,7 +426,7 @@ export class ChangeDetectorJITGenerator {
   /** @internal */
   _genOnInit(r: ProtoRecord): string {
     var br = r.bindingRecord;
-    return `if (!throwOnChange && !${this._names.getAlreadyCheckedName()}) ${this._names.getDirectiveName(br.directiveRecord.directiveIndex)}.onInit();`;
+    return `if (!throwOnChange && ${this._names.getStateName()} === ${this.changeDetectorStateVarName}.NeverChecked) ${this._names.getDirectiveName(br.directiveRecord.directiveIndex)}.onInit();`;
   }
 
   /** @internal */

modules/angular2/src/core/change_detection/codegen_logic_util.ts

@@ -12,6 +12,7 @@ import {BaseException} from 'angular2/src/core/facade/exceptions';
  */
 export class CodegenLogicUtil {
   constructor(private _names: CodegenNameUtil, private _utilName: string,
+              private _changeDetectorStateName: string,
               private _changeDetection: ChangeDetectionStrategy) {}
 
   /**
@@ -187,7 +188,7 @@ export class CodegenLogicUtil {
       var dir = directiveRecords[i];
       if (dir.callAfterContentInit) {
         res.push(
-            `if(! ${this._names.getAlreadyCheckedName()}) ${this._names.getDirectiveName(dir.directiveIndex)}.afterContentInit();`);
+            `if(${this._names.getStateName()} === ${this._changeDetectorStateName}.NeverChecked) ${this._names.getDirectiveName(dir.directiveIndex)}.afterContentInit();`);
       }
 
       if (dir.callAfterContentChecked) {
@@ -204,7 +205,7 @@ export class CodegenLogicUtil {
       var dir = directiveRecords[i];
       if (dir.callAfterViewInit) {
         res.push(
-            `if(! ${this._names.getAlreadyCheckedName()}) ${this._names.getDirectiveName(dir.directiveIndex)}.afterViewInit();`);
+            `if(${this._names.getStateName()} === ${this._changeDetectorStateName}.NeverChecked) ${this._names.getDirectiveName(dir.directiveIndex)}.afterViewInit();`);
       }
 
       if (dir.callAfterViewChecked) {

modules/angular2/src/core/change_detection/codegen_name_util.ts

@@ -8,7 +8,8 @@ import {EventBinding} from './event_binding';
 
 // The names of these fields must be kept in sync with abstract_change_detector.ts or change
 // detection will fail.
-const _ALREADY_CHECKED_ACCESSOR = "alreadyChecked";
+const _STATE_ACCESSOR = "state";
+const _CONTEXT_ACCESSOR = "context";
 const _PROP_BINDING_INDEX = "propertyBindingIndex";
 const _DIRECTIVES_ACCESSOR = "directiveIndices";
 const _DISPATCHER_ACCESSOR = "dispatcher";
@@ -77,7 +78,7 @@ export class CodegenNameUtil {
 
   getLocalsAccessorName(): string { return this._addFieldPrefix(_LOCALS_ACCESSOR); }
 
-  getAlreadyCheckedName(): string { return this._addFieldPrefix(_ALREADY_CHECKED_ACCESSOR); }
+  getStateName(): string { return this._addFieldPrefix(_STATE_ACCESSOR); }
 
   getModeName(): string { return this._addFieldPrefix(_MODE_ACCESSOR); }
 

modules/angular2/src/core/change_detection/constants.ts

@@ -1,5 +1,26 @@
 import {StringWrapper, normalizeBool, isBlank} from 'angular2/src/core/facade/lang';
 
+export enum ChangeDetectorState {
+  /**
+   * `NeverChecked` means that the change detector has not been checked yet, and
+   * initialization methods should be called during detection.
+   */
+  NeverChecked,
+
+  /**
+   * `CheckedBefore` means that the change detector has successfully completed at least
+   * one detection previously.
+   */
+  CheckedBefore,
+
+  /**
+   * `Errored` means that the change detector encountered an error checking a binding
+   * or calling a directive lifecycle method and is now in an inconsistent state. Change
+   * detectors in this state will no longer detect changes.
+   */
+  Errored
+}
+
 export enum ChangeDetectionStrategy {
   /**
    * `CheckedOnce` means that after calling detectChanges the mode of the change detector
@@ -51,6 +72,12 @@ export var CHANGE_DETECTION_STRATEGY_VALUES = [
   ChangeDetectionStrategy.OnPushObserve
 ];
 
+export var CHANGE_DETECTOR_STATE_VALUES = [
+  ChangeDetectorState.NeverChecked,
+  ChangeDetectorState.CheckedBefore,
+  ChangeDetectorState.Errored
+];
+
 export function isDefaultChangeDetectionStrategy(
     changeDetectionStrategy: ChangeDetectionStrategy): boolean {
   return isBlank(changeDetectionStrategy) ||

modules/angular2/src/core/change_detection/dynamic_change_detector.ts

@@ -9,7 +9,7 @@ import {DirectiveRecord, DirectiveIndex} from './directive_record';
 import {Locals} from './parser/locals';
 import {ChangeDetectorGenConfig} from './interfaces';
 import {ChangeDetectionUtil, SimpleChange} from './change_detection_util';
-import {ChangeDetectionStrategy} from './constants';
+import {ChangeDetectionStrategy, ChangeDetectorState} from './constants';
 import {ProtoRecord, RecordType} from './proto_record';
 
 export class DynamicChangeDetector extends AbstractChangeDetector<any> {
@@ -134,7 +134,8 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
       if (proto.isLifeCycleRecord()) {
         if (proto.name === "DoCheck" && !throwOnChange) {
           this._getDirectiveFor(directiveRecord.directiveIndex).doCheck();
-        } else if (proto.name === "OnInit" && !throwOnChange && !this.alreadyChecked) {
+        } else if (proto.name === "OnInit" && !throwOnChange &&
+                   this.state == ChangeDetectorState.NeverChecked) {
           this._getDirectiveFor(directiveRecord.directiveIndex).onInit();
         } else if (proto.name === "OnChanges" && isPresent(changes) && !throwOnChange) {
           this._getDirectiveFor(directiveRecord.directiveIndex).onChanges(changes);
@@ -170,7 +171,7 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
     var dirs = this._directiveRecords;
     for (var i = dirs.length - 1; i >= 0; --i) {
       var dir = dirs[i];
-      if (dir.callAfterContentInit && !this.alreadyChecked) {
+      if (dir.callAfterContentInit && this.state == ChangeDetectorState.NeverChecked) {
         this._getDirectiveFor(dir.directiveIndex).afterContentInit();
       }
 
@@ -184,7 +185,7 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
     var dirs = this._directiveRecords;
     for (var i = dirs.length - 1; i >= 0; --i) {
       var dir = dirs[i];
-      if (dir.callAfterViewInit && !this.alreadyChecked) {
+      if (dir.callAfterViewInit && this.state == ChangeDetectorState.NeverChecked) {
         this._getDirectiveFor(dir.directiveIndex).afterViewInit();
       }
       if (dir.callAfterViewChecked) {

modules/angular2/src/core/change_detection/jit_proto_change_detector.ts

@@ -18,6 +18,8 @@ export class JitProtoChangeDetector implements ProtoChangeDetector {
 
   /** @internal */
   _createFactory(definition: ChangeDetectorDefinition) {
-    return new ChangeDetectorJITGenerator(definition, 'util', 'AbstractChangeDetector').generate();
+    return new ChangeDetectorJITGenerator(definition, 'util', 'AbstractChangeDetector',
+                                          'ChangeDetectorStatus')
+        .generate();
   }
 }

modules/angular2/src/core/change_detection/pregen_proto_change_detector.dart

@@ -8,6 +8,8 @@ export 'package:angular2/src/core/change_detection/abstract_change_detector.dart
     show AbstractChangeDetector;
 export 'package:angular2/src/core/change_detection/change_detection.dart'
     show ChangeDetectionStrategy;
+export 'package:angular2/src/core/change_detection/constants.dart'
+    show ChangeDetectorState;
 export 'package:angular2/src/core/change_detection/directive_record.dart'
     show DirectiveIndex, DirectiveRecord;
 export 'package:angular2/src/core/change_detection/interfaces.dart'

modules/angular2/src/core/compiler/change_detector_compiler.ts

@@ -21,13 +21,16 @@ import {Injectable} from 'angular2/src/core/di';
 
 const ABSTRACT_CHANGE_DETECTOR = "AbstractChangeDetector";
 const UTIL = "ChangeDetectionUtil";
+const CHANGE_DETECTOR_STATE = "ChangeDetectorState";
 
 var ABSTRACT_CHANGE_DETECTOR_MODULE = moduleRef(
     `package:angular2/src/core/change_detection/abstract_change_detector${MODULE_SUFFIX}`);
 var UTIL_MODULE =
     moduleRef(`package:angular2/src/core/change_detection/change_detection_util${MODULE_SUFFIX}`);
 var PREGEN_PROTO_CHANGE_DETECTOR_MODULE = moduleRef(
     `package:angular2/src/core/change_detection/pregen_proto_change_detector${MODULE_SUFFIX}`);
+var CONSTANTS_MODULE =
+    moduleRef(`package:angular2/src/core/change_detection/constants${MODULE_SUFFIX}`);
 
 @Injectable()
 export class ChangeDetectionCompiler {
@@ -46,7 +49,9 @@ export class ChangeDetectionCompiler {
       var proto = new DynamicProtoChangeDetector(definition);
       return (dispatcher) => proto.instantiate(dispatcher);
     } else {
-      return new ChangeDetectorJITGenerator(definition, UTIL, ABSTRACT_CHANGE_DETECTOR).generate();
+      return new ChangeDetectorJITGenerator(definition, UTIL, ABSTRACT_CHANGE_DETECTOR,
+                                            CHANGE_DETECTOR_STATE)
+          .generate();
     }
   }
 
@@ -74,7 +79,8 @@ export class ChangeDetectionCompiler {
       } else {
         codegen = new ChangeDetectorJITGenerator(
             definition, `${UTIL_MODULE}${UTIL}`,
-            `${ABSTRACT_CHANGE_DETECTOR_MODULE}${ABSTRACT_CHANGE_DETECTOR}`);
+            `${ABSTRACT_CHANGE_DETECTOR_MODULE}${ABSTRACT_CHANGE_DETECTOR}`,
+            `${CONSTANTS_MODULE}${CHANGE_DETECTOR_STATE}`);
         factories.push(`function(dispatcher) { return new ${codegen.typeName}(dispatcher); }`);
         sourcePart = codegen.generateSource();
       }

modules/angular2/test/core/change_detection/change_detector_spec.ts

@@ -431,10 +431,12 @@ export function main() {
         describe('updating directives', () => {
           var directive1;
           var directive2;
+          var directive3;
 
           beforeEach(() => {
             directive1 = new TestDirective();
             directive2 = new TestDirective();
+            directive3 = new TestDirective(null, null, true);
           });
 
           it('should happen directly, without invoking the dispatcher', () => {
@@ -510,6 +512,30 @@ export function main() {
 
                 expect(directive1.onInitCalled).toBe(false);
               });
+
+              it('should not call onInit again if it throws', () => {
+                var cd = _createWithoutHydrate('directiveOnInit').changeDetector;
+
+                cd.hydrate(_DEFAULT_CONTEXT, null, new FakeDirectives([directive3], []), null);
+                var errored = false;
+                // First pass fails, but onInit should be called.
+                try {
+                  cd.detectChanges();
+                } catch (e) {
+                  errored = true;
+                }
+                expect(errored).toBe(true);
+                expect(directive3.onInitCalled).toBe(true);
+                directive3.onInitCalled = false;
+
+                // Second change detection also fails, but this time onInit should not be called.
+                try {
+                  cd.detectChanges();
+                } catch (e) {
+                  throw new BaseException("Second detectChanges() should not have run detection.");
+                }
+                expect(directive3.onInitCalled).toBe(false);
+              });
             });
 
             describe('afterContentInit', () => {
@@ -1336,13 +1362,19 @@ class TestDirective {
   afterViewCheckedCalled = false;
   event;
 
-  constructor(public afterContentCheckedSpy = null, public afterViewCheckedSpy = null) {}
+  constructor(public afterContentCheckedSpy = null, public afterViewCheckedSpy = null,
+              public throwOnInit = false) {}
 
   onEvent(event) { this.event = event; }
 
   doCheck() { this.doCheckCalled = true; }
 
-  onInit() { this.onInitCalled = true; }
+  onInit() {
+    this.onInitCalled = true;
+    if (this.throwOnInit) {
+      throw "simulated onInit failure";
+    }
+  }
 
   onChanges(changes) {
     var r = {};

modules_dart/transform/lib/src/transform/template_compiler/change_detector_codegen.dart

@@ -121,7 +121,7 @@ class _CodegenState {
 
     var names = new CodegenNameUtil(
         protoRecords, eventBindings, def.directiveRecords, '$genPrefix$_UTIL');
-    var logic = new CodegenLogicUtil(names, '$genPrefix$_UTIL', def.strategy);
+    var logic = new CodegenLogicUtil(names, '$genPrefix$_UTIL', '$genPrefix$_STATE', def.strategy);
     return new _CodegenState._(
         genPrefix,
         def.id,
@@ -503,7 +503,7 @@ class _CodegenState {
 
   String _genOnInit(ProtoRecord r) {
     var br = r.bindingRecord;
-    return 'if (!throwOnChange && !${_names.getAlreadyCheckedName()}) '
+    return 'if (!throwOnChange && ${_names.getStateName()} == ${_genPrefix}$_STATE.NeverChecked) '
         '${_names.getDirectiveName(br.directiveRecord.directiveIndex)}.onInit();';
   }
 
@@ -539,3 +539,4 @@ const _GEN_PROPERTY_BINDING_TARGETS_NAME =
     '${_GEN_PREFIX}_propertyBindingTargets';
 const _GEN_DIRECTIVE_INDICES_NAME = '${_GEN_PREFIX}_directiveIndices';
 const _UTIL = 'ChangeDetectionUtil';
+const _STATE = 'ChangeDetectorState';