GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix(upgrade): avoid memory leak when removing downgraded components (… · angular/angular@97310d3

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
  • packages/upgrade
    • src
      • common/src
        • angular1.ts
        • downgrade_component_adapter.ts
      • dynamic/test
        • upgrade_spec.ts
    • static/test/integration
      • downgrade_component_spec.ts
      • downgrade_module_spec.ts
Expand file treeCollapse file tree
Open diff view settings
Collapse file

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

Copy file name to clipboardExpand all lines: packages/upgrade/src/common/src/angular1.ts
+1Lines changed: 1 addition & 0 deletions
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;
Collapse file

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

Copy file name to clipboardExpand all lines: packages/upgrade/src/common/src/downgrade_component_adapter.ts
+29-2Lines changed: 29 additions & 2 deletions
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
});
Collapse file

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

Copy file name to clipboardExpand all lines: packages/upgrade/src/dynamic/test/upgrade_spec.ts
+35-16Lines changed: 35 additions & 16 deletions
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

Collapse file

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

Copy file name to clipboardExpand all lines: packages/upgrade/static/test/integration/downgrade_component_spec.ts
+24-3Lines changed: 24 additions & 3 deletions
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

Collapse file

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

Copy file name to clipboardExpand all lines: packages/upgrade/static/test/integration/downgrade_module_spec.ts
+63Lines changed: 63 additions & 0 deletions
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 (0)
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.