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
This commit is contained in:
Tobias Bosch 2017-04-28 11:50:45 -07:00 committed by Matias Niemelä
parent ac220fc2bb
commit e263e19a2a
16 changed files with 218 additions and 68 deletions

View File

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

View File

@ -560,6 +560,9 @@ export class ApplicationRef_ extends ApplicationRef {
if (this._enforceNoNewChanges) {
this._views.forEach((view) => view.checkNoChanges());
}
} catch (e) {
// Attention: Don't rethrow as it could cancel subscriptions to Observables!
this._exceptionHandler.handleError(e);
} finally {
this._runningTick = false;
wtfLeave(scope);

View File

@ -189,7 +189,14 @@ export function listenToElementOutputs(view: ViewData, compView: ViewData, def:
}
function renderEventHandlerClosure(view: ViewData, index: number, eventName: string) {
return (event: any) => dispatchEvent(view, index, eventName, event);
return (event: any) => {
try {
return dispatchEvent(view, index, eventName, event);
} catch (e) {
// Attention: Don't rethrow, to keep in sync with directive events.
view.root.errorHandler.handleError(e);
}
}
}

View File

@ -148,7 +148,14 @@ export function createDirectiveInstance(view: ViewData, def: NodeDef): any {
}
function eventHandlerClosure(view: ViewData, index: number, eventName: string) {
return (event: any) => dispatchEvent(view, index, eventName, event);
return (event: any) => {
try {
return dispatchEvent(view, index, eventName, event);
} catch (e) {
// Attention: Don't rethrow, as it would cancel Observable subscriptions!
view.root.errorHandler.handleError(e);
}
}
}
export function checkAndUpdateDirectiveInline(

View File

@ -9,6 +9,7 @@
import {isDevMode} from '../application_ref';
import {DebugElement, DebugNode, EventListener, getDebugNode, indexDebugNode, removeDebugNodeFromIndex} from '../debug/debug_node';
import {Injector} from '../di';
import {ErrorHandler} from '../error_handler';
import {NgModuleRef} from '../linker/ng_module_factory';
import {Renderer2, RendererFactory2, RendererStyleFlags2, RendererType2} from '../render/api';
import {Sanitizer} from '../security';
@ -104,11 +105,12 @@ function createRootData(
elInjector: Injector, ngModule: NgModuleRef<any>, rendererFactory: RendererFactory2,
projectableNodes: any[][], rootSelectorOrNode: any): RootData {
const sanitizer = ngModule.injector.get(Sanitizer);
const errorHandler = ngModule.injector.get(ErrorHandler);
const renderer = rendererFactory.createRenderer(null, null);
return {
ngModule,
injector: elInjector, projectableNodes,
selectorOrNode: rootSelectorOrNode, sanitizer, rendererFactory, renderer
selectorOrNode: rootSelectorOrNode, sanitizer, rendererFactory, renderer, errorHandler
};
}
@ -439,7 +441,6 @@ function callWithDebugContext(action: DebugAction, fn: any, self: any, args: any
if (isViewDebugError(e) || !_currentView) {
throw e;
}
_currentView.state |= ViewState.Errored;
throw viewWrappedDebugError(e, getCurrentDebugContext() !);
}
}

View File

@ -7,6 +7,7 @@
*/
import {Injector} from '../di';
import {ErrorHandler} from '../error_handler';
import {NgModuleRef} from '../linker/ng_module_factory';
import {QueryList} from '../linker/query_list';
import {TemplateRef} from '../linker/template_ref';
@ -323,13 +324,14 @@ export interface ViewData {
* Bitmask of states
*/
export const enum ViewState {
FirstCheck = 1 << 0,
Attached = 1 << 1,
ChecksEnabled = 1 << 2,
Errored = 1 << 3,
BeforeFirstCheck = 1 << 0,
FirstCheck = 1 << 1,
Attached = 1 << 2,
ChecksEnabled = 1 << 3,
Destroyed = 1 << 4,
CatDetectChanges = Attached | ChecksEnabled,
CatInit = BeforeFirstCheck | CatDetectChanges
}
export interface DisposableFn { (): void; }
@ -432,6 +434,7 @@ export interface RootData {
selectorOrNode: any;
renderer: Renderer2;
rendererFactory: RendererFactory2;
errorHandler: ErrorHandler;
sanitizer: Sanitizer;
}

View File

@ -100,10 +100,10 @@ export function checkAndUpdateBinding(
export function checkBindingNoChanges(
view: ViewData, def: NodeDef, bindingIdx: number, value: any) {
const oldValue = view.oldValues[def.bindingIndex + bindingIdx];
if ((view.state & ViewState.FirstCheck) || !devModeEqual(oldValue, value)) {
if ((view.state & ViewState.BeforeFirstCheck) || !devModeEqual(oldValue, value)) {
throw expressionChangedAfterItHasBeenCheckedError(
Services.createDebugContext(view, def.index), oldValue, value,
(view.state & ViewState.FirstCheck) !== 0);
(view.state & ViewState.BeforeFirstCheck) !== 0);
}
}

View File

@ -211,7 +211,7 @@ function createView(
viewContainerParent: null, parentNodeDef,
context: null,
component: null, nodes,
state: ViewState.FirstCheck | ViewState.CatDetectChanges, root, renderer,
state: ViewState.CatInit, root, renderer,
oldValues: new Array(def.bindingCount), disposables
};
return view;
@ -323,6 +323,12 @@ export function checkNoChangesView(view: ViewData) {
}
export function checkAndUpdateView(view: ViewData) {
if (view.state & ViewState.BeforeFirstCheck) {
view.state &= ~ViewState.BeforeFirstCheck;
view.state |= ViewState.FirstCheck;
} else {
view.state &= ~ViewState.FirstCheck;
}
Services.updateDirectives(view, CheckType.CheckAndUpdate);
execEmbeddedViewsAction(view, ViewAction.CheckAndUpdate);
execQueriesAction(
@ -345,7 +351,6 @@ export function checkAndUpdateView(view: ViewData) {
if (view.def.flags & ViewFlags.OnPush) {
view.state &= ~ViewState.ChecksEnabled;
}
view.state &= ~ViewState.FirstCheck;
}
export function checkAndUpdateNode(
@ -453,7 +458,7 @@ function checkNoChangesQuery(view: ViewData, nodeDef: NodeDef) {
if (queryList.dirty) {
throw expressionChangedAfterItHasBeenCheckedError(
Services.createDebugContext(view, nodeDef.index), `Query ${nodeDef.query!.id} not dirty`,
`Query ${nodeDef.query!.id} dirty`, (view.state & ViewState.FirstCheck) !== 0);
`Query ${nodeDef.query!.id} dirty`, (view.state & ViewState.BeforeFirstCheck) !== 0);
}
}
@ -543,13 +548,13 @@ function callViewAction(view: ViewData, action: ViewAction) {
switch (action) {
case ViewAction.CheckNoChanges:
if ((viewState & ViewState.CatDetectChanges) === ViewState.CatDetectChanges &&
(viewState & (ViewState.Errored | ViewState.Destroyed)) === 0) {
(viewState & ViewState.Destroyed) === 0) {
checkNoChangesView(view);
}
break;
case ViewAction.CheckAndUpdate:
if ((viewState & ViewState.CatDetectChanges) === ViewState.CatDetectChanges &&
(viewState & (ViewState.Errored | ViewState.Destroyed)) === 0) {
(viewState & ViewState.Destroyed) === 0) {
checkAndUpdateView(view);
}
break;

View File

@ -131,20 +131,33 @@ export function main() {
describe('ApplicationRef', () => {
beforeEach(() => { TestBed.configureTestingModule({imports: [createModule()]}); });
it('should throw when reentering tick', inject([ApplicationRef], (ref: ApplicationRef_) => {
const view = jasmine.createSpyObj('view', ['detach', 'attachToAppRef']);
const viewRef = jasmine.createSpyObj(
'viewRef', ['detectChanges', 'detachFromAppRef', 'attachToAppRef']);
viewRef.internalView = view;
view.ref = viewRef;
try {
ref.attachView(viewRef);
viewRef.detectChanges.and.callFake(() => ref.tick());
expect(() => ref.tick()).toThrowError('ApplicationRef.tick is called recursively');
} finally {
ref.detachView(viewRef);
}
}));
it('should throw when reentering tick', () => {
@Component({template: '{{reenter()}}'})
class ReenteringComponent {
reenterCount = 1;
reenterErr: any;
constructor(private appRef: ApplicationRef) {}
reenter() {
if (this.reenterCount--) {
try {
this.appRef.tick();
} catch (e) {
this.reenterErr = e;
}
}
}
}
const fixture = TestBed.configureTestingModule({declarations: [ReenteringComponent]})
.createComponent(ReenteringComponent);
const appRef = TestBed.get(ApplicationRef) as ApplicationRef;
appRef.attachView(fixture.componentRef.hostView);
appRef.tick();
expect(fixture.componentInstance.reenterErr.message)
.toBe('ApplicationRef.tick is called recursively');
});
describe('APP_BOOTSTRAP_LISTENER', () => {
let capturedCompRefs: ComponentRef<any>[];

View File

@ -765,6 +765,7 @@ export function main() {
try {
ctx.detectChanges(false);
} catch (e) {
expect(e.message).toBe('Boom!');
errored = true;
}
expect(errored).toBe(true);
@ -776,7 +777,8 @@ export function main() {
try {
ctx.detectChanges(false);
} catch (e) {
throw new Error('Second detectChanges() should not have run detection.');
expect(e.message).toBe('Boom!');
throw new Error('Second detectChanges() should not have called ngOnInit.');
}
expect(directiveLog.filter(['ngOnInit'])).toEqual([]);
}));

View File

@ -7,7 +7,7 @@
*/
import {CommonModule} from '@angular/common';
import {Compiler, ComponentFactory, EventEmitter, Host, Inject, Injectable, InjectionToken, Injector, NO_ERRORS_SCHEMA, NgModule, NgModuleRef, OnDestroy, ReflectiveInjector, SkipSelf} from '@angular/core';
import {Compiler, ComponentFactory, ErrorHandler, EventEmitter, Host, Inject, Injectable, InjectionToken, Injector, NO_ERRORS_SCHEMA, NgModule, NgModuleRef, OnDestroy, ReflectiveInjector, SkipSelf} from '@angular/core';
import {ChangeDetectionStrategy, ChangeDetectorRef, PipeTransform} from '@angular/core/src/change_detection/change_detection';
import {getDebugContext} from '@angular/core/src/errors';
import {ComponentFactoryResolver} from '@angular/core/src/linker/component_factory_resolver';
@ -1480,16 +1480,18 @@ function declareTests({useJit}: {useJit: boolean}) {
const tc = fixture.debugElement.children[0];
try {
tc.injector.get(DirectiveEmittingEvent).fireEvent('boom');
} catch (e) {
const c = getDebugContext(e);
expect(getDOM().nodeName(c.renderNode).toUpperCase()).toEqual('SPAN');
expect(getDOM().nodeName(c.componentRenderElement).toUpperCase()).toEqual('DIV');
expect((<Injector>c.injector).get).toBeTruthy();
expect(c.context).toBe(fixture.componentInstance);
expect(c.references['local']).toBeDefined();
}
const errorHandler = tc.injector.get(ErrorHandler);
let err: any;
spyOn(errorHandler, 'handleError').and.callFake((e: any) => err = e);
tc.injector.get(DirectiveEmittingEvent).fireEvent('boom');
expect(err).toBeTruthy();
const c = getDebugContext(err);
expect(getDOM().nodeName(c.renderNode).toUpperCase()).toEqual('SPAN');
expect(getDOM().nodeName(c.componentRenderElement).toUpperCase()).toEqual('DIV');
expect((<Injector>c.injector).get).toBeTruthy();
expect(c.context).toBe(fixture.componentInstance);
expect(c.references['local']).toBeDefined();
}));
}
});

View File

@ -6,9 +6,10 @@
* found in the LICENSE file at https://angular.io/license
*/
import {ANALYZE_FOR_ENTRY_COMPONENTS, Component, ContentChild, Directive, InjectionToken, Injector, Input, NgModule, NgModuleRef, Pipe, PipeTransform, Provider, QueryList, Renderer2, SimpleChanges, TemplateRef, ViewChildren, ViewContainerRef} from '@angular/core';
import {TestBed, fakeAsync, tick} from '@angular/core/testing';
import {By} from '@angular/platform-browser';
import {ANALYZE_FOR_ENTRY_COMPONENTS, ApplicationRef, Component, ComponentRef, ContentChild, Directive, ErrorHandler, EventEmitter, HostListener, InjectionToken, Injector, Input, NgModule, NgModuleRef, NgZone, Output, Pipe, PipeTransform, Provider, QueryList, Renderer2, SimpleChanges, TemplateRef, ViewChildren, ViewContainerRef, destroyPlatform} from '@angular/core';
import {TestBed, async, fakeAsync, inject, tick} from '@angular/core/testing';
import {BrowserModule, By, DOCUMENT} from '@angular/platform-browser';
import {platformBrowserDynamic} from '@angular/platform-browser-dynamic';
import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter';
import {expect} from '@angular/platform-browser/testing/src/matchers';
@ -16,6 +17,8 @@ export function main() {
describe('jit', () => { declareTests({useJit: true}); });
describe('no jit', () => { declareTests({useJit: false}); });
declareTestsUsingBootstrap();
}
function declareTests({useJit}: {useJit: boolean}) {
@ -386,6 +389,115 @@ function declareTests({useJit}: {useJit: boolean}) {
});
}
function declareTestsUsingBootstrap() {
// Place to put reproductions for regressions
describe('regressions using bootstrap', () => {
const COMP_SELECTOR = 'root-comp';
class MockConsole {
errors: any[][] = [];
error(...s: any[]): void { this.errors.push(s); }
}
let logger: MockConsole;
let errorHandler: ErrorHandler;
beforeEach(inject([DOCUMENT], (doc: any) => {
destroyPlatform();
const el = getDOM().createElement(COMP_SELECTOR, doc);
getDOM().appendChild(doc.body, el);
logger = new MockConsole();
errorHandler = new ErrorHandler();
errorHandler._console = logger as any;
}));
afterEach(() => { destroyPlatform(); });
if (getDOM().supportsDOMEvents()) {
// This test needs a real DOM....
it('should keep change detecting if there was an error', (done) => {
@Component({
selector: COMP_SELECTOR,
template:
'<button (click)="next()"></button><button (click)="nextAndThrow()"></button><button (dirClick)="nextAndThrow()"></button><span>Value:{{value}}</span><span>{{throwIfNeeded()}}</span>'
})
class ErrorComp {
value = 0;
thrownValue = 0;
next() { this.value++; }
nextAndThrow() {
this.value++;
this.throwIfNeeded();
}
throwIfNeeded() {
NgZone.assertInAngularZone();
if (this.thrownValue !== this.value) {
this.thrownValue = this.value;
throw new Error(`Error: ${this.value}`);
}
}
}
@Directive({selector: '[dirClick]'})
class EventDir {
@Output()
dirClick = new EventEmitter();
@HostListener('click', ['$event'])
onClick(event: any) { this.dirClick.next(event); }
}
@NgModule({
imports: [BrowserModule],
declarations: [ErrorComp, EventDir],
bootstrap: [ErrorComp],
providers: [{provide: ErrorHandler, useValue: errorHandler}],
})
class TestModule {
}
platformBrowserDynamic().bootstrapModule(TestModule).then((ref) => {
NgZone.assertNotInAngularZone();
const appRef = ref.injector.get(ApplicationRef) as ApplicationRef;
const compRef = appRef.components[0] as ComponentRef<ErrorComp>;
const compEl = compRef.location.nativeElement;
const nextBtn = compEl.children[0];
const nextAndThrowBtn = compEl.children[1];
const nextAndThrowDirBtn = compEl.children[2];
nextBtn.click();
assertValueAndErrors(compEl, 1, 0);
nextBtn.click();
assertValueAndErrors(compEl, 2, 2);
nextAndThrowBtn.click();
assertValueAndErrors(compEl, 3, 4);
nextAndThrowBtn.click();
assertValueAndErrors(compEl, 4, 6);
nextAndThrowDirBtn.click();
assertValueAndErrors(compEl, 5, 8);
nextAndThrowDirBtn.click();
assertValueAndErrors(compEl, 6, 10);
// Assert that there were no more errors
expect(logger.errors.length).toBe(12);
done();
});
function assertValueAndErrors(compEl: any, value: number, errorIndex: number) {
expect(compEl).toHaveText(`Value:${value}`);
expect(logger.errors[errorIndex][0]).toBe('ERROR');
expect(logger.errors[errorIndex][1].message).toBe(`Error: ${value}`);
expect(logger.errors[errorIndex + 1][0]).toBe('ERROR CONTEXT');
}
});
}
});
}
@Component({selector: 'my-comp', template: ''})
class MyComp1 {
constructor(public injector: Injector) {}

View File

@ -10,7 +10,7 @@ import {ResourceLoader} from '@angular/compiler';
import {SourceMap} from '@angular/compiler/src/output/source_map';
import {extractSourceMap, originalPositionFor} from '@angular/compiler/test/output/source_map_util';
import {MockResourceLoader} from '@angular/compiler/testing/src/resource_loader_mock';
import {Attribute, Component, Directive, ɵglobal} from '@angular/core';
import {Attribute, Component, Directive, ErrorHandler, ɵglobal} from '@angular/core';
import {getErrorLogger} from '@angular/core/src/errors';
import {ComponentFixture, TestBed, fakeAsync, tick} from '@angular/core/testing';
@ -231,11 +231,10 @@ export function main() {
const comp = compileAndCreateComponent(MyComp);
let error: any;
try {
comp.debugElement.children[0].children[0].triggerEventHandler('click', 'EVENT');
} catch (e) {
error = e;
}
const errorHandler = TestBed.get(ErrorHandler);
spyOn(errorHandler, 'handleError').and.callFake((e: any) => error = e);
comp.debugElement.children[0].children[0].triggerEventHandler('click', 'EVENT');
expect(error).toBeTruthy();
// the stack should point to the binding
expect(getSourcePositionForStack(error.stack)).toEqual({
line: 2,

View File

@ -230,7 +230,7 @@ export function main() {
});
}
it('should stop dirty checking views that threw errors in change detection', () => {
it('should not stop dirty checking views that threw errors in change detection', () => {
class AComp {
a: any;
}
@ -255,8 +255,8 @@ export function main() {
expect(update).toHaveBeenCalled();
update.calls.reset();
Services.checkAndUpdateView(view);
expect(update).not.toHaveBeenCalled();
expect(() => Services.checkAndUpdateView(view)).toThrowError('Test');
expect(update).toHaveBeenCalled();
});
});

View File

@ -6,9 +6,10 @@
* found in the LICENSE file at https://angular.io/license
*/
import {Injector, RenderComponentType, RootRenderer, Sanitizer, SecurityContext, ViewEncapsulation, WrappedValue, getDebugNode} from '@angular/core';
import {ErrorHandler, Injector, RenderComponentType, RootRenderer, Sanitizer, SecurityContext, ViewEncapsulation, WrappedValue, getDebugNode} from '@angular/core';
import {getDebugContext} from '@angular/core/src/errors';
import {ArgumentType, BindingFlags, DebugContext, NodeDef, NodeFlags, OutputType, RootData, Services, ViewData, ViewDefinition, ViewFlags, ViewHandleEventFn, ViewUpdateFn, anchorDef, asElementData, elementDef, rootRenderNodes, textDef, viewDef} from '@angular/core/src/view/index';
import {TestBed} from '@angular/core/testing';
import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter';
import {ARG_TYPE_VALUES, checkNodeInlineOrDynamic, createRootView, isBrowser, removeNodes} from './helper';
@ -282,17 +283,14 @@ export function main() {
});
it('should report debug info on event errors', () => {
const handleErrorSpy = spyOn(TestBed.get(ErrorHandler), 'handleError');
const addListenerSpy = spyOn(HTMLElement.prototype, 'addEventListener').and.callThrough();
const {view, rootNodes} = createAndAttachAndGetRootNodes(compViewDef([elementDef(
NodeFlags.None, null !, null !, 0, 'button', null !, null !, [[null !, 'click']],
() => { throw new Error('Test'); })]));
let err: any;
try {
addListenerSpy.calls.mostRecent().args[1]('SomeEvent');
} catch (e) {
err = e;
}
addListenerSpy.calls.mostRecent().args[1]('SomeEvent');
const err = handleErrorSpy.calls.mostRecent().args[0];
expect(err).toBeTruthy();
expect(err.message).toBe('Test');
const debugCtx = getDebugContext(err);

View File

@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/
import {AfterContentChecked, AfterContentInit, AfterViewChecked, AfterViewInit, ChangeDetectorRef, DoCheck, ElementRef, EventEmitter, Injector, OnChanges, OnDestroy, OnInit, RenderComponentType, Renderer, Renderer2, RootRenderer, Sanitizer, SecurityContext, SimpleChange, TemplateRef, ViewContainerRef, ViewEncapsulation, WrappedValue, getDebugNode} from '@angular/core';
import {AfterContentChecked, AfterContentInit, AfterViewChecked, AfterViewInit, ChangeDetectorRef, DoCheck, ElementRef, ErrorHandler, EventEmitter, Injector, OnChanges, OnDestroy, OnInit, RenderComponentType, Renderer, Renderer2, RootRenderer, Sanitizer, SecurityContext, SimpleChange, TemplateRef, ViewContainerRef, ViewEncapsulation, WrappedValue, getDebugNode} from '@angular/core';
import {getDebugContext} from '@angular/core/src/errors';
import {ArgumentType, BindingFlags, DebugContext, DepFlags, NodeDef, NodeFlags, RootData, Services, ViewData, ViewDefinition, ViewDefinitionFactory, ViewFlags, ViewHandleEventFn, ViewUpdateFn, anchorDef, asElementData, asProviderData, directiveDef, elementDef, providerDef, rootRenderNodes, textDef, viewDef} from '@angular/core/src/view/index';
import {TestBed, inject, withModule} from '@angular/core/testing';
@ -381,6 +381,7 @@ export function main() {
});
it('should report debug info on event errors', () => {
const handleErrorSpy = spyOn(TestBed.get(ErrorHandler), 'handleError');
let emitter = new EventEmitter<any>();
class SomeService {
@ -395,12 +396,8 @@ export function main() {
NodeFlags.None, null !, 0, SomeService, [], null !, {emitter: 'someEventName'})
]));
let err: any;
try {
emitter.emit('someEventInstance');
} catch (e) {
err = e;
}
emitter.emit('someEventInstance');
const err = handleErrorSpy.calls.mostRecent().args[0];
expect(err).toBeTruthy();
const debugCtx = getDebugContext(err);
expect(debugCtx.view).toBe(view);