Skip to content

Commit e8e6141

Browse files
committed
fix(template-outlet): memory leak for cached templates
When a TemplateOutlet instance was being destroyed all its locally cached views were not being destroyed. The parent component (grid) was attempting to clean up all the template outlets on the its destroy. But, this approach is weak because TemplateOutlets could have been destroyed along the way (resize, groupby, etc), so all those caches would have never been destroyed. The new strategy: * uniquely identified views (templateID.id) are meant to be managed by the parent component, so now the parent component destroys them directly and the template outlet never caches them locally. * other templates (local) are destroyed by the template outlet destroy.
1 parent 141ca8c commit e8e6141

File tree

6 files changed

+59
-35
lines changed

6 files changed

+59
-35
lines changed

projects/igniteui-angular/src/lib/directives/template-outlet/template_outlet.directive.ts

Lines changed: 29 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import {
22
Directive, EmbeddedViewRef, Input, OnChanges, ChangeDetectorRef,
3-
SimpleChange, SimpleChanges, TemplateRef, ViewContainerRef, NgZone, Output, EventEmitter
3+
SimpleChange, SimpleChanges, TemplateRef, ViewContainerRef, NgZone, Output, EventEmitter,
4+
OnDestroy,
45
} from '@angular/core';
56

67
import { IBaseEventArgs } from '../../core/utils';
@@ -12,7 +13,7 @@ import { IBaseEventArgs } from '../../core/utils';
1213
selector: '[igxTemplateOutlet]',
1314
standalone: true
1415
})
15-
export class IgxTemplateOutletDirective implements OnChanges {
16+
export class IgxTemplateOutletDirective implements OnChanges, OnDestroy {
1617
@Input() public igxTemplateOutletContext !: any;
1718

1819
@Input() public igxTemplateOutlet !: TemplateRef<any>;
@@ -51,6 +52,10 @@ export class IgxTemplateOutletDirective implements OnChanges {
5152
}
5253
}
5354

55+
public ngOnDestroy(): void {
56+
this.cleanCache();
57+
}
58+
5459
public cleanCache() {
5560
this._embeddedViewsMap.forEach((collection) => {
5661
collection.forEach((item => {
@@ -63,15 +68,6 @@ export class IgxTemplateOutletDirective implements OnChanges {
6368
this._embeddedViewsMap.clear();
6469
}
6570

66-
public cleanView(tmplID) {
67-
const embViewCollection = this._embeddedViewsMap.get(tmplID.type);
68-
const embView = embViewCollection?.get(tmplID.id);
69-
if (embView) {
70-
embView.destroy();
71-
this._embeddedViewsMap.get(tmplID.type).delete(tmplID.id);
72-
}
73-
}
74-
7571
private _recreateView() {
7672
const prevIndex = this._viewRef ? this._viewContainerRef.indexOf(this._viewRef) : -1;
7773
// detach old and create new
@@ -88,12 +84,7 @@ export class IgxTemplateOutletDirective implements OnChanges {
8884
// if context contains a template id, check if we have a view for that template already stored in the cache
8985
// if not create a copy and add it to the cache in detached state.
9086
// Note: Views in detached state do not appear in the DOM, however they remain stored in memory.
91-
const resCollection = this._embeddedViewsMap.get(this.igxTemplateOutletContext['templateID'].type);
92-
const res = resCollection?.get(this.igxTemplateOutletContext['templateID'].id);
93-
if (!res) {
94-
this._embeddedViewsMap.set(this.igxTemplateOutletContext['templateID'].type,
95-
new Map([[this.igxTemplateOutletContext['templateID'].id, this._viewRef]]));
96-
}
87+
this.cacheView(tmplId, this._viewRef);
9788
}
9889
}
9990
}
@@ -105,7 +96,7 @@ export class IgxTemplateOutletDirective implements OnChanges {
10596
if (view !== this._viewRef) {
10697
if (owner._viewContainerRef.indexOf(view) !== -1) {
10798
// detach in case view it is attached somewhere else at the moment.
108-
this.beforeViewDetach.emit({ owner: this, view: this._viewRef, context: this.igxTemplateOutletContext });
99+
this.beforeViewDetach.emit({ owner: this, view, context: this.igxTemplateOutletContext });
109100
owner._viewContainerRef.detach(owner._viewContainerRef.indexOf(view));
110101
}
111102
if (this._viewRef && this._viewContainerRef.indexOf(this._viewRef) !== -1) {
@@ -197,6 +188,26 @@ export class IgxTemplateOutletDirective implements OnChanges {
197188
return TemplateOutletAction.UpdateViewContext;
198189
}
199190
}
191+
192+
private cacheView(tmplID: { type: string, id: unknown } | undefined, viewRef: EmbeddedViewRef<any>) {
193+
if (!tmplID) {
194+
return;
195+
}
196+
197+
const hasUniqueId = tmplID.id !== undefined && tmplID.id !== null;
198+
if (hasUniqueId) {
199+
// Don't cache locally unique id views, they're handled by the parent component by using moveview instead of cache
200+
return;
201+
}
202+
203+
let resCollection = this._embeddedViewsMap.get(tmplID.type);
204+
if (!resCollection) {
205+
resCollection = new Map();
206+
this._embeddedViewsMap.set(tmplID.type, resCollection);
207+
}
208+
209+
resCollection.set(tmplID.id, viewRef);
210+
}
200211
}
201212
enum TemplateOutletAction {
202213
CreateView,

projects/igniteui-angular/src/lib/grids/grid-base.directive.ts

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ import { IgxGridSummaryService } from './summaries/grid-summary.service';
7070
import { IgxSummaryRowComponent } from './summaries/summary-row.component';
7171
import { IgxGridSelectionService } from './selection/selection.service';
7272
import { IgxEditRow, IgxCell, IgxAddRow } from './common/crud.service';
73-
import { ICachedViewLoadedEventArgs, IgxTemplateOutletDirective } from '../directives/template-outlet/template_outlet.directive';
73+
import { ICachedViewLoadedEventArgs } from '../directives/template-outlet/template_outlet.directive';
7474
import { IgxExcelStyleLoadingValuesTemplateDirective } from './filtering/excel-style/excel-style-search.component';
7575
import { IgxGridColumnResizerComponent } from './resizing/resizer.component';
7676
import { CharSeparatedValueData } from '../services/csv/char-separated-value-data';
@@ -1329,12 +1329,6 @@ export abstract class IgxGridBaseDirective implements GridType,
13291329
@ViewChild('igxRowEditingOverlayOutlet', { read: IgxOverlayOutletDirective, static: true })
13301330
public rowEditingOutletDirective: IgxOverlayOutletDirective;
13311331

1332-
/**
1333-
* @hidden @internal
1334-
*/
1335-
@ViewChildren(IgxTemplateOutletDirective, { read: IgxTemplateOutletDirective })
1336-
public tmpOutlets: QueryList<any> = new QueryList<any>();
1337-
13381332
/**
13391333
* @hidden
13401334
* @internal
@@ -4109,9 +4103,6 @@ export abstract class IgxGridBaseDirective implements GridType,
41094103
* @hidden @internal
41104104
*/
41114105
public ngOnDestroy() {
4112-
this.tmpOutlets.forEach((tmplOutlet) => {
4113-
tmplOutlet.cleanCache();
4114-
});
41154106
this._autoGeneratedColsRefs.forEach(ref => ref.destroy());
41164107
this._autoGeneratedColsRefs = [];
41174108

projects/igniteui-angular/src/lib/grids/grid/grid.component.spec.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2532,9 +2532,7 @@ describe('IgxGrid Component Tests #grid', () => {
25322532
expect(grid.getRowData(7)).toEqual({});
25332533
});
25342534

2535-
// note: it leaks when grid.groupBy() is executed because template-outlet doesn't destroy the viewrefs
2536-
// to be addressed in a separate PR
2537-
it(`Verify that getRowByIndex and RowType API returns correct data`, skipLeakCheck(() => {
2535+
it(`Verify that getRowByIndex and RowType API returns correct data`, () => {
25382536
const fix = TestBed.createComponent(IgxGridDefaultRenderingComponent);
25392537
fix.componentInstance.initColumnsRows(35, 5);
25402538
fix.detectChanges();
@@ -2693,7 +2691,7 @@ describe('IgxGrid Component Tests #grid', () => {
26932691
expect(thirdRow instanceof IgxGroupByRow).toBe(true);
26942692
expect(thirdRow.index).toBe(2);
26952693
expect(thirdRow.viewIndex).toBe(7);
2696-
}));
2694+
});
26972695

26982696
it('Verify that getRowByIndex returns correct data when paging is enabled', fakeAsync(() => {
26992697
const fix = TestBed.createComponent(IgxGridWrappedInContComponent);

projects/igniteui-angular/src/lib/grids/grid/grid.component.ts

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import {
22
Component, ChangeDetectionStrategy, Input, Output, EventEmitter, ContentChild, ViewChildren,
33
QueryList, ViewChild, TemplateRef, DoCheck, AfterContentInit, HostBinding,
4-
OnInit, AfterViewInit, ContentChildren, CUSTOM_ELEMENTS_SCHEMA, booleanAttribute
4+
OnInit, AfterViewInit, ContentChildren, CUSTOM_ELEMENTS_SCHEMA, booleanAttribute, OnDestroy,
55
} from '@angular/core';
66
import { NgTemplateOutlet, NgClass, NgStyle } from '@angular/common';
77

@@ -155,7 +155,7 @@ export interface IGroupingDoneEventArgs extends IBaseEventArgs {
155155
],
156156
schemas: [CUSTOM_ELEMENTS_SCHEMA]
157157
})
158-
export class IgxGridComponent extends IgxGridBaseDirective implements GridType, OnInit, DoCheck, AfterContentInit, AfterViewInit {
158+
export class IgxGridComponent extends IgxGridBaseDirective implements GridType, OnInit, DoCheck, AfterContentInit, AfterViewInit, OnDestroy {
159159
/**
160160
* Emitted when a new chunk of data is loaded from virtualization.
161161
*
@@ -1069,6 +1069,18 @@ export class IgxGridComponent extends IgxGridBaseDirective implements GridType,
10691069
super.ngDoCheck();
10701070
}
10711071

1072+
/**
1073+
* @hidden @internal
1074+
*/
1075+
public override ngOnDestroy() {
1076+
super.ngOnDestroy();
1077+
for (const childTemplate of this.childDetailTemplates.values()) {
1078+
if (!childTemplate.view.destroyed) {
1079+
childTemplate.view.destroy();
1080+
}
1081+
}
1082+
}
1083+
10721084
/**
10731085
* @hidden @internal
10741086
*/

projects/igniteui-angular/src/lib/grids/hierarchical-grid/hierarchical-grid.component.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -865,6 +865,12 @@ export class IgxHierarchicalGridComponent extends IgxHierarchicalGridBaseDirecti
865865

866866
/** @hidden @internal **/
867867
public override ngOnDestroy() {
868+
for (const childTemplate of this.childGridTemplates.values()) {
869+
if (!childTemplate.view.destroyed) {
870+
childTemplate.view.destroy();
871+
}
872+
}
873+
868874
if (!this.parent) {
869875
this.gridAPI.getChildGrids(true).forEach((grid) => {
870876
if (!grid.childRow.cdr.destroyed) {
@@ -926,6 +932,10 @@ export class IgxHierarchicalGridComponent extends IgxHierarchicalGridBaseDirecti
926932
const tmlpOutlet = cachedData.owner;
927933
return {
928934
$implicit: rowData,
935+
templateID: {
936+
type: 'childRow',
937+
id: rowData.rowID
938+
},
929939
moveView: view,
930940
owner: tmlpOutlet,
931941
index: this.dataView.indexOf(rowData)

projects/igniteui-angular/src/lib/grids/hierarchical-grid/row-island.component.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -566,7 +566,9 @@ export class IgxRowIslandComponent extends IgxHierarchicalGridBaseDirective
566566

567567
private cleanGridState(grid) {
568568
grid.childGridTemplates.forEach((tmpl) => {
569-
tmpl.owner.cleanView(tmpl.context.templateID);
569+
if (!tmpl.view.destroyed) {
570+
tmpl.view.destroy();
571+
}
570572
});
571573
grid.childGridTemplates.clear();
572574
grid.onRowIslandChange();

0 commit comments

Comments
 (0)