From e8e6141dbe322f2eb1355afe778a42fa583d0a33 Mon Sep 17 00:00:00 2001 From: Pablo Date: Wed, 26 Mar 2025 19:08:13 -0300 Subject: [PATCH] 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. --- .../template_outlet.directive.ts | 47 ++++++++++++------- .../src/lib/grids/grid-base.directive.ts | 11 +---- .../src/lib/grids/grid/grid.component.spec.ts | 6 +-- .../src/lib/grids/grid/grid.component.ts | 16 ++++++- .../hierarchical-grid.component.ts | 10 ++++ .../hierarchical-grid/row-island.component.ts | 4 +- 6 files changed, 59 insertions(+), 35 deletions(-) diff --git a/projects/igniteui-angular/src/lib/directives/template-outlet/template_outlet.directive.ts b/projects/igniteui-angular/src/lib/directives/template-outlet/template_outlet.directive.ts index 742cc0b98e7..aa4c782fdde 100644 --- a/projects/igniteui-angular/src/lib/directives/template-outlet/template_outlet.directive.ts +++ b/projects/igniteui-angular/src/lib/directives/template-outlet/template_outlet.directive.ts @@ -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'; @@ -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; @@ -51,6 +52,10 @@ export class IgxTemplateOutletDirective implements OnChanges { } } + public ngOnDestroy(): void { + this.cleanCache(); + } + public cleanCache() { this._embeddedViewsMap.forEach((collection) => { collection.forEach((item => { @@ -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 @@ -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); } } } @@ -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) { @@ -197,6 +188,26 @@ export class IgxTemplateOutletDirective implements OnChanges { return TemplateOutletAction.UpdateViewContext; } } + + private cacheView(tmplID: { type: string, id: unknown } | undefined, viewRef: EmbeddedViewRef) { + 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, diff --git a/projects/igniteui-angular/src/lib/grids/grid-base.directive.ts b/projects/igniteui-angular/src/lib/grids/grid-base.directive.ts index 160253bf11f..2b44dc852fd 100644 --- a/projects/igniteui-angular/src/lib/grids/grid-base.directive.ts +++ b/projects/igniteui-angular/src/lib/grids/grid-base.directive.ts @@ -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'; @@ -1329,12 +1329,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 = new QueryList(); - /** * @hidden * @internal @@ -4109,9 +4103,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 = []; diff --git a/projects/igniteui-angular/src/lib/grids/grid/grid.component.spec.ts b/projects/igniteui-angular/src/lib/grids/grid/grid.component.spec.ts index 3fb6cb02b76..7394efad1de 100644 --- a/projects/igniteui-angular/src/lib/grids/grid/grid.component.spec.ts +++ b/projects/igniteui-angular/src/lib/grids/grid/grid.component.spec.ts @@ -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(); @@ -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); diff --git a/projects/igniteui-angular/src/lib/grids/grid/grid.component.ts b/projects/igniteui-angular/src/lib/grids/grid/grid.component.ts index 07d3b4cdb9b..e2accb08b29 100644 --- a/projects/igniteui-angular/src/lib/grids/grid/grid.component.ts +++ b/projects/igniteui-angular/src/lib/grids/grid/grid.component.ts @@ -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'; @@ -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. * @@ -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 */ diff --git a/projects/igniteui-angular/src/lib/grids/hierarchical-grid/hierarchical-grid.component.ts b/projects/igniteui-angular/src/lib/grids/hierarchical-grid/hierarchical-grid.component.ts index a06f2a229f7..3744038bbfc 100644 --- a/projects/igniteui-angular/src/lib/grids/hierarchical-grid/hierarchical-grid.component.ts +++ b/projects/igniteui-angular/src/lib/grids/hierarchical-grid/hierarchical-grid.component.ts @@ -865,6 +865,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) { @@ -926,6 +932,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) diff --git a/projects/igniteui-angular/src/lib/grids/hierarchical-grid/row-island.component.ts b/projects/igniteui-angular/src/lib/grids/hierarchical-grid/row-island.component.ts index c1ff8761a48..13d6bfe8d8e 100644 --- a/projects/igniteui-angular/src/lib/grids/hierarchical-grid/row-island.component.ts +++ b/projects/igniteui-angular/src/lib/grids/hierarchical-grid/row-island.component.ts @@ -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();