feat: mark PlaywrightClient's connection as remote (microsoft#9477)

This makes artifacts work as expected for all remote scenarios.

Author: dgozman

Diff for: packages/playwright-core/src/client/artifact.ts

@@ -22,23 +22,21 @@ import { ChannelOwner } from './channelOwner';
 import { Readable } from 'stream';
 
 export class Artifact extends ChannelOwner<channels.ArtifactChannel, channels.ArtifactInitializer> {
-  _isRemote = false;
-
   static from(channel: channels.ArtifactChannel): Artifact {
     return (channel as any)._object;
   }
 
   async pathAfterFinished(): Promise<string | null> {
-    if (this._isRemote)
-      throw new Error(`Path is not available when using browserType.connect(). Use saveAs() to save a local copy.`);
+    if (this._connection.isRemote())
+      throw new Error(`Path is not available when connecting remotely. Use saveAs() to save a local copy.`);
     return this._wrapApiCall(async (channel: channels.ArtifactChannel) => {
       return (await channel.pathAfterFinished()).value || null;
     });
   }
 
   async saveAs(path: string): Promise<void> {
     return this._wrapApiCall(async (channel: channels.ArtifactChannel) => {
-      if (!this._isRemote) {
+      if (!this._connection.isRemote()) {
         await channel.saveAs({ path });
         return;
       }

Diff for: packages/playwright-core/src/client/browser.ts

@@ -29,7 +29,7 @@ export class Browser extends ChannelOwner<channels.BrowserChannel, channels.Brow
   readonly _contexts = new Set<BrowserContext>();
   private _isConnected = true;
   private _closedPromise: Promise<void>;
-  _remoteType: 'owns-connection' | 'uses-connection' | null = null;
+  _shouldCloseConnectionOnClose = false;
   private _browserType!: BrowserType;
   readonly _name: string;
 
@@ -109,7 +109,7 @@ export class Browser extends ChannelOwner<channels.BrowserChannel, channels.Brow
   async close(): Promise<void> {
     try {
       await this._wrapApiCall(async (channel: channels.BrowserChannel) => {
-        if (this._remoteType === 'owns-connection')
+        if (this._shouldCloseConnectionOnClose)
           this._connection.close();
         else
           await channel.close();

Diff for: packages/playwright-core/src/client/browserContext.ts

@@ -346,8 +346,6 @@ export class BrowserContext extends ChannelOwner<channels.BrowserContextChannel,
         if (this._options.recordHar)  {
           const har = await this._channel.harExport();
           const artifact = Artifact.from(har.artifact);
-          if (this.browser()?._remoteType)
-            artifact._isRemote = true;
           await artifact.saveAs(this._options.recordHar.path);
           await artifact.delete();
         }

Diff for: packages/playwright-core/src/client/browserType.ts

@@ -134,6 +134,7 @@ export class BrowserType extends ChannelOwner<channels.BrowserTypeChannel, chann
       const { pipe } = await channel.connect({ wsEndpoint, headers: params.headers, slowMo: params.slowMo, timeout: params.timeout });
       const closePipe = () => pipe.close().catch(() => {});
       const connection = new Connection(closePipe);
+      connection.markAsRemote();
 
       const onPipeClosed = () => {
         // Emulate all pages, contexts and the browser closing upon disconnect.
@@ -173,7 +174,7 @@ export class BrowserType extends ChannelOwner<channels.BrowserTypeChannel, chann
           }
           browser = Browser.from(playwright._initializer.preLaunchedBrowser!);
           browser._logger = logger;
-          browser._remoteType = 'owns-connection';
+          browser._shouldCloseConnectionOnClose = true;
           browser._setBrowserType((playwright as any)[browser._name]);
           browser.on(Events.Browser.Disconnected, () => {
             playwright._cleanup();
@@ -221,7 +222,6 @@ export class BrowserType extends ChannelOwner<channels.BrowserTypeChannel, chann
       const browser = Browser.from(result.browser);
       if (result.defaultContext)
         browser._contexts.add(BrowserContext.from(result.defaultContext));
-      browser._remoteType = 'uses-connection';
       browser._logger = logger;
       browser._setBrowserType(this);
       return browser;

Diff for: packages/playwright-core/src/client/connection.ts

@@ -62,13 +62,22 @@ export class Connection extends EventEmitter {
   private _rootObject: Root;
   private _disconnectedErrorMessage: string | undefined;
   private _onClose?: () => void;
+  private _isRemote = false;
 
   constructor(onClose?: () => void) {
     super();
     this._rootObject = new Root(this);
     this._onClose = onClose;
   }
 
+  markAsRemote() {
+    this._isRemote = true;
+  }
+
+  isRemote() {
+    return this._isRemote;
+  }
+
   async initializePlaywright(): Promise<Playwright> {
     return await this._rootObject.initialize();
   }

Diff for: packages/playwright-core/src/client/page.ts

@@ -128,7 +128,6 @@ export class Page extends ChannelOwner<channels.PageChannel, channels.PageInitia
     this._channel.on('domcontentloaded', () => this.emit(Events.Page.DOMContentLoaded, this));
     this._channel.on('download', ({ url, suggestedFilename, artifact }) => {
       const artifactObject = Artifact.from(artifact);
-      artifactObject._isRemote = !!this._browserContext._browser && !!this._browserContext._browser._remoteType;
       this.emit(Events.Page.Download, new Download(this, url, suggestedFilename, artifactObject));
     });
     this._channel.on('fileChooser', ({ element, isMultiple }) => this.emit(Events.Page.FileChooser, new FileChooser(this, ElementHandle.from(element), isMultiple)));
@@ -250,7 +249,7 @@ export class Page extends ChannelOwner<channels.PageChannel, channels.PageInitia
 
   private _forceVideo(): Video {
     if (!this._video)
-      this._video = new Video(this);
+      this._video = new Video(this, this._connection);
     return this._video;
   }
 

Diff for: packages/playwright-core/src/client/tracing.ts

@@ -57,8 +57,6 @@ export class Tracing implements api.Tracing {
     if (!result.artifact)
       return;
     const artifact = Artifact.from(result.artifact);
-    if (this._context._browser?._remoteType)
-      artifact._isRemote = true;
     await artifact.saveAs(path!);
     await artifact.delete();
   }

Diff for: packages/playwright-core/src/client/video.ts

@@ -17,29 +17,28 @@
 import { Page } from './page';
 import * as api from '../../types/types';
 import { Artifact } from './artifact';
+import { Connection } from './connection';
 
 export class Video implements api.Video {
   private _artifact: Promise<Artifact | null> | null = null;
   private _artifactCallback = (artifact: Artifact) => {};
   private _isRemote = false;
 
-  constructor(page: Page) {
-    const browser = page.context()._browser;
-    this._isRemote = !!browser && !!browser._remoteType;
+  constructor(page: Page, connection: Connection) {
+    this._isRemote = connection.isRemote();
     this._artifact = Promise.race([
       new Promise<Artifact>(f => this._artifactCallback = f),
       page._closedOrCrashedPromise.then(() => null),
     ]);
   }
 
   _artifactReady(artifact: Artifact) {
-    artifact._isRemote = this._isRemote;
     this._artifactCallback(artifact);
   }
 
   async path(): Promise<string> {
     if (this._isRemote)
-      throw new Error(`Path is not available when using browserType.connect(). Use saveAs() to save a local copy.`);
+      throw new Error(`Path is not available when connecting remotely. Use saveAs() to save a local copy.`);
     const artifact = await this._artifact;
     if (!artifact)
       throw new Error('Page did not produce any video frames');

Diff for: packages/playwright-core/src/remote/playwrightClient.ts

@@ -32,6 +32,7 @@ export class PlaywrightClient {
   static async connect(options: PlaywrightClientConnectOptions): Promise<PlaywrightClient> {
     const { wsEndpoint, timeout = 30000 } = options;
     const connection = new Connection();
+    connection.markAsRemote();
     const ws = new WebSocket(wsEndpoint);
     const waitForNextTask = makeWaitForNextTask();
     connection.onmessage = message => {

Diff for: tests/browsertype-connect.spec.ts

@@ -389,7 +389,7 @@ test('should saveAs videos from remote browser', async ({ browserType, startRemo
   await page.video().saveAs(savedAsPath);
   expect(fs.existsSync(savedAsPath)).toBeTruthy();
   const error = await page.video().path().catch(e => e);
-  expect(error.message).toContain('Path is not available when using browserType.connect(). Use saveAs() to save a local copy.');
+  expect(error.message).toContain('Path is not available when connecting remotely. Use saveAs() to save a local copy.');
 });
 
 test('should be able to connect 20 times to a single server without warnings', async ({ browserType, startRemoteServer }) => {
@@ -428,7 +428,7 @@ test('should save download', async ({ server, browserType, startRemoteServer },
   expect(fs.existsSync(nestedPath)).toBeTruthy();
   expect(fs.readFileSync(nestedPath).toString()).toBe('Hello world');
   const error = await download.path().catch(e => e);
-  expect(error.message).toContain('Path is not available when using browserType.connect(). Use saveAs() to save a local copy.');
+  expect(error.message).toContain('Path is not available when connecting remotely. Use saveAs() to save a local copy.');
   await browser.close();
 });
 

Diff for: tests/defaultbrowsercontext-1.spec.ts

@@ -171,7 +171,9 @@ it('should support offline option', async ({ server, launchPersistent }) => {
   expect(error).toBeTruthy();
 });
 
-it('should support acceptDownloads option', async ({ server, launchPersistent }) => {
+it('should support acceptDownloads option', async ({ server, launchPersistent, mode }) => {
+  it.skip(mode === 'service', 'download.path() is not avaialble in remote mode');
+
   const { page } = await launchPersistent({ acceptDownloads: true });
   server.setRoute('/download', (req, res) => {
     res.setHeader('Content-Type', 'application/octet-stream');

Diff for: tests/download.spec.ts

@@ -21,6 +21,8 @@ import crypto from 'crypto';
 import type { Download } from 'playwright-core';
 
 it.describe('download event', () => {
+  it.skip(({ mode }) => mode === 'service', 'download.path() is not available in remote mode');
+
   it.beforeEach(async ({ server }) => {
     server.setRoute('/download', (req, res) => {
       res.setHeader('Content-Type', 'application/octet-stream');
@@ -156,20 +158,6 @@ it.describe('download event', () => {
     await page.close();
   });
 
-  it('should save to user-specified path', async ({ browser, server }, testInfo) => {
-    const page = await browser.newPage({ acceptDownloads: true });
-    await page.setContent(`<a href="${server.PREFIX}/download">download</a>`);
-    const [ download ] = await Promise.all([
-      page.waitForEvent('download'),
-      page.click('a')
-    ]);
-    const userPath = testInfo.outputPath('download.txt');
-    await download.saveAs(userPath);
-    expect(fs.existsSync(userPath)).toBeTruthy();
-    expect(fs.readFileSync(userPath).toString()).toBe('Hello world');
-    await page.close();
-  });
-
   it('should save to user-specified path without updating original path', async ({ browser, server }, testInfo) => {
     const page = await browser.newPage({ acceptDownloads: true });
     await page.setContent(`<a href="${server.PREFIX}/download">download</a>`);
@@ -621,6 +609,30 @@ it('should be able to download a inline PDF file', async ({ browser, server, ass
   await page.close();
 });
 
+it('should save to user-specified path', async ({ browser, server, mode }, testInfo) => {
+  server.setRoute('/download', (req, res) => {
+    res.setHeader('Content-Type', 'application/octet-stream');
+    res.setHeader('Content-Disposition', 'attachment');
+    res.end(`Hello world`);
+  });
+
+  const page = await browser.newPage({ acceptDownloads: true });
+  await page.setContent(`<a href="${server.PREFIX}/download">download</a>`);
+  const [ download ] = await Promise.all([
+    page.waitForEvent('download'),
+    page.click('a')
+  ]);
+  if (mode === 'service') {
+    const error = await download.path().catch(e => e);
+    expect(error.message).toContain('Path is not available when connecting remotely. Use saveAs() to save a local copy.');
+  }
+  const userPath = testInfo.outputPath('download.txt');
+  await download.saveAs(userPath);
+  expect(fs.existsSync(userPath)).toBeTruthy();
+  expect(fs.readFileSync(userPath).toString()).toBe('Hello world');
+  await page.close();
+});
+
 async function assertDownloadToPDF(download: Download, filePath: string) {
   expect(download.suggestedFilename()).toBe(path.basename(filePath));
   const stream = await download.createReadStream();

Diff for: tests/downloads-path.spec.ts

@@ -19,6 +19,8 @@ import fs from 'fs';
 import path from 'path';
 
 it.describe('downloads path', () => {
+  it.skip(({ mode }) => mode === 'service', 'download.path() is not available in remote mode');
+
   it.beforeEach(async ({ server }) => {
     server.setRoute('/download', (req, res) => {
       res.setHeader('Content-Type', 'application/octet-stream');

Diff for: tests/video.spec.ts

@@ -152,6 +152,7 @@ function expectRedFrames(videoFile: string, size: { width: number, height: numbe
 
 it.describe('screencast', () => {
   it.slow();
+  it.skip(({ mode }) => mode === 'service', 'video.path() is not avaialble in remote mode');
 
   it('videoSize should require videosPath', async ({ browser }) => {
     const error = await browser.newContext({ videoSize: { width: 100, height: 100 } }).catch(e => e);
@@ -218,26 +219,6 @@ it.describe('screencast', () => {
     expect(fs.existsSync(path)).toBeTruthy();
   });
 
-  it('should saveAs video', async ({ browser }, testInfo) => {
-    const videosPath = testInfo.outputPath('');
-    const size = { width: 320, height: 240 };
-    const context = await browser.newContext({
-      recordVideo: {
-        dir: videosPath,
-        size
-      },
-      viewport: size,
-    });
-    const page = await context.newPage();
-    await page.evaluate(() => document.body.style.backgroundColor = 'red');
-    await page.waitForTimeout(1000);
-    await context.close();
-
-    const saveAsPath = testInfo.outputPath('my-video.webm');
-    await page.video().saveAs(saveAsPath);
-    expect(fs.existsSync(saveAsPath)).toBeTruthy();
-  });
-
   it('saveAs should throw when no video frames', async ({ browser, browserName }, testInfo) => {
     const videosPath = testInfo.outputPath('');
     const size = { width: 320, height: 240 };
@@ -668,5 +649,26 @@ it.describe('screencast', () => {
     const files = fs.readdirSync(videoDir);
     expect(files.length).toBe(1);
   });
+});
 
+it('should saveAs video', async ({ browser }, testInfo) => {
+  it.slow();
+
+  const videosPath = testInfo.outputPath('');
+  const size = { width: 320, height: 240 };
+  const context = await browser.newContext({
+    recordVideo: {
+      dir: videosPath,
+      size
+    },
+    viewport: size,
+  });
+  const page = await context.newPage();
+  await page.evaluate(() => document.body.style.backgroundColor = 'red');
+  await page.waitForTimeout(1000);
+  await context.close();
+
+  const saveAsPath = testInfo.outputPath('my-video.webm');
+  await page.video().saveAs(saveAsPath);
+  expect(fs.existsSync(saveAsPath)).toBeTruthy();
 });