From 5fbfe6996a3815c4d93ba95f53bfefa77db58f71 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Fri, 14 Feb 2020 18:54:10 +0100 Subject: [PATCH] fix(ivy): wrong context passed to ngOnDestroy when resolved multiple times (#35249) When the same provider is resolved multiple times on the same node, the first invocation had the correct context, but all subsequent ones were incorrect because we were registering the hook multiple times under different indexes in `destroyHooks`. Fixes #35167. PR Close #35249 --- packages/core/src/render3/di_setup.ts | 35 +++-- .../core/test/acceptance/providers_spec.ts | 123 ++++++++++++++++++ 2 files changed, 148 insertions(+), 10 deletions(-) diff --git a/packages/core/src/render3/di_setup.ts b/packages/core/src/render3/di_setup.ts index 1666dbcae5..483ae76f44 100644 --- a/packages/core/src/render3/di_setup.ts +++ b/packages/core/src/render3/di_setup.ts @@ -81,26 +81,18 @@ function resolveProvider( const cptViewProvidersCount = tNode.providerIndexes >> TNodeProviderIndexes.CptViewProvidersCountShift; - if (isClassProvider(provider) || isTypeProvider(provider)) { - const prototype = ((provider as ClassProvider).useClass || provider).prototype; - const ngOnDestroy = prototype.ngOnDestroy; - - if (ngOnDestroy) { - (tView.destroyHooks || (tView.destroyHooks = [])).push(tInjectables.length, ngOnDestroy); - } - } - if (isTypeProvider(provider) || !provider.multi) { // Single provider case: the factory is created and pushed immediately const factory = new NodeInjectorFactory(providerFactory, isViewProvider, ɵɵdirectiveInject); const existingFactoryIndex = indexOf( token, tInjectables, isViewProvider ? beginIndex : beginIndex + cptViewProvidersCount, endIndex); - if (existingFactoryIndex == -1) { + if (existingFactoryIndex === -1) { diPublicInInjector( getOrCreateNodeInjectorForNode( tNode as TElementNode | TContainerNode | TElementContainerNode, lView), tView, token); + registerDestroyHooksIfSupported(tView, provider, tInjectables.length); tInjectables.push(token); tNode.directiveStart++; tNode.directiveEnd++; @@ -157,6 +149,7 @@ function resolveProvider( if (!isViewProvider && doesViewProvidersFactoryExist) { lInjectablesBlueprint[existingViewProvidersFactoryIndex].providerFactory = factory; } + registerDestroyHooksIfSupported(tView, provider, tInjectables.length); tInjectables.push(token); tNode.directiveStart++; tNode.directiveEnd++; @@ -167,6 +160,10 @@ function resolveProvider( lView.push(factory); } else { // Cases 1.b and 2.b + registerDestroyHooksIfSupported( + tView, provider, existingProvidersFactoryIndex > -1 ? + existingProvidersFactoryIndex : + existingViewProvidersFactoryIndex); multiFactoryAdd( lInjectablesBlueprint ![isViewProvider ? existingViewProvidersFactoryIndex : existingProvidersFactoryIndex], providerFactory, !isViewProvider && isComponent); @@ -178,6 +175,24 @@ function resolveProvider( } } +/** + * Registers the `ngOnDestroy` hook of a provider, if the provider supports destroy hooks. + * @param tView `TView` in which to register the hook. + * @param provider Provider whose hook should be registered. + * @param contextIndex Index under which to find the context for the hook when it's being invoked. + */ +function registerDestroyHooksIfSupported( + tView: TView, provider: Exclude, contextIndex: number) { + const providerIsTypeProvider = isTypeProvider(provider); + if (providerIsTypeProvider || isClassProvider(provider)) { + const prototype = ((provider as ClassProvider).useClass || provider).prototype; + const ngOnDestroy = prototype.ngOnDestroy; + if (ngOnDestroy) { + (tView.destroyHooks || (tView.destroyHooks = [])).push(contextIndex, ngOnDestroy); + } + } +} + /** * Add a factory in a multi factory. */ diff --git a/packages/core/test/acceptance/providers_spec.ts b/packages/core/test/acceptance/providers_spec.ts index 581b2080f4..2c9e295724 100644 --- a/packages/core/test/acceptance/providers_spec.ts +++ b/packages/core/test/acceptance/providers_spec.ts @@ -223,6 +223,129 @@ describe('providers', () => { expect(logs).toEqual(['OnDestroy Existing']); }); + it('should invoke ngOnDestroy with the correct context when providing a type provider multiple times on the same node', + () => { + const resolvedServices: (DestroyService | undefined)[] = []; + const destroyContexts: (DestroyService | undefined)[] = []; + let parentService: DestroyService|undefined; + let childService: DestroyService|undefined; + + @Injectable() + class DestroyService { + constructor() { resolvedServices.push(this); } + ngOnDestroy() { destroyContexts.push(this); } + } + + @Directive({selector: '[dir-one]', providers: [DestroyService]}) + class DirOne { + constructor(service: DestroyService) { childService = service; } + } + + @Directive({selector: '[dir-two]', providers: [DestroyService]}) + class DirTwo { + constructor(service: DestroyService) { childService = service; } + } + + @Component({template: '
', providers: [DestroyService]}) + class App { + constructor(service: DestroyService) { parentService = service; } + } + + TestBed.configureTestingModule({declarations: [App, DirOne, DirTwo]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + fixture.destroy(); + + expect(parentService).toBeDefined(); + expect(childService).toBeDefined(); + expect(parentService).not.toBe(childService); + expect(resolvedServices).toEqual([parentService, childService]); + expect(destroyContexts).toEqual([parentService, childService]); + }); + + onlyInIvy('Destroy hook of useClass provider is invoked correctly') + .it('should invoke ngOnDestroy with the correct context when providing a class provider multiple times on the same node', + () => { + const resolvedServices: (DestroyService | undefined)[] = []; + const destroyContexts: (DestroyService | undefined)[] = []; + const token = new InjectionToken('token'); + let parentService: DestroyService|undefined; + let childService: DestroyService|undefined; + + @Injectable() + class DestroyService { + constructor() { resolvedServices.push(this); } + ngOnDestroy() { destroyContexts.push(this); } + } + + @Directive( + {selector: '[dir-one]', providers: [{provide: token, useClass: DestroyService}]}) + class DirOne { + constructor(@Inject(token) service: DestroyService) { childService = service; } + } + + @Directive( + {selector: '[dir-two]', providers: [{provide: token, useClass: DestroyService}]}) + class DirTwo { + constructor(@Inject(token) service: DestroyService) { childService = service; } + } + + @Component({ + template: '
', + providers: [{provide: token, useClass: DestroyService}] + }) + class App { + constructor(@Inject(token) service: DestroyService) { parentService = service; } + } + + TestBed.configureTestingModule({declarations: [App, DirOne, DirTwo]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + fixture.destroy(); + + expect(parentService).toBeDefined(); + expect(childService).toBeDefined(); + expect(parentService).not.toBe(childService); + expect(resolvedServices).toEqual([parentService, childService]); + expect(destroyContexts).toEqual([parentService, childService]); + }); + + onlyInIvy('ngOnDestroy hooks for multi providers were not supported in ViewEngine') + .it('should not invoke ngOnDestroy on multi providers', () => { + // TODO(FW-1866): currently we only assert that the hook was called, + // but we should also be checking that the correct context was passed in. + let destroyCalls = 0; + const SERVICES = new InjectionToken('SERVICES'); + + @Injectable() + class DestroyService { + ngOnDestroy() { destroyCalls++; } + } + + @Injectable() + class OtherDestroyService { + ngOnDestroy() { destroyCalls++; } + } + + @Component({ + template: '
', + providers: [ + {provide: SERVICES, useClass: DestroyService, multi: true}, + {provide: SERVICES, useClass: OtherDestroyService, multi: true}, + ] + }) + class App { + constructor(@Inject(SERVICES) s: any) {} + } + + TestBed.configureTestingModule({declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + fixture.destroy(); + + expect(destroyCalls).toBe(2); + }); + }); describe('components and directives', () => {