refactor: move console event handling (#12407)

puppeteer · May 7, 2024 · 7712dff · 7712dff
1 parent bc17e33
commit 7712dff
Show file tree

Hide file tree

Showing 8 changed files with 95 additions and 52 deletions.
diff --git a/packages/puppeteer-core/src/cdp/ExecutionContext.ts b/packages/puppeteer-core/src/cdp/ExecutionContext.ts
@@ -6,7 +6,7 @@
 
 import type {Protocol} from 'devtools-protocol';
 
-import type {CDPSession} from '../api/CDPSession.js';
+import {CDPSessionEvent, type CDPSession} from '../api/CDPSession.js';
 import type {ElementHandle} from '../api/ElementHandle.js';
 import type {JSHandle} from '../api/JSHandle.js';
 import {EventEmitter} from '../common/EventEmitter.js';
@@ -66,6 +66,7 @@ export class ExecutionContext
   extends EventEmitter<{
     /** Emitted when this execution context is disposed. */
     disposed: undefined;
+    consoleapicalled: Protocol.Runtime.ConsoleAPICalledEvent;
   }>
   implements Disposable
 {
@@ -98,6 +99,10 @@ export class ExecutionContext
     clientEmitter.on('Runtime.executionContextsCleared', async () => {
       this[disposeSymbol]();
     });
+    clientEmitter.on('Runtime.consoleAPICalled', this.#onConsoleAPI.bind(this));
+    clientEmitter.on(CDPSessionEvent.Disconnected, () => {
+      this[disposeSymbol]();
+    });
   }
 
   // Contains mapping from functions that should be bound to Puppeteer functions.
@@ -179,6 +184,13 @@ export class ExecutionContext
     }
   }
 
+  #onConsoleAPI(event: Protocol.Runtime.ConsoleAPICalledEvent): void {
+    if (event.executionContextId !== this._contextId) {
+      return;
+    }
+    this.emit('consoleapicalled', event);
+  }
+
   #bindingsInstalled = false;
   #puppeteerUtil?: Promise<JSHandle<PuppeteerUtil>>;
   get puppeteerUtil(): Promise<JSHandle<PuppeteerUtil>> {

diff --git a/packages/puppeteer-core/src/cdp/Frame.ts b/packages/puppeteer-core/src/cdp/Frame.ts
@@ -20,6 +20,7 @@ import type {
   DeviceRequestPromptManager,
 } from './DeviceRequestPrompt.js';
 import type {FrameManager} from './FrameManager.js';
+import {FrameManagerEvent} from './FrameManagerEvents.js';
 import type {IsolatedWorldChart} from './IsolatedWorld.js';
 import {IsolatedWorld} from './IsolatedWorld.js';
 import {MAIN_WORLD, PUPPETEER_WORLD} from './IsolatedWorlds.js';
@@ -74,6 +75,20 @@ export class CdpFrame extends Frame {
       this._onLoadingStarted();
       this._onLoadingStopped();
     });
+
+    this.worlds[MAIN_WORLD].emitter.on(
+      'consoleapicalled',
+      this.#onMainWorldConsoleApiCalled.bind(this)
+    );
+  }
+
+  #onMainWorldConsoleApiCalled(
+    event: Protocol.Runtime.ConsoleAPICalledEvent
+  ): void {
+    this._frameManager.emit(FrameManagerEvent.ConsoleApiCalled, [
+      this.worlds[MAIN_WORLD],
+      event,
+    ]);
   }
 
   /**

diff --git a/packages/puppeteer-core/src/cdp/FrameManagerEvents.ts b/packages/puppeteer-core/src/cdp/FrameManagerEvents.ts
@@ -4,9 +4,12 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import type Protocol from 'devtools-protocol';
+
 import type {EventType} from '../common/EventEmitter.js';
 
 import type {CdpFrame} from './Frame.js';
+import type {IsolatedWorld} from './IsolatedWorld.js';
 
 /**
  * We use symbols to prevent external parties listening to these events.
@@ -24,6 +27,7 @@ export namespace FrameManagerEvent {
   export const FrameNavigatedWithinDocument = Symbol(
     'FrameManager.FrameNavigatedWithinDocument'
   );
+  export const ConsoleApiCalled = Symbol('FrameManager.ConsoleApiCalled');
 }
 
 /**
@@ -36,4 +40,9 @@ export interface FrameManagerEvents extends Record<EventType, unknown> {
   [FrameManagerEvent.FrameSwapped]: CdpFrame;
   [FrameManagerEvent.LifecycleEvent]: CdpFrame;
   [FrameManagerEvent.FrameNavigatedWithinDocument]: CdpFrame;
+  // Emitted when a new console message is logged.
+  [FrameManagerEvent.ConsoleApiCalled]: [
+    IsolatedWorld,
+    Protocol.Runtime.ConsoleAPICalledEvent,
+  ];
 }
diff --git a/packages/puppeteer-core/src/cdp/IsolatedWorld.ts b/packages/puppeteer-core/src/cdp/IsolatedWorld.ts
@@ -44,17 +44,24 @@ export interface IsolatedWorldChart {
   [PUPPETEER_WORLD]: IsolatedWorld;
 }
 
+/**
+ * @internal
+ */
+type IsolatedWorldEmitter = EventEmitter<{
+  // Emitted when the isolated world gets a new execution context.
+  context: ExecutionContext;
+  // Emitted when the isolated world is disposed.
+  disposed: undefined;
+  // Emitted when a new console message is logged.
+  consoleapicalled: Protocol.Runtime.ConsoleAPICalledEvent;
+}>;
+
 /**
  * @internal
  */
 export class IsolatedWorld extends Realm {
   #context?: ExecutionContext;
-  #emitter = new EventEmitter<{
-    // Emitted when the isolated world gets a new execution context.
-    context: ExecutionContext;
-    // Emitted when the isolated world is disposed.
-    disposed: undefined;
-  }>();
+  #emitter: IsolatedWorldEmitter = new EventEmitter();
 
   readonly #frameOrWorker: CdpFrame | CdpWebWorker;
 
@@ -74,27 +81,40 @@ export class IsolatedWorld extends Realm {
     return this.#frameOrWorker.client;
   }
 
+  get emitter(): IsolatedWorldEmitter {
+    return this.#emitter;
+  }
+
   setContext(context: ExecutionContext): void {
     this.#context?.[disposeSymbol]();
-    context.once('disposed', () => {
-      this.#context = undefined;
-      if ('clearDocumentHandle' in this.#frameOrWorker) {
-        this.#frameOrWorker.clearDocumentHandle();
-      }
-    });
+    context.once('disposed', this.#onContextDisposed.bind(this));
+    context.on('consoleapicalled', this.#onContextConsoleApiCalled.bind(this));
     this.#context = context;
     this.#emitter.emit('context', context);
     void this.taskManager.rerunAll();
   }
 
+  #onContextDisposed(): void {
+    this.#context = undefined;
+    if ('clearDocumentHandle' in this.#frameOrWorker) {
+      this.#frameOrWorker.clearDocumentHandle();
+    }
+  }
+
+  #onContextConsoleApiCalled(
+    event: Protocol.Runtime.ConsoleAPICalledEvent
+  ): void {
+    this.#emitter.emit('consoleapicalled', event);
+  }
+
   hasContext(): boolean {
     return !!this.#context;
   }
 
   #executionContext(): ExecutionContext | undefined {
     if (this.disposed) {
       throw new Error(
-        `Execution context is not available in detached frame "${this.environment.url()}" (are you trying to evaluate?)`
+        `Execution context is not available in detached frame or worker "${this.environment.url()}" (are you trying to evaluate?)`
       );
     }
     return this.#context;
@@ -226,5 +246,6 @@ export class IsolatedWorld extends Realm {
     this.#context?.[disposeSymbol]();
     this.#emitter.emit('disposed', undefined);
     super[disposeSymbol]();
+    this.#emitter.removeAllListeners();
   }
 }
diff --git a/packages/puppeteer-core/src/cdp/Page.ts b/packages/puppeteer-core/src/cdp/Page.ts
@@ -69,6 +69,7 @@ import type {CdpFrame} from './Frame.js';
 import {FrameManager} from './FrameManager.js';
 import {FrameManagerEvent} from './FrameManagerEvents.js';
 import {CdpKeyboard, CdpMouse, CdpTouchscreen} from './Input.js';
+import type {IsolatedWorld} from './IsolatedWorld.js';
 import {MAIN_WORLD} from './IsolatedWorlds.js';
 import {releaseObject} from './JSHandle.js';
 import type {NetworkConditions} from './NetworkManager.js';
@@ -216,7 +217,6 @@ export class CdpPage extends Page {
         return this.emit(PageEvent.Load, undefined);
       },
     ],
-    ['Runtime.consoleAPICalled', this.#onConsoleAPI.bind(this)],
     ['Runtime.bindingCalled', this.#onBindingCalled.bind(this)],
     ['Page.javascriptDialogOpening', this.#onDialog.bind(this)],
     ['Runtime.exceptionThrown', this.#handleException.bind(this)],
@@ -249,6 +249,16 @@ export class CdpPage extends Page {
       this.#frameManager.on(eventName, handler);
     }
 
+    this.#frameManager.on(
+      FrameManagerEvent.ConsoleApiCalled,
+      ([world, event]: [
+        IsolatedWorld,
+        Protocol.Runtime.ConsoleAPICalledEvent,
+      ]) => {
+        this.#onConsoleAPI(world, event);
+      }
+    );
+
     for (const [eventName, handler] of this.#networkManagerHandlers) {
       // TODO: Remove any.
       this.#frameManager.networkManager.on(eventName, handler as any);
@@ -778,41 +788,12 @@ export class CdpPage extends Page {
     );
   }
 
-  async #onConsoleAPI(
+  #onConsoleAPI(
+    world: IsolatedWorld,
     event: Protocol.Runtime.ConsoleAPICalledEvent
-  ): Promise<void> {
-    if (event.executionContextId === 0) {
-      // DevTools protocol stores the last 1000 console messages. These
-      // messages are always reported even for removed execution contexts. In
-      // this case, they are marked with executionContextId = 0 and are
-      // reported upon enabling Runtime agent.
-      //
-      // Ignore these messages since:
-      // - there's no execution context we can use to operate with message
-      //   arguments
-      // - these messages are reported before Puppeteer clients can subscribe
-      //   to the 'console'
-      //   page event.
-      //
-      // @see https://github.com/puppeteer/puppeteer/issues/3865
-      return;
-    }
-    const context = this.#frameManager.getExecutionContextById(
-      event.executionContextId,
-      this.#primaryTargetClient
-    );
-    if (!context) {
-      debugError(
-        new Error(
-          `ExecutionContext not found for a console message: ${JSON.stringify(
-            event
-          )}`
-        )
-      );
-      return;
-    }
+  ): void {
     const values = event.args.map(arg => {
-      return context._world.createCdpHandle(arg);
+      return world.createCdpHandle(arg);
     });
     this.#addConsoleMessage(
       convertConsoleMessageLevel(event.type),

diff --git a/packages/puppeteer-core/src/cdp/WebWorker.ts b/packages/puppeteer-core/src/cdp/WebWorker.ts
@@ -5,7 +5,7 @@
  */
 import type {Protocol} from 'devtools-protocol';
 
-import type {CDPSession} from '../api/CDPSession.js';
+import {CDPSessionEvent, type CDPSession} from '../api/CDPSession.js';
 import type {Realm} from '../api/Realm.js';
 import {TargetType} from '../api/Target.js';
 import {WebWorker} from '../api/WebWorker.js';
@@ -60,7 +60,7 @@ export class CdpWebWorker extends WebWorker {
         new ExecutionContext(client, event.context, this.#world)
       );
     });
-    this.#client.on('Runtime.consoleAPICalled', async event => {
+    this.#world.emitter.on('consoleapicalled', async event => {
       try {
         return consoleAPICalled(
           event.type,
@@ -74,6 +74,9 @@ export class CdpWebWorker extends WebWorker {
       }
     });
     this.#client.on('Runtime.exceptionThrown', exceptionThrown);
+    this.#client.once(CDPSessionEvent.Disconnected, () => {
+      this.#world.dispose();
+    });
 
     // This might fail if the target is closed before we receive all execution contexts.
     this.#client.send('Runtime.enable').catch(debugError);

diff --git a/test/src/launcher.spec.ts b/test/src/launcher.spec.ts
@@ -73,7 +73,9 @@ describe('Launcher specs', function () {
             });
           await remote.disconnect();
           const error = await watchdog;
-          expect(error.message).toContain('Session closed.');
+          expect(error.message).toContain(
+            'Waiting for selector `div` failed: waitForFunction failed: frame got detached.'
+          );
         } finally {
           await close();
         }

diff --git a/test/src/worker.spec.ts b/test/src/worker.spec.ts
@@ -53,8 +53,8 @@ describe('Workers', function () {
       return error;
     });
     expect(error.message).atLeastOneToContain([
-      'Most likely the worker has been closed.',
       'Realm already destroyed.',
+      'Execution context is not available in detached frame',
     ]);
   });
   it('should report console logs', async () => {