From 53bf1995dbcba231d50509625468649a0dc77471 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Wed, 12 Jul 2023 14:51:13 -0700 Subject: [PATCH] chore: do not leak internal page handles after closing page (#24169) Partial fix for https://github.com/microsoft/playwright/issues/6319 After this fix, the following scenario won't leak and the context state (cookies, storage, etc) can be reused by the new page sessions: ```js for (let i = 0; i < 1000; ++i) { const page = await context.newPage(); await page.goto('...'); await page.close('...'); } ``` --- package-lock.json | 35 +++++++++++++++++++ package.json | 1 + .../src/server/dispatchers/dispatcher.ts | 2 ++ .../dispatchers/elementHandlerDispatcher.ts | 23 ++++++++++-- .../src/server/dispatchers/frameDispatcher.ts | 34 ++++++++++-------- .../server/dispatchers/jsHandleDispatcher.ts | 3 +- .../server/dispatchers/networkDispatchers.ts | 24 ++++++++----- .../src/server/dispatchers/pageDispatcher.ts | 10 +++--- packages/playwright-test/src/index.ts | 11 +++--- tests/assets/title.html | 1 + tests/library/channels.spec.ts | 12 ++++--- tests/stress/heap.spec.ts | 34 ++++++++++++++++++ 12 files changed, 148 insertions(+), 42 deletions(-) diff --git a/package-lock.json b/package-lock.json index 152c0a59c201b..6ba0d4d4a00da 100644 --- a/package-lock.json +++ b/package-lock.json @@ -49,6 +49,7 @@ "eslint-plugin-notice": "^0.9.10", "eslint-plugin-react-hooks": "^4.3.0", "formidable": "^2.1.1", + "heapdump": "^0.3.15", "license-checker": "^25.0.1", "mime": "^3.0.0", "ncp": "^2.0.0", @@ -4000,6 +4001,19 @@ "url": "https://github.com/sponsors/ljharb" } }, + "node_modules/heapdump": { + "version": "0.3.15", + "resolved": "https://registry.npmjs.org/heapdump/-/heapdump-0.3.15.tgz", + "integrity": "sha512-n8aSFscI9r3gfhOcAECAtXFaQ1uy4QSke6bnaL+iymYZ/dWs9cqDqHM+rALfsHUwukUbxsdlECZ0pKmJdQ/4OA==", + "dev": true, + "hasInstallScript": true, + "dependencies": { + "nan": "^2.13.2" + }, + "engines": { + "node": ">=0.10.0" + } + }, "node_modules/hexoid": { "version": "1.0.0", "dev": true, @@ -4531,6 +4545,12 @@ "version": "2.1.2", "license": "MIT" }, + "node_modules/nan": { + "version": "2.17.0", + "resolved": "https://registry.npmjs.org/nan/-/nan-2.17.0.tgz", + "integrity": "sha512-2ZTgtl0nJsO0KQCjEpxcIr5D+Yv90plTitZt9JBfQvVJDS5seMl3FOvsh3+9CoYWXf/1l5OaZzzF6nDm4cagaQ==", + "dev": true + }, "node_modules/nanoid": { "version": "3.3.6", "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.6.tgz", @@ -9070,6 +9090,15 @@ "integrity": "sha512-l3LCuF6MgDNwTDKkdYGEihYjt5pRPbEg46rtlmnSPlUbgmB8LOIrKJbYYFBSbnPaJexMKtiPO8hmeRjRz2Td+A==", "dev": true }, + "heapdump": { + "version": "0.3.15", + "resolved": "https://registry.npmjs.org/heapdump/-/heapdump-0.3.15.tgz", + "integrity": "sha512-n8aSFscI9r3gfhOcAECAtXFaQ1uy4QSke6bnaL+iymYZ/dWs9cqDqHM+rALfsHUwukUbxsdlECZ0pKmJdQ/4OA==", + "dev": true, + "requires": { + "nan": "^2.13.2" + } + }, "hexoid": { "version": "1.0.0", "dev": true @@ -9428,6 +9457,12 @@ "ms": { "version": "2.1.2" }, + "nan": { + "version": "2.17.0", + "resolved": "https://registry.npmjs.org/nan/-/nan-2.17.0.tgz", + "integrity": "sha512-2ZTgtl0nJsO0KQCjEpxcIr5D+Yv90plTitZt9JBfQvVJDS5seMl3FOvsh3+9CoYWXf/1l5OaZzzF6nDm4cagaQ==", + "dev": true + }, "nanoid": { "version": "3.3.6", "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.6.tgz", diff --git a/package.json b/package.json index ebf9cb6cf2598..dd4c687a2593a 100644 --- a/package.json +++ b/package.json @@ -84,6 +84,7 @@ "eslint-plugin-notice": "^0.9.10", "eslint-plugin-react-hooks": "^4.3.0", "formidable": "^2.1.1", + "heapdump": "^0.3.15", "license-checker": "^25.0.1", "mime": "^3.0.0", "ncp": "^2.0.0", diff --git a/packages/playwright-core/src/server/dispatchers/dispatcher.ts b/packages/playwright-core/src/server/dispatchers/dispatcher.ts index 6676a01d337e6..54b939542d3a1 100644 --- a/packages/playwright-core/src/server/dispatchers/dispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/dispatcher.ts @@ -80,6 +80,8 @@ export class Dispatcher { const frame = await this._elementHandle.ownerFrame(); - return { frame: frame ? FrameDispatcher.from(this.parentScope() as PageDispatcher, frame) : undefined }; + return { frame: frame ? FrameDispatcher.from(this._browserContextDispatcher(), frame) : undefined }; } async contentFrame(params: channels.ElementHandleContentFrameParams, metadata: CallMetadata): Promise { const frame = await this._elementHandle.contentFrame(); - return { frame: frame ? FrameDispatcher.from(this.parentScope() as PageDispatcher, frame) : undefined }; + return { frame: frame ? FrameDispatcher.from(this._browserContextDispatcher(), frame) : undefined }; } async getAttribute(params: channels.ElementHandleGetAttributeParams, metadata: CallMetadata): Promise { @@ -223,4 +224,20 @@ export class ElementHandleDispatcher extends JSHandleDispatcher implements chann async waitForSelector(params: channels.ElementHandleWaitForSelectorParams, metadata: CallMetadata): Promise { return { element: ElementHandleDispatcher.fromNullable(this.parentScope(), await this._elementHandle.waitForSelector(metadata, params.selector, params)) }; } + + private _browserContextDispatcher(): BrowserContextDispatcher { + const scope = this.parentScope(); + if (scope instanceof BrowserContextDispatcher) + return scope; + if (scope instanceof PageDispatcher) + return scope.parentScope(); + if ((scope instanceof WorkerDispatcher) || (scope instanceof FrameDispatcher)) { + const parentScope = scope.parentScope(); + if (parentScope instanceof BrowserContextDispatcher) + return parentScope; + return parentScope.parentScope(); + } + throw new Error('ElementHandle belongs to unexpected scope'); + } + } diff --git a/packages/playwright-core/src/server/dispatchers/frameDispatcher.ts b/packages/playwright-core/src/server/dispatchers/frameDispatcher.ts index 72c935f9f0608..3e9b4362a4c5e 100644 --- a/packages/playwright-core/src/server/dispatchers/frameDispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/frameDispatcher.ts @@ -26,30 +26,34 @@ import type { CallMetadata } from '../instrumentation'; import type { WritableStreamDispatcher } from './writableStreamDispatcher'; import { assert } from '../../utils'; import path from 'path'; +import type { BrowserContextDispatcher } from './browserContextDispatcher'; import type { PageDispatcher } from './pageDispatcher'; -export class FrameDispatcher extends Dispatcher implements channels.FrameChannel { +export class FrameDispatcher extends Dispatcher implements channels.FrameChannel { _type_Frame = true; private _frame: Frame; + private _browserContextDispatcher: BrowserContextDispatcher; - static from(scope: PageDispatcher, frame: Frame): FrameDispatcher { + static from(scope: BrowserContextDispatcher, frame: Frame): FrameDispatcher { const result = existingDispatcher(frame); return result || new FrameDispatcher(scope, frame); } - static fromNullable(scope: PageDispatcher, frame: Frame | null): FrameDispatcher | undefined { + static fromNullable(scope: BrowserContextDispatcher, frame: Frame | null): FrameDispatcher | undefined { if (!frame) return; return FrameDispatcher.from(scope, frame); } - private constructor(scope: PageDispatcher, frame: Frame) { - super(scope, frame, 'Frame', { + private constructor(scope: BrowserContextDispatcher, frame: Frame) { + const pageDispatcher = existingDispatcher(frame._page); + super(pageDispatcher || scope, frame, 'Frame', { url: frame.url(), name: frame.name(), parentFrame: FrameDispatcher.fromNullable(scope, frame.parentFrame()), loadStates: Array.from(frame._firedLifecycleEvents), }); + this._browserContextDispatcher = scope; this._frame = frame; this.addObjectListener(Frame.Events.AddLifecycle, lifecycleEvent => { this._dispatchEvent('loadstate', { add: lifecycleEvent }); @@ -62,17 +66,17 @@ export class FrameDispatcher extends Dispatcher { - return { response: ResponseDispatcher.fromNullable(this.parentScope().parentScope(), await this._frame.goto(metadata, params.url, params)) }; + return { response: ResponseDispatcher.fromNullable(this._browserContextDispatcher, await this._frame.goto(metadata, params.url, params)) }; } async frameElement(): Promise { - return { element: ElementHandleDispatcher.from(this.parentScope(), await this._frame.frameElement()) }; + return { element: ElementHandleDispatcher.from(this, await this._frame.frameElement()) }; } async evaluateExpression(params: channels.FrameEvaluateExpressionParams, metadata: CallMetadata): Promise { @@ -80,11 +84,11 @@ export class FrameDispatcher extends Dispatcher { - return { handle: ElementHandleDispatcher.fromJSHandle(this.parentScope(), await this._frame.evaluateExpressionHandle(params.expression, { isFunction: params.isFunction }, parseArgument(params.arg))) }; + return { handle: ElementHandleDispatcher.fromJSHandle(this, await this._frame.evaluateExpressionHandle(params.expression, { isFunction: params.isFunction }, parseArgument(params.arg))) }; } async waitForSelector(params: channels.FrameWaitForSelectorParams, metadata: CallMetadata): Promise { - return { element: ElementHandleDispatcher.fromNullable(this.parentScope(), await this._frame.waitForSelector(metadata, params.selector, params)) }; + return { element: ElementHandleDispatcher.fromNullable(this, await this._frame.waitForSelector(metadata, params.selector, params)) }; } async dispatchEvent(params: channels.FrameDispatchEventParams, metadata: CallMetadata): Promise { @@ -100,12 +104,12 @@ export class FrameDispatcher extends Dispatcher { - return { element: ElementHandleDispatcher.fromNullable(this.parentScope(), await this._frame.querySelector(params.selector, params)) }; + return { element: ElementHandleDispatcher.fromNullable(this, await this._frame.querySelector(params.selector, params)) }; } async querySelectorAll(params: channels.FrameQuerySelectorAllParams, metadata: CallMetadata): Promise { const elements = await this._frame.querySelectorAll(params.selector); - return { elements: elements.map(e => ElementHandleDispatcher.from(this.parentScope(), e)) }; + return { elements: elements.map(e => ElementHandleDispatcher.from(this, e)) }; } async queryCount(params: channels.FrameQueryCountParams): Promise { @@ -121,11 +125,11 @@ export class FrameDispatcher extends Dispatcher { - return { element: ElementHandleDispatcher.from(this.parentScope(), await this._frame.addScriptTag(params)) }; + return { element: ElementHandleDispatcher.from(this, await this._frame.addScriptTag(params)) }; } async addStyleTag(params: channels.FrameAddStyleTagParams, metadata: CallMetadata): Promise { - return { element: ElementHandleDispatcher.from(this.parentScope(), await this._frame.addStyleTag(params)) }; + return { element: ElementHandleDispatcher.from(this, await this._frame.addStyleTag(params)) }; } async click(params: channels.FrameClickParams, metadata: CallMetadata): Promise { @@ -249,7 +253,7 @@ export class FrameDispatcher extends Dispatcher { - return { handle: ElementHandleDispatcher.fromJSHandle(this.parentScope(), await this._frame._waitForFunctionExpression(metadata, params.expression, params.isFunction, parseArgument(params.arg), params)) }; + return { handle: ElementHandleDispatcher.fromJSHandle(this, await this._frame._waitForFunctionExpression(metadata, params.expression, params.isFunction, parseArgument(params.arg), params)) }; } async title(params: channels.FrameTitleParams, metadata: CallMetadata): Promise { diff --git a/packages/playwright-core/src/server/dispatchers/jsHandleDispatcher.ts b/packages/playwright-core/src/server/dispatchers/jsHandleDispatcher.ts index 5b338ce5955e4..1b4d7cbcfac63 100644 --- a/packages/playwright-core/src/server/dispatchers/jsHandleDispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/jsHandleDispatcher.ts @@ -21,8 +21,9 @@ import { ElementHandleDispatcher } from './elementHandlerDispatcher'; import { parseSerializedValue, serializeValue } from '../../protocol/serializers'; import type { PageDispatcher, WorkerDispatcher } from './pageDispatcher'; import type { ElectronApplicationDispatcher } from './electronDispatcher'; +import type { FrameDispatcher } from './frameDispatcher'; -export type JSHandleDispatcherParentScope = PageDispatcher | WorkerDispatcher | ElectronApplicationDispatcher; +export type JSHandleDispatcherParentScope = PageDispatcher | FrameDispatcher | WorkerDispatcher | ElectronApplicationDispatcher; export class JSHandleDispatcher extends Dispatcher implements channels.JSHandleChannel { _type_JSHandle = true; diff --git a/packages/playwright-core/src/server/dispatchers/networkDispatchers.ts b/packages/playwright-core/src/server/dispatchers/networkDispatchers.ts index 244c783e825aa..428782b8839ee 100644 --- a/packages/playwright-core/src/server/dispatchers/networkDispatchers.ts +++ b/packages/playwright-core/src/server/dispatchers/networkDispatchers.ts @@ -27,8 +27,9 @@ import type { PageDispatcher } from './pageDispatcher'; import { FrameDispatcher } from './frameDispatcher'; import { WorkerDispatcher } from './pageDispatcher'; -export class RequestDispatcher extends Dispatcher implements channels.RequestChannel { +export class RequestDispatcher extends Dispatcher implements channels.RequestChannel { _type_Request: boolean; + private _browserContextDispatcher: BrowserContextDispatcher; static from(scope: BrowserContextDispatcher, request: Request): RequestDispatcher { const result = existingDispatcher(request); @@ -41,8 +42,13 @@ export class RequestDispatcher extends Dispatcher(page) : null; + const frameDispatcher = frame ? FrameDispatcher.from(scope, frame) : null; + super(pageDispatcher || frameDispatcher || scope, request, 'Request', { + frame: FrameDispatcher.fromNullable(scope, request.frame()), serviceWorker: WorkerDispatcher.fromNullable(scope, request.serviceWorker()), url: request.url(), resourceType: request.resourceType(), @@ -53,6 +59,7 @@ export class RequestDispatcher extends Dispatcher { @@ -60,26 +67,27 @@ export class RequestDispatcher extends Dispatcher { - return { response: ResponseDispatcher.fromNullable(this.parentScope(), await this._object.response()) }; + return { response: ResponseDispatcher.fromNullable(this._browserContextDispatcher, await this._object.response()) }; } } -export class ResponseDispatcher extends Dispatcher implements channels.ResponseChannel { +export class ResponseDispatcher extends Dispatcher implements channels.ResponseChannel { _type_Response = true; static from(scope: BrowserContextDispatcher, response: Response): ResponseDispatcher { const result = existingDispatcher(response); - return result || new ResponseDispatcher(scope, response); + const requestDispatcher = RequestDispatcher.from(scope, response.request()); + return result || new ResponseDispatcher(requestDispatcher, response); } static fromNullable(scope: BrowserContextDispatcher, response: Response | null): ResponseDispatcher | undefined { return response ? ResponseDispatcher.from(scope, response) : undefined; } - private constructor(scope: BrowserContextDispatcher, response: Response) { + private constructor(scope: RequestDispatcher, response: Response) { super(scope, response, 'Response', { // TODO: responses in popups can point to non-reported requests. - request: RequestDispatcher.from(scope, response.request()), + request: scope, url: response.url(), status: response.status(), statusText: response.statusText(), diff --git a/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts b/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts index 28747305706fa..63ecc51b288de 100644 --- a/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts @@ -58,7 +58,7 @@ export class PageDispatcher extends Dispatcher this._dispatchEvent('fileChooser', { - element: ElementHandleDispatcher.from(this, fileChooser.element()), + element: ElementHandleDispatcher.from(mainFrame, fileChooser.element()), isMultiple: fileChooser.isMultiple() })); this.addObjectListener(Page.Events.FrameAttached, frame => this._onFrameAttached(frame)); @@ -297,11 +297,11 @@ export class PageDispatcher extends Dispatcher = ({ const testInfoImpl = testInfo as TestInfoImpl; const videoMode = normalizeVideoMode(video); const captureVideo = shouldCaptureVideo(videoMode, testInfo) && !_reuseContext; - const contexts = new Map(); + const contexts = new Map(); await use(async options => { const hook = hookType(testInfoImpl); @@ -343,9 +343,10 @@ const playwrightFixtures: Fixtures = ({ } } : {}; const context = await browser.newContext({ ...videoOptions, ...options }); - const contextData: { pages: Page[] } = { pages: [] }; + const contextData: { pagesWithVideo: Page[] } = { pagesWithVideo: [] }; contexts.set(context, contextData); - context.on('page', page => contextData.pages.push(page)); + if (captureVideo) + context.on('page', page => contextData.pagesWithVideo.push(page)); return context; }); @@ -361,8 +362,8 @@ const playwrightFixtures: Fixtures = ({ const testFailed = testInfo.status !== testInfo.expectedStatus; const preserveVideo = captureVideo && (videoMode === 'on' || (testFailed && videoMode === 'retain-on-failure') || (videoMode === 'on-first-retry' && testInfo.retry === 1)); if (preserveVideo) { - const { pages } = contexts.get(context)!; - const videos = pages.map(p => p.video()).filter(Boolean) as Video[]; + const { pagesWithVideo: pagesForVideo } = contexts.get(context)!; + const videos = pagesForVideo.map(p => p.video()).filter(Boolean) as Video[]; await Promise.all(videos.map(async v => { try { const savedPath = testInfo.outputPath(`video${counter ? '-' + counter : ''}.webm`); diff --git a/tests/assets/title.html b/tests/assets/title.html index 88a86ce412b07..6e81f4091a453 100644 --- a/tests/assets/title.html +++ b/tests/assets/title.html @@ -1 +1,2 @@ + Woof-Woof diff --git a/tests/library/channels.spec.ts b/tests/library/channels.spec.ts index 19c8133f167e4..0527ef6cf0f51 100644 --- a/tests/library/channels.spec.ts +++ b/tests/library/channels.spec.ts @@ -69,10 +69,11 @@ it('should scope context handles', async ({ browserType, server, expectScopeStat { _guid: 'browser-type', objects: [ { _guid: 'browser', objects: [ { _guid: 'browser-context', objects: [ - { _guid: 'request', objects: [] }, - { _guid: 'response', objects: [] }, { _guid: 'page', objects: [ { _guid: 'frame', objects: [] }, + { _guid: 'request', objects: [ + { _guid: 'response', objects: [] }, + ] }, ] }, { _guid: 'request-context', objects: [] }, { _guid: 'tracing', objects: [] } @@ -204,12 +205,13 @@ it('should not generate dispatchers for subresources w/o listeners', async ({ pa { _guid: 'browser-context', objects: [ { _guid: 'page', objects: [ - { _guid: 'frame', objects: [] } + { _guid: 'frame', objects: [] }, + { _guid: 'request', objects: [ + { _guid: 'response', objects: [] }, + ] }, ] }, - { _guid: 'request', objects: [] }, { _guid: 'request-context', objects: [] }, - { _guid: 'response', objects: [] }, { _guid: 'tracing', objects: [] } ] }, ] diff --git a/tests/stress/heap.spec.ts b/tests/stress/heap.spec.ts index 573ef4bcabc8b..f41e3128ae987 100644 --- a/tests/stress/heap.spec.ts +++ b/tests/stress/heap.spec.ts @@ -47,3 +47,37 @@ test('should not leak server-side objects', async ({ page }) => { expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/browserContext').BrowserContext)).toBe(4); expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/browser').Browser)).toBe(4); }); + +test('should not leak dispatchers after closing page', async ({ context, server }) => { + const pages = []; + const COUNT = 5; + for (let i = 0; i < COUNT; ++i) { + const page = await context.newPage(); + // ensure listeners are registered + page.on('console', () => {}); + await page.goto(server.PREFIX + '/title.html'); + await page.evaluate(async i => { + console.log('message', i); + }, i); + pages.push(page); + } + + expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/page').Page)).toBe(COUNT); + expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/dispatchers/networkDispatchers').RequestDispatcher)).toBe(COUNT); + expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/dispatchers/networkDispatchers').ResponseDispatcher)).toBe(COUNT); + expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/dispatchers/consoleMessageDispatcher').ConsoleMessageDispatcher)).toBe(COUNT); + + for (const page of pages) + await page.close(); + pages.length = 0; + + expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/page').Page)).toBe(0); + expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/dispatchers/networkDispatchers').RequestDispatcher)).toBe(0); + expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/dispatchers/networkDispatchers').ResponseDispatcher)).toBe(0); + expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/dispatchers/consoleMessageDispatcher').ConsoleMessageDispatcher)).toBe(0); + + expect(await queryObjectCount(require('../../packages/playwright-core/lib/client/page').Page)).toBeLessThan(COUNT); + expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/page').Page)).toBe(0); + expect(await queryObjectCount(require('../../packages/playwright-core/lib/client/network').Request)).toBe(0); + expect(await queryObjectCount(require('../../packages/playwright-core/lib/client/network').Response)).toBe(0); +});