From a052211dbf09d4d5cd4d5283a54e5570ca481130 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Mon, 9 May 2022 06:44:20 -0800 Subject: [PATCH] chore: do not reset internal bindings for reuse (#14019) --- .../src/client/browserContext.ts | 5 +++- packages/playwright-core/src/client/page.ts | 5 +++- .../src/server/browserContext.ts | 5 +++- .../src/server/chromium/crPage.ts | 12 ++++++---- .../src/server/firefox/ffBrowser.ts | 2 ++ .../src/server/injected/consoleApi.ts | 4 ++-- .../src/server/injected/recorder.ts | 24 +++++++++---------- packages/playwright-core/src/server/page.ts | 5 +++- .../playwright-core/src/server/recorder.ts | 12 +++++----- packages/playwright-test/src/mount.ts | 21 ++++++++-------- .../browsercontext-expose-function.spec.ts | 23 ++++++++++++++++++ tests/page/page-expose-function.spec.ts | 23 ++++++++++++++++++ 12 files changed, 102 insertions(+), 39 deletions(-) diff --git a/packages/playwright-core/src/client/browserContext.ts b/packages/playwright-core/src/client/browserContext.ts index b1a34fc051..18dc829d20 100644 --- a/packages/playwright-core/src/client/browserContext.ts +++ b/packages/playwright-core/src/client/browserContext.ts @@ -253,7 +253,10 @@ export class BrowserContext extends ChannelOwner } async _removeExposedBindings() { - this._bindings.clear(); + for (const key of this._bindings.keys()) { + if (!key.startsWith('__pw_')) + this._bindings.delete(key); + } await this._channel.removeExposedBindings(); } diff --git a/packages/playwright-core/src/client/page.ts b/packages/playwright-core/src/client/page.ts index c0e1b94339..8a03f43d72 100644 --- a/packages/playwright-core/src/client/page.ts +++ b/packages/playwright-core/src/client/page.ts @@ -333,7 +333,10 @@ export class Page extends ChannelOwner implements api.Page } async _removeExposedBindings() { - this._bindings.clear(); + for (const key of this._bindings.keys()) { + if (!key.startsWith('__pw_')) + this._bindings.delete(key); + } await this._channel.removeExposedBindings(); } diff --git a/packages/playwright-core/src/server/browserContext.ts b/packages/playwright-core/src/server/browserContext.ts index ecdcffe1f4..c9b8299173 100644 --- a/packages/playwright-core/src/server/browserContext.ts +++ b/packages/playwright-core/src/server/browserContext.ts @@ -196,7 +196,10 @@ export abstract class BrowserContext extends SdkObject { } async removeExposedBindings() { - this._pageBindings.clear(); + for (const key of this._pageBindings.keys()) { + if (!key.startsWith('__pw')) + this._pageBindings.delete(key); + } await this.doRemoveExposedBindings(); } diff --git a/packages/playwright-core/src/server/chromium/crPage.ts b/packages/playwright-core/src/server/chromium/crPage.ts index 8c936d7853..0ed9ae5594 100644 --- a/packages/playwright-core/src/server/chromium/crPage.ts +++ b/packages/playwright-core/src/server/chromium/crPage.ts @@ -825,13 +825,17 @@ class FrameSession { this._client.send('Page.addScriptToEvaluateOnNewDocument', { source: binding.source }) ]); this._exposedBindingNames.push(binding.name); - this._evaluateOnNewDocumentIdentifiers.push(response.identifier); + if (!binding.name.startsWith('__pw')) + this._evaluateOnNewDocumentIdentifiers.push(response.identifier); } async _removeExposedBindings() { - const names = this._exposedBindingNames; - this._exposedBindingNames = []; - await Promise.all(names.map(name => this._client.send('Runtime.removeBinding', { name }))); + const toRetain: string[] = []; + const toRemove: string[] = []; + for (const name of this._exposedBindingNames) + (name.startsWith('__pw_') ? toRetain : toRemove).push(name); + this._exposedBindingNames = toRetain; + await Promise.all(toRemove.map(name => this._client.send('Runtime.removeBinding', { name }))); } async _onBindingCalled(event: Protocol.Runtime.bindingCalledPayload) { diff --git a/packages/playwright-core/src/server/firefox/ffBrowser.ts b/packages/playwright-core/src/server/firefox/ffBrowser.ts index 3483e1c82a..f257bba366 100644 --- a/packages/playwright-core/src/server/firefox/ffBrowser.ts +++ b/packages/playwright-core/src/server/firefox/ffBrowser.ts @@ -335,6 +335,8 @@ export class FFBrowserContext extends BrowserContext { async doRemoveExposedBindings() { // TODO: implement me. + // This is not a critical problem, what ends up happening is + // an old binding will be restored upon page reload and will point nowhere. } async doUpdateRequestInterception(): Promise { diff --git a/packages/playwright-core/src/server/injected/consoleApi.ts b/packages/playwright-core/src/server/injected/consoleApi.ts index fb3a31e641..64d7db02c6 100644 --- a/packages/playwright-core/src/server/injected/consoleApi.ts +++ b/packages/playwright-core/src/server/injected/consoleApi.ts @@ -60,7 +60,7 @@ declare global { interface Window { playwright?: ConsoleAPIInterface; inspect: (element: Element | undefined) => void; - _playwrightResume: () => Promise; + __pw_resume: () => Promise; } } @@ -108,7 +108,7 @@ class ConsoleAPI { } private _resume() { - window._playwrightResume().catch(() => {}); + window.__pw_resume().catch(() => {}); } } diff --git a/packages/playwright-core/src/server/injected/recorder.ts b/packages/playwright-core/src/server/injected/recorder.ts index 363562f2c4..095f8d9b8d 100644 --- a/packages/playwright-core/src/server/injected/recorder.ts +++ b/packages/playwright-core/src/server/injected/recorder.ts @@ -23,11 +23,11 @@ import { Highlight } from '../injected/highlight'; declare module globalThis { - let _playwrightRecorderPerformAction: (action: actions.Action) => Promise; - let _playwrightRecorderRecordAction: (action: actions.Action) => Promise; - let _playwrightRecorderState: () => Promise; - let _playwrightRecorderSetSelector: (selector: string) => Promise; - let _playwrightRefreshOverlay: () => void; + let __pw_recorderPerformAction: (action: actions.Action) => Promise; + let __pw_recorderRecordAction: (action: actions.Action) => Promise; + let __pw_recorderState: () => Promise; + let __pw_recorderSetSelector: (selector: string) => Promise; + let __pw_refreshOverlay: () => void; } class Recorder { @@ -51,10 +51,10 @@ class Recorder { this._refreshListenersIfNeeded(); injectedScript.onGlobalListenersRemoved.add(() => this._refreshListenersIfNeeded()); - globalThis._playwrightRefreshOverlay = () => { + globalThis.__pw_refreshOverlay = () => { this._pollRecorderMode().catch(e => console.log(e)); // eslint-disable-line no-console }; - globalThis._playwrightRefreshOverlay(); + globalThis.__pw_refreshOverlay(); if (injectedScript.isUnderTest) console.error('Recorder script ready for test'); // eslint-disable-line no-console } @@ -88,7 +88,7 @@ class Recorder { const pollPeriod = 1000; if (this._pollRecorderModeTimer) clearTimeout(this._pollRecorderModeTimer); - const state = await globalThis._playwrightRecorderState().catch(e => null); + const state = await globalThis.__pw_recorderState().catch(e => null); if (!state) { this._pollRecorderModeTimer = setTimeout(() => this._pollRecorderMode(), pollPeriod); return; @@ -154,7 +154,7 @@ class Recorder { private _onClick(event: MouseEvent) { if (this._mode === 'inspecting') - globalThis._playwrightRecorderSetSelector(this._hoveredModel ? this._hoveredModel.selector : ''); + globalThis.__pw_recorderSetSelector(this._hoveredModel ? this._hoveredModel.selector : ''); if (this._shouldIgnoreMouseEvent(event)) return; if (this._actionInProgress(event)) @@ -276,7 +276,7 @@ class Recorder { } if (elementType === 'file') { - globalThis._playwrightRecorderRecordAction({ + globalThis.__pw_recorderRecordAction({ name: 'setInputFiles', selector: this._activeModel!.selector, signals: [], @@ -288,7 +288,7 @@ class Recorder { // Non-navigating actions are simply recorded by Playwright. if (this._consumedDueWrongTarget(event)) return; - globalThis._playwrightRecorderRecordAction({ + globalThis.__pw_recorderRecordAction({ name: 'fill', selector: this._activeModel!.selector, signals: [], @@ -388,7 +388,7 @@ class Recorder { private async _performAction(action: actions.Action) { this._clearHighlight(); this._performingAction = true; - await globalThis._playwrightRecorderPerformAction(action).catch(() => {}); + await globalThis.__pw_recorderPerformAction(action).catch(() => {}); this._performingAction = false; // Action could have changed DOM, update hovered model selectors. diff --git a/packages/playwright-core/src/server/page.ts b/packages/playwright-core/src/server/page.ts index ab2574567e..2317f55812 100644 --- a/packages/playwright-core/src/server/page.ts +++ b/packages/playwright-core/src/server/page.ts @@ -316,7 +316,10 @@ export class Page extends SdkObject { } async removeExposedBindings() { - this._pageBindings.clear(); + for (const key of this._pageBindings.keys()) { + if (!key.startsWith('__pw')) + this._pageBindings.delete(key); + } await this._delegate.removeExposedBindings(); } diff --git a/packages/playwright-core/src/server/recorder.ts b/packages/playwright-core/src/server/recorder.ts index 0e56dbb047..8b7df7bc2e 100644 --- a/packages/playwright-core/src/server/recorder.ts +++ b/packages/playwright-core/src/server/recorder.ts @@ -130,7 +130,7 @@ export class Recorder implements InstrumentationListener { this._recorderApp?.setFileIfNeeded(data.primaryFileName); }); - await this._context.exposeBinding('_playwrightRecorderState', false, source => { + await this._context.exposeBinding('__pw_recorderState', false, source => { let actionSelector = this._highlightedSelector; let actionPoint: Point | undefined; for (const [metadata, sdkObject] of this._currentCallsMetadata) { @@ -147,13 +147,13 @@ export class Recorder implements InstrumentationListener { return uiState; }); - await this._context.exposeBinding('_playwrightRecorderSetSelector', false, async (_, selector: string) => { + await this._context.exposeBinding('__pw_recorderSetSelector', false, async (_, selector: string) => { this._setMode('none'); await this._recorderApp?.setSelector(selector, true); await this._recorderApp?.bringToFront(); }); - await this._context.exposeBinding('_playwrightResume', false, () => { + await this._context.exposeBinding('__pw_resume', false, () => { this._debugger.resume(false); }); await this._context.extendInjectedScript(consoleApiSource.source); @@ -189,7 +189,7 @@ export class Recorder implements InstrumentationListener { private _refreshOverlay() { for (const page of this._context.pages()) - page.mainFrame().evaluateExpression('window._playwrightRefreshOverlay()', false, undefined, 'main').catch(() => {}); + page.mainFrame().evaluateExpression('window.__pw_refreshOverlay()', false, undefined, 'main').catch(() => {}); } async onBeforeCall(sdkObject: SdkObject, metadata: CallMetadata) { @@ -359,11 +359,11 @@ class ContextRecorder extends EventEmitter { // Input actions that potentially lead to navigation are intercepted on the page and are // performed by the Playwright. - await this._context.exposeBinding('_playwrightRecorderPerformAction', false, + await this._context.exposeBinding('__pw_recorderPerformAction', false, (source: BindingSource, action: actions.Action) => this._performAction(source.frame, action)); // Other non-essential actions are simply being recorded. - await this._context.exposeBinding('_playwrightRecorderRecordAction', false, + await this._context.exposeBinding('__pw_recorderRecordAction', false, (source: BindingSource, action: actions.Action) => this._recordAction(source.frame, action)); await this._context.extendInjectedScript(recorderSource.source); diff --git a/packages/playwright-test/src/mount.ts b/packages/playwright-test/src/mount.ts index 099aac44ce..c3d8316837 100644 --- a/packages/playwright-test/src/mount.ts +++ b/packages/playwright-test/src/mount.ts @@ -15,13 +15,17 @@ */ import type { Fixtures, Locator, Page, PlaywrightTestArgs, PlaywrightTestOptions, PlaywrightWorkerArgs, ViewportSize } from './types'; -import { createGuid } from 'playwright-core/lib/utils'; + +let boundCallbacksForMount: Function[] = []; export const fixtures: Fixtures Promise }, PlaywrightWorkerArgs & { _workerPage: Page }> = { _workerPage: [async ({ browser }, use) => { const page = await (browser as any)._wrapApiCall(async () => { const page = await browser.newPage(); await page.addInitScript('navigator.serviceWorker.register = () => {}'); + await page.exposeFunction('__pw_dispatch', (ordinal: number, args: any[]) => { + boundCallbacksForMount[ordinal](...args); + }); return page; }); await use(page); @@ -42,6 +46,7 @@ export const fixtures: Fixtures { - callbacks[ordinal](...args); - }); + wrapFunctions(component, page, boundCallbacksForMount); // WebKit does not wait for deferred scripts. await page.waitForFunction(() => !!(window as any).playwrightMount); - const selector = await page.evaluate(async ({ component, dispatchMethod }) => { + const selector = await page.evaluate(async ({ component }) => { const unwrapFunctions = (object: any) => { for (const [key, value] of Object.entries(object)) { if (typeof value === 'string' && (value as string).startsWith('__pw_func_')) { const ordinal = +value.substring('__pw_func_'.length); object[key] = (...args: any[]) => { - (window as any)[dispatchMethod](ordinal, args); + (window as any)['__pw_dispatch'](ordinal, args); }; } else if (typeof value === 'object' && value) { unwrapFunctions(value); @@ -85,7 +84,7 @@ async function innerMount(page: Page, jsxOrType: any, options: any, viewport: Vi unwrapFunctions(component); return await (window as any).playwrightMount(component); - }, { component, dispatchMethod }); + }, { component }); return selector; } diff --git a/tests/library/browsercontext-expose-function.spec.ts b/tests/library/browsercontext-expose-function.spec.ts index 57fc466316..ee364357eb 100644 --- a/tests/library/browsercontext-expose-function.spec.ts +++ b/tests/library/browsercontext-expose-function.spec.ts @@ -94,3 +94,26 @@ it('should work with CSP', async ({ page, context, server }) => { await page.evaluate(() => (window as any).hi()); expect(called).toBe(true); }); + +it('should re-add binding after reset', async ({ page, context }) => { + await context.exposeFunction('add', function(a, b) { + return Promise.resolve(a - b); + }); + expect(await page.evaluate('add(7, 6)')).toBe(1); + + await (context as any)._removeExposedBindings(); + await context.exposeFunction('add', function(a, b) { + return Promise.resolve(a + b); + }); + expect(await page.evaluate('add(5, 6)')).toBe(11); + await page.reload(); + expect(await page.evaluate('add(5, 6)')).toBe(11); +}); + +it('should retain internal binding after reset', async ({ page, context }) => { + await context.exposeFunction('__pw_add', function(a, b) { + return Promise.resolve(a + b); + }); + await (context as any)._removeExposedBindings(); + expect(await page.evaluate('__pw_add(5, 6)')).toBe(11); +}); diff --git a/tests/page/page-expose-function.spec.ts b/tests/page/page-expose-function.spec.ts index c2ce11ebd6..449f9ef563 100644 --- a/tests/page/page-expose-function.spec.ts +++ b/tests/page/page-expose-function.spec.ts @@ -261,3 +261,26 @@ it('should work with setContent', async ({ page, server }) => { await page.setContent(''); expect(await page.evaluate('window.result')).toBe(6); }); + +it('should re-add binding after reset', async ({ page }) => { + await page.exposeFunction('add', function(a, b) { + return Promise.resolve(a - b); + }); + expect(await page.evaluate('add(7, 6)')).toBe(1); + + await (page as any)._removeExposedBindings(); + await page.exposeFunction('add', function(a, b) { + return Promise.resolve(a + b); + }); + expect(await page.evaluate('add(5, 6)')).toBe(11); + await page.reload(); + expect(await page.evaluate('add(5, 6)')).toBe(11); +}); + +it('should retain internal binding after reset', async ({ page }) => { + await page.exposeFunction('__pw_add', function(a, b) { + return Promise.resolve(a + b); + }); + await (page as any)._removeExposedBindings(); + expect(await page.evaluate('__pw_add(5, 6)')).toBe(11); +});