Filip/playwright
1
0
Fork 0
Code Issues Pull requests Actions Packages Projects Releases Wiki Activity

fix(screenshot): do not stall on hideHiglight attempt 2 (#13222)

Browse source 
It turns out that "non stalling evaluate" can stall in Chromium
in some weird conditions, like `document.open` after some weird
`iframe.src` value.

We now only hide highlight in those frames where we did install
highlight in the first place.
This commit is contained in:
Dmitry Gozman 2022-03-31 15:33:29 -07:00 committed by GitHub
parent 297edb02f1
commit 9fc95dda84
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 28 additions and 9 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

24
packages/playwright-core/src/server/screenshotter.ts
View file

@ -238,11 +238,18 @@ export class Screenshotter {
}));
}
async _maskElements(progress: Progress, options: ScreenshotOptions) {
if (!options.mask || !options.mask.length)
return false;
async _maskElements(progress: Progress, options: ScreenshotOptions): Promise<() => Promise<void>> {
const framesToParsedSelectors: MultiMap<Frame, ParsedSelector> = new MultiMap();
const cleanup = async () => {
await Promise.all([...framesToParsedSelectors.keys()].map(async frame => {
await frame.hideHighlight();
}));
};
if (!options.mask || !options.mask.length)
return cleanup;
await Promise.all((options.mask || []).map(async ({ frame, selector }) => {
const pair = await frame.resolveFrameForSelectorNoWait(selector);
if (pair)
@ -253,8 +260,8 @@ export class Screenshotter {
await Promise.all([...framesToParsedSelectors.keys()].map(async frame => {
await frame.maskSelectors(framesToParsedSelectors.get(frame));
}));
progress.cleanupWhenAborted(() => this._page.hideHighlight());
return true;
progress.cleanupWhenAborted(cleanup);
return cleanup;
}
private async _screenshot(progress: Progress, format: 'png' | 'jpeg', documentRect: types.Rect | undefined, viewportRect: types.Rect | undefined, fitsViewport: boolean, options: ScreenshotOptions): Promise<Buffer> {
@ -268,14 +275,13 @@ export class Screenshotter {
}
progress.throwIfAborted(); // Avoid extra work.
const hasHighlight = await this._maskElements(progress, options);
const cleanupHighlight = await this._maskElements(progress, options);
progress.throwIfAborted(); // Avoid extra work.
const buffer = await this._page._delegate.takeScreenshot(progress, format, documentRect, viewportRect, options.quality, fitsViewport, options.size || 'device');
progress.throwIfAborted(); // Avoid restoring after failure - should be done by cleanup.
if (hasHighlight)
await this._page.hideHighlight();
await cleanupHighlight();
progress.throwIfAborted(); // Avoid restoring after failure - should be done by cleanup.
if (shouldSetDefaultBackground)

13
tests/page/page-screenshot.spec.ts
View file

@ -473,6 +473,19 @@ it.describe('page screenshot', () => {
await route.fulfill({ body: '' });
await done;
});
it('should work when subframe used document.open after a weird url', async ({ page, server }) => {
await page.goto(server.EMPTY_PAGE);
await page.evaluate(() => {
const iframe = document.createElement('iframe');
iframe.src = 'javascript:hi';
document.body.appendChild(iframe);
iframe.contentDocument.open();
iframe.contentDocument.write('Hello');
iframe.contentDocument.close();
});
await page.screenshot({ mask: [ page.locator('non-existent') ] });
});
});
});