From 817a130cdc71140acfafb3aafbab7f3e267a9f08 Mon Sep 17 00:00:00 2001
From: Pavel Feldman <pavel.feldman@gmail.com>
Date: Wed, 1 Nov 2023 16:36:39 -0700
Subject: [PATCH] chore: dispose-based callback termination (#27911)

---
 docs/src/api/class-apirequestcontext.md       |  7 +-----
 .../server/dispatchers/artifactDispatcher.ts  |  4 +++-
 .../dispatchers/browserContextDispatcher.ts   |  1 +
 .../server/dispatchers/browserDispatcher.ts   |  6 +++--
 .../dispatchers/cdpSessionDispatcher.ts       |  6 +++--
 .../src/server/dispatchers/dispatcher.ts      | 23 +++++++++++++++----
 .../src/server/dispatchers/frameDispatcher.ts |  1 +
 .../server/dispatchers/jsHandleDispatcher.ts  |  7 ++++--
 .../server/dispatchers/networkDispatchers.ts  | 10 ++++----
 .../src/server/dispatchers/pageDispatcher.ts  |  2 ++
 .../playwright-core/src/server/network.ts     | 10 +++-----
 packages/playwright-core/types/types.d.ts     | 16 ++-----------
 packages/protocol/src/callMetadata.ts         |  1 +
 tests/library/browsercontext-fetch.spec.ts    |  7 +++---
 tests/library/global-fetch.spec.ts            |  5 ++--
 tests/page/page-close.spec.ts                 |  2 +-
 tests/page/page-evaluate.spec.ts              |  2 +-
 17 files changed, 59 insertions(+), 51 deletions(-)

diff --git a/docs/src/api/class-apirequestcontext.md b/docs/src/api/class-apirequestcontext.md
index d1e6b5bcd6..dacd5dd343 100644
--- a/docs/src/api/class-apirequestcontext.md
+++ b/docs/src/api/class-apirequestcontext.md
@@ -180,12 +180,7 @@ context cookies from the response. The method will automatically follow redirect
 ## async method: APIRequestContext.dispose
 * since: v1.16
 
-All responses returned by [`method: APIRequestContext.get`] and similar methods are stored in the memory, so that you can later call [`method: APIResponse.body`]. This method
-discards all stored responses, and makes [`method: APIResponse.body`] throw "Response disposed" error.
-
-If this [APIRequestContext] is obtained via [`property: BrowserContext.request`] or [`property: Page.request`], it will keep working until its owning [BrowserContext] closes.
-
-If this [APIRequestContext] was created by [`method: APIRequest.newContext`], this method discards all its resources, calling any method on disposed [APIRequestContext] will throw an exception.
+All responses returned by [`method: APIRequestContext.get`] and similar methods are stored in the memory, so that you can later call [`method: APIResponse.body`].This method discards all its resources, calling any method on disposed [APIRequestContext] will throw an exception.
 
 ## async method: APIRequestContext.fetch
 * since: v1.16
diff --git a/packages/playwright-core/src/server/dispatchers/artifactDispatcher.ts b/packages/playwright-core/src/server/dispatchers/artifactDispatcher.ts
index 11e161fe49..ee549222f1 100644
--- a/packages/playwright-core/src/server/dispatchers/artifactDispatcher.ts
+++ b/packages/playwright-core/src/server/dispatchers/artifactDispatcher.ts
@@ -21,6 +21,7 @@ import { StreamDispatcher } from './streamDispatcher';
 import fs from 'fs';
 import { mkdirIfNeeded } from '../../utils/fileUtils';
 import type { Artifact } from '../artifact';
+import type { CallMetadata } from '../instrumentation';
 
 export class ArtifactDispatcher extends Dispatcher<Artifact, channels.ArtifactChannel, DispatcherScope> implements channels.ArtifactChannel {
   _type_Artifact = true;
@@ -105,7 +106,8 @@ export class ArtifactDispatcher extends Dispatcher<Artifact, channels.ArtifactCh
     await this._object.cancel();
   }
 
-  async delete(): Promise<void> {
+  async delete(_: any, metadata: CallMetadata): Promise<void> {
+    metadata.closesScope = true;
     await this._object.delete();
     this._dispose();
   }
diff --git a/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts b/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts
index b64384b029..8bfc2ade0b 100644
--- a/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts
+++ b/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts
@@ -273,6 +273,7 @@ export class BrowserContextDispatcher extends Dispatcher<BrowserContext, channel
   }
 
   async close(params: channels.BrowserContextCloseParams, metadata: CallMetadata): Promise<void> {
+    metadata.closesScope = true;
     await this._context.close(params);
   }
 
diff --git a/packages/playwright-core/src/server/dispatchers/browserDispatcher.ts b/packages/playwright-core/src/server/dispatchers/browserDispatcher.ts
index e41c29a97c..7a24f55ab8 100644
--- a/packages/playwright-core/src/server/dispatchers/browserDispatcher.ts
+++ b/packages/playwright-core/src/server/dispatchers/browserDispatcher.ts
@@ -55,11 +55,13 @@ export class BrowserDispatcher extends Dispatcher<Browser, channels.BrowserChann
     await this._object.stopPendingOperations(params.reason);
   }
 
-  async close(params: channels.BrowserCloseParams): Promise<void> {
+  async close(params: channels.BrowserCloseParams, metadata: CallMetadata): Promise<void> {
+    metadata.closesScope = true;
     await this._object.close(params);
   }
 
-  async killForTests(): Promise<void> {
+  async killForTests(_: any, metadata: CallMetadata): Promise<void> {
+    metadata.closesScope = true;
     await this._object.killForTests();
   }
 
diff --git a/packages/playwright-core/src/server/dispatchers/cdpSessionDispatcher.ts b/packages/playwright-core/src/server/dispatchers/cdpSessionDispatcher.ts
index 4450e48d14..bf9dc61d76 100644
--- a/packages/playwright-core/src/server/dispatchers/cdpSessionDispatcher.ts
+++ b/packages/playwright-core/src/server/dispatchers/cdpSessionDispatcher.ts
@@ -19,6 +19,7 @@ import type * as channels from '@protocol/channels';
 import { Dispatcher } from './dispatcher';
 import type { BrowserDispatcher } from './browserDispatcher';
 import type { BrowserContextDispatcher } from './browserContextDispatcher';
+import type { CallMetadata } from '../instrumentation';
 
 export class CDPSessionDispatcher extends Dispatcher<CDPSession, channels.CDPSessionChannel, BrowserDispatcher | BrowserContextDispatcher> implements channels.CDPSessionChannel {
   _type_CDPSession = true;
@@ -33,7 +34,8 @@ export class CDPSessionDispatcher extends Dispatcher<CDPSession, channels.CDPSes
     return { result: await this._object.send(params.method as any, params.params) };
   }
 
-  async detach(): Promise<void> {
-    return this._object.detach();
+  async detach(_: any, metadata: CallMetadata): Promise<void> {
+    metadata.closesScope = true;
+    await this._object.detach();
   }
 }
diff --git a/packages/playwright-core/src/server/dispatchers/dispatcher.ts b/packages/playwright-core/src/server/dispatchers/dispatcher.ts
index c2facfa457..c55558efd4 100644
--- a/packages/playwright-core/src/server/dispatchers/dispatcher.ts
+++ b/packages/playwright-core/src/server/dispatchers/dispatcher.ts
@@ -17,7 +17,7 @@
 import { EventEmitter } from 'events';
 import type * as channels from '@protocol/channels';
 import { findValidator, ValidationError, createMetadataValidator, type ValidatorContext } from '../../protocol/validator';
-import { assert, isUnderTest, monotonicTime, rewriteErrorMessage } from '../../utils';
+import { LongStandingScope, assert, isUnderTest, monotonicTime, rewriteErrorMessage } from '../../utils';
 import { TargetClosedError, isTargetClosedError, serializeError } from '../errors';
 import type { CallMetadata } from '../instrumentation';
 import { SdkObject } from '../instrumentation';
@@ -51,6 +51,7 @@ export class Dispatcher<Type extends { guid: string }, ChannelType, ParentScopeT
   readonly _guid: string;
   readonly _type: string;
   _object: Type;
+  private _openScope = new LongStandingScope();
 
   constructor(parent: ParentScopeType | DispatcherConnection, object: Type, type: string, initializer: channels.InitializerTraits<Type>) {
     super();
@@ -93,6 +94,17 @@ export class Dispatcher<Type extends { guid: string }, ChannelType, ParentScopeT
     this._connection.sendAdopt(this, child);
   }
 
+  async _handleCommand(callMetadata: CallMetadata, method: string, validParams: any) {
+    const commandPromise = (this as any)[method](validParams, callMetadata);
+    try {
+      return await this._openScope.race(commandPromise);
+    } catch (e) {
+      if (callMetadata.closesScope && isTargetClosedError(e))
+        return await commandPromise;
+      throw e;
+    }
+  }
+
   _dispatchEvent<T extends keyof channels.EventsTraits<ChannelType>>(method: T, params?: channels.EventsTraits<ChannelType>[T]) {
     if (this._disposed) {
       if (isUnderTest())
@@ -105,14 +117,14 @@ export class Dispatcher<Type extends { guid: string }, ChannelType, ParentScopeT
   }
 
   _dispose(reason?: 'gc') {
-    this._disposeRecursively();
+    this._disposeRecursively(new TargetClosedError());
     this._connection.sendDispose(this, reason);
   }
 
   protected _onDispose() {
   }
 
-  private _disposeRecursively() {
+  private _disposeRecursively(error: Error) {
     assert(!this._disposed, `${this._guid} is disposed more than once`);
     this._onDispose();
     this._disposed = true;
@@ -126,9 +138,10 @@ export class Dispatcher<Type extends { guid: string }, ChannelType, ParentScopeT
 
     // Dispose all children.
     for (const dispatcher of [...this._dispatchers.values()])
-      dispatcher._disposeRecursively();
+      dispatcher._disposeRecursively(error);
     this._dispatchers.clear();
     delete (this._object as any)[dispatcherSymbol];
+    this._openScope.close(error);
   }
 
   _debugScopeState(): any {
@@ -325,7 +338,7 @@ export class DispatcherConnection {
 
     await sdkObject?.instrumentation.onBeforeCall(sdkObject, callMetadata);
     try {
-      const result = await (dispatcher as any)[method](validParams, callMetadata);
+      const result = await dispatcher._handleCommand(callMetadata, method, validParams);
       const validator = findValidator(dispatcher._type, method, 'Result');
       callMetadata.result = validator(result, '', { tChannelImpl: this._tChannelImplToWire.bind(this), binary: this._isLocal ? 'buffer' : 'toBase64' });
     } catch (e) {
diff --git a/packages/playwright-core/src/server/dispatchers/frameDispatcher.ts b/packages/playwright-core/src/server/dispatchers/frameDispatcher.ts
index 3e9b4362a4..cfd03bfe16 100644
--- a/packages/playwright-core/src/server/dispatchers/frameDispatcher.ts
+++ b/packages/playwright-core/src/server/dispatchers/frameDispatcher.ts
@@ -265,6 +265,7 @@ export class FrameDispatcher extends Dispatcher<Frame, channels.FrameChannel, Br
   }
 
   async expect(params: channels.FrameExpectParams, metadata: CallMetadata): Promise<channels.FrameExpectResult> {
+    metadata.closesScope = true;
     const expectedValue = params.expectedValue ? parseArgument(params.expectedValue) : undefined;
     const result = await this._frame.expect(metadata, params.selector, { ...params, expectedValue });
     if (result.received !== undefined)
diff --git a/packages/playwright-core/src/server/dispatchers/jsHandleDispatcher.ts b/packages/playwright-core/src/server/dispatchers/jsHandleDispatcher.ts
index 1b4d7cbcfa..8fa5093560 100644
--- a/packages/playwright-core/src/server/dispatchers/jsHandleDispatcher.ts
+++ b/packages/playwright-core/src/server/dispatchers/jsHandleDispatcher.ts
@@ -22,6 +22,7 @@ import { parseSerializedValue, serializeValue } from '../../protocol/serializers
 import type { PageDispatcher, WorkerDispatcher } from './pageDispatcher';
 import type { ElectronApplicationDispatcher } from './electronDispatcher';
 import type { FrameDispatcher } from './frameDispatcher';
+import type { CallMetadata } from '../instrumentation';
 
 export type JSHandleDispatcherParentScope = PageDispatcher | FrameDispatcher | WorkerDispatcher | ElectronApplicationDispatcher;
 
@@ -66,8 +67,10 @@ export class JSHandleDispatcher extends Dispatcher<js.JSHandle, channels.JSHandl
     return { count: await this._object.objectCount() };
   }
 
-  async dispose() {
-    await this._object.dispose();
+  async dispose(_: any, metadata: CallMetadata) {
+    metadata.closesScope = true;
+    this._object.dispose();
+    this._dispose();
   }
 }
 
diff --git a/packages/playwright-core/src/server/dispatchers/networkDispatchers.ts b/packages/playwright-core/src/server/dispatchers/networkDispatchers.ts
index 325020f802..f2c92c9836 100644
--- a/packages/playwright-core/src/server/dispatchers/networkDispatchers.ts
+++ b/packages/playwright-core/src/server/dispatchers/networkDispatchers.ts
@@ -15,7 +15,7 @@
  */
 
 import type * as channels from '@protocol/channels';
-import { APIRequestContext } from '../fetch';
+import type { APIRequestContext } from '../fetch';
 import type { CallMetadata } from '../instrumentation';
 import type { Request, Response, Route } from '../network';
 import { WebSocket } from '../network';
@@ -191,17 +191,17 @@ export class APIRequestContextDispatcher extends Dispatcher<APIRequestContext, c
       tracing,
     });
 
-    this.addObjectListener(APIRequestContext.Events.Dispose, () => this._dispose());
-
     this.adopt(tracing);
   }
 
-  async storageState(params?: channels.APIRequestContextStorageStateParams): Promise<channels.APIRequestContextStorageStateResult> {
+  async storageState(): Promise<channels.APIRequestContextStorageStateResult> {
     return this._object.storageState();
   }
 
-  async dispose(params?: channels.APIRequestContextDisposeParams): Promise<void> {
+  async dispose(_: channels.APIRequestContextDisposeParams, metadata: CallMetadata): Promise<void> {
+    metadata.closesScope = true;
     await this._object.dispose();
+    this._dispose();
   }
 
   async fetch(params: channels.APIRequestContextFetchParams, metadata: CallMetadata): Promise<channels.APIRequestContextFetchResult> {
diff --git a/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts b/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts
index 6689ff218c..814538dbee 100644
--- a/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts
+++ b/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts
@@ -200,6 +200,8 @@ export class PageDispatcher extends Dispatcher<Page, channels.PageChannel, Brows
   }
 
   async close(params: channels.PageCloseParams, metadata: CallMetadata): Promise<void> {
+    if (!params.runBeforeUnload)
+      metadata.closesScope = true;
     await this._page.close(metadata, params);
   }
 
diff --git a/packages/playwright-core/src/server/network.ts b/packages/playwright-core/src/server/network.ts
index 6f4d09d76c..fdf0573ac3 100644
--- a/packages/playwright-core/src/server/network.ts
+++ b/packages/playwright-core/src/server/network.ts
@@ -20,7 +20,7 @@ import type * as frames from './frames';
 import type * as types from './types';
 import type * as channels from '@protocol/channels';
 import { assert } from '../utils';
-import { LongStandingScope, ManualPromise } from '../utils/manualPromise';
+import { ManualPromise } from '../utils/manualPromise';
 import { SdkObject } from './instrumentation';
 import type { HeadersArray, NameValue } from '../common/types';
 import { APIRequestContext } from './fetch';
@@ -129,10 +129,6 @@ export class Request extends SdkObject {
     this._isFavicon = url.endsWith('/favicon.ico') || !!redirectedFrom?._isFavicon;
   }
 
-  private _targetClosedScope(): LongStandingScope {
-    return this._serviceWorker?.openScope || this._frame?._page.openScope || new LongStandingScope();
-  }
-
   _setFailureText(failureText: string) {
     this._failureText = failureText;
     this._waitForResponsePromise.resolve(null);
@@ -183,11 +179,11 @@ export class Request extends SdkObject {
   }
 
   async rawRequestHeaders(): Promise<HeadersArray> {
-    return this._overrides?.headers || this._targetClosedScope().race(this._rawRequestHeadersPromise);
+    return this._overrides?.headers || this._rawRequestHeadersPromise;
   }
 
   response(): PromiseLike<Response | null> {
-    return this._targetClosedScope().race(this._waitForResponsePromise);
+    return this._waitForResponsePromise;
   }
 
   _existingResponse(): Response | null {
diff --git a/packages/playwright-core/types/types.d.ts b/packages/playwright-core/types/types.d.ts
index 9f4ab6438b..8483e5af54 100644
--- a/packages/playwright-core/types/types.d.ts
+++ b/packages/playwright-core/types/types.d.ts
@@ -15258,20 +15258,8 @@ export interface APIRequestContext {
    * All responses returned by
    * [apiRequestContext.get(url[, options])](https://playwright.dev/docs/api/class-apirequestcontext#api-request-context-get)
    * and similar methods are stored in the memory, so that you can later call
-   * [apiResponse.body()](https://playwright.dev/docs/api/class-apiresponse#api-response-body). This method discards all
-   * stored responses, and makes
-   * [apiResponse.body()](https://playwright.dev/docs/api/class-apiresponse#api-response-body) throw "Response disposed"
-   * error.
-   *
-   * If this {@link APIRequestContext} is obtained via
-   * [browserContext.request](https://playwright.dev/docs/api/class-browsercontext#browser-context-request) or
-   * [page.request](https://playwright.dev/docs/api/class-page#page-request), it will keep working until its owning
-   * {@link BrowserContext} closes.
-   *
-   * If this {@link APIRequestContext} was created by
-   * [apiRequest.newContext([options])](https://playwright.dev/docs/api/class-apirequest#api-request-new-context), this
-   * method discards all its resources, calling any method on disposed {@link APIRequestContext} will throw an
-   * exception.
+   * [apiResponse.body()](https://playwright.dev/docs/api/class-apiresponse#api-response-body).This method discards all
+   * its resources, calling any method on disposed {@link APIRequestContext} will throw an exception.
    */
   dispose(): Promise<void>;
 
diff --git a/packages/protocol/src/callMetadata.ts b/packages/protocol/src/callMetadata.ts
index 7681c5f5f9..3f5fc78764 100644
--- a/packages/protocol/src/callMetadata.ts
+++ b/packages/protocol/src/callMetadata.ts
@@ -42,4 +42,5 @@ export type CallMetadata = {
   objectId?: string;
   pageId?: string;
   frameId?: string;
+  closesScope?: boolean;
 };
diff --git a/tests/library/browsercontext-fetch.spec.ts b/tests/library/browsercontext-fetch.spec.ts
index f9c83b9e72..bb0bb15362 100644
--- a/tests/library/browsercontext-fetch.spec.ts
+++ b/tests/library/browsercontext-fetch.spec.ts
@@ -22,6 +22,7 @@ import { pipeline } from 'stream';
 import zlib from 'zlib';
 import { contextTest as it, expect } from '../config/browserTest';
 import { suppressCertificateWarning } from '../config/utils';
+import { kTargetClosedErrorMessage } from 'tests/config/errors';
 
 it.skip(({ mode }) => mode !== 'default');
 
@@ -1078,7 +1079,7 @@ it('should abort requests when browser context closes', async ({ contextFactory,
     server.waitForRequest('/empty.html').then(() => context.close())
   ]);
   expect(error instanceof Error).toBeTruthy();
-  expect(error.message).toContain('Request context disposed');
+  expect(error.message).toContain(kTargetClosedErrorMessage);
   await connectionClosed;
 });
 
@@ -1193,8 +1194,8 @@ it('should update host header on redirect', async ({ context, server }) => {
   expect((await reqPromise).headers.host).toBe(new URL(server.CROSS_PROCESS_PREFIX).host);
 });
 
-it('should keep working after dispose', async ({ context, server }) => {
+it('should not work after dispose', async ({ context, server }) => {
   it.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/27822' });
   await context.request.dispose();
-  await expect(await context.request.get(server.EMPTY_PAGE)).toBeOK();
+  expect(await context.request.get(server.EMPTY_PAGE).catch(e => e.message)).toContain(kTargetClosedErrorMessage);
 });
diff --git a/tests/library/global-fetch.spec.ts b/tests/library/global-fetch.spec.ts
index a16b4b6172..ce38964483 100644
--- a/tests/library/global-fetch.spec.ts
+++ b/tests/library/global-fetch.spec.ts
@@ -18,6 +18,7 @@ import os from 'os';
 import * as util from 'util';
 import { getPlaywrightVersion } from '../../packages/playwright-core/lib/utils/userAgent';
 import { expect, playwrightTest as it } from '../config/browserTest';
+import { kTargetClosedErrorMessage } from 'tests/config/errors';
 
 it.skip(({ mode }) => mode !== 'default');
 
@@ -226,7 +227,7 @@ it('should abort requests when context is disposed', async ({ playwright, server
   ]);
   for (const result of results.slice(0, -1)) {
     expect(result instanceof Error).toBeTruthy();
-    expect(result.message).toContain('Request context disposed');
+    expect(result.message).toContain(kTargetClosedErrorMessage);
   }
   await connectionClosed;
 });
@@ -242,7 +243,7 @@ it('should abort redirected requests when context is disposed', async ({ playwri
     server.waitForRequest('/test').then(() => request.dispose())
   ]);
   expect(result instanceof Error).toBeTruthy();
-  expect(result.message).toContain('Request context disposed');
+  expect(result.message).toContain(kTargetClosedErrorMessage);
   await connectionClosed;
 });
 
diff --git a/tests/page/page-close.spec.ts b/tests/page/page-close.spec.ts
index cb9b028d97..5c7d24110a 100644
--- a/tests/page/page-close.spec.ts
+++ b/tests/page/page-close.spec.ts
@@ -21,7 +21,7 @@ it.skip(({ isWebView2 }) => isWebView2, 'Page.close() is not supported in WebVie
 
 it('should close page with active dialog', async ({ page }) => {
   await page.setContent(`<button onclick="setTimeout(() => alert(1))">alert</button>`);
-  void page.click('button');
+  void page.click('button').catch(() => {});
   await page.waitForEvent('dialog');
   await page.close();
 });
diff --git a/tests/page/page-evaluate.spec.ts b/tests/page/page-evaluate.spec.ts
index 699e3e7302..37aa0a6c8f 100644
--- a/tests/page/page-evaluate.spec.ts
+++ b/tests/page/page-evaluate.spec.ts
@@ -452,7 +452,7 @@ it('should throw if underlying element was disposed', async ({ page }) => {
   await element.dispose();
   let error = null;
   await page.evaluate(e => e.textContent, element).catch(e => error = e);
-  expect(error.message).toContain('JSHandle is disposed');
+  expect(error.message).toContain('no object with guid');
 });
 
 it('should simulate a user gesture', async ({ page }) => {