From c497fe17a83e44702bb7da5abfacc11cf82d072c Mon Sep 17 00:00:00 2001 From: crisbeto Date: Mon, 20 Apr 2020 20:49:08 +0200 Subject: [PATCH 1/2] fix(core): attempt to recover from user errors during creation If there's an error during the first creation pass of a `TView`, the data structure may be corrupted which will cause framework assertion failures downstream which can mask the user's error. These changes add a new flag to the `TView` that indicates whether the first creation pass was successful, and if it wasn't we try re-create the `TView`. Fixes #31221. --- .../src/render3/instructions/lview_debug.ts | 1 + .../core/src/render3/instructions/shared.ts | 28 +- packages/core/src/render3/interfaces/view.ts | 6 + .../test/acceptance/view_insertion_spec.ts | 262 +++++++++++++++++- 4 files changed, 283 insertions(+), 14 deletions(-) diff --git a/packages/core/src/render3/instructions/lview_debug.ts b/packages/core/src/render3/instructions/lview_debug.ts index fd3030084afce..6beed812e9248 100644 --- a/packages/core/src/render3/instructions/lview_debug.ts +++ b/packages/core/src/render3/instructions/lview_debug.ts @@ -143,6 +143,7 @@ export const TViewConstructor = class TView implements ITView { public firstChild: ITNode|null, // public schemas: SchemaMetadata[]|null, // public consts: TConstants|null, // + public incompleteFirstPass: boolean // ) {} get template_(): string { diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 045561ee452a5..d5b18c87c8d73 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -374,6 +374,14 @@ export function renderView(tView: TView, lView: LView, context: T): void { renderChildComponents(lView, components); } + } catch (error) { + // If we didn't manage to get past the first template pass due to + // an error, mark the view as corrupted so we can try to recover. + if (tView.firstCreatePass) { + tView.incompleteFirstPass = true; + } + + throw error; } finally { lView[FLAGS] &= ~LViewFlags.CreationMode; leaveView(); @@ -598,10 +606,17 @@ export function saveResolvedLocalsInData( * @returns TView */ export function getOrCreateTComponentView(def: ComponentDef): TView { - return def.tView || - (def.tView = createTView( - TViewType.Component, -1, def.template, def.decls, def.vars, def.directiveDefs, - def.pipeDefs, def.viewQuery, def.schemas, def.consts)); + const tView = def.tView; + + // Create a TView if there isn't one, or recreate it if the first create pass didn't + // complete successfuly since we can't know for sure whether it's in a usable shape. + if (tView === null || tView.incompleteFirstPass) { + return def.tView = createTView( + TViewType.Component, -1, def.template, def.decls, def.vars, def.directiveDefs, + def.pipeDefs, def.viewQuery, def.schemas, def.consts); + } + + return tView; } @@ -662,7 +677,9 @@ export function createTView( typeof pipes === 'function' ? pipes() : pipes, // pipeRegistry: PipeDefList|null, null, // firstChild: TNode|null, schemas, // schemas: SchemaMetadata[]|null, - consts) : // consts: TConstants|null + consts, // consts: TConstants|null + false // incompleteFirstPass: boolean + ) : { type: type, id: viewIndex, @@ -694,6 +711,7 @@ export function createTView( firstChild: null, schemas: schemas, consts: consts, + incompleteFirstPass: false }; } diff --git a/packages/core/src/render3/interfaces/view.ts b/packages/core/src/render3/interfaces/view.ts index eb943a97ffae8..4fc01b3f2bb2f 100644 --- a/packages/core/src/render3/interfaces/view.ts +++ b/packages/core/src/render3/interfaces/view.ts @@ -684,6 +684,12 @@ export interface TView { * Used for directive matching, attribute bindings, local definitions and more. */ consts: TConstants|null; + + /** + * Indicates that there was an error before we managed to complete the first create pass of the + * view. This means that the view is likely corrupted and we should try to recover it. + */ + incompleteFirstPass: boolean; } export const enum RootContextFlags { diff --git a/packages/core/test/acceptance/view_insertion_spec.ts b/packages/core/test/acceptance/view_insertion_spec.ts index 265816486503b..f7811ebd3d470 100644 --- a/packages/core/test/acceptance/view_insertion_spec.ts +++ b/packages/core/test/acceptance/view_insertion_spec.ts @@ -7,7 +7,7 @@ */ import {CommonModule} from '@angular/common'; -import {ChangeDetectorRef, Component, ComponentFactoryResolver, Directive, EmbeddedViewRef, Injector, NgModule, TemplateRef, ViewChild, ViewContainerRef, ViewRef} from '@angular/core'; +import {ChangeDetectorRef, Component, ComponentFactoryResolver, Directive, EmbeddedViewRef, Injector, Input, NgModule, TemplateRef, ViewChild, ViewContainerRef, ViewRef} from '@angular/core'; import {TestBed} from '@angular/core/testing'; import {By} from '@angular/platform-browser'; import {onlyInIvy} from '@angular/private/testing'; @@ -281,9 +281,9 @@ describe('view insertion', () => { function createAndInsertViews(beforeTpl: string): any { TestBed.overrideTemplate(TestCmpt, ` - insert + insert ${beforeTpl} - +
`); const fixture = TestBed.createComponent(TestCmpt); @@ -377,12 +377,12 @@ describe('view insertion', () => { it('should insert before a ng-container with a ViewContainerRef on it', () => { @Component({ selector: 'app-root', - template: ` + template: `
start|
|end
- + test ` }) @@ -398,11 +398,11 @@ describe('view insertion', () => { const fixture = TestBed.createComponent(AppComponent); fixture.detectChanges(); - expect(fixture.debugElement.nativeElement.textContent).toBe('start|test|end'); + expect(fixture.nativeElement.textContent).toBe('start|test|end'); fixture.componentInstance.insertTpl = true; fixture.detectChanges(); - expect(fixture.debugElement.nativeElement.textContent).toBe('start|testtest|end'); + expect(fixture.nativeElement.textContent).toBe('start|testtest|end'); }); }); @@ -485,7 +485,7 @@ describe('view insertion', () => { @Component({ selector: 'test-cmpt', template: ` - insert + insert
` }) @@ -614,7 +614,7 @@ describe('view insertion', () => { it('should properly insert before views in a ViewContainerRef injected on ng-container', () => { @Component({ selector: 'app-root', - template: ` + template: ` {{parameter}} @@ -643,4 +643,248 @@ describe('view insertion', () => { expect(fixture.nativeElement.textContent.trim()).toContain('2 1'); }); }); + + describe('create mode error handling', () => { + it('should consistently report errors raised a directive constructor', () => { + @Directive({ + selector: '[failInConstructorAlways]', + }) + class FailInConstructorAlways { + constructor() { + throw new Error('Error in a constructor'); + } + } + + @Component({ + template: `
`, + }) + class TestCmpt { + } + + TestBed.configureTestingModule({ + declarations: [TestCmpt, FailInConstructorAlways], + }); + + expect(() => { + TestBed.createComponent(TestCmpt); + }).toThrowError('Error in a constructor'); + + expect(() => { + TestBed.createComponent(TestCmpt); + }).toThrowError('Error in a constructor'); + }); + + it('should render even if a directive constructor throws in the first create pass', () => { + let firstRun = true; + + @Directive({ + selector: '[failInConstructorOnce]', + }) + class FailInConstructorOnce { + constructor() { + if (firstRun) { + firstRun = false; + throw new Error('Error in a constructor'); + } + } + } + + @Component({ + template: `
OK
`, + }) + class TestCmpt { + } + + TestBed.configureTestingModule({ + declarations: [TestCmpt, FailInConstructorOnce], + }); + + expect(() => { + TestBed.createComponent(TestCmpt); + }).toThrowError('Error in a constructor'); + + const fixture = TestBed.createComponent(TestCmpt); + expect(fixture.nativeElement.textContent).toContain('OK'); + }); + + onlyInIvy('Test depends on static inputs being set during creation') + .it('should consistently report errors raised a directive input setter', () => { + @Directive({ + selector: '[failInInputAlways]', + }) + class FailInInputAlways { + @Input() + set failInInputAlways(_: string) { + throw new Error('Error in an input'); + } + } + + @Component({ + template: `
`, + }) + class TestCmpt { + } + + TestBed.configureTestingModule({ + declarations: [TestCmpt, FailInInputAlways], + }); + + expect(() => { + TestBed.createComponent(TestCmpt); + }).toThrowError('Error in an input'); + + expect(() => { + TestBed.createComponent(TestCmpt); + }).toThrowError('Error in an input'); + }); + + it('should consistently report errors raised a static query setter', () => { + @Directive({ + selector: '[someDir]', + }) + class SomeDirective { + } + + @Component({ + template: `
`, + }) + class TestCmpt { + @ViewChild(SomeDirective, {static: true}) + set directive(_: SomeDirective) { + throw new Error('Error in static query setter'); + } + } + + TestBed.configureTestingModule({ + declarations: [TestCmpt, SomeDirective], + }); + + expect(() => { + TestBed.createComponent(TestCmpt); + }).toThrowError('Error in static query setter'); + + expect(() => { + TestBed.createComponent(TestCmpt); + }).toThrowError('Error in static query setter'); + }); + + it('should match a static query, even if its setter throws in the first create pass', () => { + let hasThrown = false; + + @Directive({ + selector: '[someDir]', + }) + class SomeDirective { + } + + @Component({ + template: `
`, + }) + class TestCmpt { + @ViewChild(SomeDirective, {static: true}) + get directive() { + return this._directive; + } + set directive(directiveInstance: SomeDirective) { + if (!hasThrown) { + hasThrown = true; + throw new Error('Error in static query setter'); + } + + this._directive = directiveInstance; + } + + private _directive!: SomeDirective; + } + + TestBed.configureTestingModule({ + declarations: [TestCmpt, SomeDirective], + }); + + expect(() => { + TestBed.createComponent(TestCmpt); + }).toThrowError('Error in static query setter'); + + const fixture = TestBed.createComponent(TestCmpt); + + expect(fixture.componentInstance.directive).toBeInstanceOf(SomeDirective); + }); + + it('should render a recursive component if it throws during the first creation pass', () => { + let hasThrown = false; + + @Component({ + selector: 'test', + template: `OK`, + }) + class TestCmpt { + constructor() { + if (!hasThrown) { + hasThrown = true; + throw new Error('Error in a constructor'); + } + } + } + + @Component({ + template: ``, + }) + class App { + } + + TestBed.configureTestingModule({ + declarations: [App, TestCmpt], + }); + + expect(() => { + TestBed.createComponent(App); + }).toThrowError('Error in a constructor'); + + const fixture = TestBed.createComponent(App); + expect(fixture.nativeElement.textContent).toContain('OKOKOK'); + }); + + it('should continue detecting changes if a directive throws in its constructor', () => { + let firstRun = true; + + @Directive({ + selector: '[failInConstructorOnce]', + }) + class FailInConstructorOnce { + constructor() { + if (firstRun) { + firstRun = false; + throw new Error('Error in a constructor'); + } + } + } + + @Component({ + template: `
{{value}}
`, + }) + class TestCmpt { + value = 0; + } + + TestBed.configureTestingModule({ + declarations: [TestCmpt, FailInConstructorOnce], + }); + + expect(() => { + TestBed.createComponent(TestCmpt); + }).toThrowError('Error in a constructor'); + + const fixture = TestBed.createComponent(TestCmpt); + fixture.detectChanges(); + expect(fixture.nativeElement.textContent).toContain('0'); + + fixture.componentInstance.value = 1; + fixture.detectChanges(); + expect(fixture.nativeElement.textContent).toContain('1'); + + fixture.componentInstance.value = 2; + fixture.detectChanges(); + expect(fixture.nativeElement.textContent).toContain('2'); + }); + }); }); From 77c64ee1f1f6dd089c356af0a5b07873ad1685fa Mon Sep 17 00:00:00 2001 From: crisbeto Date: Tue, 28 Apr 2020 22:01:50 +0200 Subject: [PATCH 2/2] fixup! fix(core): attempt to recover from user errors during creation --- goldens/size-tracking/integration-payloads.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/goldens/size-tracking/integration-payloads.json b/goldens/size-tracking/integration-payloads.json index 0bff4575c24b8..18be2ed2aab73 100644 --- a/goldens/size-tracking/integration-payloads.json +++ b/goldens/size-tracking/integration-payloads.json @@ -30,7 +30,7 @@ "master": { "uncompressed": { "runtime-es2015": 1485, - "main-es2015": 137320, + "main-es2015": 137897, "polyfills-es2015": 37334 } } @@ -49,7 +49,7 @@ "master": { "uncompressed": { "runtime-es2015": 2289, - "main-es2015": 226728, + "main-es2015": 227258, "polyfills-es2015": 36657, "5-es2015": 779 }