From d3bb56c8c5906fde13878b3f245a5e190ab8765a Mon Sep 17 00:00:00 2001 From: arturovt Date: Tue, 1 Dec 2020 23:45:12 +0200 Subject: [PATCH] fix(upgrade): fix memory leaks when the component is destroyed Currently we're storing the real injector on the element inside the `ParentInjectorPromise` and its `resolve()` method. This causes a memory leak if we don't remove the reference when the component is destroyed. We also setup the `$destroy` listener and never release it, the problem is that this listener captures `this`, as long as there is a reference, `this` will not be GC'd. PR Close #39911 --- packages/upgrade/src/common/src/angular1.ts | 4 +- .../common/src/downgrade_component_adapter.ts | 59 +++++++++++----- .../integration/downgrade_component_spec.ts | 67 +++++++++++++++++++ 3 files changed, 111 insertions(+), 19 deletions(-) diff --git a/packages/upgrade/src/common/src/angular1.ts b/packages/upgrade/src/common/src/angular1.ts index b62bf6b71832..21753fa4f666 100644 --- a/packages/upgrade/src/common/src/angular1.ts +++ b/packages/upgrade/src/common/src/angular1.ts @@ -123,6 +123,7 @@ export interface ICloneAttachFunction { } export type IAugmentedJQuery = Node[]&{ on?: (name: string, fn: () => void) => void; + off?: (name: string) => void; data?: (name: string, value?: any) => any; text?: () => string; inheritedData?: (name: string, value?: any) => any; @@ -135,7 +136,8 @@ export type IAugmentedJQuery = Node[]&{ injector?: () => IInjectorService; triggerHandler?: (eventTypeOrObject: string|Event, extraParameters?: any[]) => IAugmentedJQuery; remove?: () => void; - removeData?: () => void; + removeData?: (name: string) => void; + scope?: () => IScope; }; export interface IProvider { $get: IInjectable; diff --git a/packages/upgrade/src/common/src/downgrade_component_adapter.ts b/packages/upgrade/src/common/src/downgrade_component_adapter.ts index 152df408eb44..4919ef84136b 100644 --- a/packages/upgrade/src/common/src/downgrade_component_adapter.ts +++ b/packages/upgrade/src/common/src/downgrade_component_adapter.ts @@ -10,8 +10,8 @@ import {ApplicationRef, ChangeDetectorRef, ComponentFactory, ComponentRef, Event import {IAttributes, IAugmentedJQuery, ICompileService, IInjectorService, INgModelController, IParseService, IScope} from './angular1'; import {PropertyBinding} from './component_info'; -import {$SCOPE} from './constants'; -import {getTypeName, hookupNgModel, strictEquals} from './util'; +import {$SCOPE, INJECTOR_KEY} from './constants'; +import {controllerKey, getTypeName, hookupNgModel, strictEquals} from './util'; const INITIAL_VALUE = { __UNINITIALIZED__: true @@ -29,6 +29,7 @@ export class DowngradeComponentAdapter { private changeDetector!: ChangeDetectorRef; // TODO(issue/24571): remove '!'. private viewChangeDetector!: ChangeDetectorRef; + private unwatchFns: Function[] = []; constructor( private element: IAugmentedJQuery, private attrs: IAttributes, private scope: IScope, @@ -126,7 +127,8 @@ export class DowngradeComponentAdapter { const watchFn = (prop => (currValue: any, prevValue: any) => this.updateInput(prop, prevValue, currValue))(input.prop); - this.componentScope.$watch(expr, watchFn); + const unwatch = this.componentScope.$watch(expr, watchFn); + this.unwatchFns.push(unwatch); } } @@ -135,25 +137,28 @@ export class DowngradeComponentAdapter { const prototype = this.componentFactory.componentType.prototype; this.implementsOnChanges = !!(prototype && (prototype).ngOnChanges); - this.componentScope.$watch(() => this.inputChangeCount, this.wrapCallback(() => { - // Invoke `ngOnChanges()` - if (this.implementsOnChanges) { - const inputChanges = this.inputChanges; - this.inputChanges = {}; - (this.component).ngOnChanges(inputChanges!); - } + const unwatch = + this.componentScope.$watch(() => this.inputChangeCount, this.wrapCallback(() => { + // Invoke `ngOnChanges()` + if (this.implementsOnChanges) { + const inputChanges = this.inputChanges; + this.inputChanges = {}; + (this.component).ngOnChanges(inputChanges!); + } - this.viewChangeDetector.markForCheck(); + this.viewChangeDetector.markForCheck(); - // If opted out of propagating digests, invoke change detection when inputs change. - if (!propagateDigest) { - detectChanges(); - } - })); + // If opted out of propagating digests, invoke change detection when inputs change. + if (!propagateDigest) { + detectChanges(); + } + })); + this.unwatchFns.push(unwatch); // If not opted out of propagating digests, invoke change detection on every digest if (propagateDigest) { - this.componentScope.$watch(this.wrapCallback(detectChanges)); + const unwatch = this.componentScope.$watch(this.wrapCallback(detectChanges)); + this.unwatchFns.push(unwatch); } // If necessary, attach the view so that it will be dirty-checked. @@ -217,11 +222,29 @@ export class DowngradeComponentAdapter { let destroyed = false; this.element.on!('$destroy', () => this.componentScope.$destroy()); - this.componentScope.$on('$destroy', () => { + const removeOnDestroyListenerFn = this.componentScope.$on('$destroy', () => { if (!destroyed) { destroyed = true; testabilityRegistry.unregisterApplication(this.componentRef.location.nativeElement); destroyComponentRef(); + // We're storing the real injector on the element inside the `ParentInjectorPromise` + // and its `resolve()` method. This causes a memory leak if we don't + // remove the reference when the component is destroyed. + this.element.removeData!(controllerKey(INJECTOR_KEY)); + // We can't call `element.remove()` during the digest cycle since it will throw + // a runtime error, but we have to de-register the `$destroy` listener. + // The `$destroy` listener that we added previously on the element captures `this`, + // as long as there is a reference to `() => this.componentScope.$destroy()`, + // `this` will not be GC'd. + this.element.off!('$destroy'); + // We also have captured `this` in that callback and it's not removed automatically + // by the `componentScope`. + removeOnDestroyListenerFn(); + // This will clear all watchers inside the `$$watchers` property since all those watchers + // capture `this` inside their callbacks. + while (this.unwatchFns.length) { + this.unwatchFns.pop()!(); + } } }); } diff --git a/packages/upgrade/static/test/integration/downgrade_component_spec.ts b/packages/upgrade/static/test/integration/downgrade_component_spec.ts index e5ad0978b4f4..77ed6ef04505 100644 --- a/packages/upgrade/static/test/integration/downgrade_component_spec.ts +++ b/packages/upgrade/static/test/integration/downgrade_component_spec.ts @@ -8,6 +8,7 @@ import {ChangeDetectionStrategy, Compiler, Component, destroyPlatform, Directive, ElementRef, EventEmitter, Injector, Input, NgModule, NgModuleRef, OnChanges, OnDestroy, Output, SimpleChanges} from '@angular/core'; import {fakeAsync, tick, waitForAsync} from '@angular/core/testing'; +import {expect} from '@angular/core/testing/src/testing_internal'; import {BrowserModule} from '@angular/platform-browser'; import {platformBrowserDynamic} from '@angular/platform-browser-dynamic'; import {downgradeComponent, UpgradeComponent, UpgradeModule} from '@angular/upgrade/static'; @@ -627,6 +628,72 @@ withEachNg1Version(() => { }); })); + it('should remove the data from the element', waitForAsync(() => { + @Component({selector: 'ng2', template: ''}) + class Ng2Component { + } + + @NgModule({ + declarations: [Ng2Component], + entryComponents: [Ng2Component], + imports: [BrowserModule, UpgradeModule], + }) + class Ng2Module { + ngDoBootstrap() {} + } + + const ng1Module = angular.module_('ng1', []).directive( + 'ng2', downgradeComponent({component: Ng2Component})); + + const element = html(''); + + bootstrap(platformBrowserDynamic(), Ng2Module, element, ng1Module).then(upgrade => { + const ng2Element = angular.element(element); + expect(ng2Element.data!('$$$angularInjectorController')).toBeTruthy(); + + const $rootScope = upgrade.$injector.get('$rootScope') as angular.IRootScopeService; + $rootScope.$destroy(); + + expect(ng2Element.data!('$$$angularInjectorController')).toBeUndefined(); + }); + })); + + it('should cleanup $$watchers and $$listeners on the scope', waitForAsync(() => { + @Component({selector: 'ng2', template: ''}) + class Ng2Component { + } + + @NgModule({ + declarations: [Ng2Component], + entryComponents: [Ng2Component], + imports: [BrowserModule, UpgradeModule], + }) + class Ng2Module { + ngDoBootstrap() {} + } + + const ng1Module = angular.module_('ng1', []).directive( + 'ng2', downgradeComponent({component: Ng2Component})); + + const element = html(''); + + bootstrap(platformBrowserDynamic(), Ng2Module, element, ng1Module).then(upgrade => { + const scope = angular.element(element).scope!(); + + // We don't know how many watchers there can be, but let's ensure + // that there is at least 1 watcher. + expect(scope.$$watchersCount).toBeGreaterThan(0); + // The same thing with listeners. + expect(Object.keys(scope.$$listenerCount).length).toBeGreaterThan(0); + + const $rootScope = upgrade.$injector.get('$rootScope') as angular.IRootScopeService; + $rootScope.$destroy(); + + expect(scope.$$watchersCount).toEqual(0); + expect(Object.keys(scope.$$listenerCount).length).toEqual(0); + }); + })); + it('should work when compiled outside the dom (by fallback to the root ng2.injector)', waitForAsync(() => { @Component({selector: 'ng2', template: 'test'})