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

@@ -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<string>();
   #mainFrame?: Frame;
   #client: CDPSession;
+  /**
+   * Keeps track of OOPIF targets/frames (target ID == frame ID for OOPIFs)
+   * that are being initialized.
+   */
+  #framesPendingTargetInit = new Map<string, DeferredPromise<void>>();
+  /**
+   * Keeps track of frames that are in the process of being attached in #onFrameAttached.
+   */
+  #framesPendingAttachment = new Map<string, DeferredPromise<void>>();
 
   /**
    * @internal
@@ -132,8 +146,19 @@ export class FrameManager extends EventEmitter {
     });
   }
 
-  async initialize(client: CDPSession = this.#client): Promise<void> {
+  async initialize(
+    targetId: string,
+    client: CDPSession = this.#client
+  ): Promise<void> {
     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<void> {
@@ -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);
+    const complete = (parentFrame: Frame) => {
-    this.#frames.set(frame._id, frame);
+      assert(parentFrame, `Parent frame ${parentFrameId} not found`);
-    this.emit(FrameManagerEmittedEvents.FrameAttached, frame);
+      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
+    const frame = isMainFrame ? this.#mainFrame : this.#frames.get(frameId);
-      ? this.#mainFrame
-      : this.#frames.get(framePayload.id);
-    assert(
-      isMainFrame || frame,
-      'We either navigate top level or have old version of the navigated frame'
-    );
 
-    // Detach all child frames first.
+    const complete = (frame?: Frame) => {
-    if (frame) {
+      assert(
-      for (const child of frame.childFrames()) {
+        isMainFrame || frame,
-        this.#removeFramesRecursively(child);
+        `Missing frame isMainFrame=${isMainFrame}, frameId=${frameId}`
-      }
+      );
-    }
 
-    // Update or create main frame.
+      // Detach all child frames first.
-    if (isMainFrame) {
       if (frame) {
-        // Update frame id to retain frame identity on cross-process navigation.
+        for (const child of frame.childFrames()) {
-        this.#frames.delete(frame._id);
+          this.#removeFramesRecursively(child);
-        frame._id = framePayload.id;
+        }
-      } else {
-        // Initial main frame navigation.
-        frame = new Frame(this, null, framePayload.id, this.#client);
       }
-      this.#frames.set(framePayload.id, frame);
+
-      this.#mainFrame = frame;
+      // 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;
+      }
+
+      // 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);
     }
-
-    // Update frame payload.
-    assert(frame);
-    frame._navigated(framePayload);
-
-    this.emit(FrameManagerEmittedEvents.FrameNavigated, frame);
   }
 
   async _ensureIsolatedWorld(session: CDPSession, name: string): Promise<void> {

src/common/Page.ts

@@ -630,7 +630,7 @@ export class Page extends EventEmitter {
 
   async #initialize(): Promise<void> {
     await Promise.all([
-      this.#frameManager.initialize(),
+      this.#frameManager.initialize(this.#target._targetId),
       this.#client.send('Performance.enable'),
       this.#client.send('Log.enable'),
     ]);

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<T> = {
+  promise: Promise<T>;
+  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<T>(
+  timeoutMessage: string,
+  timeout = 5000
+): DeferredPromise<T> {
+  let resolver = (_: T): void => {};
+  let rejector = (_: Error) => {};
+  const taskPromise = new Promise<T>((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);
+    },
+  };
+}

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();