From e17a05230d0aac880940a5bd176cd06baf4e32b9 Mon Sep 17 00:00:00 2001 From: Alex Rudenko Date: Fri, 22 Jul 2022 11:47:30 +0200 Subject: [PATCH] fix: address flakiness in frame handling When we attach to a frame, we send a call to get the page frame tree from CDP. Based on the tree data we look up the parent frame if parentId is provided. The problem is that the call to get the page frame tree could take arbitrary time and the calls for the parent and child frames might happen at the same time. So the situation where the frame tree for the child frame is resolved before the parent frame is known is fairly common. This PR addresses the issue by awaiting for the parent frame id before attempting to register a child frame. --- src/common/FrameManager.ts | 156 ++++++++++++++++++++++++++++--------- src/common/Page.ts | 2 +- test/src/oopif.spec.ts | 1 + 3 files changed, 122 insertions(+), 37 deletions(-) diff --git a/src/common/FrameManager.ts b/src/common/FrameManager.ts index b2ddde456f613..2a104d170a923 100644 --- a/src/common/FrameManager.ts +++ b/src/common/FrameManager.ts @@ -33,6 +33,38 @@ import {debugError, isErrorLike} from './util.js'; const UTILITY_WORLD_NAME = '__puppeteer_utility_world__'; +type Resolvable = { + promise: Promise; + resolve: (_: T) => void; + reject: (_: Error) => void; +}; + +function createPromiseResolverWithTimeout( + timeout: number, + timeoutMessage: string +): Resolvable { + let resolver = (_: T): void => {}; + let rejector = (_: Error) => {}; + const taskPromise = new Promise((resolve, reject) => { + resolver = resolve; + rejector = reject; + }); + const timeoutId = setTimeout(() => { + rejector(new Error(timeoutMessage)); + }, timeout); + return { + promise: taskPromise, + resolve: (value: T) => { + clearTimeout(timeoutId); + resolver(value); + }, + reject: (err: Error) => { + clearTimeout(timeoutId); + rejector(err); + }, + }; +} + /** * We use symbols to prevent external parties listening to these events. * They are internal to Puppeteer. @@ -64,6 +96,8 @@ export class FrameManager extends EventEmitter { #isolatedWorlds = new Set(); #mainFrame?: Frame; #client: CDPSession; + #pendingFrameTargets = new Map>(); + #framesPendingAttachment = new Map>(); /** * @internal @@ -132,8 +166,20 @@ export class FrameManager extends EventEmitter { }); } - async initialize(client: CDPSession = this.#client): Promise { + async initialize( + targetId: string, + client: CDPSession = this.#client + ): Promise { try { + if (!this.#pendingFrameTargets.has(targetId)) { + this.#pendingFrameTargets.set( + targetId, + createPromiseResolverWithTimeout( + 2000, + `Waiting for target frame ${targetId} failed` + ) + ); + } const result = await Promise.all([ client.send('Page.enable'), client.send('Page.getFrameTree'), @@ -162,6 +208,9 @@ export class FrameManager extends EventEmitter { } throw error; + } finally { + this.#pendingFrameTargets.get(targetId)?.resolve(); + this.#pendingFrameTargets.delete(targetId); } } @@ -262,7 +311,7 @@ export class FrameManager extends EventEmitter { frame._updateClient(target._session()!); } this.setupEventListeners(target._session()!); - this.initialize(target._session()); + this.initialize(target._getTargetInfo().targetId, target._session()); } async onDetachedFromTarget(target: Target): Promise { @@ -341,7 +390,7 @@ export class FrameManager extends EventEmitter { #onFrameAttached( session: CDPSession, frameId: string, - parentFrameId?: string + parentFrameId: string ): void { if (this.#frames.has(frameId)) { const frame = this.#frames.get(frameId)!; @@ -353,50 +402,85 @@ export class FrameManager extends EventEmitter { } return; } - assert(parentFrameId); const parentFrame = this.#frames.get(parentFrameId); - assert(parentFrame, `Parent frame ${parentFrameId} not found`); - const frame = new Frame(this, parentFrame, frameId, session); - this.#frames.set(frame._id, frame); - this.emit(FrameManagerEmittedEvents.FrameAttached, frame); + + const complete = (parentFrame: Frame) => { + assert(parentFrame, `Parent frame ${parentFrameId} not found`); + const frame = new Frame(this, parentFrame, frameId, session); + this.#frames.set(frame._id, frame); + this.emit(FrameManagerEmittedEvents.FrameAttached, frame); + }; + + if (parentFrame) { + return complete(parentFrame); + } + + if (this.#pendingFrameTargets.has(parentFrameId)) { + if (!this.#framesPendingAttachment.has(frameId)) { + this.#framesPendingAttachment.set( + frameId, + createPromiseResolverWithTimeout( + 2000, + `Waiting for frame ${frameId} to attach failed` + ) + ); + } + this.#pendingFrameTargets.get(parentFrameId)!.promise.then(() => { + complete(this.#frames.get(parentFrameId)!); + this.#framesPendingAttachment.get(frameId)?.resolve(); + this.#framesPendingAttachment.delete(frameId); + }); + return; + } + + throw new Error(`Parent frame ${parentFrameId} not found`); } #onFrameNavigated(framePayload: Protocol.Page.Frame): void { + const frameId = framePayload.id; const isMainFrame = !framePayload.parentId; - let frame = isMainFrame - ? this.#mainFrame - : this.#frames.get(framePayload.id); - assert( - isMainFrame || frame, - 'We either navigate top level or have old version of the navigated frame' - ); + const frame = isMainFrame ? this.#mainFrame : this.#frames.get(frameId); - // Detach all child frames first. - if (frame) { - for (const child of frame.childFrames()) { - this.#removeFramesRecursively(child); - } - } + const complete = (frame?: Frame) => { + assert( + isMainFrame || frame, + `Missing frame isMainFrame=${isMainFrame}, frameId=${frameId}` + ); - // Update or create main frame. - if (isMainFrame) { + // Detach all child frames first. if (frame) { - // Update frame id to retain frame identity on cross-process navigation. - this.#frames.delete(frame._id); - frame._id = framePayload.id; - } else { - // Initial main frame navigation. - frame = new Frame(this, null, framePayload.id, this.#client); + for (const child of frame.childFrames()) { + this.#removeFramesRecursively(child); + } } - this.#frames.set(framePayload.id, frame); - this.#mainFrame = frame; - } - // Update frame payload. - assert(frame); - frame._navigated(framePayload); + // Update or create main frame. + if (isMainFrame) { + if (frame) { + // Update frame id to retain frame identity on cross-process navigation. + this.#frames.delete(frame._id); + frame._id = frameId; + } else { + // Initial main frame navigation. + frame = new Frame(this, null, frameId, this.#client); + } + this.#frames.set(frameId, frame); + this.#mainFrame = frame; + } - this.emit(FrameManagerEmittedEvents.FrameNavigated, frame); + // Update frame payload. + assert(frame); + frame._navigated(framePayload); + + this.emit(FrameManagerEmittedEvents.FrameNavigated, frame); + }; + if (this.#framesPendingAttachment.has(frameId)) { + this.#framesPendingAttachment.get(frameId)!.promise.then(() => { + complete(isMainFrame ? this.#mainFrame : this.#frames.get(frameId)); + }); + } else { + complete(frame); + } } async _ensureIsolatedWorld(session: CDPSession, name: string): Promise { diff --git a/src/common/Page.ts b/src/common/Page.ts index 496e2216f3286..d195f4d8a3c03 100644 --- a/src/common/Page.ts +++ b/src/common/Page.ts @@ -630,7 +630,7 @@ export class Page extends EventEmitter { async #initialize(): Promise { await Promise.all([ - this.#frameManager.initialize(), + this.#frameManager.initialize(this.#target._targetId), this.#client.send('Performance.enable'), this.#client.send('Log.enable'), ]); diff --git a/test/src/oopif.spec.ts b/test/src/oopif.spec.ts index f747910df24b7..17762636d2338 100644 --- a/test/src/oopif.spec.ts +++ b/test/src/oopif.spec.ts @@ -419,6 +419,7 @@ describeChromeOnly('OOPIF', function () { await target.page(); browser1.disconnect(); }); + itFailsFirefox('should support lazy OOP frames', async () => { const {server} = getTestState();