Revert "feat(trace): record trace upon browser closure (#31563)" (#31677)

This reverts commit bc27ca225e. Considered
too risky.

packages/playwright-core/src/client/browser.ts

@@ -50,7 +50,6 @@ export class Browser extends ChannelOwner<channels.BrowserChannel> implements ap
     super(parent, type, guid, initializer);
     this._name = initializer.name;
     this._channel.on('close', () => this._didClose());
-    this._channel.on('beforeClose', () => this._beforeClose());
     this._closedPromise = new Promise(f => this.once(Events.Browser.Disconnected, f));
   }
 
@@ -139,14 +138,10 @@ export class Browser extends ChannelOwner<channels.BrowserChannel> implements ap
   async close(options: { reason?: string } = {}): Promise<void> {
     this._closeReason = options.reason;
     try {
-      if (this._shouldCloseConnectionOnClose) {
+      if (this._shouldCloseConnectionOnClose)
-        // We cannot run beforeClose when remote disconnects, because there is no physical connection anymore.
-        // However, we can run it for an explicit browser.close() call.
-        await this._instrumentation.runBeforeCloseBrowser(this);
         this._connection.close();
-      } else {
+      else
         await this._channel.close(options);
-      }
       await this._closedPromise;
     } catch (e) {
       if (isTargetClosedError(e))
@@ -155,13 +150,6 @@ export class Browser extends ChannelOwner<channels.BrowserChannel> implements ap
     }
   }
 
-  async _beforeClose() {
-    await this._wrapApiCall(async () => {
-      await this._instrumentation.runBeforeCloseBrowser(this);
-      await this._channel.beforeCloseFinished().catch(() => {});
-    }, true);
-  }
-
   _didClose() {
     this._isConnected = false;
     this.emit(Events.Browser.Disconnected, this);

packages/playwright-core/src/client/channelOwner.ts

@@ -143,7 +143,7 @@ export abstract class ChannelOwner<T extends channels.Channel = channels.Channel
           if (validator) {
             return async (params: any) => {
               return await this._wrapApiCall(async apiZone => {
-                const { apiName, frames, csi, callCookie, stepId } = apiZone.reported || this._type === 'JsonPipe' ? { apiName: undefined, csi: undefined, callCookie: undefined, frames: [], stepId: undefined } : apiZone;
+                const { apiName, frames, csi, callCookie, stepId } = apiZone.reported ? { apiName: undefined, csi: undefined, callCookie: undefined, frames: [], stepId: undefined } : apiZone;
                 apiZone.reported = true;
                 let currentStepId = stepId;
                 if (csi && apiName) {

packages/playwright-core/src/client/clientInstrumentation.ts

@@ -15,7 +15,6 @@
  */
 
 import type { StackFrame } from '@protocol/channels';
-import type { Browser } from './browser';
 import type { BrowserContext } from './browserContext';
 import type { APIRequestContext } from './fetch';
 
@@ -31,7 +30,6 @@ export interface ClientInstrumentation {
   runAfterCreateRequestContext(context: APIRequestContext): Promise<void>;
   runBeforeCloseBrowserContext(context: BrowserContext): Promise<void>;
   runBeforeCloseRequestContext(context: APIRequestContext): Promise<void>;
-  runBeforeCloseBrowser(browser: Browser): Promise<void>;
 }
 
 export interface ClientInstrumentationListener {
@@ -43,7 +41,6 @@ export interface ClientInstrumentationListener {
   runAfterCreateRequestContext?(context: APIRequestContext): Promise<void>;
   runBeforeCloseBrowserContext?(context: BrowserContext): Promise<void>;
   runBeforeCloseRequestContext?(context: APIRequestContext): Promise<void>;
-  runBeforeCloseBrowser?(browser: Browser): Promise<void>;
 }
 
 export function createInstrumentation(): ClientInstrumentation {

packages/playwright-core/src/protocol/validator.ts

@@ -607,10 +607,7 @@ scheme.BrowserInitializer = tObject({
   version: tString,
   name: tString,
 });
-scheme.BrowserBeforeCloseEvent = tOptional(tObject({}));
 scheme.BrowserCloseEvent = tOptional(tObject({}));
-scheme.BrowserBeforeCloseFinishedParams = tOptional(tObject({}));
-scheme.BrowserBeforeCloseFinishedResult = tOptional(tObject({}));
 scheme.BrowserCloseParams = tObject({
   reason: tOptional(tString),
 });

packages/playwright-core/src/server/browser.ts

@@ -55,7 +55,6 @@ export type BrowserOptions = {
 export abstract class Browser extends SdkObject {
 
   static Events = {
-    BeforeClose: 'beforeClose',
     Disconnected: 'disconnected',
   };
 
@@ -67,7 +66,6 @@ export abstract class Browser extends SdkObject {
   private _contextForReuse: { context: BrowserContext, hash: string } | undefined;
   _closeReason: string | undefined;
   _isCollocatedWithServer: boolean = true;
-  private _needsBeforeCloseEvent = false;
 
   constructor(parent: SdkObject, options: BrowserOptions) {
     super(parent, 'browser');
@@ -152,18 +150,7 @@ export abstract class Browser extends SdkObject {
     return video?.artifact;
   }
 
-  setNeedsBeforeCloseEvent(needsBeforeCloseEvent: boolean) {
-    this._needsBeforeCloseEvent = needsBeforeCloseEvent;
-  }
-
   _didClose() {
-    if (this._needsBeforeCloseEvent)
-      this.emit(Browser.Events.BeforeClose);
-    else
-      this.beforeCloseFinished();
-  }
-
-  beforeCloseFinished() {
     for (const context of this.contexts())
       context._browserClosed();
     if (this._defaultContext)

packages/playwright-core/src/server/dispatchers/browserDispatcher.ts

@@ -34,15 +34,9 @@ export class BrowserDispatcher extends Dispatcher<Browser, channels.BrowserChann
 
   constructor(scope: BrowserTypeDispatcher, browser: Browser) {
     super(scope, browser, 'Browser', { version: browser.version(), name: browser.options.name });
-    browser.setNeedsBeforeCloseEvent(true);
-    this.addObjectListener(Browser.Events.BeforeClose, () => this._dispatchEvent('beforeClose'));
     this.addObjectListener(Browser.Events.Disconnected, () => this._didClose());
   }
 
-  override _onDispose() {
-    this._object.setNeedsBeforeCloseEvent(false);
-  }
-
   _didClose() {
     this._dispatchEvent('close');
     this._dispose();
@@ -61,10 +55,6 @@ export class BrowserDispatcher extends Dispatcher<Browser, channels.BrowserChann
     await this._object.stopPendingOperations(params.reason);
   }
 
-  async beforeCloseFinished() {
-    this._object.beforeCloseFinished();
-  }
-
   async close(params: channels.BrowserCloseParams, metadata: CallMetadata): Promise<void> {
     metadata.potentiallyClosesScope = true;
     await this._object.close(params);
@@ -109,7 +99,6 @@ export class ConnectedBrowserDispatcher extends Dispatcher<Browser, channels.Bro
 
   constructor(scope: RootDispatcher, browser: Browser) {
     super(scope, browser, 'Browser', { version: browser.version(), name: browser.options.name });
-    this.addObjectListener(Browser.Events.BeforeClose, () => this.emit('beforeClose'));
     // When we have a remotely-connected browser, each client gets a fresh Selector instance,
     // so that two clients do not interfere between each other.
     this.selectors = new Selectors();
@@ -133,10 +122,6 @@ export class ConnectedBrowserDispatcher extends Dispatcher<Browser, channels.Bro
     await this._object.stopPendingOperations(params.reason);
   }
 
-  async beforeCloseFinished() {
-    this._object.beforeCloseFinished();
-  }
-
   async close(): Promise<void> {
     // Client should not send us Browser.close.
   }

packages/playwright/src/index.ts

@@ -537,10 +537,6 @@ class ArtifactsRecorder {
     await this._stopTracing(tracing);
   }
 
-  async willCloseBrowser(browser: Browser) {
-    await Promise.all(browser.contexts().map(context => this._stopTracing(context.tracing)));
-  }
-
   async didFinishTestFunction() {
     const captureScreenshots = this._screenshotMode === 'on' || (this._screenshotMode === 'only-on-failure' && this._testInfo._isFailure());
     if (captureScreenshots)
@@ -763,10 +759,6 @@ class InstrumentationConnector implements TestLifecycleInstrumentation, ClientIn
   async runBeforeCloseRequestContext(context: APIRequestContext) {
     await this._artifactsRecorder?.willCloseRequestContext(context);
   }
-
-  async runBeforeCloseBrowser(browser: Browser) {
-    await this._artifactsRecorder?.willCloseBrowser(browser);
-  }
 }
 
 const connector = new InstrumentationConnector();

packages/protocol/src/channels.ts

@@ -1120,12 +1120,10 @@ export type BrowserInitializer = {
   name: string,
 };
 export interface BrowserEventTarget {
-  on(event: 'beforeClose', callback: (params: BrowserBeforeCloseEvent) => void): this;
   on(event: 'close', callback: (params: BrowserCloseEvent) => void): this;
 }
 export interface BrowserChannel extends BrowserEventTarget, Channel {
   _type_Browser: boolean;
-  beforeCloseFinished(params?: BrowserBeforeCloseFinishedParams, metadata?: CallMetadata): Promise<BrowserBeforeCloseFinishedResult>;
   close(params: BrowserCloseParams, metadata?: CallMetadata): Promise<BrowserCloseResult>;
   killForTests(params?: BrowserKillForTestsParams, metadata?: CallMetadata): Promise<BrowserKillForTestsResult>;
   defaultUserAgentForTest(params?: BrowserDefaultUserAgentForTestParams, metadata?: CallMetadata): Promise<BrowserDefaultUserAgentForTestResult>;
@@ -1136,11 +1134,7 @@ export interface BrowserChannel extends BrowserEventTarget, Channel {
   startTracing(params: BrowserStartTracingParams, metadata?: CallMetadata): Promise<BrowserStartTracingResult>;
   stopTracing(params?: BrowserStopTracingParams, metadata?: CallMetadata): Promise<BrowserStopTracingResult>;
 }
-export type BrowserBeforeCloseEvent = {};
 export type BrowserCloseEvent = {};
-export type BrowserBeforeCloseFinishedParams = {};
-export type BrowserBeforeCloseFinishedOptions = {};
-export type BrowserBeforeCloseFinishedResult = void;
 export type BrowserCloseParams = {
   reason?: string,
 };
@@ -1464,7 +1458,6 @@ export type BrowserStopTracingResult = {
 };
 
 export interface BrowserEvents {
-  'beforeClose': BrowserBeforeCloseEvent;
   'close': BrowserCloseEvent;
 }
 

packages/protocol/src/protocol.yml

@@ -934,8 +934,6 @@ Browser:
 
   commands:
 
-    beforeCloseFinished:
-
     close:
       parameters:
         reason: string?
@@ -1013,8 +1011,6 @@ Browser:
 
   events:
 
-    beforeClose:
-
     close:
 
 ConsoleMessage:

tests/playwright-test/playwright.connect.spec.ts

@@ -15,7 +15,6 @@
  */
 
 import { test, expect } from './playwright-test-fixtures';
-import { parseTrace } from '../config/utils';
 
 test('should work with connectOptions', async ({ runInlineTest }) => {
   const result = await runInlineTest({
@@ -168,48 +167,3 @@ test('should print debug log when failed to connect', async ({ runInlineTest })
   expect(result.output).toContain('b-debug-log-string');
   expect(result.results[0].attachments).toEqual([]);
 });
-
-test('should save trace when remote browser is closed', async ({ runInlineTest }) => {
-  const result = await runInlineTest({
-    'playwright.config.js': `
-      module.exports = {
-        globalSetup: './global-setup',
-        use: {
-          trace: 'on',
-          connectOptions: { wsEndpoint: process.env.CONNECT_WS_ENDPOINT },
-        },
-      };
-    `,
-    'global-setup.ts': `
-      import { chromium } from '@playwright/test';
-      module.exports = async () => {
-        const server = await chromium.launchServer();
-        process.env.CONNECT_WS_ENDPOINT = server.wsEndpoint();
-        return () => server.close();
-      };
-    `,
-    'a.test.ts': `
-      import { test, expect } from '@playwright/test';
-      test('pass', async ({ browser }) => {
-        const page = await browser.newPage();
-        await page.setContent('<script>console.log("from the page")</script>');
-        await browser.close();
-      });
-    `,
-  });
-  expect(result.exitCode).toBe(0);
-  expect(result.passed).toBe(1);
-
-  const tracePath = test.info().outputPath('test-results', 'a-pass', 'trace.zip');
-  const trace = await parseTrace(tracePath);
-  expect(trace.actionTree).toEqual([
-    'Before Hooks',
-    '  fixture: browser',
-    '    browserType.connect',
-    'browser.newPage',
-    'page.setContent',
-    'After Hooks',
-  ]);
-  // Check console events to make sure that library trace is recorded.
-  expect(trace.events).toContainEqual(expect.objectContaining({ type: 'console', text: 'from the page' }));
-});

tests/playwright-test/playwright.trace.spec.ts

@@ -1268,71 +1268,3 @@ test('should take a screenshot-on-failure in workerStorageState', async ({ runIn
   expect(result.failed).toBe(1);
   expect(fs.existsSync(test.info().outputPath('test-results', 'a-fail', 'test-failed-1.png'))).toBeTruthy();
 });
-
-test('should record trace upon implicit browser.close in a failed test', async ({ runInlineTest }) => {
-  test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/31541' });
-
-  const result = await runInlineTest({
-    'a.spec.ts': `
-      import { test, expect } from '@playwright/test';
-      test('fail', async ({ browser }) => {
-        const page = await browser.newPage();
-        await page.setContent('<script>console.log("from the page");</script>');
-        expect(1).toBe(2);
-      });
-    `,
-  }, { trace: 'on' });
-  expect(result.exitCode).toBe(1);
-  expect(result.failed).toBe(1);
-
-  const tracePath = test.info().outputPath('test-results', 'a-fail', 'trace.zip');
-  const trace = await parseTrace(tracePath);
-  expect(trace.actionTree).toEqual([
-    'Before Hooks',
-    '  fixture: browser',
-    '    browserType.launch',
-    'browser.newPage',
-    'page.setContent',
-    'expect.toBe',
-    'After Hooks',
-    'Worker Cleanup',
-    '  fixture: browser',
-  ]);
-  // Check console events to make sure that library trace is recorded.
-  expect(trace.events).toContainEqual(expect.objectContaining({ type: 'console', text: 'from the page' }));
-});
-
-test('should record trace upon browser crash', async ({ runInlineTest }) => {
-  test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/31537' });
-
-  const result = await runInlineTest({
-    'a.spec.ts': `
-      import { test, expect } from '@playwright/test';
-      test('fail', async ({ browser }) => {
-        const page = await browser.newPage();
-        await page.setContent('<script>console.log("from the page");</script>');
-        await (browser as any)._channel.killForTests();
-        await page.goto('data:text/html,<div>will not load</div>');
-      });
-    `,
-  }, { trace: 'on' });
-  expect(result.exitCode).toBe(1);
-  expect(result.failed).toBe(1);
-
-  const tracePath = test.info().outputPath('test-results', 'a-fail', 'trace.zip');
-  const trace = await parseTrace(tracePath);
-  expect(trace.actionTree).toEqual([
-    'Before Hooks',
-    '  fixture: browser',
-    '    browserType.launch',
-    'browser.newPage',
-    'page.setContent',
-    'proxy.killForTests',
-    'page.goto',
-    'After Hooks',
-    'Worker Cleanup',
-    '  fixture: browser',
-  ]);
-  // Check console events to make sure that library trace is recorded.
-  expect(trace.events).toContainEqual(expect.objectContaining({ type: 'console', text: 'from the page' }));
-});