Skip to content

Commit e263e19

Browse files
tboschmatsko
tbosch
authored and
matsko
committed
fix(core): don’t stop change detection because of errors
- prevents unsubscribing from the zone on error - prevents unsubscribing from directive `EventEmitter`s on error - prevents detaching views in dev mode if there on error - ensures that `ngOnInit` is only called 1x (also in prod mode) Fixes #9531 Fixes #2413 Fixes #15925
1 parent ac220fc commit e263e19

16 files changed

+218
-68
lines changed

‎modules/benchmarks/src/tree/ng2_next/tree.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
*/
88

99
import {NgIf} from '@angular/common';
10-
import {ComponentFactory, ComponentFactoryResolver, ComponentRef, Injector, NgModuleRef, RendererFactory2, RootRenderer, Sanitizer, TemplateRef, ViewContainerRef} from '@angular/core';
10+
import {ComponentFactory, ComponentFactoryResolver, ComponentRef, ErrorHandler, Injector, NgModuleRef, RendererFactory2, RootRenderer, Sanitizer, TemplateRef, ViewContainerRef} from '@angular/core';
1111
import {ArgumentType, BindingFlags, NodeFlags, ViewDefinition, ViewFlags, anchorDef, createComponentFactory, directiveDef, elementDef, initServicesIfNeeded, textDef, viewDef} from '@angular/core/src/view/index';
1212
import {DomRendererFactory2} from '@angular/platform-browser/src/dom/dom_renderer';
1313
import {DomSanitizerImpl, SafeStyle} from '@angular/platform-browser/src/security/dom_sanitization_service';
@@ -108,6 +108,7 @@ export class AppModule implements Injector, NgModuleRef<any> {
108108
case Sanitizer:
109109
return this.sanitizer;
110110
case RootRenderer:
111+
case ErrorHandler:
111112
return null;
112113
case NgModuleRef:
113114
return this;

‎packages/core/src/application_ref.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -560,6 +560,9 @@ export class ApplicationRef_ extends ApplicationRef {
560560
if (this._enforceNoNewChanges) {
561561
this._views.forEach((view) => view.checkNoChanges());
562562
}
563+
} catch (e) {
564+
// Attention: Don't rethrow as it could cancel subscriptions to Observables!
565+
this._exceptionHandler.handleError(e);
563566
} finally {
564567
this._runningTick = false;
565568
wtfLeave(scope);

‎packages/core/src/view/element.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,14 @@ export function listenToElementOutputs(view: ViewData, compView: ViewData, def:
189189
}
190190

191191
function renderEventHandlerClosure(view: ViewData, index: number, eventName: string) {
192-
return (event: any) => dispatchEvent(view, index, eventName, event);
192+
return (event: any) => {
193+
try {
194+
return dispatchEvent(view, index, eventName, event);
195+
} catch (e) {
196+
// Attention: Don't rethrow, to keep in sync with directive events.
197+
view.root.errorHandler.handleError(e);
198+
}
199+
}
193200
}
194201

195202

‎packages/core/src/view/provider.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,14 @@ export function createDirectiveInstance(view: ViewData, def: NodeDef): any {
148148
}
149149

150150
function eventHandlerClosure(view: ViewData, index: number, eventName: string) {
151-
return (event: any) => dispatchEvent(view, index, eventName, event);
151+
return (event: any) => {
152+
try {
153+
return dispatchEvent(view, index, eventName, event);
154+
} catch (e) {
155+
// Attention: Don't rethrow, as it would cancel Observable subscriptions!
156+
view.root.errorHandler.handleError(e);
157+
}
158+
}
152159
}
153160

154161
export function checkAndUpdateDirectiveInline(

‎packages/core/src/view/services.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import {isDevMode} from '../application_ref';
1010
import {DebugElement, DebugNode, EventListener, getDebugNode, indexDebugNode, removeDebugNodeFromIndex} from '../debug/debug_node';
1111
import {Injector} from '../di';
12+
import {ErrorHandler} from '../error_handler';
1213
import {NgModuleRef} from '../linker/ng_module_factory';
1314
import {Renderer2, RendererFactory2, RendererStyleFlags2, RendererType2} from '../render/api';
1415
import {Sanitizer} from '../security';
@@ -104,11 +105,12 @@ function createRootData(
104105
elInjector: Injector, ngModule: NgModuleRef<any>, rendererFactory: RendererFactory2,
105106
projectableNodes: any[][], rootSelectorOrNode: any): RootData {
106107
const sanitizer = ngModule.injector.get(Sanitizer);
108+
const errorHandler = ngModule.injector.get(ErrorHandler);
107109
const renderer = rendererFactory.createRenderer(null, null);
108110
return {
109111
ngModule,
110112
injector: elInjector, projectableNodes,
111-
selectorOrNode: rootSelectorOrNode, sanitizer, rendererFactory, renderer
113+
selectorOrNode: rootSelectorOrNode, sanitizer, rendererFactory, renderer, errorHandler
112114
};
113115
}
114116

@@ -439,7 +441,6 @@ function callWithDebugContext(action: DebugAction, fn: any, self: any, args: any
439441
if (isViewDebugError(e) || !_currentView) {
440442
throw e;
441443
}
442-
_currentView.state |= ViewState.Errored;
443444
throw viewWrappedDebugError(e, getCurrentDebugContext() !);
444445
}
445446
}

‎packages/core/src/view/types.ts‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
*/
88

99
import {Injector} from '../di';
10+
import {ErrorHandler} from '../error_handler';
1011
import {NgModuleRef} from '../linker/ng_module_factory';
1112
import {QueryList} from '../linker/query_list';
1213
import {TemplateRef} from '../linker/template_ref';
@@ -323,13 +324,14 @@ export interface ViewData {
323324
* Bitmask of states
324325
*/
325326
export const enum ViewState {
326-
FirstCheck = 1 << 0,
327-
Attached = 1 << 1,
328-
ChecksEnabled = 1 << 2,
329-
Errored = 1 << 3,
327+
BeforeFirstCheck = 1 << 0,
328+
FirstCheck = 1 << 1,
329+
Attached = 1 << 2,
330+
ChecksEnabled = 1 << 3,
330331
Destroyed = 1 << 4,
331332

332333
CatDetectChanges = Attached | ChecksEnabled,
334+
CatInit = BeforeFirstCheck | CatDetectChanges
333335
}
334336

335337
export interface DisposableFn { (): void; }
@@ -432,6 +434,7 @@ export interface RootData {
432434
selectorOrNode: any;
433435
renderer: Renderer2;
434436
rendererFactory: RendererFactory2;
437+
errorHandler: ErrorHandler;
435438
sanitizer: Sanitizer;
436439
}
437440

‎packages/core/src/view/util.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,10 +100,10 @@ export function checkAndUpdateBinding(
100100
export function checkBindingNoChanges(
101101
view: ViewData, def: NodeDef, bindingIdx: number, value: any) {
102102
const oldValue = view.oldValues[def.bindingIndex + bindingIdx];
103-
if ((view.state & ViewState.FirstCheck) || !devModeEqual(oldValue, value)) {
103+
if ((view.state & ViewState.BeforeFirstCheck) || !devModeEqual(oldValue, value)) {
104104
throw expressionChangedAfterItHasBeenCheckedError(
105105
Services.createDebugContext(view, def.index), oldValue, value,
106-
(view.state & ViewState.FirstCheck) !== 0);
106+
(view.state & ViewState.BeforeFirstCheck) !== 0);
107107
}
108108
}
109109

‎packages/core/src/view/view.ts‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ function createView(
211211
viewContainerParent: null, parentNodeDef,
212212
context: null,
213213
component: null, nodes,
214-
state: ViewState.FirstCheck | ViewState.CatDetectChanges, root, renderer,
214+
state: ViewState.CatInit, root, renderer,
215215
oldValues: new Array(def.bindingCount), disposables
216216
};
217217
return view;
@@ -323,6 +323,12 @@ export function checkNoChangesView(view: ViewData) {
323323
}
324324

325325
export function checkAndUpdateView(view: ViewData) {
326+
if (view.state & ViewState.BeforeFirstCheck) {
327+
view.state &= ~ViewState.BeforeFirstCheck;
328+
view.state |= ViewState.FirstCheck;
329+
} else {
330+
view.state &= ~ViewState.FirstCheck;
331+
}
326332
Services.updateDirectives(view, CheckType.CheckAndUpdate);
327333
execEmbeddedViewsAction(view, ViewAction.CheckAndUpdate);
328334
execQueriesAction(
@@ -345,7 +351,6 @@ export function checkAndUpdateView(view: ViewData) {
345351
if (view.def.flags & ViewFlags.OnPush) {
346352
view.state &= ~ViewState.ChecksEnabled;
347353
}
348-
view.state &= ~ViewState.FirstCheck;
349354
}
350355

351356
export function checkAndUpdateNode(
@@ -453,7 +458,7 @@ function checkNoChangesQuery(view: ViewData, nodeDef: NodeDef) {
453458
if (queryList.dirty) {
454459
throw expressionChangedAfterItHasBeenCheckedError(
455460
Services.createDebugContext(view, nodeDef.index), `Query ${nodeDef.query!.id} not dirty`,
456-
`Query ${nodeDef.query!.id} dirty`, (view.state & ViewState.FirstCheck) !== 0);
461+
`Query ${nodeDef.query!.id} dirty`, (view.state & ViewState.BeforeFirstCheck) !== 0);
457462
}
458463
}
459464

@@ -543,13 +548,13 @@ function callViewAction(view: ViewData, action: ViewAction) {
543548
switch (action) {
544549
case ViewAction.CheckNoChanges:
545550
if ((viewState & ViewState.CatDetectChanges) === ViewState.CatDetectChanges &&
546-
(viewState & (ViewState.Errored | ViewState.Destroyed)) === 0) {
551+
(viewState & ViewState.Destroyed) === 0) {
547552
checkNoChangesView(view);
548553
}
549554
break;
550555
case ViewAction.CheckAndUpdate:
551556
if ((viewState & ViewState.CatDetectChanges) === ViewState.CatDetectChanges &&
552-
(viewState & (ViewState.Errored | ViewState.Destroyed)) === 0) {
557+
(viewState & ViewState.Destroyed) === 0) {
553558
checkAndUpdateView(view);
554559
}
555560
break;

‎packages/core/test/application_ref_spec.ts‎

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -131,20 +131,33 @@ export function main() {
131131
describe('ApplicationRef', () => {
132132
beforeEach(() => { TestBed.configureTestingModule({imports: [createModule()]}); });
133133

134-
it('should throw when reentering tick', inject([ApplicationRef], (ref: ApplicationRef_) => {
135-
const view = jasmine.createSpyObj('view', ['detach', 'attachToAppRef']);
136-
const viewRef = jasmine.createSpyObj(
137-
'viewRef', ['detectChanges', 'detachFromAppRef', 'attachToAppRef']);
138-
viewRef.internalView = view;
139-
view.ref = viewRef;
140-
try {
141-
ref.attachView(viewRef);
142-
viewRef.detectChanges.and.callFake(() => ref.tick());
143-
expect(() => ref.tick()).toThrowError('ApplicationRef.tick is called recursively');
144-
} finally {
145-
ref.detachView(viewRef);
146-
}
147-
}));
134+
it('should throw when reentering tick', () => {
135+
@Component({template: '{{reenter()}}'})
136+
class ReenteringComponent {
137+
reenterCount = 1;
138+
reenterErr: any;
139+
140+
constructor(private appRef: ApplicationRef) {}
141+
142+
reenter() {
143+
if (this.reenterCount--) {
144+
try {
145+
this.appRef.tick();
146+
} catch (e) {
147+
this.reenterErr = e;
148+
}
149+
}
150+
}
151+
}
152+
153+
const fixture = TestBed.configureTestingModule({declarations: [ReenteringComponent]})
154+
.createComponent(ReenteringComponent);
155+
const appRef = TestBed.get(ApplicationRef) as ApplicationRef;
156+
appRef.attachView(fixture.componentRef.hostView);
157+
appRef.tick();
158+
expect(fixture.componentInstance.reenterErr.message)
159+
.toBe('ApplicationRef.tick is called recursively');
160+
});
148161

149162
describe('APP_BOOTSTRAP_LISTENER', () => {
150163
let capturedCompRefs: ComponentRef<any>[];

‎packages/core/test/linker/change_detection_integration_spec.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -765,6 +765,7 @@ export function main() {
765765
try {
766766
ctx.detectChanges(false);
767767
} catch (e) {
768+
expect(e.message).toBe('Boom!');
768769
errored = true;
769770
}
770771
expect(errored).toBe(true);
@@ -776,7 +777,8 @@ export function main() {
776777
try {
777778
ctx.detectChanges(false);
778779
} catch (e) {
779-
throw new Error('Second detectChanges() should not have run detection.');
780+
expect(e.message).toBe('Boom!');
781+
throw new Error('Second detectChanges() should not have called ngOnInit.');
780782
}
781783
expect(directiveLog.filter(['ngOnInit'])).toEqual([]);
782784
}));

0 commit comments

Comments
 (0)