Skip to content

fix(template-outlet): memory leak for cached templates #15608

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {
Directive, EmbeddedViewRef, Input, OnChanges, ChangeDetectorRef,
SimpleChange, SimpleChanges, TemplateRef, ViewContainerRef, NgZone, Output, EventEmitter
SimpleChange, SimpleChanges, TemplateRef, ViewContainerRef, NgZone, Output, EventEmitter,
OnDestroy,
} from '@angular/core';

import { IBaseEventArgs } from '../../core/utils';
Expand All @@ -12,7 +13,7 @@ import { IBaseEventArgs } from '../../core/utils';
selector: '[igxTemplateOutlet]',
standalone: true
})
export class IgxTemplateOutletDirective implements OnChanges {
export class IgxTemplateOutletDirective implements OnChanges, OnDestroy {
@Input() public igxTemplateOutletContext !: any;

@Input() public igxTemplateOutlet !: TemplateRef<any>;
Expand Down Expand Up @@ -51,6 +52,10 @@ export class IgxTemplateOutletDirective implements OnChanges {
}
}

public ngOnDestroy(): void {
this.cleanCache();
}

public cleanCache() {
this._embeddedViewsMap.forEach((collection) => {
collection.forEach((item => {
Expand All @@ -63,15 +68,6 @@ export class IgxTemplateOutletDirective implements OnChanges {
this._embeddedViewsMap.clear();
}

public cleanView(tmplID) {
const embViewCollection = this._embeddedViewsMap.get(tmplID.type);
const embView = embViewCollection?.get(tmplID.id);
if (embView) {
embView.destroy();
this._embeddedViewsMap.get(tmplID.type).delete(tmplID.id);
}
}

private _recreateView() {
const prevIndex = this._viewRef ? this._viewContainerRef.indexOf(this._viewRef) : -1;
// detach old and create new
Expand All @@ -88,12 +84,7 @@ export class IgxTemplateOutletDirective implements OnChanges {
// if context contains a template id, check if we have a view for that template already stored in the cache
// if not create a copy and add it to the cache in detached state.
// Note: Views in detached state do not appear in the DOM, however they remain stored in memory.
const resCollection = this._embeddedViewsMap.get(this.igxTemplateOutletContext['templateID'].type);
const res = resCollection?.get(this.igxTemplateOutletContext['templateID'].id);
if (!res) {
this._embeddedViewsMap.set(this.igxTemplateOutletContext['templateID'].type,
new Map([[this.igxTemplateOutletContext['templateID'].id, this._viewRef]]));
}
this.cacheView(tmplId, this._viewRef);
}
}
}
Expand All @@ -105,7 +96,7 @@ export class IgxTemplateOutletDirective implements OnChanges {
if (view !== this._viewRef) {
if (owner._viewContainerRef.indexOf(view) !== -1) {
// detach in case view it is attached somewhere else at the moment.
this.beforeViewDetach.emit({ owner: this, view: this._viewRef, context: this.igxTemplateOutletContext });
this.beforeViewDetach.emit({ owner: this, view, context: this.igxTemplateOutletContext });
owner._viewContainerRef.detach(owner._viewContainerRef.indexOf(view));
}
if (this._viewRef && this._viewContainerRef.indexOf(this._viewRef) !== -1) {
Expand Down Expand Up @@ -197,6 +188,26 @@ export class IgxTemplateOutletDirective implements OnChanges {
return TemplateOutletAction.UpdateViewContext;
}
}

private cacheView(tmplID: { type: string, id: unknown } | undefined, viewRef: EmbeddedViewRef<any>) {
if (!tmplID) {
return;
}

const hasUniqueId = tmplID.id !== undefined && tmplID.id !== null;
if (hasUniqueId) {
// Don't cache locally unique id views, they're handled by the parent component by using moveview instead of cache
return;
}

let resCollection = this._embeddedViewsMap.get(tmplID.type);
if (!resCollection) {
resCollection = new Map();
this._embeddedViewsMap.set(tmplID.type, resCollection);
}

resCollection.set(tmplID.id, viewRef);
}
}
enum TemplateOutletAction {
CreateView,
Expand Down
11 changes: 1 addition & 10 deletions projects/igniteui-angular/src/lib/grids/grid-base.directive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ import { IgxGridSummaryService } from './summaries/grid-summary.service';
import { IgxSummaryRowComponent } from './summaries/summary-row.component';
import { IgxGridSelectionService } from './selection/selection.service';
import { IgxEditRow, IgxCell, IgxAddRow } from './common/crud.service';
import { ICachedViewLoadedEventArgs, IgxTemplateOutletDirective } from '../directives/template-outlet/template_outlet.directive';
import { ICachedViewLoadedEventArgs } from '../directives/template-outlet/template_outlet.directive';
import { IgxExcelStyleLoadingValuesTemplateDirective } from './filtering/excel-style/excel-style-search.component';
import { IgxGridColumnResizerComponent } from './resizing/resizer.component';
import { CharSeparatedValueData } from '../services/csv/char-separated-value-data';
Expand Down Expand Up @@ -1330,12 +1330,6 @@ export abstract class IgxGridBaseDirective implements GridType,
@ViewChild('igxRowEditingOverlayOutlet', { read: IgxOverlayOutletDirective, static: true })
public rowEditingOutletDirective: IgxOverlayOutletDirective;

/**
* @hidden @internal
*/
@ViewChildren(IgxTemplateOutletDirective, { read: IgxTemplateOutletDirective })
public tmpOutlets: QueryList<any> = new QueryList<any>();

/**
* @hidden
* @internal
Expand Down Expand Up @@ -4124,9 +4118,6 @@ export abstract class IgxGridBaseDirective implements GridType,
* @hidden @internal
*/
public ngOnDestroy() {
this.tmpOutlets.forEach((tmplOutlet) => {
tmplOutlet.cleanCache();
});
this._autoGeneratedColsRefs.forEach(ref => ref.destroy());
this._autoGeneratedColsRefs = [];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2532,9 +2532,7 @@ describe('IgxGrid Component Tests #grid', () => {
expect(grid.getRowData(7)).toEqual({});
});

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

it('Verify that getRowByIndex returns correct data when paging is enabled', fakeAsync(() => {
const fix = TestBed.createComponent(IgxGridWrappedInContComponent);
Expand Down
16 changes: 14 additions & 2 deletions projects/igniteui-angular/src/lib/grids/grid/grid.component.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {
Component, ChangeDetectionStrategy, Input, Output, EventEmitter, ContentChild, ViewChildren,
QueryList, ViewChild, TemplateRef, DoCheck, AfterContentInit, HostBinding,
OnInit, AfterViewInit, ContentChildren, CUSTOM_ELEMENTS_SCHEMA, booleanAttribute
OnInit, AfterViewInit, ContentChildren, CUSTOM_ELEMENTS_SCHEMA, booleanAttribute, OnDestroy,
} from '@angular/core';
import { NgTemplateOutlet, NgClass, NgStyle } from '@angular/common';

Expand Down Expand Up @@ -155,7 +155,7 @@ export interface IGroupingDoneEventArgs extends IBaseEventArgs {
],
schemas: [CUSTOM_ELEMENTS_SCHEMA]
})
export class IgxGridComponent extends IgxGridBaseDirective implements GridType, OnInit, DoCheck, AfterContentInit, AfterViewInit {
export class IgxGridComponent extends IgxGridBaseDirective implements GridType, OnInit, DoCheck, AfterContentInit, AfterViewInit, OnDestroy {
/**
* Emitted when a new chunk of data is loaded from virtualization.
*
Expand Down Expand Up @@ -1069,6 +1069,18 @@ export class IgxGridComponent extends IgxGridBaseDirective implements GridType,
super.ngDoCheck();
}

/**
* @hidden @internal
*/
public override ngOnDestroy() {
super.ngOnDestroy();
for (const childTemplate of this.childDetailTemplates.values()) {
if (!childTemplate.view.destroyed) {
childTemplate.view.destroy();
}
}
}

/**
* @hidden @internal
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -867,6 +867,12 @@ export class IgxHierarchicalGridComponent extends IgxHierarchicalGridBaseDirecti

/** @hidden @internal **/
public override ngOnDestroy() {
for (const childTemplate of this.childGridTemplates.values()) {
if (!childTemplate.view.destroyed) {
childTemplate.view.destroy();
}
}

if (!this.parent) {
this.gridAPI.getChildGrids(true).forEach((grid) => {
if (!grid.childRow.cdr.destroyed) {
Expand Down Expand Up @@ -928,6 +934,10 @@ export class IgxHierarchicalGridComponent extends IgxHierarchicalGridBaseDirecti
const tmlpOutlet = cachedData.owner;
return {
$implicit: rowData,
templateID: {
type: 'childRow',
id: rowData.rowID
},
moveView: view,
owner: tmlpOutlet,
index: this.dataView.indexOf(rowData)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,9 @@ export class IgxRowIslandComponent extends IgxHierarchicalGridBaseDirective

private cleanGridState(grid) {
grid.childGridTemplates.forEach((tmpl) => {
tmpl.owner.cleanView(tmpl.context.templateID);
if (!tmpl.view.destroyed) {
tmpl.view.destroy();
}
});
grid.childGridTemplates.clear();
grid.onRowIslandChange();
Expand Down
Loading