From e3629dc1df67546c10fe0f10d4b8e08ecc3b3d4d Mon Sep 17 00:00:00 2001 From: Max Schmitt Date: Wed, 11 Dec 2024 23:07:03 -0800 Subject: [PATCH] fix: validate ffmpeg on context creation (#33903) --- .../src/server/bidi/bidiBrowser.ts | 4 ++ .../src/server/browserContext.ts | 32 +++++++++----- .../src/server/chromium/crBrowser.ts | 4 ++ .../src/server/chromium/crPage.ts | 3 ++ .../src/server/firefox/ffBrowser.ts | 4 ++ packages/playwright-core/src/server/page.ts | 8 +--- .../src/server/webkit/wkBrowser.ts | 4 ++ ...playwright-cli-install-should-work.spec.ts | 44 +++++++++++++++++++ 8 files changed, 84 insertions(+), 19 deletions(-) diff --git a/packages/playwright-core/src/server/bidi/bidiBrowser.ts b/packages/playwright-core/src/server/bidi/bidiBrowser.ts index 9861fc80cf..acefc08fb2 100644 --- a/packages/playwright-core/src/server/bidi/bidiBrowser.ts +++ b/packages/playwright-core/src/server/bidi/bidiBrowser.ts @@ -211,6 +211,10 @@ export class BidiBrowserContext extends BrowserContext { return this._bidiPages().map(bidiPage => bidiPage._initializedPage).filter(Boolean) as Page[]; } + pagesOrErrors() { + return this._bidiPages().map(bidiPage => bidiPage.pageOrError()); + } + async newPageDelegate(): Promise { assertBrowserContextIsNotOwned(this); const { context } = await this._browser._browserSession.send('browsingContext.create', { diff --git a/packages/playwright-core/src/server/browserContext.ts b/packages/playwright-core/src/server/browserContext.ts index 025bd0f388..6d9227c0a7 100644 --- a/packages/playwright-core/src/server/browserContext.ts +++ b/packages/playwright-core/src/server/browserContext.ts @@ -259,6 +259,7 @@ export abstract class BrowserContext extends SdkObject { // BrowserContext methods. abstract pages(): Page[]; + abstract pagesOrErrors(): Promise[]; abstract newPageDelegate(): Promise; abstract addCookies(cookies: channels.SetNetworkCookie[]): Promise; abstract setGeolocation(geolocation?: types.Geolocation): Promise; @@ -358,29 +359,36 @@ export abstract class BrowserContext extends SdkObject { this._timeoutSettings.setDefaultTimeout(timeout); } - async _loadDefaultContextAsIs(progress: Progress): Promise { - if (!this.pages().length) { + async _loadDefaultContextAsIs(progress: Progress): Promise { + let pageOrError; + if (!this.pagesOrErrors().length) { const waitForEvent = helper.waitForEvent(progress, this, BrowserContext.Events.Page); progress.cleanupWhenAborted(() => waitForEvent.dispose); - const page = (await waitForEvent.promise) as Page; - if (page._pageIsError) - throw page._pageIsError; + // Race against BrowserContext.close + pageOrError = await Promise.race([ + waitForEvent.promise as Promise, + this._closePromise, + ]); + // Consider Page initialization errors + if (pageOrError instanceof Page) + pageOrError = await pageOrError._delegate.pageOrError(); + } else { + pageOrError = await this.pagesOrErrors()[0]; } - const pages = this.pages(); - if (pages[0]._pageIsError) - throw pages[0]._pageIsError; - await pages[0].mainFrame()._waitForLoadState(progress, 'load'); - return pages; + if (pageOrError instanceof Error) + throw pageOrError; + await pageOrError.mainFrame()._waitForLoadState(progress, 'load'); + return pageOrError; } async _loadDefaultContext(progress: Progress) { - const pages = await this._loadDefaultContextAsIs(progress); + const defaultPage = await this._loadDefaultContextAsIs(progress); const browserName = this._browser.options.name; if ((this._options.isMobile && browserName === 'chromium') || (this._options.locale && browserName === 'webkit')) { // Workaround for: // - chromium fails to change isMobile for existing page; // - webkit fails to change locale for existing page. - const oldPage = pages[0]; + const oldPage = defaultPage; await this.newPage(progress.metadata); await oldPage.close(progress.metadata); } diff --git a/packages/playwright-core/src/server/chromium/crBrowser.ts b/packages/playwright-core/src/server/chromium/crBrowser.ts index a6d77e4ae7..1aba1ed8cb 100644 --- a/packages/playwright-core/src/server/chromium/crBrowser.ts +++ b/packages/playwright-core/src/server/chromium/crBrowser.ts @@ -368,6 +368,10 @@ export class CRBrowserContext extends BrowserContext { return this._crPages().map(crPage => crPage._initializedPage).filter(Boolean) as Page[]; } + pagesOrErrors() { + return this._crPages().map(crPage => crPage.pageOrError()); + } + async newPageDelegate(): Promise { assertBrowserContextIsNotOwned(this); diff --git a/packages/playwright-core/src/server/chromium/crPage.ts b/packages/playwright-core/src/server/chromium/crPage.ts index bfad678f1c..a7ee34d2c5 100644 --- a/packages/playwright-core/src/server/chromium/crPage.ts +++ b/packages/playwright-core/src/server/chromium/crPage.ts @@ -432,6 +432,9 @@ class FrameSession { this._firstNonInitialNavigationCommittedFulfill = f; this._firstNonInitialNavigationCommittedReject = r; }); + // The Promise is not always awaited (e.g. FrameSession._initialize can throw) + // so we catch errors here to prevent unhandled promise rejection. + this._firstNonInitialNavigationCommittedPromise.catch(() => {}); } _isMainFrame(): boolean { diff --git a/packages/playwright-core/src/server/firefox/ffBrowser.ts b/packages/playwright-core/src/server/firefox/ffBrowser.ts index b26a2850ee..afc0671f70 100644 --- a/packages/playwright-core/src/server/firefox/ffBrowser.ts +++ b/packages/playwright-core/src/server/firefox/ffBrowser.ts @@ -271,6 +271,10 @@ export class FFBrowserContext extends BrowserContext { return this._ffPages().map(ffPage => ffPage._initializedPage).filter(pageOrNull => !!pageOrNull) as Page[]; } + pagesOrErrors() { + return this._ffPages().map(ffPage => ffPage.pageOrError()); + } + async newPageDelegate(): Promise { assertBrowserContextIsNotOwned(this); const { targetId } = await this._browser.session.send('Browser.newPage', { diff --git a/packages/playwright-core/src/server/page.ts b/packages/playwright-core/src/server/page.ts index d626b1ed3c..48c0827c08 100644 --- a/packages/playwright-core/src/server/page.ts +++ b/packages/playwright-core/src/server/page.ts @@ -164,7 +164,6 @@ export class Page extends SdkObject { _clientRequestInterceptor: network.RouteHandler | undefined; _serverRequestInterceptor: network.RouteHandler | undefined; _ownedContext: BrowserContext | undefined; - _pageIsError: Error | undefined; _video: Artifact | null = null; _opener: Page | undefined; private _isServerSideOnly = false; @@ -208,7 +207,7 @@ export class Page extends SdkObject { // context/browser closure. Just ignore the page. if (this._browserContext.isClosingOrClosed()) return; - this._setIsError(error); + this._frameManager.createDummyMainFrameIfNeeded(); } this._initialized = true; this.emitOnContext(contextEvent, this); @@ -709,11 +708,6 @@ export class Page extends SdkObject { await this._ownedContext.close(options); } - private _setIsError(error: Error) { - this._pageIsError = error; - this._frameManager.createDummyMainFrameIfNeeded(); - } - isClosed(): boolean { return this._closedState === 'closed'; } diff --git a/packages/playwright-core/src/server/webkit/wkBrowser.ts b/packages/playwright-core/src/server/webkit/wkBrowser.ts index f4f9f732a5..4e5467fd17 100644 --- a/packages/playwright-core/src/server/webkit/wkBrowser.ts +++ b/packages/playwright-core/src/server/webkit/wkBrowser.ts @@ -243,6 +243,10 @@ export class WKBrowserContext extends BrowserContext { return this._wkPages().map(wkPage => wkPage._initializedPage).filter(pageOrNull => !!pageOrNull) as Page[]; } + pagesOrErrors() { + return this._wkPages().map(wkPage => wkPage.pageOrError()); + } + async newPageDelegate(): Promise { assertBrowserContextIsNotOwned(this); const { pageProxyId } = await this._browser._browserSession.send('Playwright.createPage', { browserContextId: this._browserContextId }); diff --git a/tests/installation/playwright-cli-install-should-work.spec.ts b/tests/installation/playwright-cli-install-should-work.spec.ts index 1c0cea6277..d1a8bd3d04 100755 --- a/tests/installation/playwright-cli-install-should-work.spec.ts +++ b/tests/installation/playwright-cli-install-should-work.spec.ts @@ -14,6 +14,7 @@ * limitations under the License. */ import { test, expect } from './npmTest'; +import { chromium } from '@playwright/test'; import path from 'path'; test.use({ isolateBrowsers: true }); @@ -95,3 +96,46 @@ test('install playwright-chromium should work', async ({ exec, installedSoftware await exec('npx playwright install chromium'); await exec('node sanity.js playwright-chromium chromium'); }); + +test('should print error if recording video without ffmpeg', async ({ exec, writeFiles }) => { + await exec('npm i playwright'); + + await writeFiles({ + 'launch.js': ` + const playwright = require('playwright'); + (async () => { + const browser = await playwright.chromium.launch({ executablePath: ${JSON.stringify(chromium.executablePath())} }); + try { + const context = await browser.newContext({ recordVideo: { dir: 'videos' } }); + const page = await context.newPage(); + } finally { + await browser.close(); + } + })().catch(e => { + console.error(e); + process.exit(1); + }); + `, + 'launchPersistentContext.js': ` + const playwright = require('playwright'); + process.on('unhandledRejection', (e) => console.error('unhandledRejection', e)); + (async () => { + const context = await playwright.chromium.launchPersistentContext('', { executablePath: ${JSON.stringify(chromium.executablePath())}, recordVideo: { dir: 'videos' } }); + })().catch(e => { + console.error(e); + process.exit(1); + }); + `, + }); + + await test.step('BrowserType.launch', async () => { + const result = await exec('node', 'launch.js', { expectToExitWithError: true }); + expect(result).toContain(`browserContext.newPage: Executable doesn't exist at`); + }); + + await test.step('BrowserType.launchPersistentContext', async () => { + const result = await exec('node', 'launchPersistentContext.js', { expectToExitWithError: true }); + expect(result).not.toContain('unhandledRejection'); + expect(result).toContain(`browserType.launchPersistentContext: Executable doesn't exist at`); + }); +});