feat(trace): record trace upon browser closure (microsoft#31563)

Retaining traces in the following scenarios:
- browser crash;
- manual `browser.close()`;
- implicit `browser.close()` from the `browser` fixture upon test end.

This does not affect the library, where `browser.close()` will not
retain the trace and will close the browser as fast as possible.

References microsoft#31541, microsoft#31535, microsoft#31537.

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

@@ -50,6 +50,7 @@ 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));
   }
 
@@ -138,10 +139,14 @@ 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))
@@ -150,6 +155,13 @@ 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 ? { apiName: undefined, csi: undefined, callCookie: undefined, frames: [], stepId: undefined } : apiZone;
+                const { apiName, frames, csi, callCookie, stepId } = apiZone.reported || this._type === 'JsonPipe' ? { 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,6 +15,7 @@
  */
 
 import type { StackFrame } from '@protocol/channels';
+import type { Browser } from './browser';
 import type { BrowserContext } from './browserContext';
 import type { APIRequestContext } from './fetch';
 
@@ -30,6 +31,7 @@ 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 {
@@ -41,6 +43,7 @@ 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

@@ -589,7 +589,10 @@ 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,6 +55,7 @@ export type BrowserOptions = {
 export abstract class Browser extends SdkObject {
 
   static Events = {
+    BeforeClose: 'beforeClose',
     Disconnected: 'disconnected',
   };
 
@@ -66,6 +67,7 @@ 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');
@@ -142,7 +144,18 @@ 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,9 +34,15 @@ 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();
@@ -55,6 +61,10 @@ 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);
@@ -99,6 +109,7 @@ 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();
@@ -122,6 +133,10 @@ 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

@@ -511,6 +511,10 @@ 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)
@@ -733,6 +737,10 @@ 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

@@ -1084,10 +1084,12 @@ 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>;
@@ -1098,7 +1100,11 @@ 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,
 };
@@ -1386,6 +1392,7 @@ export type BrowserStopTracingResult = {
 };
 
 export interface BrowserEvents {
+  'beforeClose': BrowserBeforeCloseEvent;
   'close': BrowserCloseEvent;
 }
 

packages/protocol/src/protocol.yml

@@ -904,6 +904,8 @@ Browser:
 
   commands:
 
+    beforeCloseFinished:
+
     close:
       parameters:
         reason: string?
@@ -981,6 +983,8 @@ Browser:
 
   events:
 
+    beforeClose:
+
     close:
 
 ConsoleMessage:

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

@@ -15,6 +15,7 @@
  */
 
 import { test, expect } from './playwright-test-fixtures';
+import { parseTrace } from '../config/utils';
 
 test('should work with connectOptions', async ({ runInlineTest }) => {
   const result = await runInlineTest({
@@ -167,3 +168,48 @@ 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

@@ -1235,3 +1235,71 @@ 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' }));
+});