From 35e8c440c21ee8f076016a97252ef13a46f3de10 Mon Sep 17 00:00:00 2001
From: Dmitry Gozman <dgozman@gmail.com>
Date: Thu, 14 Dec 2023 10:42:08 -0800
Subject: [PATCH] fix(dispatchers): separate gc bucket for main frame,
 increased limit (#28629)

- Keep main frames in a separate bucket, so that page methods that
redirect to the main frame continue to work.
- Increase default dispatchers limit to `10_000`.
- Increase dispatchers limit for `JSHandle`/`ElementHandle` to
`100_000`.

Fixes #28320, #28503.
---
 .../src/server/dispatchers/dispatcher.ts      | 46 ++++++++++---------
 .../src/server/dispatchers/frameDispatcher.ts |  8 +++-
 tests/stress/frames.spec.ts                   | 24 ++++++++--
 3 files changed, 52 insertions(+), 26 deletions(-)

diff --git a/packages/playwright-core/src/server/dispatchers/dispatcher.ts b/packages/playwright-core/src/server/dispatchers/dispatcher.ts
index 9bcca908ec..6fffb9d83b 100644
--- a/packages/playwright-core/src/server/dispatchers/dispatcher.ts
+++ b/packages/playwright-core/src/server/dispatchers/dispatcher.ts
@@ -34,9 +34,15 @@ export function existingDispatcher<DispatcherType>(object: any): DispatcherType
   return object[dispatcherSymbol];
 }
 
-let maxDispatchers = 1000;
+let maxDispatchersOverride: number | undefined;
 export function setMaxDispatchersForTest(value: number | undefined) {
-  maxDispatchers = value || 1000;
+  maxDispatchersOverride = value;
+}
+function maxDispatchersForBucket(gcBucket: string) {
+  return maxDispatchersOverride ?? {
+    'JSHandle': 100000,
+    'ElementHandle': 100000,
+  }[gcBucket] ?? 10000;
 }
 
 export class Dispatcher<Type extends { guid: string }, ChannelType, ParentScopeType extends DispatcherScope> extends EventEmitter implements channels.Channel {
@@ -50,10 +56,11 @@ export class Dispatcher<Type extends { guid: string }, ChannelType, ParentScopeT
 
   readonly _guid: string;
   readonly _type: string;
+  readonly _gcBucket: string;
   _object: Type;
   private _openScope = new LongStandingScope();
 
-  constructor(parent: ParentScopeType | DispatcherConnection, object: Type, type: string, initializer: channels.InitializerTraits<Type>) {
+  constructor(parent: ParentScopeType | DispatcherConnection, object: Type, type: string, initializer: channels.InitializerTraits<Type>, gcBucket?: string) {
     super();
 
     this._connection = parent instanceof DispatcherConnection ? parent : parent._connection;
@@ -63,6 +70,7 @@ export class Dispatcher<Type extends { guid: string }, ChannelType, ParentScopeT
     this._guid = guid;
     this._type = type;
     this._object = object;
+    this._gcBucket = gcBucket ?? type;
 
     (object as any)[dispatcherSymbol] = this;
 
@@ -74,7 +82,7 @@ export class Dispatcher<Type extends { guid: string }, ChannelType, ParentScopeT
 
     if (this._parent)
       this._connection.sendCreate(this._parent, type, guid, initializer, this._parent._object);
-    this._connection.maybeDisposeStaleDispatchers(type);
+    this._connection.maybeDisposeStaleDispatchers(this._gcBucket);
   }
 
   parentScope(): ParentScopeType {
@@ -133,7 +141,7 @@ export class Dispatcher<Type extends { guid: string }, ChannelType, ParentScopeT
 
     // Clean up from parent and connection.
     this._parent?._dispatchers.delete(this._guid);
-    const list = this._connection._dispatchersByType.get(this._type);
+    const list = this._connection._dispatchersByBucket.get(this._gcBucket);
     list?.delete(this._guid);
     this._connection._dispatchers.delete(this._guid);
 
@@ -178,8 +186,7 @@ export class RootDispatcher extends Dispatcher<{ guid: '' }, any, any> {
 
 export class DispatcherConnection {
   readonly _dispatchers = new Map<string, DispatcherScope>();
-  // Collect stale dispatchers by type.
-  readonly _dispatchersByType = new Map<string, Set<string>>();
+  readonly _dispatchersByBucket = new Map<string, Set<string>>();
   onmessage = (message: object) => {};
   private _waitOperations = new Map<string, CallMetadata>();
   private _isLocal: boolean;
@@ -248,26 +255,23 @@ export class DispatcherConnection {
   registerDispatcher(dispatcher: DispatcherScope) {
     assert(!this._dispatchers.has(dispatcher._guid));
     this._dispatchers.set(dispatcher._guid, dispatcher);
-    const type = dispatcher._type;
-
-    let list = this._dispatchersByType.get(type);
+    let list = this._dispatchersByBucket.get(dispatcher._gcBucket);
     if (!list) {
       list = new Set();
-      this._dispatchersByType.set(type, list);
+      this._dispatchersByBucket.set(dispatcher._gcBucket, list);
     }
     list.add(dispatcher._guid);
   }
 
-  maybeDisposeStaleDispatchers(type: string) {
-    const list = this._dispatchersByType.get(type);
-    if (list && list.size > maxDispatchers)
-      this._disposeStaleDispatchers(type, list);
-  }
-
-  private _disposeStaleDispatchers(type: string, dispatchers: Set<string>) {
-    const dispatchersArray = [...dispatchers];
-    this._dispatchersByType.set(type, new Set(dispatchersArray.slice(maxDispatchers / 10)));
-    for (let i = 0; i < maxDispatchers / 10; ++i) {
+  maybeDisposeStaleDispatchers(gcBucket: string) {
+    const maxDispatchers = maxDispatchersForBucket(gcBucket);
+    const list = this._dispatchersByBucket.get(gcBucket);
+    if (!list || list.size <= maxDispatchers)
+      return;
+    const dispatchersArray = [...list];
+    const disposeCount = (maxDispatchers / 10) | 0;
+    this._dispatchersByBucket.set(gcBucket, new Set(dispatchersArray.slice(disposeCount)));
+    for (let i = 0; i < disposeCount; ++i) {
       const d = this._dispatchers.get(dispatchersArray[i]);
       if (!d)
         continue;
diff --git a/packages/playwright-core/src/server/dispatchers/frameDispatcher.ts b/packages/playwright-core/src/server/dispatchers/frameDispatcher.ts
index b8e69d070d..b335f27ea0 100644
--- a/packages/playwright-core/src/server/dispatchers/frameDispatcher.ts
+++ b/packages/playwright-core/src/server/dispatchers/frameDispatcher.ts
@@ -25,6 +25,7 @@ import { RequestDispatcher } from './networkDispatchers';
 import type { CallMetadata } from '../instrumentation';
 import type { BrowserContextDispatcher } from './browserContextDispatcher';
 import type { PageDispatcher } from './pageDispatcher';
+import { debugAssert } from '../../utils';
 
 export class FrameDispatcher extends Dispatcher<Frame, channels.FrameChannel, BrowserContextDispatcher | PageDispatcher> implements channels.FrameChannel {
   _type_Frame = true;
@@ -43,13 +44,18 @@ export class FrameDispatcher extends Dispatcher<Frame, channels.FrameChannel, Br
   }
 
   private constructor(scope: BrowserContextDispatcher, frame: Frame) {
+    // Main frames are gc'ed separately from any other frames, so that
+    // methods on Page that redirect to the main frame remain operational.
+    // Note: we cannot check parentFrame() here because it may be null after the frame has been detached.
+    debugAssert(frame._page.mainFrame(), 'Cannot determine whether the frame is a main frame');
+    const gcBucket = frame._page.mainFrame() === frame ? 'MainFrame' : 'Frame';
     const pageDispatcher = existingDispatcher<PageDispatcher>(frame._page);
     super(pageDispatcher || scope, frame, 'Frame', {
       url: frame.url(),
       name: frame.name(),
       parentFrame: FrameDispatcher.fromNullable(scope, frame.parentFrame()),
       loadStates: Array.from(frame._firedLifecycleEvents),
-    });
+    }, gcBucket);
     this._browserContextDispatcher = scope;
     this._frame = frame;
     this.addObjectListener(Frame.Events.AddLifecycle, lifecycleEvent => {
diff --git a/tests/stress/frames.spec.ts b/tests/stress/frames.spec.ts
index 48489e7262..a438ec689f 100644
--- a/tests/stress/frames.spec.ts
+++ b/tests/stress/frames.spec.ts
@@ -14,18 +14,22 @@
  * limitations under the License.
  */
 
-import { contextTest as test } from '../config/browserTest';
+import { contextTest as test, expect } from '../config/browserTest';
 
 test.slow();
 
 test('cycle frames', async ({ page, server }) => {
-  const kFrameCount = 1200;
+  require('../../packages/playwright-core/lib/server/dispatchers/dispatcher').setMaxDispatchersForTest(100);
+
+  const kFrameCount = 310;
 
   await page.goto(server.EMPTY_PAGE);
   let cb;
   const promise = new Promise(f => cb = f);
   let counter = 0;
-  page.on('frameattached', () => {
+  page.on('frameattached', async () => {
+    // Make sure we can access page.
+    await page.title();
     if (++counter === kFrameCount)
       cb();
   });
@@ -40,5 +44,17 @@ test('cycle frames', async ({ page, server }) => {
     }
   }, { url: server.PREFIX + '/one-style.html', count: kFrameCount }).catch(() => {});
   await promise;
-  await new Promise(f => setTimeout(f, 500));
+  await page.waitForTimeout(500);
+
+  require('../../packages/playwright-core/lib/server/dispatchers/dispatcher').setMaxDispatchersForTest(null);
+});
+
+test('cycle handles', async ({ page, server }) => {
+  await page.goto(server.EMPTY_PAGE);
+  await page.setContent(`<div><span>hi</span></div>`.repeat(2000));
+  const divs = await page.$$('div');
+  for (const div of divs) {
+    const span = await div.$('span');
+    expect(await span.textContent()).toBe('hi');
+  }
 });