From 32c760f5e7bb13356d1a0319687d2afc4537dc0f Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Mon, 24 Jun 2019 13:14:05 -0700 Subject: [PATCH] fix(ivy): don't mask errors by calling lifecycle hooks after a crash (#31244) The Angular runtime frequently calls into user code (for example, when writing to a property binding). Since user code can throw errors, calls to it are frequently wrapped in a try-finally block. In Ivy, the following pattern is common: ```typescript enterView(); try { callUserCode(); } finally { leaveView(); } ``` This has a significant problem, however: `leaveView` has a side effect: it calls any pending lifecycle hooks that might've been scheduled during the current round of change detection. Generally it's a bad idea to run lifecycle hooks after the application has crashed. The application is in an inconsistent state - directives may not be instantiated fully, queries may not be resolved, bindings may not have been applied, etc. Invariants that the app code relies upon may not hold. Further crashes or broken behavior are likely. Frequently, lifecycle hooks are used to make assertions about these invariants. When these assertions fail, they will throw and "swallow" the original error, making debugging of the problem much more difficult. This commit modifies `leaveView` to understand whether the application is currently crashing, via a parameter `safeToRunHooks`. This parameter is set by modifying the above pattern: ```typescript enterView(); let safeToRunHooks = false; try { callUserCode(); safeToRunHooks = true; } finally { leaveView(..., safeToRunHooks); } ``` If `callUserCode` crashes, then `safeToRunHooks` will never be set to `true` and `leaveView` won't call any further user code. The original error will then propagate back up the stack and be reported correctly. A test is added to verify this behavior. PR Close #31244 --- integration/_payload-limits.json | 2 +- packages/core/src/render3/component.ts | 6 +++- packages/core/src/render3/component_ref.ts | 6 +++- .../src/render3/instructions/embedded_view.ts | 5 ++- .../core/src/render3/instructions/shared.ts | 16 ++++++++-- packages/core/src/render3/state.ts | 11 ++++--- .../core/test/acceptance/integration_spec.ts | 32 ++++++++++++++++++- packages/core/test/render3/di_spec.ts | 4 ++- 8 files changed, 69 insertions(+), 13 deletions(-) diff --git a/integration/_payload-limits.json b/integration/_payload-limits.json index 35c68b565d..7a79c70b08 100644 --- a/integration/_payload-limits.json +++ b/integration/_payload-limits.json @@ -12,7 +12,7 @@ "master": { "uncompressed": { "runtime": 1440, - "main": 14664, + "main": 14847, "polyfills": 43567 } } diff --git a/packages/core/src/render3/component.ts b/packages/core/src/render3/component.ts index ed6cd447e7..41e0007709 100644 --- a/packages/core/src/render3/component.ts +++ b/packages/core/src/render3/component.ts @@ -136,6 +136,9 @@ export function renderComponent( const oldView = enterView(rootView, null); let component: T; + + // Will become true if the `try` block executes with no errors. + let safeToRunHooks = false; try { if (rendererFactory.begin) rendererFactory.begin(); const componentView = createRootComponentView( @@ -149,8 +152,9 @@ export function renderComponent( rootView[FLAGS] &= ~LViewFlags.CreationMode; resetPreOrderHookFlags(rootView); refreshDescendantViews(rootView); // update mode pass + safeToRunHooks = true; } finally { - leaveView(oldView); + leaveView(oldView, safeToRunHooks); if (rendererFactory.end) rendererFactory.end(); } diff --git a/packages/core/src/render3/component_ref.ts b/packages/core/src/render3/component_ref.ts index 13da2b898d..a3d8ad2131 100644 --- a/packages/core/src/render3/component_ref.ts +++ b/packages/core/src/render3/component_ref.ts @@ -177,6 +177,9 @@ export class ComponentFactory extends viewEngine_ComponentFactory { let component: T; let tElementNode: TElementNode; + + // Will become true if the `try` block executes with no errors. + let safeToRunHooks = false; try { const componentView = createRootComponentView( hostRNode, this.componentDef, rootLView, rendererFactory, renderer); @@ -199,8 +202,9 @@ export class ComponentFactory extends viewEngine_ComponentFactory { addToViewTree(rootLView, componentView); refreshDescendantViews(rootLView); + safeToRunHooks = true; } finally { - leaveView(oldLView); + leaveView(oldLView, safeToRunHooks); } const componentRef = new ComponentRef( diff --git a/packages/core/src/render3/instructions/embedded_view.ts b/packages/core/src/render3/instructions/embedded_view.ts index e805c59143..94d6d2f1b3 100644 --- a/packages/core/src/render3/instructions/embedded_view.ts +++ b/packages/core/src/render3/instructions/embedded_view.ts @@ -140,6 +140,9 @@ export function ɵɵembeddedViewEnd(): void { refreshDescendantViews(lView); // update mode pass const lContainer = lView[PARENT] as LContainer; ngDevMode && assertLContainerOrUndefined(lContainer); - leaveView(lContainer[PARENT] !); + // It's always safe to run hooks here, as `leaveView` is not called during the 'finally' block + // of a try-catch-finally statement, so it can never be reached while unwinding the stack due to + // an error being thrown. + leaveView(lContainer[PARENT] !, /* safeToRunHooks */ true); setPreviousOrParentTNode(viewHost !, false); } diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 41690362cc..3d215c4ad2 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -390,6 +390,8 @@ export function renderEmbeddedTemplate(viewToRender: LView, tView: TView, con // This is a root view inside the view tree tickRootContext(getRootContext(viewToRender)); } else { + // Will become true if the `try` block executes with no errors. + let safeToRunHooks = false; try { setPreviousOrParentTNode(null !, true); @@ -404,8 +406,9 @@ export function renderEmbeddedTemplate(viewToRender: LView, tView: TView, con viewToRender[TVIEW].firstTemplatePass = false; refreshDescendantViews(viewToRender); + safeToRunHooks = true; } finally { - leaveView(oldView !); + leaveView(oldView !, safeToRunHooks); setPreviousOrParentTNode(_previousOrParentTNode, _isParent); } } @@ -417,6 +420,9 @@ export function renderComponentOrTemplate( const oldView = enterView(hostView, hostView[T_HOST]); const normalExecutionPath = !getCheckNoChangesMode(); const creationModeIsActive = isCreationMode(hostView); + + // Will become true if the `try` block executes with no errors. + let safeToRunHooks = false; try { if (normalExecutionPath && !creationModeIsActive && rendererFactory.begin) { rendererFactory.begin(); @@ -434,11 +440,12 @@ export function renderComponentOrTemplate( resetPreOrderHookFlags(hostView); templateFn && executeTemplate(hostView, templateFn, RenderFlags.Update, context); refreshDescendantViews(hostView); + safeToRunHooks = true; } finally { if (normalExecutionPath && !creationModeIsActive && rendererFactory.end) { rendererFactory.end(); } - leaveView(oldView); + leaveView(oldView, safeToRunHooks); } } @@ -1724,6 +1731,8 @@ export function checkView(hostView: LView, component: T) { const templateFn = hostTView.template !; const creationMode = isCreationMode(hostView); + // Will become true if the `try` block executes with no errors. + let safeToRunHooks = false; try { resetPreOrderHookFlags(hostView); creationMode && executeViewQueryFn(RenderFlags.Create, hostTView, component); @@ -1733,8 +1742,9 @@ export function checkView(hostView: LView, component: T) { if (!creationMode || hostTView.staticViewQueries) { executeViewQueryFn(RenderFlags.Update, hostTView, component); } + safeToRunHooks = true; } finally { - leaveView(oldView); + leaveView(oldView, safeToRunHooks); } } diff --git a/packages/core/src/render3/state.ts b/packages/core/src/render3/state.ts index 582d77e7bc..4d89ee54ac 100644 --- a/packages/core/src/render3/state.ts +++ b/packages/core/src/render3/state.ts @@ -465,17 +465,20 @@ export function resetComponentState() { * the direction of traversal (up or down the view tree) a bit clearer. * * @param newView New state to become active + * @param safeToRunHooks Whether the runtime is in a state where running lifecycle hooks is valid. + * This is not always the case (for example, the application may have crashed and `leaveView` is + * being executed while unwinding the call stack). */ -export function leaveView(newView: LView): void { +export function leaveView(newView: LView, safeToRunHooks: boolean): void { const tView = lView[TVIEW]; if (isCreationMode(lView)) { lView[FLAGS] &= ~LViewFlags.CreationMode; } else { try { resetPreOrderHookFlags(lView); - executeHooks( - lView, tView.viewHooks, tView.viewCheckHooks, checkNoChangesMode, - InitPhaseState.AfterViewInitHooksToBeRun, undefined); + safeToRunHooks && executeHooks( + lView, tView.viewHooks, tView.viewCheckHooks, checkNoChangesMode, + InitPhaseState.AfterViewInitHooksToBeRun, undefined); } finally { // Views are clean and in update mode after being checked, so these bits are cleared lView[FLAGS] &= ~(LViewFlags.Dirty | LViewFlags.FirstLViewPass); diff --git a/packages/core/test/acceptance/integration_spec.ts b/packages/core/test/acceptance/integration_spec.ts index 2a59030167..cd4942877e 100644 --- a/packages/core/test/acceptance/integration_spec.ts +++ b/packages/core/test/acceptance/integration_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ import {CommonModule} from '@angular/common'; -import {Component, ContentChild, Directive, ElementRef, EventEmitter, HostBinding, HostListener, Input, OnInit, Output, QueryList, TemplateRef, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core'; +import {Component, ContentChild, Directive, ElementRef, EventEmitter, HostBinding, HostListener, Input, NgModule, OnInit, Output, QueryList, TemplateRef, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core'; import {ngDevModeResetPerfCounters} from '@angular/core/src/util/ng_dev_mode'; import {TestBed} from '@angular/core/testing'; import {By} from '@angular/platform-browser'; @@ -1737,4 +1737,34 @@ describe('acceptance integration tests', () => { expect(clicks).toBe(1); }); + it('should not mask errors thrown during lifecycle hooks', () => { + @Directive({ + selector: '[dir]', + inputs: ['dir'], + }) + class Dir { + get dir(): any { return null; } + + set dir(value: any) { throw new Error('this error is expected'); } + } + + @Component({ + template: '
', + }) + class Cmp { + ngAfterViewInit(): void { + // This lifecycle hook should never run, since attempting to bind to Dir's input will throw + // an error. If the runtime continues to run lifecycle hooks after that error, then it will + // execute this hook and throw this error, which will mask the real problem. This test + // verifies this don't happen. + throw new Error('this error is unexpected'); + } + } + + TestBed.configureTestingModule({ + declarations: [Cmp, Dir], + }); + const fixture = TestBed.createComponent(Cmp); + expect(() => fixture.detectChanges()).toThrowError('this error is expected'); + }); }); diff --git a/packages/core/test/render3/di_spec.ts b/packages/core/test/render3/di_spec.ts index 3ccc9ab7a2..1cf4569217 100644 --- a/packages/core/test/render3/di_spec.ts +++ b/packages/core/test/render3/di_spec.ts @@ -607,6 +607,7 @@ describe('di', () => { null, createTView(-1, null, 1, 0, null, null, null, null), null, LViewFlags.CheckAlways, null, null, {} as any, {} as any); const oldView = enterView(contentView, null); + let safeToRunHooks = false; try { const parentTNode = getOrCreateTNode(contentView[TVIEW], null, 0, TNodeType.Element, null, null); @@ -617,8 +618,9 @@ describe('di', () => { const injector = getOrCreateNodeInjectorForNode(parentTNode, contentView); expect(injector).not.toEqual(-1); + safeToRunHooks = true; } finally { - leaveView(oldView); + leaveView(oldView, safeToRunHooks); } }); });