fix: ignore frames removed by PDF viewer

PDF viewer in Chrome replaces the inner iframes with an extension web view. The iframes are still observable in Puppeteer up until the point when they get replaces. In this case, Chrome sends the frameDetached event with the reason swap. Unlike the regular swap where a frame is replace with an OOPIF or vice versa, with PDF viewer the frame is just gone. This PR works around this issue by ignoring any frames that are being swapped.
puppeteer · Apr 30, 2024 · a10f0c7 · a10f0c7
1 parent bed9a76
commit a10f0c7
Show file tree

Hide file tree

Showing 8 changed files with 62 additions and 6 deletions.
diff --git a/packages/puppeteer-core/src/cdp/Frame.ts b/packages/puppeteer-core/src/cdp/Frame.ts
@@ -35,6 +35,7 @@ import type {CdpPage} from './Page.js';
 export class CdpFrame extends Frame {
   #url = '';
   #detached = false;
+  #isBeingSwapped = false;
   #client!: CDPSession;
   worlds!: IsolatedWorldChart;
 
@@ -86,6 +87,7 @@ export class CdpFrame extends Frame {
   }
 
   updateClient(client: CDPSession, keepWorlds = false): void {
+    this.#isBeingSwapped = false;
     this.#client = client;
     if (!keepWorlds) {
       // Clear the current contexts on previous world instances.
@@ -320,6 +322,7 @@ export class CdpFrame extends Frame {
   _navigated(framePayload: Protocol.Page.Frame): void {
     this._name = framePayload.name;
     this.#url = `${framePayload.url}${framePayload.urlFragment || ''}`;
+    this.#isBeingSwapped = false;
   }
 
   _navigatedWithinDocument(url: string): void {
@@ -359,4 +362,13 @@ export class CdpFrame extends Frame {
   exposeFunction(): never {
     throw new UnsupportedOperation();
   }
+
+  onFrameSwap(): void {
+    this.#isBeingSwapped = true;
+    this.emit(FrameEvent.FrameSwapped, undefined);
+  }
+
+  get isBeingSwapped(): boolean {
+    return this.#isBeingSwapped;
+  }
 }
diff --git a/packages/puppeteer-core/src/cdp/FrameManager.ts b/packages/puppeteer-core/src/cdp/FrameManager.ts
@@ -262,7 +262,9 @@ export class FrameManager extends EventEmitter<FrameManagerEvents> {
   }
 
   frames(): CdpFrame[] {
-    return Array.from(this._frameTree.frames());
+    return Array.from(this._frameTree.frames()).filter(frame => {
+      return !frame.isBeingSwapped;
+    });
   }
 
   frame(frameId: string): CdpFrame | null {
@@ -463,7 +465,7 @@ export class FrameManager extends EventEmitter<FrameManagerEvents> {
         break;
       case 'swap':
         this.emit(FrameManagerEvent.FrameSwapped, frame);
-        frame.emit(FrameEvent.FrameSwapped, undefined);
+        frame.onFrameSwap();
         break;
     }
   }

diff --git a/packages/puppeteer-core/src/cdp/LifecycleWatcher.ts b/packages/puppeteer-core/src/cdp/LifecycleWatcher.ts
@@ -255,6 +255,7 @@ export class LifecycleWatcher {
       for (const child of frame.childFrames()) {
         if (
           child._hasStartedLoading &&
+          !child.isBeingSwapped &&
           !checkLifecycle(child, expectedLifecycle)
         ) {
           return false;

diff --git a/test/TestExpectations.json b/test/TestExpectations.json
@@ -569,6 +569,13 @@
     "expectations": ["SKIP"],
     "comment": "TODO: add a comment explaining why this expectation is required (include links to issues)"
   },
+  {
+    "testIdPattern": "[oopif.spec] OOPIF should load a page with a PDF viewer",
+    "platforms": ["darwin", "linux", "win32"],
+    "parameters": ["chrome-headless-shell"],
+    "expectations": ["FAIL"],
+    "comment": "chrome-headless-shell does not have a PDF viewer"
+  },
   {
     "testIdPattern": "[page.spec] Page Page.addScriptTag should throw when added with content to the CSP page",
     "platforms": ["darwin", "linux", "win32"],
@@ -2918,6 +2925,13 @@
     "expectations": ["SKIP"],
     "comment": "Failed previously and currently times out"
   },
+  {
+    "testIdPattern": "[oopif.spec] OOPIF should load a page with a PDF viewer",
+    "platforms": ["darwin", "linux", "win32"],
+    "parameters": ["chrome", "webDriverBiDi"],
+    "expectations": ["FAIL"],
+    "comment": "https://github.com/GoogleChromeLabs/chromium-bidi/issues/2175"
+  },
   {
     "testIdPattern": "[oopif.spec] OOPIF should report google.com frame",
     "platforms": ["darwin", "linux", "win32"],

diff --git a/test/assets/pdf-viewer.html b/test/assets/pdf-viewer.html
@@ -0,0 +1,3 @@
+<!DOCTYPE html>
+
+<iframe src="sample.pdf"></iframe>
diff --git a/test/assets/sample.pdf b/test/assets/sample.pdf
diff --git a/test/src/cdp/TargetManager.spec.ts b/test/src/cdp/TargetManager.spec.ts
@@ -64,9 +64,11 @@ describe('TargetManager', () => {
     await framePromise;
     expect(await context.pages()).toHaveLength(1);
     expect(targetManager.getAvailableTargets().size).toBe(5);
-    expect(page.frames()).toHaveLength(2);
+    await page.waitForFrame(() => {
+      return page.frames().length === 2;
+    });
 
-    // // attach a remote frame iframe.
+    // attach a remote frame iframe.
     framePromise = page.waitForFrame(frame => {
       return frame.url() === server.CROSS_PROCESS_PREFIX + '/empty.html';
     });
@@ -78,7 +80,9 @@ describe('TargetManager', () => {
     await framePromise;
     expect(await context.pages()).toHaveLength(1);
     expect(targetManager.getAvailableTargets().size).toBe(6);
-    expect(page.frames()).toHaveLength(3);
+    await page.waitForFrame(() => {
+      return page.frames().length === 3;
+    });
 
     framePromise = page.waitForFrame(frame => {
       return frame.url() === server.CROSS_PROCESS_PREFIX + '/empty.html';
@@ -91,6 +95,8 @@ describe('TargetManager', () => {
     await framePromise;
     expect(await context.pages()).toHaveLength(1);
     expect(targetManager.getAvailableTargets().size).toBe(7);
-    expect(page.frames()).toHaveLength(4);
+    await page.waitForFrame(() => {
+      return page.frames().length === 4;
+    });
   });
 });
diff --git a/test/src/oopif.spec.ts b/test/src/oopif.spec.ts
@@ -425,6 +425,24 @@ describe('OOPIF', function () {
     ).toEqual([true, true, false]);
   });
 
+  it('should load a page with a PDF viewer', async () => {
+    const {page, server} = state;
+
+    await page.goto(server.PREFIX + '/pdf-viewer.html', {
+      waitUntil: 'networkidle2',
+    });
+
+    expect(
+      await Promise.all(
+        page.frames().map(async frame => {
+          return await frame.evaluate(() => {
+            return window.location.pathname;
+          });
+        })
+      )
+    ).toEqual(['/pdf-viewer.html', '/sample.pdf']);
+  });
+
   describe('waitForFrame', () => {
     it('should resolve immediately if the frame already exists', async () => {
       const {server, page} = state;