From bcb6860ef59a6a88f52aa98961aebc40492ba4fa Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Thu, 26 Sep 2024 19:33:07 -0700 Subject: [PATCH] chore: fix cross browser leak tests (#32843) --- .../playwright-core/src/client/jsHandle.ts | 7 -- .../playwright-core/src/protocol/validator.ts | 6 - .../src/server/bidi/bidiExecutionContext.ts | 4 - .../src/server/chromium/crExecutionContext.ts | 8 -- .../server/dispatchers/jsHandleDispatcher.ts | 4 - .../src/server/firefox/ffExecutionContext.ts | 4 - .../playwright-core/src/server/javascript.ts | 11 -- .../src/server/webkit/wkExecutionContext.ts | 4 - packages/protocol/src/channels.ts | 6 - packages/protocol/src/protocol.yml | 4 - tests/page/page-leaks.spec.ts | 117 +++++++++--------- tests/page/page-object-count.spec.ts | 25 ---- 12 files changed, 57 insertions(+), 143 deletions(-) delete mode 100644 tests/page/page-object-count.spec.ts diff --git a/packages/playwright-core/src/client/jsHandle.ts b/packages/playwright-core/src/client/jsHandle.ts index bb85894870811..8577400f3df1e 100644 --- a/packages/playwright-core/src/client/jsHandle.ts +++ b/packages/playwright-core/src/client/jsHandle.ts @@ -78,13 +78,6 @@ export class JSHandle extends ChannelOwner im } } - async _objectCount() { - return await this._wrapApiCall(async () => { - const { count } = await this._channel.objectCount(); - return count; - }); - } - override toString(): string { return this._preview; } diff --git a/packages/playwright-core/src/protocol/validator.ts b/packages/playwright-core/src/protocol/validator.ts index 4069b906c76bc..c8cd65e47e3ca 100644 --- a/packages/playwright-core/src/protocol/validator.ts +++ b/packages/playwright-core/src/protocol/validator.ts @@ -1843,12 +1843,6 @@ scheme.JSHandleJsonValueResult = tObject({ value: tType('SerializedValue'), }); scheme.ElementHandleJsonValueResult = tType('JSHandleJsonValueResult'); -scheme.JSHandleObjectCountParams = tOptional(tObject({})); -scheme.ElementHandleObjectCountParams = tType('JSHandleObjectCountParams'); -scheme.JSHandleObjectCountResult = tObject({ - count: tNumber, -}); -scheme.ElementHandleObjectCountResult = tType('JSHandleObjectCountResult'); scheme.ElementHandleInitializer = tObject({ preview: tString, }); diff --git a/packages/playwright-core/src/server/bidi/bidiExecutionContext.ts b/packages/playwright-core/src/server/bidi/bidiExecutionContext.ts index c037ba44b4b97..201b3f999db11 100644 --- a/packages/playwright-core/src/server/bidi/bidiExecutionContext.ts +++ b/packages/playwright-core/src/server/bidi/bidiExecutionContext.ts @@ -122,10 +122,6 @@ export class BidiExecutionContext implements js.ExecutionContextDelegate { }); } - objectCount(objectId: js.ObjectId): Promise { - throw new Error('Method not implemented.'); - } - async rawCallFunction(functionDeclaration: string, arg: bidi.Script.LocalValue): Promise { const response = await this._session.send('script.callFunction', { functionDeclaration, diff --git a/packages/playwright-core/src/server/chromium/crExecutionContext.ts b/packages/playwright-core/src/server/chromium/crExecutionContext.ts index c6952f5c82b38..d54505d2836ef 100644 --- a/packages/playwright-core/src/server/chromium/crExecutionContext.ts +++ b/packages/playwright-core/src/server/chromium/crExecutionContext.ts @@ -102,14 +102,6 @@ export class CRExecutionContext implements js.ExecutionContextDelegate { async releaseHandle(objectId: js.ObjectId): Promise { await releaseObject(this._client, objectId); } - - async objectCount(objectId: js.ObjectId): Promise { - const result = await this._client.send('Runtime.queryObjects', { - prototypeObjectId: objectId - }); - const match = result.objects.description!.match(/Array\((\d+)\)/)!; - return +match[1]; - } } function rewriteError(error: Error): Protocol.Runtime.evaluateReturnValue { diff --git a/packages/playwright-core/src/server/dispatchers/jsHandleDispatcher.ts b/packages/playwright-core/src/server/dispatchers/jsHandleDispatcher.ts index 07e9d38ee5c2b..33960e72d5e74 100644 --- a/packages/playwright-core/src/server/dispatchers/jsHandleDispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/jsHandleDispatcher.ts @@ -63,10 +63,6 @@ export class JSHandleDispatcher extends Dispatcher { - return { count: await this._object.objectCount() }; - } - async dispose(_: any, metadata: CallMetadata) { metadata.potentiallyClosesScope = true; this._object.dispose(); diff --git a/packages/playwright-core/src/server/firefox/ffExecutionContext.ts b/packages/playwright-core/src/server/firefox/ffExecutionContext.ts index 3a6b931a47c17..ba981da9426e2 100644 --- a/packages/playwright-core/src/server/firefox/ffExecutionContext.ts +++ b/packages/playwright-core/src/server/firefox/ffExecutionContext.ts @@ -98,10 +98,6 @@ export class FFExecutionContext implements js.ExecutionContextDelegate { objectId }); } - - objectCount(objectId: js.ObjectId): Promise { - throw new Error('Method not implemented in Firefox.'); - } } function checkException(exceptionDetails?: Protocol.Runtime.ExceptionDetails) { diff --git a/packages/playwright-core/src/server/javascript.ts b/packages/playwright-core/src/server/javascript.ts index 663df78b5b86e..40dee4888a481 100644 --- a/packages/playwright-core/src/server/javascript.ts +++ b/packages/playwright-core/src/server/javascript.ts @@ -58,7 +58,6 @@ export interface ExecutionContextDelegate { getProperties(context: ExecutionContext, objectId: ObjectId): Promise>; createHandle(context: ExecutionContext, remoteObject: RemoteObject): JSHandle; releaseHandle(objectId: ObjectId): Promise; - objectCount(objectId: ObjectId): Promise; } export class ExecutionContext extends SdkObject { @@ -126,10 +125,6 @@ export class ExecutionContext extends SdkObject { return this._utilityScriptPromise; } - async objectCount(objectId: ObjectId): Promise { - return this._delegate.objectCount(objectId); - } - async doSlowMo() { // overridden in FrameExecutionContext } @@ -246,12 +241,6 @@ export class JSHandle extends SdkObject { if (this._previewCallback) this._previewCallback(preview); } - - async objectCount(): Promise { - if (!this._objectId) - throw new Error('Can only count objects for a handle that points to the constructor prototype'); - return this._context.objectCount(this._objectId); - } } export async function evaluate(context: ExecutionContext, returnByValue: boolean, pageFunction: Function | string, ...args: any[]): Promise { diff --git a/packages/playwright-core/src/server/webkit/wkExecutionContext.ts b/packages/playwright-core/src/server/webkit/wkExecutionContext.ts index 8d6bbff084e05..4416df7a9e45c 100644 --- a/packages/playwright-core/src/server/webkit/wkExecutionContext.ts +++ b/packages/playwright-core/src/server/webkit/wkExecutionContext.ts @@ -116,10 +116,6 @@ export class WKExecutionContext implements js.ExecutionContextDelegate { async releaseHandle(objectId: js.ObjectId): Promise { await this._session.send('Runtime.releaseObject', { objectId }); } - - objectCount(objectId: js.ObjectId): Promise { - throw new Error('Method not implemented in WebKit.'); - } } function potentiallyUnserializableValue(remoteObject: Protocol.Runtime.RemoteObject): any { diff --git a/packages/protocol/src/channels.ts b/packages/protocol/src/channels.ts index b99490a95565b..5a4985c253be6 100644 --- a/packages/protocol/src/channels.ts +++ b/packages/protocol/src/channels.ts @@ -3226,7 +3226,6 @@ export interface JSHandleChannel extends JSHandleEventTarget, Channel { getPropertyList(params?: JSHandleGetPropertyListParams, metadata?: CallMetadata): Promise; getProperty(params: JSHandleGetPropertyParams, metadata?: CallMetadata): Promise; jsonValue(params?: JSHandleJsonValueParams, metadata?: CallMetadata): Promise; - objectCount(params?: JSHandleObjectCountParams, metadata?: CallMetadata): Promise; } export type JSHandlePreviewUpdatedEvent = { preview: string, @@ -3278,11 +3277,6 @@ export type JSHandleJsonValueOptions = {}; export type JSHandleJsonValueResult = { value: SerializedValue, }; -export type JSHandleObjectCountParams = {}; -export type JSHandleObjectCountOptions = {}; -export type JSHandleObjectCountResult = { - count: number, -}; export interface JSHandleEvents { 'previewUpdated': JSHandlePreviewUpdatedEvent; diff --git a/packages/protocol/src/protocol.yml b/packages/protocol/src/protocol.yml index e9891adf6bd05..c89c2b96f2865 100644 --- a/packages/protocol/src/protocol.yml +++ b/packages/protocol/src/protocol.yml @@ -2488,10 +2488,6 @@ JSHandle: returns: value: SerializedValue - objectCount: - returns: - count: number - events: previewUpdated: diff --git a/tests/page/page-leaks.spec.ts b/tests/page/page-leaks.spec.ts index ce356f1e12365..abd33e651ecfe 100644 --- a/tests/page/page-leaks.spec.ts +++ b/tests/page/page-leaks.spec.ts @@ -41,16 +41,36 @@ function leakedJSHandles(): string { return lines.join('\n'); } -async function objectCounts(pageImpl, constructorName: string): Promise<{ main: number, utility: number }> { +async function weakRefObjects(pageImpl: any, selector: string) { + for (const world of ['main', 'utility']) { + const context = await pageImpl.mainFrame()._context(world); + await context.evaluate(selector => { + const elements = document.querySelectorAll(selector); + globalThis.weakRefs = globalThis.weakRefs || []; + for (const element of elements) + globalThis.weakRefs.push(new WeakRef(element)); + }, selector); + } +} + +async function weakRefCount(pageImpl): Promise<{ main: number, utility: number }> { const result = { main: 0, utility: 0 }; for (const world of ['main', 'utility']) { + await pageImpl.requestGC(); const context = await pageImpl.mainFrame()._context(world); - const prototype = await context.evaluateHandle(name => (window as any)[name].prototype, constructorName); - result[world] = await prototype.objectCount(); + result[world] = await context.evaluate(() => globalThis.weakRefs.filter(r => !!r.deref()).length); } return result; } +async function checkWeakRefs(pageImpl, from: number, to: number) { + await expect(async () => { + const counts = await weakRefCount(pageImpl); + expect(counts.main + counts.utility).toBeGreaterThanOrEqual(from); + expect(counts.main + counts.utility).toBeLessThan(to); + }).toPass(); +} + test.beforeEach(() => { (globalThis as any).leakedJSHandles = new Map(); }); @@ -59,14 +79,12 @@ test.afterEach(() => { (globalThis as any).leakedJSHandles = null; }); -test('click should not leak', async ({ page, browserName, toImpl }) => { +test('click should not leak', async ({ page, toImpl }) => { await page.setContent(`
`); - // Create JS wrappers for static elements. - await page.evaluate(() => document.querySelectorAll('button')); for (let i = 0; i < 25; ++i) { await page.evaluate(i => { @@ -74,24 +92,19 @@ test('click should not leak', async ({ page, browserName, toImpl }) => { element.textContent = 'dynamic ' + i; document.getElementById('buttons').appendChild(element); }, i); - await page.locator('#buttons > button').click(); - await page.evaluate(() => { - document.getElementById('buttons').textContent = ''; - }); + await page.locator('#buttons > button').last().click(); } - expect(leakedJSHandles()).toBeFalsy(); + await weakRefObjects(toImpl(page), 'button'); + await page.evaluate(() => { + document.getElementById('buttons').textContent = ''; + }); - if (browserName === 'chromium') { - await expect(async () => { - const counts = await objectCounts(toImpl(page), 'HTMLButtonElement'); - expect(counts.main + counts.utility).toBeGreaterThanOrEqual(2); - expect(counts.main + counts.utility).toBeLessThan(25); - }).toPass(); - } + expect(leakedJSHandles()).toBeFalsy(); + await checkWeakRefs(toImpl(page), 2, 25); }); -test('fill should not leak', async ({ page, mode, browserName, toImpl }) => { +test('fill should not leak', async ({ page, mode, toImpl }) => { test.skip(mode !== 'default'); await page.setContent(` @@ -99,32 +112,25 @@ test('fill should not leak', async ({ page, mode, browserName, toImpl }) => {
`); - // Create JS wrappers for static elements. - await page.evaluate(() => document.querySelectorAll('input')); for (let i = 0; i < 25; ++i) { - await page.evaluate(i => { + await page.evaluate(() => { const element = document.createElement('input'); document.getElementById('inputs').appendChild(element); - }, i); - await page.locator('#inputs > input').fill('input ' + i); - await page.evaluate(() => { - document.getElementById('inputs').textContent = ''; }); + await page.locator('#inputs > input').last().fill('input ' + i); } - expect(leakedJSHandles()).toBeFalsy(); + await weakRefObjects(toImpl(page), 'input'); + await page.evaluate(() => { + document.getElementById('inputs').textContent = ''; + }); - if (browserName === 'chromium') { - await expect(async () => { - const counts = await objectCounts(toImpl(page), 'HTMLInputElement'); - expect(counts.main + counts.utility).toBeGreaterThanOrEqual(2); - expect(counts.main + counts.utility).toBeLessThan(25); - }).toPass(); - } + expect(leakedJSHandles()).toBeFalsy(); + await checkWeakRefs(toImpl(page), 2, 25); }); -test('expect should not leak', async ({ page, mode, browserName, toImpl }) => { +test('expect should not leak', async ({ page, mode, toImpl }) => { test.skip(mode !== 'default'); await page.setContent(` @@ -132,6 +138,7 @@ test('expect should not leak', async ({ page, mode, browserName, toImpl }) => {
`); + await weakRefObjects(toImpl(page), 'button'); for (let i = 0; i < 25; ++i) { await page.evaluate(i => { @@ -139,24 +146,19 @@ test('expect should not leak', async ({ page, mode, browserName, toImpl }) => { element.textContent = 'dynamic ' + i; document.getElementById('buttons').appendChild(element); }, i); - await expect(page.locator('#buttons > button')).toBeVisible(); - await page.evaluate(() => { - document.getElementById('buttons').textContent = ''; - }); + await expect(page.locator('#buttons > button').last()).toBeVisible(); } - expect(leakedJSHandles()).toBeFalsy(); + await weakRefObjects(toImpl(page), 'button'); + await page.evaluate(() => { + document.getElementById('buttons').textContent = ''; + }); - if (browserName === 'chromium') { - await expect(async () => { - const counts = await objectCounts(toImpl(page), 'HTMLButtonElement'); - expect(counts.main + counts.utility).toBeGreaterThanOrEqual(2); - expect(counts.main + counts.utility).toBeLessThan(25); - }).toPass(); - } + expect(leakedJSHandles()).toBeFalsy(); + await checkWeakRefs(toImpl(page), 2, 25); }); -test('waitFor should not leak', async ({ page, mode, browserName, toImpl }) => { +test('waitFor should not leak', async ({ page, mode, toImpl }) => { test.skip(mode !== 'default'); await page.setContent(` @@ -171,19 +173,14 @@ test('waitFor should not leak', async ({ page, mode, browserName, toImpl }) => { element.textContent = 'dynamic ' + i; document.getElementById('buttons').appendChild(element); }, i); - await page.locator('#buttons > button').waitFor(); - await page.evaluate(() => { - document.getElementById('buttons').textContent = ''; - }); + await page.locator('#buttons > button').last().waitFor(); } - expect(leakedJSHandles()).toBeFalsy(); + await weakRefObjects(toImpl(page), 'button'); + await page.evaluate(() => { + document.getElementById('buttons').textContent = ''; + }); - if (browserName === 'chromium') { - await expect(async () => { - const counts = await objectCounts(toImpl(page), 'HTMLButtonElement'); - expect(counts.main + counts.utility).toBeGreaterThanOrEqual(2); - expect(counts.main + counts.utility).toBeLessThan(25); - }).toPass(); - } + expect(leakedJSHandles()).toBeFalsy(); + await checkWeakRefs(toImpl(page), 2, 25); }); diff --git a/tests/page/page-object-count.spec.ts b/tests/page/page-object-count.spec.ts deleted file mode 100644 index 57377b76e73dd..0000000000000 --- a/tests/page/page-object-count.spec.ts +++ /dev/null @@ -1,25 +0,0 @@ -/** - * Copyright (c) Microsoft Corporation. - * - * 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 { test, expect } from './pageTest'; - -test('should count objects', async ({ page, browserName }) => { - test.skip(browserName !== 'chromium'); - await page.setContent(''); - await page.evaluate(() => document.querySelectorAll('button')); - const proto = await page.evaluateHandle(() => HTMLButtonElement.prototype); - expect(await (proto as any)._objectCount()).toBe(1); -});