From b3a39e246e680a1e3080e6aa235f4050b79c6c56 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 11 Sep 2020 14:03:01 -0700 Subject: [PATCH] fix(snapshot): do not let a single frame fail the whole snapshot Sometimes, we are unable to take a frame snapshot. The most common example would be "frame is stuck during the navigation in Chromium", where we cannot evaluate until the frame is done navigating. In this case, use all other frames and just stub the failing ones with "Snapshot is not available". Chances are, noone will even see this frame because it's an invisible tracking iframe. --- src/trace/snapshotter.ts | 146 ++++++++++++++++++----------------- src/trace/tracer.ts | 75 +++++++++--------- src/utils/timeoutSettings.ts | 9 ++- test/snapshot.spec.ts | 2 +- 4 files changed, 120 insertions(+), 112 deletions(-) diff --git a/src/trace/snapshotter.ts b/src/trace/snapshotter.ts index e72b3bbb3d7fe..bbd50ff448000 100644 --- a/src/trace/snapshotter.ts +++ b/src/trace/snapshotter.ts @@ -18,10 +18,11 @@ import { BrowserContext } from '../server/browserContext'; import { Page } from '../server/page'; import * as network from '../server/network'; import { helper, RegisteredListener } from '../server/helper'; -import { Progress } from '../server/progress'; +import { Progress, runAbortableTask } from '../server/progress'; import { debugLogger } from '../utils/debugLogger'; import { Frame } from '../server/frames'; import * as js from '../server/javascript'; +import * as types from '../server/types'; import { SnapshotData, takeSnapshotInFrame } from './snapshotterInjected'; import { assert, calculateSha1, createGuid } from '../utils/utils'; @@ -109,23 +110,37 @@ export class Snapshotter { this._delegate.onBlob({ sha1, buffer: body }); } - async takeSnapshot(progress: Progress, page: Page): Promise { + async takeSnapshot(page: Page, timeout: number): Promise { assert(page.context() === this._context); const frames = page.frames(); - const promises = frames.map(frame => this._snapshotFrame(progress, frame)); - const results = await Promise.all(promises); + const frameSnapshotPromises = frames.map(async frame => { + // TODO: use different timeout depending on the frame depth/origin + // to avoid waiting for too long for some useless frame. + const frameResult = await runAbortableTask(progress => this._snapshotFrame(progress, frame), timeout).catch(e => null); + if (frameResult) + return frameResult; + const frameSnapshot = { + frameId: frame._id, + url: frame.url(), + html: 'Snapshot is not available', + resourceOverrides: [], + }; + return { snapshot: frameSnapshot, mapping: new Map() }; + }); - const mainFrame = results[0]; - if (!mainFrame) + const viewportSize = await this._getViewportSize(page, timeout); + const results = await Promise.all(frameSnapshotPromises); + + if (!viewportSize) return null; + + const mainFrame = results[0]; if (!mainFrame.snapshot.url.startsWith('http')) mainFrame.snapshot.url = 'http://playwright.snapshot/'; const mapping = new Map(); for (const result of results) { - if (!result) - continue; for (const [key, value] of result.mapping) mapping.set(key, value); } @@ -133,8 +148,6 @@ export class Snapshotter { const childFrames: FrameSnapshot[] = []; for (let i = 1; i < results.length; i++) { const result = results[i]; - if (!result) - continue; const frame = frames[i]; if (!mapping.has(frame)) continue; @@ -143,75 +156,68 @@ export class Snapshotter { childFrames.push(frameSnapshot); } - let viewportSize = page.viewportSize(); - if (!viewportSize) { - try { - if (!progress.isRunning()) - return null; - - const context = await page.mainFrame()._utilityContext(); - viewportSize = await context.evaluateInternal(() => { - return { - width: Math.max(document.body.offsetWidth, document.documentElement.offsetWidth), - height: Math.max(document.body.offsetHeight, document.documentElement.offsetHeight), - }; - }); - } catch (e) { - return null; - } - } - return { viewportSize, frames: [mainFrame.snapshot, ...childFrames], }; } + private async _getViewportSize(page: Page, timeout: number): Promise { + return runAbortableTask(async progress => { + const viewportSize = page.viewportSize(); + if (viewportSize) + return viewportSize; + const context = await page.mainFrame()._utilityContext(); + return context.evaluateInternal(() => { + return { + width: Math.max(document.body.offsetWidth, document.documentElement.offsetWidth), + height: Math.max(document.body.offsetHeight, document.documentElement.offsetHeight), + }; + }); + }, timeout).catch(e => null); + } + private async _snapshotFrame(progress: Progress, frame: Frame): Promise { - try { - if (!progress.isRunning()) - return null; - - const context = await frame._utilityContext(); - const guid = createGuid(); - const removeNoScript = !frame._page.context()._options.javaScriptEnabled; - const result = await js.evaluate(context, false /* returnByValue */, takeSnapshotInFrame, guid, removeNoScript) as js.JSHandle; - if (!progress.isRunning()) - return null; - - const properties = await result.getProperties(); - const data = await properties.get('data')!.jsonValue() as SnapshotData; - const frameElements = await properties.get('frameElements')!.getProperties(); - result.dispose(); - - const snapshot: FrameSnapshot = { - frameId: frame._id, - url: frame.url(), - html: data.html, - resourceOverrides: [], - }; - const mapping = new Map(); - - for (const { url, content } of data.resourceOverrides) { - const buffer = Buffer.from(content); - const sha1 = calculateSha1(buffer); - this._delegate.onBlob({ sha1, buffer }); - snapshot.resourceOverrides.push({ url, sha1 }); - } - - for (let i = 0; i < data.frameUrls.length; i++) { - const element = frameElements.get(String(i))!.asElement(); - if (!element) - continue; - const frame = await element.contentFrame().catch(e => null); - if (frame) - mapping.set(frame, data.frameUrls[i]); - } - - return { snapshot, mapping }; - } catch (e) { + if (!progress.isRunning()) return null; + + const context = await frame._utilityContext(); + const guid = createGuid(); + const removeNoScript = !frame._page.context()._options.javaScriptEnabled; + const result = await js.evaluate(context, false /* returnByValue */, takeSnapshotInFrame, guid, removeNoScript) as js.JSHandle; + if (!progress.isRunning()) + return null; + + const properties = await result.getProperties(); + const data = await properties.get('data')!.jsonValue() as SnapshotData; + const frameElements = await properties.get('frameElements')!.getProperties(); + result.dispose(); + + const snapshot: FrameSnapshot = { + frameId: frame._id, + url: frame.url(), + html: data.html, + resourceOverrides: [], + }; + const mapping = new Map(); + + for (const { url, content } of data.resourceOverrides) { + const buffer = Buffer.from(content); + const sha1 = calculateSha1(buffer); + this._delegate.onBlob({ sha1, buffer }); + snapshot.resourceOverrides.push({ url, sha1 }); } + + for (let i = 0; i < data.frameUrls.length; i++) { + const element = frameElements.get(String(i))!.asElement(); + if (!element) + continue; + const frame = await element.contentFrame().catch(e => null); + if (frame) + mapping.set(frame, data.frameUrls[i]); + } + + return { snapshot, mapping }; } } diff --git a/src/trace/tracer.ts b/src/trace/tracer.ts index a23d2bd457b26..0f918e3b76bc2 100644 --- a/src/trace/tracer.ts +++ b/src/trace/tracer.ts @@ -23,11 +23,11 @@ import * as fs from 'fs'; import { calculateSha1, createGuid, mkdirIfNeeded, monotonicTime } from '../utils/utils'; import { ActionResult, InstrumentingAgent, instrumentingAgents, ActionMetadata } from '../server/instrumentation'; import { Page } from '../server/page'; -import { Progress, runAbortableTask } from '../server/progress'; import { Snapshotter } from './snapshotter'; import * as types from '../server/types'; import type { ElementHandle } from '../server/dom'; import { helper, RegisteredListener } from '../server/helper'; +import { DEFAULT_TIMEOUT } from '../utils/timeoutSettings'; const fsWriteFileAsync = util.promisify(fs.writeFile.bind(fs)); const fsAppendFileAsync = util.promisify(fs.appendFile.bind(fs)); @@ -59,20 +59,28 @@ export class Tracer implements InstrumentingAgent { } async onContextDestroyed(context: BrowserContext): Promise { - const contextTracer = this._contextTracers.get(context); - if (contextTracer) { - await contextTracer.dispose(); - this._contextTracers.delete(context); + try { + const contextTracer = this._contextTracers.get(context); + if (contextTracer) { + await contextTracer.dispose(); + this._contextTracers.delete(context); + } + } catch (e) { + // Do not throw from instrumentation. } } async onAfterAction(result: ActionResult, metadata?: ActionMetadata): Promise { - if (!metadata) - return; - const contextTracer = this._contextTracers.get(metadata.page.context()); - if (!contextTracer) - return; - await contextTracer.recordAction(result, metadata); + try { + if (!metadata) + return; + const contextTracer = this._contextTracers.get(metadata.page.context()); + if (!contextTracer) + return; + await contextTracer.recordAction(result, metadata); + } catch (e) { + // Do not throw from instrumentation. + } } } @@ -124,33 +132,21 @@ class ContextTracer implements SnapshotterDelegate { } async captureSnapshot(page: Page, options: types.TimeoutOptions & { label?: string } = {}): Promise { - await runAbortableTask(async progress => { - const label = options.label || 'snapshot'; - const snapshot = await this._takeSnapshot(progress, page); - if (!snapshot) - return; - const event: ActionTraceEvent = { - type: 'action', - contextId: this._contextId, - action: 'snapshot', - label, - snapshot, - }; - this._appendTraceEvent(event); - }, page._timeoutSettings.timeout(options)); + const snapshot = await this._takeSnapshot(page, options.timeout); + if (!snapshot) + return; + const event: ActionTraceEvent = { + type: 'action', + contextId: this._contextId, + action: 'snapshot', + label: options.label || 'snapshot', + snapshot, + }; + this._appendTraceEvent(event); } async recordAction(result: ActionResult, metadata: ActionMetadata) { - let snapshot: { sha1: string, duration: number } | undefined; - try { - // Use 20% of the default timeout. - // Never use zero timeout to avoid stalling because of snapshot. - const timeout = (metadata.page._timeoutSettings.timeout({}) / 5) || 6000; - snapshot = await runAbortableTask(progress => this._takeSnapshot(progress, metadata.page), timeout); - } catch (e) { - snapshot = undefined; - } - + const snapshot = await this._takeSnapshot(metadata.page); const event: ActionTraceEvent = { type: 'action', contextId: this._contextId, @@ -196,9 +192,14 @@ class ContextTracer implements SnapshotterDelegate { return typeof target === 'string' ? target : await target._previewPromise; } - private async _takeSnapshot(progress: Progress, page: Page): Promise<{ sha1: string, duration: number } | undefined> { + private async _takeSnapshot(page: Page, timeout: number = 0): Promise<{ sha1: string, duration: number } | undefined> { + if (!timeout) { + // Never use zero timeout to avoid stalling because of snapshot. + // Use 20% of the default timeout. + timeout = (page._timeoutSettings.timeout({}) || DEFAULT_TIMEOUT) / 5; + } const startTime = monotonicTime(); - const snapshot = await this._snapshotter.takeSnapshot(progress, page); + const snapshot = await this._snapshotter.takeSnapshot(page, timeout); if (!snapshot) return; const buffer = Buffer.from(JSON.stringify(snapshot)); diff --git a/src/utils/timeoutSettings.ts b/src/utils/timeoutSettings.ts index 3188bd0267bd8..06280b5a2611f 100644 --- a/src/utils/timeoutSettings.ts +++ b/src/utils/timeoutSettings.ts @@ -17,7 +17,8 @@ import { isDebugMode } from './utils'; -const DEFAULT_TIMEOUT = isDebugMode() ? 0 : 30000; +export const DEFAULT_TIMEOUT = 30000; +const TIMEOUT = isDebugMode() ? 0 : DEFAULT_TIMEOUT; export class TimeoutSettings { private _parent: TimeoutSettings | undefined; @@ -45,7 +46,7 @@ export class TimeoutSettings { return this._defaultTimeout; if (this._parent) return this._parent.navigationTimeout(options); - return DEFAULT_TIMEOUT; + return TIMEOUT; } timeout(options: { timeout?: number }): number { @@ -55,12 +56,12 @@ export class TimeoutSettings { return this._defaultTimeout; if (this._parent) return this._parent.timeout(options); - return DEFAULT_TIMEOUT; + return TIMEOUT; } static timeout(options: { timeout?: number }): number { if (typeof options.timeout === 'number') return options.timeout; - return DEFAULT_TIMEOUT; + return TIMEOUT; } } diff --git a/test/snapshot.spec.ts b/test/snapshot.spec.ts index c379325c31b42..5ef6d26fa5961 100644 --- a/test/snapshot.spec.ts +++ b/test/snapshot.spec.ts @@ -20,5 +20,5 @@ it('should not throw', (test, parameters) => { test.skip(!options.TRACING); }, async ({page, server, playwright, toImpl}) => { await page.goto(server.PREFIX + '/snapshot/snapshot-with-css.html'); - await (playwright as any).__tracer.captureSnapshot(toImpl(page), { timeout: 5000, label: 'snapshot' }); + await (playwright as any).__tracer.captureSnapshot(toImpl(page), { label: 'snapshot' }); });