fix(socks): cleanup event listeners upon disconnect (#19671)

References #19661.
microsoft · Dec 23, 2022 · 83418aa · 83418aa
1 parent 06c7f1f
commit 83418aa
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 4 deletions.
diff --git a/packages/playwright-core/src/server/dispatchers/playwrightDispatcher.ts b/packages/playwright-core/src/server/dispatchers/playwrightDispatcher.ts
@@ -33,6 +33,7 @@ import { ConnectedBrowserDispatcher } from './browserDispatcher';
 import { createGuid } from '../../utils';
 import type { AndroidDevice } from '../android/android';
 import { AndroidDeviceDispatcher } from './androidDispatcher';
+import { eventsHelper, type RegisteredListener } from '../../utils/eventsHelper';
 
 export class PlaywrightDispatcher extends Dispatcher<Playwright, channels.PlaywrightChannel, RootDispatcher> implements channels.PlaywrightChannel {
   _type_Playwright;
@@ -76,14 +77,17 @@ export class PlaywrightDispatcher extends Dispatcher<Playwright, channels.Playwr
 class SocksSupportDispatcher extends Dispatcher<{ guid: string }, channels.SocksSupportChannel, RootDispatcher> implements channels.SocksSupportChannel {
   _type_SocksSupport: boolean;
   private _socksProxy: SocksProxy;
+  private _socksListeners: RegisteredListener[];
 
   constructor(scope: RootDispatcher, socksProxy: SocksProxy) {
     super(scope, { guid: 'socksSupport@' + createGuid() }, 'SocksSupport', {});
     this._type_SocksSupport = true;
     this._socksProxy = socksProxy;
-    socksProxy.on(SocksProxy.Events.SocksRequested, (payload: SocksSocketRequestedPayload) => this._dispatchEvent('socksRequested', payload));
-    socksProxy.on(SocksProxy.Events.SocksData, (payload: SocksSocketDataPayload) => this._dispatchEvent('socksData', payload));
-    socksProxy.on(SocksProxy.Events.SocksClosed, (payload: SocksSocketClosedPayload) => this._dispatchEvent('socksClosed', payload));
+    this._socksListeners = [
+      eventsHelper.addEventListener(socksProxy, SocksProxy.Events.SocksRequested, (payload: SocksSocketRequestedPayload) => this._dispatchEvent('socksRequested', payload)),
+      eventsHelper.addEventListener(socksProxy, SocksProxy.Events.SocksData, (payload: SocksSocketDataPayload) => this._dispatchEvent('socksData', payload)),
+      eventsHelper.addEventListener(socksProxy, SocksProxy.Events.SocksClosed, (payload: SocksSocketClosedPayload) => this._dispatchEvent('socksClosed', payload)),
+    ];
   }
 
   async socksConnected(params: channels.SocksSupportSocksConnectedParams): Promise<void> {
@@ -105,4 +109,8 @@ class SocksSupportDispatcher extends Dispatcher<{ guid: string }, channels.Socks
   async socksEnd(params: channels.SocksSupportSocksEndParams): Promise<void> {
     this._socksProxy?.sendSocketEnd(params);
   }
+
+  override _onDispose() {
+    eventsHelper.removeEventListeners(this._socksListeners);
+  }
 }
diff --git a/tests/config/remote-server-impl.js b/tests/config/remote-server-impl.js
@@ -2,13 +2,15 @@ const fs = require('fs');
 const cluster = require('cluster');
 
 async function start() {
-  const { browserTypeName, launchOptions, stallOnClose, disconnectOnSIGHUP, exitOnFile } = JSON.parse(process.argv[2]);
+  const { browserTypeName, launchOptions, stallOnClose, disconnectOnSIGHUP, exitOnFile, exitOnWarning } = JSON.parse(process.argv[2]);
   if (stallOnClose) {
     launchOptions.__testHookGracefullyClose = () => {
       console.log(`(stalled=>true)`);
       return new Promise(() => { });
     };
   }
+  if (exitOnWarning)
+    process.on('warning', () => process.exit(43));
 
   const playwright = require('playwright-core');
 

diff --git a/tests/config/remoteServer.ts b/tests/config/remoteServer.ts
@@ -58,6 +58,7 @@ export type RemoteServerOptions = {
   stallOnClose?: boolean;
   disconnectOnSIGHUP?: boolean;
   exitOnFile?: string;
+  exitOnWarning?: boolean;
   inCluster?: boolean;
   url?: string;
 };

diff --git a/tests/library/browsertype-connect.spec.ts b/tests/library/browsertype-connect.spec.ts
@@ -845,4 +845,20 @@ test.describe('launchServer only', () => {
     expect((await page.goto(server.EMPTY_PAGE).catch(e => e)).message).toContain('has been closed');
     expect((await page.waitForNavigation().catch(e => e)).message).toContain('Navigation failed because page was closed');
   });
+
+  test('should be able to reconnect to a browser 12 times without warnings', async ({ connect, startRemoteServer, server }) => {
+    test.slow();
+    const remoteServer = await startRemoteServer('launchServer', { exitOnWarning: true });
+    for (let i = 0; i < 12; i++) {
+      await test.step('connect #' + i, async () => {
+        const browser = await connect(remoteServer.wsEndpoint());
+        const browserContext = await browser.newContext();
+        expect(browserContext.pages().length).toBe(0);
+        const page = await browserContext.newPage();
+        expect(await page.evaluate('11 * 11')).toBe(121);
+        await page.goto(server.EMPTY_PAGE);
+        await browser.close();
+      });
+    }
+  });
 });