fix: custom sessions should not emit targetcreated events (#8788)

Closes #8787
puppeteer · Aug 16, 2022 · 3fad05d · 3fad05d
1 parent 65a5ce8
commit 3fad05d
Show file tree

Hide file tree

Showing 5 changed files with 54 additions and 14 deletions.
diff --git a/src/common/Browser.ts b/src/common/Browser.ts
@@ -492,8 +492,11 @@ export class Browser extends EventEmitter {
       session,
       context,
       this.#targetManager,
-      () => {
-        return this.#connection.createSession(targetInfo);
+      (isAutoAttachEmulated: boolean) => {
+        return this.#connection._createSession(
+          targetInfo,
+          isAutoAttachEmulated
+        );
       },
       this.#ignoreHTTPSErrors,
       this.#defaultViewport ?? null,

diff --git a/src/common/ChromeTargetManager.ts b/src/common/ChromeTargetManager.ts
@@ -236,7 +236,7 @@ export class ChromeTargetManager extends EventEmitter implements TargetManager {
       // Special case (https://crbug.com/1338156): currently, shared_workers
       // don't get auto-attached. This should be removed once the auto-attach
       // works.
-      await this.#connection.createSession(event.targetInfo);
+      await this.#connection._createSession(event.targetInfo, true);
     }
   };
 
@@ -300,6 +300,10 @@ export class ChromeTargetManager extends EventEmitter implements TargetManager {
         .catch(debugError);
     };
 
+    if (!this.#connection.isAutoAttached(targetInfo.targetId)) {
+      return;
+    }
+
     // Special case for service workers: being attached to service workers will
     // prevent them from ever being destroyed. Therefore, we silently detach
     // from service workers unless the connection was manually created via
@@ -364,7 +368,9 @@ export class ChromeTargetManager extends EventEmitter implements TargetManager {
     }
 
     this.#targetsIdsForInit.delete(target._targetId);
-    this.emit(TargetManagerEmittedEvents.TargetAvailable, target);
+    if (!existingTarget) {
+      this.emit(TargetManagerEmittedEvents.TargetAvailable, target);
+    }
     this.#finishInitializationIfReady();
 
     // TODO: the browser might be shutting down here. What do we do with the

diff --git a/src/common/Connection.ts b/src/common/Connection.ts
@@ -229,23 +229,36 @@ export class Connection extends EventEmitter {
   }
 
   /**
-   * @param targetInfo - The target info
-   * @returns The CDP session that is created
+   * @internal
    */
-  async createSession(
-    targetInfo: Protocol.Target.TargetInfo
+  async _createSession(
+    targetInfo: Protocol.Target.TargetInfo,
+    isAutoAttachEmulated = true
   ): Promise<CDPSession> {
-    this.#manuallyAttached.add(targetInfo.targetId);
+    if (!isAutoAttachEmulated) {
+      this.#manuallyAttached.add(targetInfo.targetId);
+    }
     const {sessionId} = await this.send('Target.attachToTarget', {
       targetId: targetInfo.targetId,
       flatten: true,
     });
+    this.#manuallyAttached.delete(targetInfo.targetId);
     const session = this.#sessions.get(sessionId);
     if (!session) {
       throw new Error('CDPSession creation failed.');
     }
     return session;
   }
+
+  /**
+   * @param targetInfo - The target info
+   * @returns The CDP session that is created
+   */
+  async createSession(
+    targetInfo: Protocol.Target.TargetInfo
+  ): Promise<CDPSession> {
+    return await this._createSession(targetInfo, false);
+  }
 }
 
 /**

diff --git a/src/common/Target.ts b/src/common/Target.ts
@@ -30,7 +30,7 @@ export class Target {
   #browserContext: BrowserContext;
   #session?: CDPSession;
   #targetInfo: Protocol.Target.TargetInfo;
-  #sessionFactory: () => Promise<CDPSession>;
+  #sessionFactory: (isAutoAttachEmulated: boolean) => Promise<CDPSession>;
   #ignoreHTTPSErrors: boolean;
   #defaultViewport?: Viewport;
   #pagePromise?: Promise<Page>;
@@ -76,7 +76,7 @@ export class Target {
     session: CDPSession | undefined,
     browserContext: BrowserContext,
     targetManager: TargetManager,
-    sessionFactory: () => Promise<CDPSession>,
+    sessionFactory: (isAutoAttachEmulated: boolean) => Promise<CDPSession>,
     ignoreHTTPSErrors: boolean,
     defaultViewport: Viewport | null,
     screenshotTaskQueue: TaskQueue,
@@ -132,7 +132,7 @@ export class Target {
    * Creates a Chrome Devtools Protocol session attached to the target.
    */
   createCDPSession(): Promise<CDPSession> {
-    return this.#sessionFactory();
+    return this.#sessionFactory(false);
   }
 
   /**
@@ -155,7 +155,9 @@ export class Target {
   async page(): Promise<Page | null> {
     if (this._isPageTargetCallback(this.#targetInfo) && !this.#pagePromise) {
       this.#pagePromise = (
-        this.#session ? Promise.resolve(this.#session) : this.#sessionFactory()
+        this.#session
+          ? Promise.resolve(this.#session)
+          : this.#sessionFactory(true)
       ).then(client => {
         return Page._create(
           client,
@@ -182,7 +184,9 @@ export class Target {
     if (!this.#workerPromise) {
       // TODO(einbinder): Make workers send their console logs.
       this.#workerPromise = (
-        this.#session ? Promise.resolve(this.#session) : this.#sessionFactory()
+        this.#session
+          ? Promise.resolve(this.#session)
+          : this.#sessionFactory(false)
       ).then(client => {
         return new WebWorker(
           client,

diff --git a/test/src/CDPSession.spec.ts b/test/src/CDPSession.spec.ts
@@ -42,6 +42,20 @@ describeChromeOnly('Target.createCDPSession', function () {
     });
     expect(foo).toBe('bar');
   });
+
+  it('should not report created targets for custom CDP sessions', async () => {
+    const {browser} = getTestState();
+    let called = 0;
+    browser.browserContexts()[0]!.on('targetcreated', async target => {
+      called++;
+      if (called > 1) {
+        throw new Error('Too many targets created');
+      }
+      await target.createCDPSession();
+    });
+    await browser.newPage();
+  });
+
   it('should send events', async () => {
     const {page, server} = getTestState();