Skip to content

Commit

Permalink
fix: validate ffmpeg on context creation (#33903)
Browse files Browse the repository at this point in the history
  • Loading branch information
mxschmitt authored Dec 12, 2024
1 parent 081f455 commit e3629dc
Show file tree
Hide file tree
Showing 8 changed files with 84 additions and 19 deletions.
4 changes: 4 additions & 0 deletions packages/playwright-core/src/server/bidi/bidiBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<PageDelegate> {
assertBrowserContextIsNotOwned(this);
const { context } = await this._browser._browserSession.send('browsingContext.create', {
Expand Down
32 changes: 20 additions & 12 deletions packages/playwright-core/src/server/browserContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ export abstract class BrowserContext extends SdkObject {

// BrowserContext methods.
abstract pages(): Page[];
abstract pagesOrErrors(): Promise<Page | Error>[];
abstract newPageDelegate(): Promise<PageDelegate>;
abstract addCookies(cookies: channels.SetNetworkCookie[]): Promise<void>;
abstract setGeolocation(geolocation?: types.Geolocation): Promise<void>;
Expand Down Expand Up @@ -358,29 +359,36 @@ export abstract class BrowserContext extends SdkObject {
this._timeoutSettings.setDefaultTimeout(timeout);
}

async _loadDefaultContextAsIs(progress: Progress): Promise<Page[]> {
if (!this.pages().length) {
async _loadDefaultContextAsIs(progress: Progress): Promise<Page> {
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<Page>,
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);
}
Expand Down
4 changes: 4 additions & 0 deletions packages/playwright-core/src/server/chromium/crBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<PageDelegate> {
assertBrowserContextIsNotOwned(this);

Expand Down
3 changes: 3 additions & 0 deletions packages/playwright-core/src/server/chromium/crPage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 4 additions & 0 deletions packages/playwright-core/src/server/firefox/ffBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<PageDelegate> {
assertBrowserContextIsNotOwned(this);
const { targetId } = await this._browser.session.send('Browser.newPage', {
Expand Down
8 changes: 1 addition & 7 deletions packages/playwright-core/src/server/page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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';
}
Expand Down
4 changes: 4 additions & 0 deletions packages/playwright-core/src/server/webkit/wkBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<PageDelegate> {
assertBrowserContextIsNotOwned(this);
const { pageProxyId } = await this._browser._browserSession.send('Playwright.createPage', { browserContextId: this._browserContextId });
Expand Down
44 changes: 44 additions & 0 deletions tests/installation/playwright-cli-install-should-work.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand Down Expand Up @@ -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`);
});
});

0 comments on commit e3629dc

Please sign in to comment.