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 · f715ed5 · f715ed5
1 parent bed9a76
commit f715ed5
Show file tree

Hide file tree

Showing 8 changed files with 46 additions and 3 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"],

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
@@ -66,7 +66,7 @@ describe('TargetManager', () => {
     expect(targetManager.getAvailableTargets().size).toBe(5);
     expect(page.frames()).toHaveLength(2);
 
-    // // attach a remote frame iframe.
+    // attach a remote frame iframe.
     framePromise = page.waitForFrame(frame => {
       return frame.url() === server.CROSS_PROCESS_PREFIX + '/empty.html';
     });

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;