fix(grid): fix leaks in grid and improve leaks reporting

pmoleri · pmoleri · commit 83b72d58336b · 2025-02-26T15:59:13.000-03:00
diff --git a/projects/igniteui-angular/src/lib/grids/grid-base.directive.ts b/projects/igniteui-angular/src/lib/grids/grid-base.directive.ts
@@ -4,6 +4,7 @@ import {
     AfterViewInit,
     booleanAttribute,
     ChangeDetectorRef,
+    ComponentRef,
     ContentChild,
     ContentChildren,
     createComponent,
@@ -3121,6 +3122,7 @@ export abstract class IgxGridBaseDirective implements GridType,
     protected _sortingOptions: ISortingOptions = { mode: 'multiple' };
     protected _filterStrategy: IFilteringStrategy = new FilteringStrategy();
     protected _autoGeneratedCols = [];
+    protected _autoGeneratedColsRefs: ComponentRef<IgxColumnComponent>[] = [];
     protected _dataView = [];
     protected _lastSearchInfo: ISearchInfo = {
         searchText: '',
@@ -3920,15 +3922,19 @@ export abstract class IgxGridBaseDirective implements GridType,
     /** @hidden @internal */
     public setUpPaginator() {
         if (this.paginator) {
-            this.paginator.pageChange.pipe(takeWhile(() => !!this.paginator), filter(() => !this._init))
+            this.paginator.pageChange
+                .pipe(takeWhile(() => !!this.paginator), filter(() => !this._init))
+                .pipe(takeUntil(this.destroy$))
                 .subscribe(() => {
                     this.selectionService.clear(true);
                     this.crudService.endEdit(false);
                     this.pipeTrigger++;
                     this.navigateTo(0);
                     this.notifyChanges();
                 });
-            this.paginator.perPageChange.pipe(takeWhile(() => !!this.paginator), filter(() => !this._init))
+            this.paginator.perPageChange
+                .pipe(takeWhile(() => !!this.paginator), filter(() => !this._init))
+                .pipe(takeUntil(this.destroy$))
                 .subscribe(() => {
                     this.selectionService.clear(true);
                     this.page = 0;
@@ -3996,8 +4002,10 @@ export abstract class IgxGridBaseDirective implements GridType,
      */
     public _zoneBegoneListeners() {
         this.zone.runOutsideAngular(() => {
-            this.verticalScrollContainer.getScroll().addEventListener('scroll', this.verticalScrollHandler.bind(this));
-            this.headerContainer?.getScroll().addEventListener('scroll', this.horizontalScrollHandler.bind(this));
+            this.verticalScrollHandler = this.verticalScrollHandler.bind(this);
+            this.horizontalScrollHandler = this.horizontalScrollHandler.bind(this);
+            this.verticalScrollContainer.getScroll().addEventListener('scroll', this.verticalScrollHandler);
+            this.headerContainer?.getScroll().addEventListener('scroll', this.horizontalScrollHandler);
             if (this.hasColumnsToAutosize) {
                 this.headerContainer?.dataChanged.pipe(takeUntil(this.destroy$)).subscribe(() => {
                     this.cdr.detectChanges();
@@ -4025,7 +4033,7 @@ export abstract class IgxGridBaseDirective implements GridType,
         this._zoneBegoneListeners();
 
         const vertScrDC = this.verticalScrollContainer.displayContainer;
-        vertScrDC.addEventListener('scroll', this.preventContainerScroll.bind(this));
+        vertScrDC.addEventListener('scroll', this.preventContainerScroll);
 
         this._pinnedRowList.changes
             .pipe(takeUntil(this.destroy$))
@@ -4100,6 +4108,8 @@ export abstract class IgxGridBaseDirective implements GridType,
         this.tmpOutlets.forEach((tmplOutlet) => {
             tmplOutlet.cleanCache();
         });
+        this._autoGeneratedColsRefs.forEach(ref => ref.destroy());
+        this._autoGeneratedColsRefs = [];
 
         this.destroy$.next(true);
         this.destroy$.complete();
@@ -6106,7 +6116,7 @@ export abstract class IgxGridBaseDirective implements GridType,
         this.configureRowEditingOverlay(id, this.rowList.length <= MIN_ROW_EDITING_COUNT_THRESHOLD);
 
         this.rowEditingOverlay.open(this.rowEditSettings);
-        this.rowEditingOverlay.element.addEventListener('wheel', this.rowEditingWheelHandler.bind(this));
+        this.rowEditingOverlay.element.addEventListener('wheel', this.rowEditingWheelHandler);
     }
 
     /**
@@ -6238,7 +6248,7 @@ export abstract class IgxGridBaseDirective implements GridType,
     /**
      * @hidden
      */
-    public rowEditingWheelHandler(event: WheelEvent) {
+    public rowEditingWheelHandler = (event: WheelEvent) => {
         if (event.deltaY > 0) {
             this.verticalScrollContainer.scrollNext();
         } else {
@@ -7089,11 +7099,14 @@ export abstract class IgxGridBaseDirective implements GridType,
         const fields = this.generateDataFields(data);
         const columns = [];
 
+        this._autoGeneratedColsRefs.forEach(ref => ref.destroy());
+        this._autoGeneratedColsRefs = [];
         fields.forEach((field) => {
             const ref = createComponent(IgxColumnComponent, { environmentInjector: this.envInjector, elementInjector: this.injector });
             ref.instance.field = field;
             ref.instance.dataType = this.resolveDataTypes(data[0][field]);
             ref.changeDetectorRef.detectChanges();
+            this._autoGeneratedColsRefs.push(ref);
             columns.push(ref.instance);
         });
         this._autoGeneratedCols = columns;
@@ -7401,7 +7414,7 @@ export abstract class IgxGridBaseDirective implements GridType,
         }
 
         if (delayScrolling) {
-            this.verticalScrollContainer.dataChanged.pipe(first()).subscribe(() => {
+            this.verticalScrollContainer.dataChanged.pipe(first(), takeUntil(this.destroy$)).subscribe(() => {
                 this.scrollDirective(this.verticalScrollContainer,
                     typeof (row) === 'number' ? row : this.unpinnedDataView.indexOf(row));
             });
@@ -7555,7 +7568,7 @@ export abstract class IgxGridBaseDirective implements GridType,
         let row = this.summariesRowList.filter(s => s.index !== 0).concat(this.rowList.toArray()).find(r => r.index === rowIndex);
         if (!row) {
             if ((this as any).totalItemCount) {
-                this.verticalScrollContainer.dataChanged.pipe(first()).subscribe(() => {
+                this.verticalScrollContainer.dataChanged.pipe(first(), takeUntil(this.destroy$)).subscribe(() => {
                     this.cdr.detectChanges();
                     row = this.summariesRowList.filter(s => s.index !== 0).concat(this.rowList.toArray()).find(r => r.index === rowIndex);
                     const cbArgs = this.getNavigationArguments(row, visibleColIndex);
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
@@ -15,7 +15,7 @@ import { SampleTestData } from '../../test-utils/sample-test-data.spec';
 import { BasicGridComponent } from '../../test-utils/grid-base-components.spec';
 import { UIInteractions, wait } from '../../test-utils/ui-interactions.spec';
 import { IgxStringFilteringOperand, IgxNumberFilteringOperand } from '../../data-operations/filtering-condition';
-import { configureTestSuite } from '../../test-utils/configure-suite';
+import { configureTestSuite, skipLeakCheck } from '../../test-utils/configure-suite';
 import { GridSelectionMode, Size } from '../common/enums';
 import { FilteringExpressionsTree } from '../../data-operations/filtering-expressions-tree';
 import { FilteringLogic } from '../../data-operations/filtering-expression.interface';
@@ -37,7 +37,7 @@ describe('IgxGrid Component Tests #grid', () => {
     const TBODY_CLASS = '.igx-grid__tbody-content';
     const THEAD_CLASS = '.igx-grid-thead';
 
-    configureTestSuite();
+    configureTestSuite({ checkLeaks: true });
 
     describe('IgxGrid - input properties', () => {
         beforeAll(waitForAsync(() => {
@@ -1502,7 +1502,7 @@ describe('IgxGrid Component Tests #grid', () => {
         }));
 
         it('should render correct columns if after scrolling right container size changes so that all columns become visible.',
-            async () => {
+            skipLeakCheck(async () => {
                 const fix = TestBed.createComponent(IgxGridDefaultRenderingComponent);
                 fix.detectChanges();
                 const grid = fix.componentInstance.grid;
@@ -1523,8 +1523,9 @@ describe('IgxGrid Component Tests #grid', () => {
                 expect(headers.length).toEqual(5);
                 for (let i = 0; i < headers.length; i++) {
                     expect(headers[i].context.column.field).toEqual(grid.columnList.get(i).field);
+                    // Note: We use skipLeakCheck because using `headers[i].context` messes up memory leak detection
                 }
-            });
+            }));
 
         it('Should render date and number values based on default formatting', fakeAsync(() => {
             const fixture = TestBed.createComponent(IgxGridFormattingComponent);
@@ -2715,6 +2716,7 @@ describe('IgxGrid Component Tests #grid', () => {
 
         afterEach(() => {
             observer?.disconnect();
+            observer = null;
         });
 
         it('should render the grid in a certain amount of time', async () => {
diff --git a/projects/igniteui-angular/src/lib/test-utils/configure-suite.ts b/projects/igniteui-angular/src/lib/test-utils/configure-suite.ts
@@ -1,7 +1,7 @@
 import { NgModuleRef } from '@angular/core';
 import { TestBed, getTestBed, ComponentFixture, waitForAsync } from '@angular/core/testing';
 
-const checkLeaks = 'gc' in window;
+const checkLeaksAvailable = typeof window.gc === 'function';
 
 const debug = false;
 function debugLog(...args) {
@@ -10,11 +10,16 @@ function debugLog(...args) {
     }
 }
 
+let _skipLeakCheck = false;
+const throwOnLeak = true;
+
 interface ConfigureOptions {
     /**
      * Check for memory leaks when the tests finishes.
      * Note, this only works in Chrome configurations with expose the gc.
-     * Caveats: if there are pending (non-cancelled) timers or animation frames it may report false positives.
+     * Caveats:
+     *   * if there are pending (non-cancelled) timers or animation frames it may report false positives.
+     *   * if there's a beforeEach create it must be cleaned up in an afterEach to avoid being detected as a leak
      */
     checkLeaks?: boolean;
 }
@@ -27,11 +32,13 @@ interface ConfigureOptions {
  */
 
 export const configureTestSuite = (configureActionOrOptions?: (() => TestBed) | ConfigureOptions, options: ConfigureOptions = {}) => {
+    setupJasmineCurrentTest();
+
     const configureAction = typeof configureActionOrOptions === 'function' ? configureActionOrOptions : undefined;
     options = (configureActionOrOptions && typeof configureActionOrOptions === 'object') ? configureActionOrOptions : options;
-    options.checkLeaks = options.checkLeaks && checkLeaks;
+    options.checkLeaks = options.checkLeaks && checkLeaksAvailable;
 
-    let componentRefs: WeakRef<{}>[];
+    let componentRefs: { test: string, ref: WeakRef<{}> }[];
     const moduleRefs = new Set<NgModuleRef<any>>();
 
     const testBed = getTestBed();
@@ -57,7 +64,9 @@ export const configureTestSuite = (configureActionOrOptions?: (() => TestBed) |
             componentRefs = [];
             testBed.createComponent = function () {
                 const fixture = originCreateComponent.apply(testBed, arguments);
-                componentRefs.push(new WeakRef(fixture.componentInstance));
+                if (!_skipLeakCheck) {
+                    componentRefs.push({ test: jasmine['currentTest'].fullName, ref: new WeakRef(fixture.componentInstance) });
+                }
                 return fixture;
             };
         }
@@ -73,13 +82,19 @@ export const configureTestSuite = (configureActionOrOptions?: (() => TestBed) |
 
     function reportLeaks() {
         gc();
-        const leaks = componentRefs.map(ref => ref.deref()).filter(i => !!i);
+        const leaks = componentRefs.map(({test, ref}) => ({ test, instance: ref.deref()})).filter(item => !!item.instance);
         if (leaks.length > 0) {
             console.warn(`Detected ${leaks.length} leaks:`);
-            const classNames = [...new Set(leaks.map(i => i.constructor.name))];
-            for (const name of classNames) {
-                const count = leaks.filter(i => i.constructor.name === name).length;
-                console.warn(` · ${name}: ${count}`);
+            for (const test of new Set(leaks.map(l => l.test))) {
+                const testLeaks = leaks.filter(l => l.test === test).map(l => l.instance);
+                const classNames = new Set(testLeaks.map(i => i.constructor.name));
+                for (const name of classNames) {
+                    const count = testLeaks.filter(i => i.constructor.name === name).length;
+                    console.warn(` · ${name}: ${count} - ${test}`);
+                }
+            }
+            if (throwOnLeak) {
+                throw new Error(`Detected ${leaks.length} leaks`);
             }
         } else {
             debugLog('No leaks detected');
@@ -124,6 +139,37 @@ export const configureTestSuite = (configureActionOrOptions?: (() => TestBed) |
     });
 };
 
+/** Calls to Testbed.createComponent() inside this wrapper wont be tracked for leaks */
+export function skipLeakCheck(fn: () => void | Promise<unknown>) {
+    return function() {
+        _skipLeakCheck = true;
+        const res = fn();
+        if (res instanceof Promise) {
+            return res.finally(() => {
+                _skipLeakCheck = false;
+            });
+        } else {
+            _skipLeakCheck = false;
+            return res;
+        }
+    }
+}
+
+let setupJasmineCurrentTestDone = false;
+function setupJasmineCurrentTest() {
+    if (!setupJasmineCurrentTestDone) {
+        jasmine.getEnv().addReporter({
+            specStarted(result) {
+                jasmine['currentTest'] = result;
+            },
+            specDone(result) {
+                jasmine['currentTest'] = result;
+            },
+        });
+        setupJasmineCurrentTestDone = true;
+    }
+}
+
 // TODO: enable on re-run by selecting enable debug logging
 // https://docs.github.com/en/actions/monitoring-and-troubleshooting-workflows/troubleshooting-workflows/enabling-debug-logging
 const shardLogging = false;
