fix: fix last memory leak and make memory CI more strict (#4229)

e2e/e2e.d.ts

@@ -15,6 +15,7 @@ declare global {
     // eslint-disable-next-line ts/naming-convention
     interface Window {
         E2EControllerAPI: IE2EControllerAPI;
+        univer: any;
         // eslint-disable-next-line ts/no-explicit-any
         univerAPI: any;
     }

e2e/memory/memory.spec.ts

@@ -16,53 +16,44 @@
 
 /* eslint-disable no-console */
 
-import type { Page } from '@playwright/test';
 import { expect, test } from '@playwright/test';
+import { getMetrics } from './util';
 
-const MAX_MEMORY_OVERFLOW = 5_000_000; // 5MB
+const MAX_UNIT_MEMORY_OVERFLOW = 1_000_000; // 1MB
+
+// There are some compiled code and global cache, so we make some room
+// for this. But we need to make sure that a Univer object cannot fit
+// in this size.
+const MAX_UNIVER_MEMORY_OVERFLOW = 5_000_000;
 
 test('memory', async ({ page }) => {
+    test.setTimeout(60_000);
+
     await page.goto('http://localhost:3000/sheets/');
-    await page.waitForTimeout(2000);
+    await page.waitForTimeout(5000);
 
     const memoryBeforeLoad = (await getMetrics(page)).JSHeapUsedSize;
-    console.log('Memory before load:', memoryBeforeLoad);
+    console.log('Memory before load (B):', memoryBeforeLoad);
 
     await page.evaluate(() => window.E2EControllerAPI.loadAndRelease(1));
-    await page.waitForTimeout(5000); // wait for long enough to let the GC do its job
+    await page.waitForTimeout(5000);
     const memoryAfterFirstLoad = (await getMetrics(page)).JSHeapUsedSize;
-    console.log('Memory after first load:', memoryAfterFirstLoad);
+    console.log('Memory after first load (B):', memoryAfterFirstLoad);
 
     await page.evaluate(() => window.E2EControllerAPI.loadAndRelease(2));
+    await page.waitForTimeout(5000);
     const memoryAfterSecondLoad = (await getMetrics(page)).JSHeapUsedSize;
-    console.log('Memory after second load:', memoryAfterSecondLoad);
+    console.log('Memory after second load (B):', memoryAfterSecondLoad);
 
-    const notLeaking = (memoryAfterSecondLoad <= memoryAfterFirstLoad)
-        || (memoryAfterSecondLoad - memoryAfterFirstLoad <= MAX_MEMORY_OVERFLOW);
-    expect(notLeaking).toBeTruthy();
+    await page.evaluate(() => window.univer.dispose());
+    await page.waitForTimeout(5000);
+    const memoryAfterDisposingUniver = (await getMetrics(page)).JSHeapUsedSize;
+    console.log('Memory after disposing univer (B):', memoryAfterDisposingUniver);
+
+    const noUnitLeaking = memoryAfterSecondLoad - memoryAfterFirstLoad <= MAX_UNIT_MEMORY_OVERFLOW;
+    expect(noUnitLeaking).toBeTruthy();
+
+    const noUniverLeaking = memoryAfterDisposingUniver - memoryBeforeLoad <= MAX_UNIVER_MEMORY_OVERFLOW;
+    expect(noUniverLeaking).toBeTruthy();
 });
 
-interface IMetrics {
-    JSHeapUsedSize: number;
-}
-
-/**
- * Return a performance metric from the chrome cdp session.
- * Chrome only.
- * @param {Page} page page to attach cdpClient
- * @return {IMetrics}
- * @see {@link https://github.com/microsoft/playwright/issues/18071}
- */
-async function getMetrics(page: Page): Promise<IMetrics> {
-    const client = await page.context().newCDPSession(page);
-    await client.send('Performance.enable');
-    const perfMetricObject = await client.send('Performance.getMetrics');
-    const extractedMetric = perfMetricObject?.metrics;
-    const metricObject = extractedMetric.reduce((acc, { name, value }) => {
-        acc[name] = value;
-        return acc;
-    }, {});
-
-    return metricObject as unknown as IMetrics;
-}
-

e2e/memory/util.ts

@@ -0,0 +1,43 @@
+/**
+ * Copyright 2023-present DreamNum Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import type { Page } from '@playwright/test';
+
+interface IMetrics {
+    JSHeapUsedSize: number;
+}
+
+/**
+ * Return a performance metric from the chrome cdp session.
+ * Chrome only.
+ * @param {Page} page page to attach cdpClient
+ * @return {IMetrics}
+ * @see {@link https://github.com/microsoft/playwright/issues/18071}
+ */
+export async function getMetrics(page: Page): Promise<IMetrics> {
+    const client = await page.context().newCDPSession(page);
+    await client.send('Performance.enable');
+    await client.send('HeapProfiler.collectGarbage');
+    const perfMetricObject = await client.send('Performance.getMetrics');
+    const extractedMetric = perfMetricObject?.metrics;
+    const metricObject = extractedMetric.reduce((acc, { name, value }) => {
+        acc[name] = value;
+        return acc;
+    }, {});
+
+    return metricObject as unknown as IMetrics;
+}
+

packages/sheets-ui/src/facade/f-univer.ts

@@ -26,8 +26,8 @@ import type {
 } from '@univerjs/engine-render';
 import { FUniver, toDisposable } from '@univerjs/core';
 import { IRenderManagerService } from '@univerjs/engine-render';
-import { FSheetHooks } from '@univerjs/sheets/facade';
 import { SHEET_VIEW_KEY } from '@univerjs/sheets-ui';
+import { FSheetHooks } from '@univerjs/sheets/facade';
 
 export interface IFUniverSheetsUIMixin {
     /**

packages/sheets-ui/src/views/sheet-bar/sheet-bar-tabs/utils/slide-tab-bar.ts

@@ -858,13 +858,14 @@ export class SlideTabBar {
     }
 
     destroy() {
+        this.removeListener();
+
         this._downActionX = 0;
         this._moveActionX = 0;
         this._compareDirection = 0;
         this._compareIndex = 0;
         this._slideTabItems = [];
         this._activeTabItem = null;
-        this.removeListener();
 
         // TODO@Dushusir: If set to null, the types in other places need to be judged
         // this._slideTabBar = null;

packages/uniscript/src/services/script-editor.service.ts

@@ -31,6 +31,14 @@ export class ScriptEditorService extends Disposable {
         super();
     }
 
+    override dispose(): void {
+        super.dispose();
+
+        if (this._editorInstance) {
+            this._editorInstance.dispose();
+        }
+    }
+
     setEditorInstance(editor: editor.IStandaloneCodeEditor): IDisposable {
         this._editorInstance = editor;
         return toDisposable(() => (this._editorInstance = null));
@@ -43,10 +51,7 @@ export class ScriptEditorService extends Disposable {
     requireVscodeEditor(): void {
         if (!window.MonacoEnvironment) {
             const config = this._configService.getConfig<IUniverUniscriptConfig>(UNISCRIPT_PLUGIN_CONFIG_KEY);
-
-            window.MonacoEnvironment = {
-                getWorkerUrl: config?.getWorkerUrl,
-            };
+            window.MonacoEnvironment = { getWorkerUrl: config?.getWorkerUrl };
         }
     }
 }

playwright.config.ts

@@ -22,6 +22,7 @@ export default defineConfig({
     fullyParallel: true,
     /* Fail the build on CI if you accidentally left test.only in the source code. */
     forbidOnly: !!process.env.CI,
+    timeout: 30_000,
     /* Retry on CI only */
     retries: process.env.CI ? 2 : 0,
     /* Opt out of parallel tests on CI. */