From 6dc43a475bf72a81ddc7d226f722488b04ed2582 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Mon, 7 Dec 2020 19:55:34 +0200 Subject: [PATCH] fix(upgrade): avoid memory leak when removing downgraded components (#39965) Previously, due to the way the AngularJS and Angular clean-up processes interfere with each other when removing an AngularJS element that contains a downgraded Angular component, the data associated with the host element of the downgraded component was not removed. This data was kept in an internal AngularJS cache, which prevented the element and component instance from being garbage-collected, leading to memory leaks. This commit fixes this by ensuring the element data is explicitly removed when cleaning up a downgraded component. NOTE: This is essentially the equivalent of #26209 but for downgraded (instead of upgraded) components. Fixes #39911 Closes #39921 PR Close #39965 --- packages/upgrade/src/common/src/angular1.ts | 1 + .../common/src/downgrade_component_adapter.ts | 31 ++++++++- .../upgrade/src/dynamic/test/upgrade_spec.ts | 51 ++++++++++----- .../integration/downgrade_component_spec.ts | 27 +++++++- .../test/integration/downgrade_module_spec.ts | 63 +++++++++++++++++++ 5 files changed, 152 insertions(+), 21 deletions(-) diff --git a/packages/upgrade/src/common/src/angular1.ts b/packages/upgrade/src/common/src/angular1.ts index b62bf6b718..b4a8b94d12 100644 --- a/packages/upgrade/src/common/src/angular1.ts +++ b/packages/upgrade/src/common/src/angular1.ts @@ -126,6 +126,7 @@ export type IAugmentedJQuery = Node[]&{ data?: (name: string, value?: any) => any; text?: () => string; inheritedData?: (name: string, value?: any) => any; + children?: () => IAugmentedJQuery; contents?: () => IAugmentedJQuery; parent?: () => IAugmentedJQuery; empty?: () => void; diff --git a/packages/upgrade/src/common/src/downgrade_component_adapter.ts b/packages/upgrade/src/common/src/downgrade_component_adapter.ts index 91b84ab676..5022a58aac 100644 --- a/packages/upgrade/src/common/src/downgrade_component_adapter.ts +++ b/packages/upgrade/src/common/src/downgrade_component_adapter.ts @@ -8,7 +8,7 @@ import {ApplicationRef, ChangeDetectorRef, ComponentFactory, ComponentRef, EventEmitter, Injector, OnChanges, SimpleChange, SimpleChanges, StaticProvider, Testability, TestabilityRegistry, Type} from '@angular/core'; -import {IAttributes, IAugmentedJQuery, ICompileService, INgModelController, IParseService, IScope} from './angular1'; +import {element as angularElement, IAttributes, IAugmentedJQuery, ICompileService, INgModelController, IParseService, IScope} from './angular1'; import {PropertyBinding} from './component_info'; import {$SCOPE} from './constants'; import {getTypeName, hookupNgModel, strictEquals} from './util'; @@ -216,11 +216,38 @@ export class DowngradeComponentAdapter { const destroyComponentRef = this.wrapCallback(() => this.componentRef.destroy()); let destroyed = false; - this.element.on!('$destroy', () => this.componentScope.$destroy()); + this.element.on!('$destroy', () => { + // The `$destroy` event may have been triggered by the `cleanData()` call in the + // `componentScope` `$destroy` handler below. In that case, we don't want to call + // `componentScope.$destroy()` again. + if (!destroyed) this.componentScope.$destroy(); + }); this.componentScope.$on('$destroy', () => { if (!destroyed) { destroyed = true; testabilityRegistry.unregisterApplication(this.componentRef.location.nativeElement); + + // The `componentScope` might be getting destroyed, because an ancestor element is being + // removed/destroyed. If that is the case, jqLite/jQuery would normally invoke `cleanData()` + // on the removed element and all descendants. + // https://github.com/angular/angular.js/blob/2e72ea13fa98bebf6ed4b5e3c45eaf5f990ed16f/src/jqLite.js#L349-L355 + // https://github.com/jquery/jquery/blob/6984d1747623dbc5e87fd6c261a5b6b1628c107c/src/manipulation.js#L182 + // + // Here, however, `destroyComponentRef()` may under some circumstances remove the element + // from the DOM and therefore it will no longer be a descendant of the removed element when + // `cleanData()` is called. This would result in a memory leak, because the element's data + // and event handlers (and all objects directly or indirectly referenced by them) would be + // retained. + // + // To ensure the element is always properly cleaned up, we manually call `cleanData()` on + // this element and its descendants before destroying the `ComponentRef`. + // + // NOTE: + // `cleanData()` also will invoke the AngularJS `$destroy` event on the element: + // https://github.com/angular/angular.js/blob/2e72ea13fa98bebf6ed4b5e3c45eaf5f990ed16f/src/Angular.js#L1932-L1945 + angularElement.cleanData(this.element); + angularElement.cleanData((this.element[0] as Element).querySelectorAll('*')); + destroyComponentRef(); } }); diff --git a/packages/upgrade/src/dynamic/test/upgrade_spec.ts b/packages/upgrade/src/dynamic/test/upgrade_spec.ts index 9ab7ba7ff8..3d33cc59be 100644 --- a/packages/upgrade/src/dynamic/test/upgrade_spec.ts +++ b/packages/upgrade/src/dynamic/test/upgrade_spec.ts @@ -665,23 +665,16 @@ withEachNg1Version(() => { it('should properly run cleanup when ng1 directive is destroyed', waitForAsync(() => { const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module)); const ng1Module = angular.module_('ng1', []); - const onDestroyed: EventEmitter = new EventEmitter(); + let ng2ComponentDestroyed = false; - ng1Module.directive('ng1', () => { - return { - template: '
', - controller: function($rootScope: any, $timeout: Function) { - $timeout(() => { - $rootScope.destroyIt = true; - }); - } - }; - }); + ng1Module.directive('ng1', () => ({ + template: '
', + })); - @Component({selector: 'ng2', template: 'test'}) + @Component({selector: 'ng2', template: '
  • test1
  • test2
'}) class Ng2 { ngOnDestroy() { - onDestroyed.emit('destroyed'); + ng2ComponentDestroyed = true; } } @@ -695,9 +688,35 @@ withEachNg1Version(() => { ng1Module.directive('ng2', adapter.downgradeNg2Component(Ng2)); const element = html(''); adapter.bootstrap(element, ['ng1']).ready((ref) => { - onDestroyed.subscribe(() => { - ref.dispose(); - }); + const ng2Element = angular.element(element.querySelector('ng2') as Element); + const ng2Descendants = + Array.from(element.querySelectorAll('ng2 li')).map(angular.element); + let ng2ElementDestroyed = false; + let ng2DescendantsDestroyed = ng2Descendants.map(() => false); + + ng2Element.data!('test', 42); + ng2Descendants.forEach((elem, i) => elem.data!('test', i)); + ng2Element.on!('$destroy', () => ng2ElementDestroyed = true); + ng2Descendants.forEach( + (elem, i) => elem.on!('$destroy', () => ng2DescendantsDestroyed[i] = true)); + + expect(element.textContent).toBe('test1test2'); + expect(ng2Element.data!('test')).toBe(42); + ng2Descendants.forEach((elem, i) => expect(elem.data!('test')).toBe(i)); + expect(ng2ElementDestroyed).toBe(false); + expect(ng2DescendantsDestroyed).toEqual([false, false]); + expect(ng2ComponentDestroyed).toBe(false); + + ref.ng1RootScope.$apply('destroyIt = true'); + + expect(element.textContent).toBe(''); + expect(ng2Element.data!('test')).toBeUndefined(); + ng2Descendants.forEach(elem => expect(elem.data!('test')).toBeUndefined()); + expect(ng2ElementDestroyed).toBe(true); + expect(ng2DescendantsDestroyed).toEqual([true, true]); + expect(ng2ComponentDestroyed).toBe(true); + + ref.dispose(); }); })); diff --git a/packages/upgrade/static/test/integration/downgrade_component_spec.ts b/packages/upgrade/static/test/integration/downgrade_component_spec.ts index e5ad0978b4..ec88d423d1 100644 --- a/packages/upgrade/static/test/integration/downgrade_component_spec.ts +++ b/packages/upgrade/static/test/integration/downgrade_component_spec.ts @@ -536,7 +536,7 @@ withEachNg1Version(() => { it('should properly run cleanup when ng1 directive is destroyed', waitForAsync(() => { let destroyed = false; - @Component({selector: 'ng2', template: 'test'}) + @Component({selector: 'ng2', template: '
  • test1
  • test2
'}) class Ng2Component implements OnDestroy { ngOnDestroy() { destroyed = true; @@ -563,14 +563,35 @@ withEachNg1Version(() => { platformBrowserDynamic().bootstrapModule(Ng2Module).then((ref) => { const adapter = ref.injector.get(UpgradeModule) as UpgradeModule; adapter.bootstrap(element, [ng1Module.name]); - expect(element.textContent).toContain('test'); + + const ng2Element = angular.element(element.querySelector('ng2') as Element); + const ng2Descendants = + Array.from(element.querySelectorAll('ng2 li')).map(angular.element); + let ng2ElementDestroyed = false; + let ng2DescendantsDestroyed = [false, false]; + + ng2Element.data!('test', 42); + ng2Descendants.forEach((elem, i) => elem.data!('test', i)); + ng2Element.on!('$destroy', () => ng2ElementDestroyed = true); + ng2Descendants.forEach( + (elem, i) => elem.on!('$destroy', () => ng2DescendantsDestroyed[i] = true)); + + expect(element.textContent).toBe('test1test2'); expect(destroyed).toBe(false); + expect(ng2Element.data!('test')).toBe(42); + ng2Descendants.forEach((elem, i) => expect(elem.data!('test')).toBe(i)); + expect(ng2ElementDestroyed).toBe(false); + expect(ng2DescendantsDestroyed).toEqual([false, false]); const $rootScope = adapter.$injector.get('$rootScope'); $rootScope.$apply('destroyIt = true'); - expect(element.textContent).not.toContain('test'); + expect(element.textContent).toBe(''); expect(destroyed).toBe(true); + expect(ng2Element.data!('test')).toBeUndefined(); + ng2Descendants.forEach(elem => expect(elem.data!('test')).toBeUndefined()); + expect(ng2ElementDestroyed).toBe(true); + expect(ng2DescendantsDestroyed).toEqual([true, true]); }); })); diff --git a/packages/upgrade/static/test/integration/downgrade_module_spec.ts b/packages/upgrade/static/test/integration/downgrade_module_spec.ts index 020081077f..cc34ec57b7 100644 --- a/packages/upgrade/static/test/integration/downgrade_module_spec.ts +++ b/packages/upgrade/static/test/integration/downgrade_module_spec.ts @@ -1187,6 +1187,69 @@ withEachNg1Version(() => { }); })); + it('should properly run cleanup when a downgraded component is destroyed', + waitForAsync(() => { + let destroyed = false; + + @Component({selector: 'ng2', template: '
  • test1
  • test2
'}) + class Ng2Component implements OnDestroy { + ngOnDestroy() { + destroyed = true; + } + } + + @NgModule({ + declarations: [Ng2Component], + entryComponents: [Ng2Component], + imports: [BrowserModule], + }) + class Ng2Module { + ngDoBootstrap() {} + } + + const bootstrapFn = (extraProviders: StaticProvider[]) => + platformBrowserDynamic(extraProviders).bootstrapModule(Ng2Module); + const lazyModuleName = downgradeModule(bootstrapFn); + const ng1Module = + angular.module_('ng1', [lazyModuleName]) + .directive( + 'ng2', downgradeComponent({component: Ng2Component, propagateDigest})); + + const element = html('
'); + const $injector = angular.bootstrap(element, [ng1Module.name]); + const $rootScope = $injector.get($ROOT_SCOPE) as angular.IRootScopeService; + + setTimeout(() => { // Wait for the module to be bootstrapped. + const ng2Element = angular.element(element.querySelector('ng2') as Element); + const ng2Descendants = + Array.from(element.querySelectorAll('ng2 li')).map(angular.element); + let ng2ElementDestroyed = false; + let ng2DescendantsDestroyed = [false, false]; + + ng2Element.data!('test', 42); + ng2Descendants.forEach((elem, i) => elem.data!('test', i)); + ng2Element.on!('$destroy', () => ng2ElementDestroyed = true); + ng2Descendants.forEach( + (elem, i) => elem.on!('$destroy', () => ng2DescendantsDestroyed[i] = true)); + + expect(element.textContent).toBe('test1test2'); + expect(destroyed).toBe(false); + expect(ng2Element.data!('test')).toBe(42); + ng2Descendants.forEach((elem, i) => expect(elem.data!('test')).toBe(i)); + expect(ng2ElementDestroyed).toBe(false); + expect(ng2DescendantsDestroyed).toEqual([false, false]); + + $rootScope.$apply('hideNg2 = true'); + + expect(element.textContent).toBe(''); + expect(destroyed).toBe(true); + expect(ng2Element.data!('test')).toBeUndefined(); + ng2Descendants.forEach(elem => expect(elem.data!('test')).toBeUndefined()); + expect(ng2ElementDestroyed).toBe(true); + expect(ng2DescendantsDestroyed).toEqual([true, true]); + }); + })); + it('should only retrieve the Angular zone once (and cache it for later use)', fakeAsync(() => { let count = 0;