Skip to content

Commit 97310d3

Browse files
gkalpakmhevery
gkalpak
authored and
mhevery
committed
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
1 parent 166c783 commit 97310d3

File tree

5 files changed

+152
-21
lines changed

5 files changed

+152
-21
lines changed

‎packages/upgrade/src/common/src/angular1.ts

+1
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,7 @@ export type IAugmentedJQuery = Node[]&{
126126
data?: (name: string, value?: any) => any;
127127
text?: () => string;
128128
inheritedData?: (name: string, value?: any) => any;
129+
children?: () => IAugmentedJQuery;
129130
contents?: () => IAugmentedJQuery;
130131
parent?: () => IAugmentedJQuery;
131132
empty?: () => void;

‎packages/upgrade/src/common/src/downgrade_component_adapter.ts

+29-2
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
import {ApplicationRef, ChangeDetectorRef, ComponentFactory, ComponentRef, EventEmitter, Injector, OnChanges, SimpleChange, SimpleChanges, StaticProvider, Testability, TestabilityRegistry, Type} from '@angular/core';
1010

11-
import {IAttributes, IAugmentedJQuery, ICompileService, INgModelController, IParseService, IScope} from './angular1';
11+
import {element as angularElement, IAttributes, IAugmentedJQuery, ICompileService, INgModelController, IParseService, IScope} from './angular1';
1212
import {PropertyBinding} from './component_info';
1313
import {$SCOPE} from './constants';
1414
import {getTypeName, hookupNgModel, strictEquals} from './util';
@@ -216,11 +216,38 @@ export class DowngradeComponentAdapter {
216216
const destroyComponentRef = this.wrapCallback(() => this.componentRef.destroy());
217217
let destroyed = false;
218218

219-
this.element.on!('$destroy', () => this.componentScope.$destroy());
219+
this.element.on!('$destroy', () => {
220+
// The `$destroy` event may have been triggered by the `cleanData()` call in the
221+
// `componentScope` `$destroy` handler below. In that case, we don't want to call
222+
// `componentScope.$destroy()` again.
223+
if (!destroyed) this.componentScope.$destroy();
224+
});
220225
this.componentScope.$on('$destroy', () => {
221226
if (!destroyed) {
222227
destroyed = true;
223228
testabilityRegistry.unregisterApplication(this.componentRef.location.nativeElement);
229+
230+
// The `componentScope` might be getting destroyed, because an ancestor element is being
231+
// removed/destroyed. If that is the case, jqLite/jQuery would normally invoke `cleanData()`
232+
// on the removed element and all descendants.
233+
// https://github.com/angular/angular.js/blob/2e72ea13fa98bebf6ed4b5e3c45eaf5f990ed16f/src/jqLite.js#L349-L355
234+
// https://github.com/jquery/jquery/blob/6984d1747623dbc5e87fd6c261a5b6b1628c107c/src/manipulation.js#L182
235+
//
236+
// Here, however, `destroyComponentRef()` may under some circumstances remove the element
237+
// from the DOM and therefore it will no longer be a descendant of the removed element when
238+
// `cleanData()` is called. This would result in a memory leak, because the element's data
239+
// and event handlers (and all objects directly or indirectly referenced by them) would be
240+
// retained.
241+
//
242+
// To ensure the element is always properly cleaned up, we manually call `cleanData()` on
243+
// this element and its descendants before destroying the `ComponentRef`.
244+
//
245+
// NOTE:
246+
// `cleanData()` also will invoke the AngularJS `$destroy` event on the element:
247+
// https://github.com/angular/angular.js/blob/2e72ea13fa98bebf6ed4b5e3c45eaf5f990ed16f/src/Angular.js#L1932-L1945
248+
angularElement.cleanData(this.element);
249+
angularElement.cleanData((this.element[0] as Element).querySelectorAll('*'));
250+
224251
destroyComponentRef();
225252
}
226253
});

‎packages/upgrade/src/dynamic/test/upgrade_spec.ts

+35-16
Original file line numberDiff line numberDiff line change
@@ -665,23 +665,16 @@ withEachNg1Version(() => {
665665
it('should properly run cleanup when ng1 directive is destroyed', waitForAsync(() => {
666666
const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module));
667667
const ng1Module = angular.module_('ng1', []);
668-
const onDestroyed: EventEmitter<string> = new EventEmitter<string>();
668+
let ng2ComponentDestroyed = false;
669669

670-
ng1Module.directive('ng1', () => {
671-
return {
672-
template: '<div ng-if="!destroyIt"><ng2></ng2></div>',
673-
controller: function($rootScope: any, $timeout: Function) {
674-
$timeout(() => {
675-
$rootScope.destroyIt = true;
676-
});
677-
}
678-
};
679-
});
670+
ng1Module.directive('ng1', () => ({
671+
template: '<div ng-if="!destroyIt"><ng2></ng2></div>',
672+
}));
680673

681-
@Component({selector: 'ng2', template: 'test'})
674+
@Component({selector: 'ng2', template: '<ul><li>test1</li><li>test2</li></ul>'})
682675
class Ng2 {
683676
ngOnDestroy() {
684-
onDestroyed.emit('destroyed');
677+
ng2ComponentDestroyed = true;
685678
}
686679
}
687680

@@ -695,9 +688,35 @@ withEachNg1Version(() => {
695688
ng1Module.directive('ng2', adapter.downgradeNg2Component(Ng2));
696689
const element = html('<ng1></ng1>');
697690
adapter.bootstrap(element, ['ng1']).ready((ref) => {
698-
onDestroyed.subscribe(() => {
699-
ref.dispose();
700-
});
691+
const ng2Element = angular.element(element.querySelector('ng2') as Element);
692+
const ng2Descendants =
693+
Array.from(element.querySelectorAll('ng2 li')).map(angular.element);
694+
let ng2ElementDestroyed = false;
695+
let ng2DescendantsDestroyed = ng2Descendants.map(() => false);
696+
697+
ng2Element.data!('test', 42);
698+
ng2Descendants.forEach((elem, i) => elem.data!('test', i));
699+
ng2Element.on!('$destroy', () => ng2ElementDestroyed = true);
700+
ng2Descendants.forEach(
701+
(elem, i) => elem.on!('$destroy', () => ng2DescendantsDestroyed[i] = true));
702+
703+
expect(element.textContent).toBe('test1test2');
704+
expect(ng2Element.data!('test')).toBe(42);
705+
ng2Descendants.forEach((elem, i) => expect(elem.data!('test')).toBe(i));
706+
expect(ng2ElementDestroyed).toBe(false);
707+
expect(ng2DescendantsDestroyed).toEqual([false, false]);
708+
expect(ng2ComponentDestroyed).toBe(false);
709+
710+
ref.ng1RootScope.$apply('destroyIt = true');
711+
712+
expect(element.textContent).toBe('');
713+
expect(ng2Element.data!('test')).toBeUndefined();
714+
ng2Descendants.forEach(elem => expect(elem.data!('test')).toBeUndefined());
715+
expect(ng2ElementDestroyed).toBe(true);
716+
expect(ng2DescendantsDestroyed).toEqual([true, true]);
717+
expect(ng2ComponentDestroyed).toBe(true);
718+
719+
ref.dispose();
701720
});
702721
}));
703722

‎packages/upgrade/static/test/integration/downgrade_component_spec.ts

+24-3
Original file line numberDiff line numberDiff line change
@@ -536,7 +536,7 @@ withEachNg1Version(() => {
536536

537537
it('should properly run cleanup when ng1 directive is destroyed', waitForAsync(() => {
538538
let destroyed = false;
539-
@Component({selector: 'ng2', template: 'test'})
539+
@Component({selector: 'ng2', template: '<ul><li>test1</li><li>test2</li></ul>'})
540540
class Ng2Component implements OnDestroy {
541541
ngOnDestroy() {
542542
destroyed = true;
@@ -563,14 +563,35 @@ withEachNg1Version(() => {
563563
platformBrowserDynamic().bootstrapModule(Ng2Module).then((ref) => {
564564
const adapter = ref.injector.get(UpgradeModule) as UpgradeModule;
565565
adapter.bootstrap(element, [ng1Module.name]);
566-
expect(element.textContent).toContain('test');
566+
567+
const ng2Element = angular.element(element.querySelector('ng2') as Element);
568+
const ng2Descendants =
569+
Array.from(element.querySelectorAll('ng2 li')).map(angular.element);
570+
let ng2ElementDestroyed = false;
571+
let ng2DescendantsDestroyed = [false, false];
572+
573+
ng2Element.data!('test', 42);
574+
ng2Descendants.forEach((elem, i) => elem.data!('test', i));
575+
ng2Element.on!('$destroy', () => ng2ElementDestroyed = true);
576+
ng2Descendants.forEach(
577+
(elem, i) => elem.on!('$destroy', () => ng2DescendantsDestroyed[i] = true));
578+
579+
expect(element.textContent).toBe('test1test2');
567580
expect(destroyed).toBe(false);
581+
expect(ng2Element.data!('test')).toBe(42);
582+
ng2Descendants.forEach((elem, i) => expect(elem.data!('test')).toBe(i));
583+
expect(ng2ElementDestroyed).toBe(false);
584+
expect(ng2DescendantsDestroyed).toEqual([false, false]);
568585

569586
const $rootScope = adapter.$injector.get('$rootScope');
570587
$rootScope.$apply('destroyIt = true');
571588

572-
expect(element.textContent).not.toContain('test');
589+
expect(element.textContent).toBe('');
573590
expect(destroyed).toBe(true);
591+
expect(ng2Element.data!('test')).toBeUndefined();
592+
ng2Descendants.forEach(elem => expect(elem.data!('test')).toBeUndefined());
593+
expect(ng2ElementDestroyed).toBe(true);
594+
expect(ng2DescendantsDestroyed).toEqual([true, true]);
574595
});
575596
}));
576597

‎packages/upgrade/static/test/integration/downgrade_module_spec.ts

+63
Original file line numberDiff line numberDiff line change
@@ -1187,6 +1187,69 @@ withEachNg1Version(() => {
11871187
});
11881188
}));
11891189

1190+
it('should properly run cleanup when a downgraded component is destroyed',
1191+
waitForAsync(() => {
1192+
let destroyed = false;
1193+
1194+
@Component({selector: 'ng2', template: '<ul><li>test1</li><li>test2</li></ul>'})
1195+
class Ng2Component implements OnDestroy {
1196+
ngOnDestroy() {
1197+
destroyed = true;
1198+
}
1199+
}
1200+
1201+
@NgModule({
1202+
declarations: [Ng2Component],
1203+
entryComponents: [Ng2Component],
1204+
imports: [BrowserModule],
1205+
})
1206+
class Ng2Module {
1207+
ngDoBootstrap() {}
1208+
}
1209+
1210+
const bootstrapFn = (extraProviders: StaticProvider[]) =>
1211+
platformBrowserDynamic(extraProviders).bootstrapModule(Ng2Module);
1212+
const lazyModuleName = downgradeModule<Ng2Module>(bootstrapFn);
1213+
const ng1Module =
1214+
angular.module_('ng1', [lazyModuleName])
1215+
.directive(
1216+
'ng2', downgradeComponent({component: Ng2Component, propagateDigest}));
1217+
1218+
const element = html('<div><div ng-if="!hideNg2"><ng2></ng2></div></div>');
1219+
const $injector = angular.bootstrap(element, [ng1Module.name]);
1220+
const $rootScope = $injector.get($ROOT_SCOPE) as angular.IRootScopeService;
1221+
1222+
setTimeout(() => { // Wait for the module to be bootstrapped.
1223+
const ng2Element = angular.element(element.querySelector('ng2') as Element);
1224+
const ng2Descendants =
1225+
Array.from(element.querySelectorAll('ng2 li')).map(angular.element);
1226+
let ng2ElementDestroyed = false;
1227+
let ng2DescendantsDestroyed = [false, false];
1228+
1229+
ng2Element.data!('test', 42);
1230+
ng2Descendants.forEach((elem, i) => elem.data!('test', i));
1231+
ng2Element.on!('$destroy', () => ng2ElementDestroyed = true);
1232+
ng2Descendants.forEach(
1233+
(elem, i) => elem.on!('$destroy', () => ng2DescendantsDestroyed[i] = true));
1234+
1235+
expect(element.textContent).toBe('test1test2');
1236+
expect(destroyed).toBe(false);
1237+
expect(ng2Element.data!('test')).toBe(42);
1238+
ng2Descendants.forEach((elem, i) => expect(elem.data!('test')).toBe(i));
1239+
expect(ng2ElementDestroyed).toBe(false);
1240+
expect(ng2DescendantsDestroyed).toEqual([false, false]);
1241+
1242+
$rootScope.$apply('hideNg2 = true');
1243+
1244+
expect(element.textContent).toBe('');
1245+
expect(destroyed).toBe(true);
1246+
expect(ng2Element.data!('test')).toBeUndefined();
1247+
ng2Descendants.forEach(elem => expect(elem.data!('test')).toBeUndefined());
1248+
expect(ng2ElementDestroyed).toBe(true);
1249+
expect(ng2DescendantsDestroyed).toEqual([true, true]);
1250+
});
1251+
}));
1252+
11901253
it('should only retrieve the Angular zone once (and cache it for later use)',
11911254
fakeAsync(() => {
11921255
let count = 0;

0 commit comments

Comments
 (0)