From f07f907c7cbcf89ece8d508135ba4ab92833c578 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Wed, 23 Oct 2024 10:24:53 -0700 Subject: [PATCH 1/2] cherry-pick(#33240): fix(recorder): do not leak when instantiated in snapshots --- .../src/server/injected/injectedScript.ts | 11 ++++++-- .../src/server/injected/recorder/recorder.ts | 2 +- packages/trace-viewer/src/ui/snapshotTab.tsx | 4 +++ tests/library/trace-viewer.spec.ts | 27 +++++++++++++++++++ 4 files changed, 41 insertions(+), 3 deletions(-) diff --git a/packages/playwright-core/src/server/injected/injectedScript.ts b/packages/playwright-core/src/server/injected/injectedScript.ts index 69fe959f81847..a1f4772e858b1 100644 --- a/packages/playwright-core/src/server/injected/injectedScript.ts +++ b/packages/playwright-core/src/server/injected/injectedScript.ts @@ -147,13 +147,19 @@ export class InjectedScript { builtinSetTimeout(callback: Function, timeout: number) { if (this.window.__pwClock?.builtin) return this.window.__pwClock.builtin.setTimeout(callback, timeout); - return setTimeout(callback, timeout); + return this.window.setTimeout(callback, timeout); + } + + builtinClearTimeout(timeout: number | undefined) { + if (this.window.__pwClock?.builtin) + return this.window.__pwClock.builtin.clearTimeout(timeout); + return this.window.clearTimeout(timeout); } builtinRequestAnimationFrame(callback: FrameRequestCallback) { if (this.window.__pwClock?.builtin) return this.window.__pwClock.builtin.requestAnimationFrame(callback); - return requestAnimationFrame(callback); + return this.window.requestAnimationFrame(callback); } eval(expression: string): any { @@ -1543,6 +1549,7 @@ declare global { __pwClock?: { builtin: { setTimeout: Window['setTimeout'], + clearTimeout: Window['clearTimeout'], requestAnimationFrame: Window['requestAnimationFrame'], } } diff --git a/packages/playwright-core/src/server/injected/recorder/recorder.ts b/packages/playwright-core/src/server/injected/recorder/recorder.ts index cdc29a105063e..f2a6e5e8ab6af 100644 --- a/packages/playwright-core/src/server/injected/recorder/recorder.ts +++ b/packages/playwright-core/src/server/injected/recorder/recorder.ts @@ -1051,7 +1051,7 @@ export class Recorder { recreationInterval = this.injectedScript.builtinSetTimeout(recreate, 500); }; recreationInterval = this.injectedScript.builtinSetTimeout(recreate, 500); - this._listeners.push(() => clearInterval(recreationInterval)); + this._listeners.push(() => this.injectedScript.builtinClearTimeout(recreationInterval)); this.overlay?.install(); this.document.adoptedStyleSheets.push(this._stylesheet); diff --git a/packages/trace-viewer/src/ui/snapshotTab.tsx b/packages/trace-viewer/src/ui/snapshotTab.tsx index 82e4282c6fdd1..48f911a97a659 100644 --- a/packages/trace-viewer/src/ui/snapshotTab.tsx +++ b/packages/trace-viewer/src/ui/snapshotTab.tsx @@ -315,6 +315,10 @@ function createRecorders(recorders: { recorder: Recorder, frameSelector: string const recorder = new Recorder(injectedScript); win._injectedScript = injectedScript; win._recorder = { recorder, frameSelector: parentFrameSelector }; + if (isUnderTest) { + (window as any)._weakRecordersForTest = (window as any)._weakRecordersForTest || new Set(); + (window as any)._weakRecordersForTest.add(new WeakRef(recorder)); + } } recorders.push(win._recorder); diff --git a/tests/library/trace-viewer.spec.ts b/tests/library/trace-viewer.spec.ts index 71689fa4c14f8..7c2646960120f 100644 --- a/tests/library/trace-viewer.spec.ts +++ b/tests/library/trace-viewer.spec.ts @@ -1390,6 +1390,33 @@ test('should show baseURL in metadata pane', { await expect(traceViewer.metadataTab).toContainText('baseURL:https://example.com'); }); +test('should not leak recorders', { + annotation: { type: 'issue', description: 'https://github.com/microsoft/playwright/issues/33086' }, +}, async ({ showTraceViewer }) => { + const traceViewer = await showTraceViewer([traceFile]); + + const counts = async () => { + return await traceViewer.page.evaluate(() => { + const weakSet = (window as any)._weakRecordersForTest || new Set(); + const weakList = [...weakSet]; + const aliveList = weakList.filter(r => !!r.deref()); + return { total: weakList.length, alive: aliveList.length }; + }); + }; + + await traceViewer.snapshotFrame('page.goto'); + await traceViewer.snapshotFrame('page.evaluate'); + await traceViewer.page.requestGC(); + await expect.poll(() => counts()).toEqual({ total: 4, alive: 1 }); + + await traceViewer.snapshotFrame('page.setContent'); + await traceViewer.snapshotFrame('page.goto'); + await traceViewer.snapshotFrame('page.evaluate'); + await traceViewer.snapshotFrame('page.setContent'); + await traceViewer.page.requestGC(); + await expect.poll(() => counts()).toEqual({ total: 8, alive: 1 }); +}); + test('should serve css without content-type', async ({ page, runAndTrace, server }) => { server.setRoute('/one-style.css', (req, res) => { res.writeHead(200); From c6f2730473d8cc1eb200cbb8b32ef86c0c72e6a6 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Thu, 24 Oct 2024 02:47:34 -0700 Subject: [PATCH 2/2] test: unflake "should not leak recorders" (#33264) --- tests/library/trace-viewer.spec.ts | 31 ++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/tests/library/trace-viewer.spec.ts b/tests/library/trace-viewer.spec.ts index 7c2646960120f..818abb744f8d2 100644 --- a/tests/library/trace-viewer.spec.ts +++ b/tests/library/trace-viewer.spec.ts @@ -1395,26 +1395,37 @@ test('should not leak recorders', { }, async ({ showTraceViewer }) => { const traceViewer = await showTraceViewer([traceFile]); - const counts = async () => { + const aliveCount = async () => { return await traceViewer.page.evaluate(() => { const weakSet = (window as any)._weakRecordersForTest || new Set(); const weakList = [...weakSet]; const aliveList = weakList.filter(r => !!r.deref()); - return { total: weakList.length, alive: aliveList.length }; + return aliveList.length; }); }; - await traceViewer.snapshotFrame('page.goto'); - await traceViewer.snapshotFrame('page.evaluate'); + await expect(traceViewer.snapshotContainer.contentFrame().locator('body')).toContainText(`Hi, I'm frame`); + + const frame1 = await traceViewer.snapshotFrame('page.goto'); + await expect(frame1.locator('body')).toContainText('Hello world'); + + const frame2 = await traceViewer.snapshotFrame('page.evaluate'); + await expect(frame2.locator('button')).toBeVisible(); + await traceViewer.page.requestGC(); - await expect.poll(() => counts()).toEqual({ total: 4, alive: 1 }); + await expect.poll(() => aliveCount()).toBeLessThanOrEqual(2); // two snapshot iframes + + const frame3 = await traceViewer.snapshotFrame('page.setViewportSize'); + await expect(frame3.locator('body')).toContainText(`Hi, I'm frame`); + + const frame4 = await traceViewer.snapshotFrame('page.goto'); + await expect(frame4.locator('body')).toContainText('Hello world'); + + const frame5 = await traceViewer.snapshotFrame('page.evaluate'); + await expect(frame5.locator('button')).toBeVisible(); - await traceViewer.snapshotFrame('page.setContent'); - await traceViewer.snapshotFrame('page.goto'); - await traceViewer.snapshotFrame('page.evaluate'); - await traceViewer.snapshotFrame('page.setContent'); await traceViewer.page.requestGC(); - await expect.poll(() => counts()).toEqual({ total: 8, alive: 1 }); + await expect.poll(() => aliveCount()).toBeLessThanOrEqual(2); // two snapshot iframes }); test('should serve css without content-type', async ({ page, runAndTrace, server }) => {