Skip to content

Commit d1b54d6

Browse files
alxhubalxhub
committed
fix(core): Add an error state for ChangeDetectors that is set when bindings 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 #4323. Closes #4953
1 parent c930a53 commit d1b54d6

12 files changed

Lines changed: 110 additions & 27 deletions

File tree

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

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import {
1313
} from './exceptions';
1414
import {BindingTarget} from './binding_record';
1515
import {Locals} from './parser/locals';
16-
import {ChangeDetectionStrategy} from './constants';
16+
import {ChangeDetectionStrategy, ChangeDetectorState} from './constants';
1717
import {wtfCreateScope, wtfLeave, WtfScopeFn} from '../profile/profile';
1818
import {isObservable} from './observable_facade';
1919

@@ -33,7 +33,7 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
3333

3434
// The names of the below fields must be kept in sync with codegen_name_util.ts or
3535
// change detection will fail.
36-
alreadyChecked: any = false;
36+
state: ChangeDetectorState = ChangeDetectorState.NeverChecked;
3737
context: T;
3838
locals: Locals = null;
3939
mode: ChangeDetectionStrategy = null;
@@ -80,7 +80,7 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
8080

8181
runDetectChanges(throwOnChange: boolean): void {
8282
if (this.mode === ChangeDetectionStrategy.Detached ||
83-
this.mode === ChangeDetectionStrategy.Checked)
83+
this.mode === ChangeDetectionStrategy.Checked || this.state === ChangeDetectorState.Errored)
8484
return;
8585
var s = _scope_check(this.id, throwOnChange);
8686

@@ -95,7 +95,7 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
9595
if (this.mode === ChangeDetectionStrategy.CheckOnce)
9696
this.mode = ChangeDetectionStrategy.Checked;
9797

98-
this.alreadyChecked = true;
98+
this.state = ChangeDetectorState.CheckedBefore;
9999
wtfLeave(s);
100100
}
101101

@@ -112,6 +112,10 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
112112
try {
113113
this.detectChangesInRecordsInternal(throwOnChange);
114114
} catch (e) {
115+
// throwOnChange errors aren't counted as fatal errors.
116+
if (!(e instanceof ExpressionChangedAfterItHasBeenCheckedException)) {
117+
this.state = ChangeDetectorState.Errored;
118+
}
115119
this._throwError(e, e.stack);
116120
}
117121
}
@@ -137,7 +141,7 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
137141
this.locals = locals;
138142
this.pipes = pipes;
139143
this.hydrateDirectives(directives);
140-
this.alreadyChecked = false;
144+
this.state = ChangeDetectorState.NeverChecked;
141145
}
142146

143147
// Subclasses should override this method to hydrate any directives.

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ library change_detection.change_detection_jit_generator;
99
class ChangeDetectorJITGenerator {
1010
String typeName;
1111
ChangeDetectorJITGenerator(
12-
definition, changeDetectionUtilVarName, abstractChangeDetectorVarName) {}
12+
definition, changeDetectionUtilVarName, abstractChangeDetectorVarName, changeDetectorStateVarName) {}
1313

1414
generate() {
1515
throw "Jit Change Detection is not supported in Dart";

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

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import {codify} from './codegen_facade';
1313
import {EventBinding} from './event_binding';
1414
import {BindingTarget} from './binding_record';
1515
import {ChangeDetectorGenConfig, ChangeDetectorDefinition} from './interfaces';
16-
import {ChangeDetectionStrategy} from './constants';
16+
import {ChangeDetectionStrategy, ChangeDetectorState} from './constants';
1717
import {createPropertyRecords, createEventRecords} from './proto_change_detector';
1818

1919
/**
@@ -41,7 +41,8 @@ export class ChangeDetectorJITGenerator {
4141
typeName: string;
4242

4343
constructor(definition: ChangeDetectorDefinition, private changeDetectionUtilVarName: string,
44-
private abstractChangeDetectorVarName: string) {
44+
private abstractChangeDetectorVarName: string,
45+
private changeDetectorStateVarName: string) {
4546
var propertyBindingRecords = createPropertyRecords(definition);
4647
var eventBindingRecords = createEventRecords(definition);
4748
var propertyBindingTargets = definition.bindingRecords.map(b => b.target);
@@ -55,8 +56,9 @@ export class ChangeDetectorJITGenerator {
5556
this.directiveRecords = definition.directiveRecords;
5657
this._names = new CodegenNameUtil(this.records, this.eventBindings, this.directiveRecords,
5758
this.changeDetectionUtilVarName);
58-
this._logic = new CodegenLogicUtil(this._names, this.changeDetectionUtilVarName,
59-
this.changeDetectionStrategy);
59+
this._logic =
60+
new CodegenLogicUtil(this._names, this.changeDetectionUtilVarName,
61+
this.changeDetectorStateVarName, this.changeDetectionStrategy);
6062
this.typeName = sanitizeName(`ChangeDetector_${this.id}`);
6163
}
6264

@@ -68,7 +70,8 @@ export class ChangeDetectorJITGenerator {
6870
}
6971
`;
7072
return new Function(this.abstractChangeDetectorVarName, this.changeDetectionUtilVarName,
71-
factorySource)(AbstractChangeDetector, ChangeDetectionUtil);
73+
this.changeDetectorStateVarName, factorySource)(
74+
AbstractChangeDetector, ChangeDetectionUtil, ChangeDetectorState);
7275
}
7376

7477
generateSource(): string {
@@ -423,7 +426,7 @@ export class ChangeDetectorJITGenerator {
423426
/** @internal */
424427
_genOnInit(r: ProtoRecord): string {
425428
var br = r.bindingRecord;
426-
return `if (!throwOnChange && !${this._names.getAlreadyCheckedName()}) ${this._names.getDirectiveName(br.directiveRecord.directiveIndex)}.onInit();`;
429+
return `if (!throwOnChange && ${this._names.getStateName()} === ${this.changeDetectorStateVarName}.NeverChecked) ${this._names.getDirectiveName(br.directiveRecord.directiveIndex)}.onInit();`;
427430
}
428431

429432
/** @internal */

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,14 @@ import {BindingTarget} from './binding_record';
66
import {DirectiveRecord} from './directive_record';
77
import {ChangeDetectionStrategy} from './constants';
88
import {BaseException} from 'angular2/src/core/facade/exceptions';
9+
import {IS_DART} from 'angular2/src/core/compiler/util';
910

1011
/**
1112
* Class responsible for providing change detection logic for change detector classes.
1213
*/
1314
export class CodegenLogicUtil {
1415
constructor(private _names: CodegenNameUtil, private _utilName: string,
16+
private _changeDetectorStateName: string,
1517
private _changeDetection: ChangeDetectionStrategy) {}
1618

1719
/**
@@ -182,12 +184,13 @@ export class CodegenLogicUtil {
182184

183185
genContentLifecycleCallbacks(directiveRecords: DirectiveRecord[]): string[] {
184186
var res = [];
187+
var eq = IS_DART ? '==' : '===';
185188
// NOTE(kegluneq): Order is important!
186189
for (var i = directiveRecords.length - 1; i >= 0; --i) {
187190
var dir = directiveRecords[i];
188191
if (dir.callAfterContentInit) {
189192
res.push(
190-
`if(! ${this._names.getAlreadyCheckedName()}) ${this._names.getDirectiveName(dir.directiveIndex)}.afterContentInit();`);
193+
`if(${this._names.getStateName()} ${eq} ${this._changeDetectorStateName}.NeverChecked) ${this._names.getDirectiveName(dir.directiveIndex)}.afterContentInit();`);
191194
}
192195

193196
if (dir.callAfterContentChecked) {
@@ -199,12 +202,13 @@ export class CodegenLogicUtil {
199202

200203
genViewLifecycleCallbacks(directiveRecords: DirectiveRecord[]): string[] {
201204
var res = [];
205+
var eq = IS_DART ? '==' : '===';
202206
// NOTE(kegluneq): Order is important!
203207
for (var i = directiveRecords.length - 1; i >= 0; --i) {
204208
var dir = directiveRecords[i];
205209
if (dir.callAfterViewInit) {
206210
res.push(
207-
`if(! ${this._names.getAlreadyCheckedName()}) ${this._names.getDirectiveName(dir.directiveIndex)}.afterViewInit();`);
211+
`if(${this._names.getStateName()} ${eq} ${this._changeDetectorStateName}.NeverChecked) ${this._names.getDirectiveName(dir.directiveIndex)}.afterViewInit();`);
208212
}
209213

210214
if (dir.callAfterViewChecked) {

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@ import {EventBinding} from './event_binding';
88

99
// The names of these fields must be kept in sync with abstract_change_detector.ts or change
1010
// detection will fail.
11-
const _ALREADY_CHECKED_ACCESSOR = "alreadyChecked";
11+
const _STATE_ACCESSOR = "state";
12+
const _CONTEXT_ACCESSOR = "context";
1213
const _PROP_BINDING_INDEX = "propertyBindingIndex";
1314
const _DIRECTIVES_ACCESSOR = "directiveIndices";
1415
const _DISPATCHER_ACCESSOR = "dispatcher";
@@ -77,7 +78,7 @@ export class CodegenNameUtil {
7778

7879
getLocalsAccessorName(): string { return this._addFieldPrefix(_LOCALS_ACCESSOR); }
7980

80-
getAlreadyCheckedName(): string { return this._addFieldPrefix(_ALREADY_CHECKED_ACCESSOR); }
81+
getStateName(): string { return this._addFieldPrefix(_STATE_ACCESSOR); }
8182

8283
getModeName(): string { return this._addFieldPrefix(_MODE_ACCESSOR); }
8384

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,26 @@
11
import {StringWrapper, normalizeBool, isBlank} from 'angular2/src/core/facade/lang';
22

3+
export enum ChangeDetectorState {
4+
/**
5+
* `NeverChecked` means that the change detector has not been checked yet, and
6+
* initialization methods should be called during detection.
7+
*/
8+
NeverChecked,
9+
10+
/**
11+
* `CheckedBefore` means that the change detector has successfully completed at least
12+
* one detection previously.
13+
*/
14+
CheckedBefore,
15+
16+
/**
17+
* `Errored` means that the change detector encountered an error checking a binding
18+
* or calling a directive lifecycle method and is now in an inconsistent state. Change
19+
* detectors in this state will no longer detect changes.
20+
*/
21+
Errored
22+
}
23+
324
export enum ChangeDetectionStrategy {
425
/**
526
* `CheckedOnce` means that after calling detectChanges the mode of the change detector
@@ -51,6 +72,12 @@ export var CHANGE_DETECTION_STRATEGY_VALUES = [
5172
ChangeDetectionStrategy.OnPushObserve
5273
];
5374

75+
export var CHANGE_DETECTOR_STATE_VALUES = [
76+
ChangeDetectorState.NeverChecked,
77+
ChangeDetectorState.CheckedBefore,
78+
ChangeDetectorState.Errored
79+
];
80+
5481
export function isDefaultChangeDetectionStrategy(
5582
changeDetectionStrategy: ChangeDetectionStrategy): boolean {
5683
return isBlank(changeDetectionStrategy) ||

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import {DirectiveRecord, DirectiveIndex} from './directive_record';
99
import {Locals} from './parser/locals';
1010
import {ChangeDetectorGenConfig} from './interfaces';
1111
import {ChangeDetectionUtil, SimpleChange} from './change_detection_util';
12-
import {ChangeDetectionStrategy} from './constants';
12+
import {ChangeDetectionStrategy, ChangeDetectorState} from './constants';
1313
import {ProtoRecord, RecordType} from './proto_record';
1414

1515
export class DynamicChangeDetector extends AbstractChangeDetector<any> {
@@ -134,7 +134,8 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
134134
if (proto.isLifeCycleRecord()) {
135135
if (proto.name === "DoCheck" && !throwOnChange) {
136136
this._getDirectiveFor(directiveRecord.directiveIndex).doCheck();
137-
} else if (proto.name === "OnInit" && !throwOnChange && !this.alreadyChecked) {
137+
} else if (proto.name === "OnInit" && !throwOnChange &&
138+
this.state == ChangeDetectorState.NeverChecked) {
138139
this._getDirectiveFor(directiveRecord.directiveIndex).onInit();
139140
} else if (proto.name === "OnChanges" && isPresent(changes) && !throwOnChange) {
140141
this._getDirectiveFor(directiveRecord.directiveIndex).onChanges(changes);
@@ -170,7 +171,7 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
170171
var dirs = this._directiveRecords;
171172
for (var i = dirs.length - 1; i >= 0; --i) {
172173
var dir = dirs[i];
173-
if (dir.callAfterContentInit && !this.alreadyChecked) {
174+
if (dir.callAfterContentInit && this.state == ChangeDetectorState.NeverChecked) {
174175
this._getDirectiveFor(dir.directiveIndex).afterContentInit();
175176
}
176177

@@ -184,7 +185,7 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
184185
var dirs = this._directiveRecords;
185186
for (var i = dirs.length - 1; i >= 0; --i) {
186187
var dir = dirs[i];
187-
if (dir.callAfterViewInit && !this.alreadyChecked) {
188+
if (dir.callAfterViewInit && this.state == ChangeDetectorState.NeverChecked) {
188189
this._getDirectiveFor(dir.directiveIndex).afterViewInit();
189190
}
190191
if (dir.callAfterViewChecked) {

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ export class JitProtoChangeDetector implements ProtoChangeDetector {
1818

1919
/** @internal */
2020
_createFactory(definition: ChangeDetectorDefinition) {
21-
return new ChangeDetectorJITGenerator(definition, 'util', 'AbstractChangeDetector').generate();
21+
return new ChangeDetectorJITGenerator(definition, 'util', 'AbstractChangeDetector',
22+
'ChangeDetectorStatus')
23+
.generate();
2224
}
2325
}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ export 'package:angular2/src/core/change_detection/abstract_change_detector.dart
88
show AbstractChangeDetector;
99
export 'package:angular2/src/core/change_detection/change_detection.dart'
1010
show ChangeDetectionStrategy;
11+
export 'package:angular2/src/core/change_detection/constants.dart'
12+
show ChangeDetectorState;
1113
export 'package:angular2/src/core/change_detection/directive_record.dart'
1214
show DirectiveIndex, DirectiveRecord;
1315
export 'package:angular2/src/core/change_detection/interfaces.dart'

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,16 @@ import {Injectable} from 'angular2/src/core/di';
2121

2222
const ABSTRACT_CHANGE_DETECTOR = "AbstractChangeDetector";
2323
const UTIL = "ChangeDetectionUtil";
24+
const CHANGE_DETECTOR_STATE = "ChangeDetectorState";
2425

2526
var ABSTRACT_CHANGE_DETECTOR_MODULE = moduleRef(
2627
`package:angular2/src/core/change_detection/abstract_change_detector${MODULE_SUFFIX}`);
2728
var UTIL_MODULE =
2829
moduleRef(`package:angular2/src/core/change_detection/change_detection_util${MODULE_SUFFIX}`);
2930
var PREGEN_PROTO_CHANGE_DETECTOR_MODULE = moduleRef(
3031
`package:angular2/src/core/change_detection/pregen_proto_change_detector${MODULE_SUFFIX}`);
32+
var CONSTANTS_MODULE =
33+
moduleRef(`package:angular2/src/core/change_detection/constants${MODULE_SUFFIX}`);
3134

3235
@Injectable()
3336
export class ChangeDetectionCompiler {
@@ -46,7 +49,9 @@ export class ChangeDetectionCompiler {
4649
var proto = new DynamicProtoChangeDetector(definition);
4750
return (dispatcher) => proto.instantiate(dispatcher);
4851
} else {
49-
return new ChangeDetectorJITGenerator(definition, UTIL, ABSTRACT_CHANGE_DETECTOR).generate();
52+
return new ChangeDetectorJITGenerator(definition, UTIL, ABSTRACT_CHANGE_DETECTOR,
53+
CHANGE_DETECTOR_STATE)
54+
.generate();
5055
}
5156
}
5257

@@ -74,7 +79,8 @@ export class ChangeDetectionCompiler {
7479
} else {
7580
codegen = new ChangeDetectorJITGenerator(
7681
definition, `${UTIL_MODULE}${UTIL}`,
77-
`${ABSTRACT_CHANGE_DETECTOR_MODULE}${ABSTRACT_CHANGE_DETECTOR}`);
82+
`${ABSTRACT_CHANGE_DETECTOR_MODULE}${ABSTRACT_CHANGE_DETECTOR}`,
83+
`${CONSTANTS_MODULE}${CHANGE_DETECTOR_STATE}`);
7884
factories.push(`function(dispatcher) { return new ${codegen.typeName}(dispatcher); }`);
7985
sourcePart = codegen.generateSource();
8086
}

0 commit comments

Comments
 (0)