Home Original page

fix(upgrade): avoid memory leak when removing downgraded components (… · angular/angular@97310d3

5 files changed

lines changed

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;

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

});

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
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
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;