From d77827937937228b0b626f60a3d44c06c2802a2b Mon Sep 17 00:00:00 2001
From: Alex Rudenko <alexrudenko@chromium.org>
Date: Tue, 8 Aug 2023 12:49:13 +0200
Subject: [PATCH] refactor: turn Frame into EventEmitter

---
 docs/api/puppeteer.frame.md                   |  4 +-
 packages/puppeteer-core/src/api/Frame.ts      |  7 ++-
 packages/puppeteer-core/src/common/Frame.ts   | 18 +++++-
 .../puppeteer-core/src/common/FrameManager.ts | 28 ++++++---
 .../src/common/IsolatedWorld.ts               |  2 +-
 .../src/common/LifecycleWatcher.ts            | 63 ++++++-------------
 test/src/launcher.spec.ts                     | 14 ++---
 7 files changed, 72 insertions(+), 64 deletions(-)

diff --git a/docs/api/puppeteer.frame.md b/docs/api/puppeteer.frame.md
index 79bfcadd3303d..b01d1e4f80085 100644
--- a/docs/api/puppeteer.frame.md
+++ b/docs/api/puppeteer.frame.md
@@ -11,9 +11,11 @@ To understand frames, you can think of frames as `<iframe>` elements. Just like
 #### Signature:
 
 ```typescript
-export declare class Frame
+export declare class Frame extends EventEmitter
 ```
 
+**Extends:** [EventEmitter](./puppeteer.eventemitter.md)
+
 ## Remarks
 
 Frame lifecycles are controlled by three events that are all dispatched on the parent [page](./puppeteer.frame.page.md):
diff --git a/packages/puppeteer-core/src/api/Frame.ts b/packages/puppeteer-core/src/api/Frame.ts
index ed105935d1f54..53a8dea7538c1 100644
--- a/packages/puppeteer-core/src/api/Frame.ts
+++ b/packages/puppeteer-core/src/api/Frame.ts
@@ -19,6 +19,7 @@ import {HTTPResponse} from '../api/HTTPResponse.js';
 import {Page, WaitTimeoutOptions} from '../api/Page.js';
 import {CDPSession} from '../common/Connection.js';
 import {DeviceRequestPrompt} from '../common/DeviceRequestPrompt.js';
+import {EventEmitter} from '../common/EventEmitter.js';
 import {ExecutionContext} from '../common/ExecutionContext.js';
 import {getQueryHandlerAndSelector} from '../common/GetQueryHandler.js';
 import {
@@ -223,7 +224,7 @@ export interface FrameAddStyleTagOptions {
  *
  * @public
  */
-export class Frame {
+export class Frame extends EventEmitter {
   /**
    * @internal
    */
@@ -251,7 +252,9 @@ export class Frame {
   /**
    * @internal
    */
-  constructor() {}
+  constructor() {
+    super();
+  }
 
   /**
    * The page associated with the frame.
diff --git a/packages/puppeteer-core/src/common/Frame.ts b/packages/puppeteer-core/src/common/Frame.ts
index ea32041ace176..3d93dc4d84206 100644
--- a/packages/puppeteer-core/src/common/Frame.ts
+++ b/packages/puppeteer-core/src/common/Frame.ts
@@ -37,6 +37,20 @@ import {LifecycleWatcher, PuppeteerLifeCycleEvent} from './LifecycleWatcher.js';
 import {EvaluateFunc, EvaluateFuncWith, HandleFor, NodeFor} from './types.js';
 import {withSourcePuppeteerURLIfNone} from './util.js';
 
+/**
+ * We use symbols to prevent external parties listening to these events.
+ * They are internal to Puppeteer.
+ *
+ * @internal
+ */
+export const FrameEmittedEvents = {
+  FrameNavigated: Symbol('Frame.FrameNavigated'),
+  FrameSwapped: Symbol('Frame.FrameSwapped'),
+  LifecycleEvent: Symbol('Frame.LifecycleEvent'),
+  FrameNavigatedWithinDocument: Symbol('Frame.FrameNavigatedWithinDocument'),
+  FrameDetached: Symbol('Frame.FrameDetached'),
+};
+
 /**
  * @internal
  */
@@ -106,7 +120,7 @@ export class Frame extends BaseFrame {
 
     let ensureNewDocumentNavigation = false;
     const watcher = new LifecycleWatcher(
-      this._frameManager,
+      this._frameManager.networkManager,
       this,
       waitUntil,
       timeout
@@ -180,7 +194,7 @@ export class Frame extends BaseFrame {
       timeout = this._frameManager.timeoutSettings.navigationTimeout(),
     } = options;
     const watcher = new LifecycleWatcher(
-      this._frameManager,
+      this._frameManager.networkManager,
       this,
       waitUntil,
       timeout
diff --git a/packages/puppeteer-core/src/common/FrameManager.ts b/packages/puppeteer-core/src/common/FrameManager.ts
index 493ece643176c..3f6c6a0b4bc1e 100644
--- a/packages/puppeteer-core/src/common/FrameManager.ts
+++ b/packages/puppeteer-core/src/common/FrameManager.ts
@@ -20,12 +20,15 @@ import {Page} from '../api/Page.js';
 import {assert} from '../util/assert.js';
 import {isErrorLike} from '../util/ErrorLike.js';
 
-import {CDPSession, isTargetClosedError} from './Connection.js';
+import {
+  CDPSession,
+  CDPSessionEmittedEvents,
+  isTargetClosedError,
+} from './Connection.js';
 import {DeviceRequestPromptManager} from './DeviceRequestPrompt.js';
 import {EventEmitter} from './EventEmitter.js';
 import {ExecutionContext} from './ExecutionContext.js';
-import {Frame} from './Frame.js';
-import {Frame as CDPFrame} from './Frame.js';
+import {Frame, FrameEmittedEvents} from './Frame.js';
 import {FrameTree} from './FrameTree.js';
 import {IsolatedWorld} from './IsolatedWorld.js';
 import {MAIN_WORLD, PUPPETEER_WORLD} from './IsolatedWorlds.js';
@@ -54,8 +57,6 @@ export const FrameManagerEmittedEvents = {
   FrameNavigatedWithinDocument: Symbol(
     'FrameManager.FrameNavigatedWithinDocument'
   ),
-  ExecutionContextCreated: Symbol('FrameManager.ExecutionContextCreated'),
-  ExecutionContextDestroyed: Symbol('FrameManager.ExecutionContextDestroyed'),
 };
 
 /**
@@ -111,6 +112,12 @@ export class FrameManager extends EventEmitter {
     this.#networkManager = new NetworkManager(client, ignoreHTTPSErrors, this);
     this.#timeoutSettings = timeoutSettings;
     this.setupEventListeners(this.#client);
+    client.once(CDPSessionEmittedEvents.Disconnected, () => {
+      const mainFrame = this._frameTree.getMainFrame();
+      if (mainFrame) {
+        this.#removeFramesRecursively(mainFrame);
+      }
+    });
   }
 
   private setupEventListeners(session: CDPSession) {
@@ -248,6 +255,7 @@ export class FrameManager extends EventEmitter {
     }
     frame._onLifecycleEvent(event.loaderId, event.name);
     this.emit(FrameManagerEmittedEvents.LifecycleEvent, frame);
+    frame.emit(FrameEmittedEvents.LifecycleEvent);
   }
 
   #onFrameStartedLoading(frameId: string): void {
@@ -265,6 +273,7 @@ export class FrameManager extends EventEmitter {
     }
     frame._onLoadingStopped();
     this.emit(FrameManagerEmittedEvents.LifecycleEvent, frame);
+    frame.emit(FrameEmittedEvents.LifecycleEvent);
   }
 
   #handleFrameTree(
@@ -309,7 +318,7 @@ export class FrameManager extends EventEmitter {
       return;
     }
 
-    frame = new CDPFrame(this, frameId, parentFrameId, session);
+    frame = new Frame(this, frameId, parentFrameId, session);
     this._frameTree.addFrame(frame);
     this.emit(FrameManagerEmittedEvents.FrameAttached, frame);
   }
@@ -335,7 +344,7 @@ export class FrameManager extends EventEmitter {
         frame._id = frameId;
       } else {
         // Initial main frame navigation.
-        frame = new CDPFrame(this, frameId, undefined, this.#client);
+        frame = new Frame(this, frameId, undefined, this.#client);
       }
       this._frameTree.addFrame(frame);
     }
@@ -343,6 +352,7 @@ export class FrameManager extends EventEmitter {
     frame = await this._frameTree.waitForFrame(frameId);
     frame._navigated(framePayload);
     this.emit(FrameManagerEmittedEvents.FrameNavigated, frame);
+    frame.emit(FrameEmittedEvents.FrameNavigated);
   }
 
   async #createIsolatedWorld(session: CDPSession, name: string): Promise<void> {
@@ -385,7 +395,9 @@ export class FrameManager extends EventEmitter {
     }
     frame._navigatedWithinDocument(url);
     this.emit(FrameManagerEmittedEvents.FrameNavigatedWithinDocument, frame);
+    frame.emit(FrameEmittedEvents.FrameNavigatedWithinDocument);
     this.emit(FrameManagerEmittedEvents.FrameNavigated, frame);
+    frame.emit(FrameEmittedEvents.FrameNavigated);
   }
 
   #onFrameDetached(
@@ -402,6 +414,7 @@ export class FrameManager extends EventEmitter {
       }
     } else if (reason === 'swap') {
       this.emit(FrameManagerEmittedEvents.FrameSwapped, frame);
+      frame?.emit(FrameEmittedEvents.FrameSwapped);
     }
   }
 
@@ -478,5 +491,6 @@ export class FrameManager extends EventEmitter {
     frame._detach();
     this._frameTree.removeFrame(frame);
     this.emit(FrameManagerEmittedEvents.FrameDetached, frame);
+    frame.emit(FrameEmittedEvents.FrameDetached, frame);
   }
 }
diff --git a/packages/puppeteer-core/src/common/IsolatedWorld.ts b/packages/puppeteer-core/src/common/IsolatedWorld.ts
index 9bd39a662a2a1..8f8bcd6118219 100644
--- a/packages/puppeteer-core/src/common/IsolatedWorld.ts
+++ b/packages/puppeteer-core/src/common/IsolatedWorld.ts
@@ -293,7 +293,7 @@ export class IsolatedWorld implements Realm {
     await setPageContent(this, html);
 
     const watcher = new LifecycleWatcher(
-      this.#frameManager,
+      this.#frameManager.networkManager,
       this.#frame,
       waitUntil,
       timeout
diff --git a/packages/puppeteer-core/src/common/LifecycleWatcher.ts b/packages/puppeteer-core/src/common/LifecycleWatcher.ts
index 8a90502955574..8ed77393140b5 100644
--- a/packages/puppeteer-core/src/common/LifecycleWatcher.ts
+++ b/packages/puppeteer-core/src/common/LifecycleWatcher.ts
@@ -18,12 +18,10 @@ import {HTTPResponse} from '../api/HTTPResponse.js';
 import {assert} from '../util/assert.js';
 import {Deferred} from '../util/Deferred.js';
 
-import {CDPSessionEmittedEvents} from './Connection.js';
 import {TimeoutError} from './Errors.js';
-import {Frame} from './Frame.js';
-import {FrameManager, FrameManagerEmittedEvents} from './FrameManager.js';
+import {Frame, FrameEmittedEvents} from './Frame.js';
 import {HTTPRequest} from './HTTPRequest.js';
-import {NetworkManagerEmittedEvents} from './NetworkManager.js';
+import {NetworkManager, NetworkManagerEmittedEvents} from './NetworkManager.js';
 import {
   addEventListener,
   PuppeteerEventListener,
@@ -62,7 +60,6 @@ const puppeteerToProtocolLifecycle = new Map<
  */
 export class LifecycleWatcher {
   #expectedLifecycle: ProtocolLifeCycleEvent[];
-  #frameManager: FrameManager;
   #frame: Frame;
   #timeout: number;
   #navigationRequest: HTTPRequest | null = null;
@@ -80,7 +77,7 @@ export class LifecycleWatcher {
   #navigationResponseReceived?: Deferred<void>;
 
   constructor(
-    frameManager: FrameManager,
+    networkManager: NetworkManager,
     frame: Frame,
     waitUntil: PuppeteerLifeCycleEvent | PuppeteerLifeCycleEvent[],
     timeout: number
@@ -97,55 +94,46 @@ export class LifecycleWatcher {
       return protocolEvent as ProtocolLifeCycleEvent;
     });
 
-    this.#frameManager = frameManager;
     this.#frame = frame;
     this.#timeout = timeout;
     this.#eventListeners = [
       addEventListener(
-        frameManager.client,
-        CDPSessionEmittedEvents.Disconnected,
-        this.#terminate.bind(
-          this,
-          new Error('Navigation failed because browser has disconnected!')
-        )
-      ),
-      addEventListener(
-        this.#frameManager,
-        FrameManagerEmittedEvents.LifecycleEvent,
+        frame,
+        FrameEmittedEvents.LifecycleEvent,
         this.#checkLifecycleComplete.bind(this)
       ),
       addEventListener(
-        this.#frameManager,
-        FrameManagerEmittedEvents.FrameNavigatedWithinDocument,
+        frame,
+        FrameEmittedEvents.FrameNavigatedWithinDocument,
         this.#navigatedWithinDocument.bind(this)
       ),
       addEventListener(
-        this.#frameManager,
-        FrameManagerEmittedEvents.FrameNavigated,
+        frame,
+        FrameEmittedEvents.FrameNavigated,
         this.#navigated.bind(this)
       ),
       addEventListener(
-        this.#frameManager,
-        FrameManagerEmittedEvents.FrameSwapped,
+        frame,
+        FrameEmittedEvents.FrameSwapped,
         this.#frameSwapped.bind(this)
       ),
       addEventListener(
-        this.#frameManager,
-        FrameManagerEmittedEvents.FrameDetached,
+        frame,
+        FrameEmittedEvents.FrameDetached,
         this.#onFrameDetached.bind(this)
       ),
       addEventListener(
-        this.#frameManager.networkManager,
+        networkManager,
         NetworkManagerEmittedEvents.Request,
         this.#onRequest.bind(this)
       ),
       addEventListener(
-        this.#frameManager.networkManager,
+        networkManager,
         NetworkManagerEmittedEvents.Response,
         this.#onResponse.bind(this)
       ),
       addEventListener(
-        this.#frameManager.networkManager,
+        networkManager,
         NetworkManagerEmittedEvents.RequestFailed,
         this.#onRequestFailed.bind(this)
       ),
@@ -204,10 +192,6 @@ export class LifecycleWatcher {
     return this.#navigationRequest ? this.#navigationRequest.response() : null;
   }
 
-  #terminate(error: Error): void {
-    this.#terminationDeferred.resolve(error);
-  }
-
   sameDocumentNavigationPromise(): Promise<Error | undefined> {
     return this.#sameDocumentNavigationDeferred.valueOrThrow();
   }
@@ -224,25 +208,16 @@ export class LifecycleWatcher {
     return this.#terminationDeferred.valueOrThrow();
   }
 
-  #navigatedWithinDocument(frame: Frame): void {
-    if (frame !== this.#frame) {
-      return;
-    }
+  #navigatedWithinDocument(): void {
     this.#hasSameDocumentNavigation = true;
     this.#checkLifecycleComplete();
   }
 
-  #navigated(frame: Frame): void {
-    if (frame !== this.#frame) {
-      return;
-    }
+  #navigated(): void {
     this.#checkLifecycleComplete();
   }
 
-  #frameSwapped(frame: Frame): void {
-    if (frame !== this.#frame) {
-      return;
-    }
+  #frameSwapped(): void {
     this.#swapped = true;
     this.#checkLifecycleComplete();
   }
diff --git a/test/src/launcher.spec.ts b/test/src/launcher.spec.ts
index 41d29754b558c..9e36b705bad84 100644
--- a/test/src/launcher.spec.ts
+++ b/test/src/launcher.spec.ts
@@ -59,7 +59,7 @@ describe('Launcher specs', function () {
           const error = await navigationPromise;
           expect(
             [
-              'Navigation failed because browser has disconnected!',
+              'Navigating frame was detached',
               'Protocol error (Page.navigate): Target closed.',
             ].includes(error.message)
           ).toBeTruthy();
@@ -82,7 +82,7 @@ describe('Launcher specs', function () {
             });
           remote.disconnect();
           const error = await watchdog;
-          expect(error.message).toContain('Protocol error');
+          expect(error.message).toContain('frame got detached');
         } finally {
           await close();
         }
@@ -766,11 +766,6 @@ describe('Launcher specs', function () {
 
           const pages = await remoteBrowser.pages();
 
-          await page2.close();
-          await page1.close();
-          remoteBrowser.disconnect();
-          await browser.close();
-
           expect(
             pages
               .map((p: Page) => {
@@ -778,6 +773,11 @@ describe('Launcher specs', function () {
               })
               .sort()
           ).toEqual(['about:blank', server.EMPTY_PAGE]);
+
+          await page2.close();
+          await page1.close();
+          remoteBrowser.disconnect();
+          await browser.close();
         } finally {
           await close();
         }