From 02c07cc0ff73e0c0f502baf079771b3a891b4fb8 Mon Sep 17 00:00:00 2001 From: Alex Rudenko Date: Mon, 25 Jul 2022 11:20:38 +0200 Subject: [PATCH] fix: address flakiness in frame handling (#8688) 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 | 136 +++++++++++++++++++++++++++---------- src/common/Page.ts | 2 +- src/common/util.ts | 43 ++++++++++++ test/src/oopif.spec.ts | 1 + 4 files changed, 144 insertions(+), 38 deletions(-) diff --git a/src/common/FrameManager.ts b/src/common/FrameManager.ts index b2ddde456f613..5cfea9a9acbfc 100644 --- a/src/common/FrameManager.ts +++ b/src/common/FrameManager.ts @@ -29,7 +29,12 @@ import {Page} from './Page.js'; import {Target} from './Target.js'; import {TimeoutSettings} from './TimeoutSettings.js'; import {EvaluateFunc, HandleFor, NodeFor} from './types.js'; -import {debugError, isErrorLike} from './util.js'; +import { + createDeferredPromiseWithTimer, + debugError, + DeferredPromise, + isErrorLike, +} from './util.js'; const UTILITY_WORLD_NAME = '__puppeteer_utility_world__'; @@ -64,6 +69,15 @@ export class FrameManager extends EventEmitter { #isolatedWorlds = new Set(); #mainFrame?: Frame; #client: CDPSession; + /** + * Keeps track of OOPIF targets/frames (target ID == frame ID for OOPIFs) + * that are being initialized. + */ + #framesPendingTargetInit = new Map>(); + /** + * Keeps track of frames that are in the process of being attached in #onFrameAttached. + */ + #framesPendingAttachment = new Map>(); /** * @internal @@ -132,8 +146,19 @@ export class FrameManager extends EventEmitter { }); } - async initialize(client: CDPSession = this.#client): Promise { + async initialize( + targetId: string, + client: CDPSession = this.#client + ): Promise { try { + if (!this.#framesPendingTargetInit.has(targetId)) { + this.#framesPendingTargetInit.set( + targetId, + createDeferredPromiseWithTimer( + `Waiting for target frame ${targetId} failed` + ) + ); + } const result = await Promise.all([ client.send('Page.enable'), client.send('Page.getFrameTree'), @@ -162,6 +187,9 @@ export class FrameManager extends EventEmitter { } throw error; + } finally { + this.#framesPendingTargetInit.get(targetId)?.resolve(); + this.#framesPendingTargetInit.delete(targetId); } } @@ -262,7 +290,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 +369,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 +381,84 @@ 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.#framesPendingTargetInit.has(parentFrameId)) { + if (!this.#framesPendingAttachment.has(frameId)) { + this.#framesPendingAttachment.set( + frameId, + createDeferredPromiseWithTimer( + `Waiting for frame ${frameId} to attach failed` + ) + ); + } + this.#framesPendingTargetInit.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/src/common/util.ts b/src/common/util.ts index d9878ec11ed26..c68ddab5a5e82 100644 --- a/src/common/util.ts +++ b/src/common/util.ts @@ -523,3 +523,46 @@ export function isErrnoException(obj: unknown): obj is NodeJS.ErrnoException { ('errno' in obj || 'code' in obj || 'path' in obj || 'syscall' in obj) ); } + +/** + * @internal + */ +export type DeferredPromise = { + promise: Promise; + resolve: (_: T) => void; + reject: (_: Error) => void; +}; + +/** + * Creates an returns a promise along with the resolve/reject functions. + * + * If the promise has not been resolved/rejected withing the `timeout` period, + * the promise gets rejected with a timeout error. + * + * @internal + */ +export function createDeferredPromiseWithTimer( + timeoutMessage: string, + timeout = 5000 +): DeferredPromise { + 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); + }, + }; +} 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();