Skip to content

Commit

Permalink
fix(upgrade): avoid memory leak when removing downgraded components
Browse files Browse the repository at this point in the history
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 angular#26209 but for downgraded (instead
of upgraded) components.

Fixes angular#39911
Closes angular#39921
  • Loading branch information
gkalpak committed Dec 3, 2020
1 parent 1b283d3 commit 2839ba1
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 2 deletions.
1 change: 1 addition & 0 deletions packages/upgrade/src/common/src/angular1.ts
Expand Up @@ -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;
Expand Down
30 changes: 28 additions & 2 deletions packages/upgrade/src/common/src/downgrade_component_adapter.ts
Expand Up @@ -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';
Expand Down Expand Up @@ -216,11 +216,37 @@ 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()` will remove the element from the DOM and therefore
// it will no longer be a descendant of the removed element when `cleanData()` will be
// 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.children!());

destroyComponentRef();
}
});
Expand Down

0 comments on commit 2839ba1

Please sign in to comment.