feat: add support for using existing cdp websocket in launchServer #4344

berstend · 2020-11-04T19:43:52Z

This PR:

adds a cdpWebsocketEndpoint option to .launchServer
adds a Playwright user-agent header to websocket connections (.connect & .launchServer)
fixes [REGRESSION]: chromium.connect does not work with vanilla CDP servers anymore #4054

CDP

As a side effect of switching to the new wire protocol we lost the ability to connect to existing CDP websockets. This is an issue for services that wish to make existing browsers (not created by Playwright) available to Playwright clients.

User-Agent

Adding a user-agent makes it much easier for services that wish to expose a single websocket to both Playwright and Puppeteer clients. In combination with cdpWebsocketEndpoint Playwright clients can then be piped through a wire protocol websocket created by launchServer that in turn will communicate with an existing CDP websocket endpoint.

                   +------------+                             +---------------+
pptr client  +-->  |            |  +----------------------->  |               |
                   |  custom    |                             |  Chromium CDP |
                   |  websocket |        playwright           |  websocket    |
  pw client  +-->  |            |  +-->  wire protocol  +-->  |               |
                   +------------+        websocket            +---------------+

I tried following the available naming pattern & style guide, kindly let me know if there are more preferred options. :-)

ghost · 2020-11-04T19:44:05Z

All CLA requirements met.

berstend · 2020-11-04T19:47:47Z

Urgh, seems like optional chaining is not supported in CI (or rather node v10.15.3)

JoelEinbinder · 2020-11-04T20:02:51Z

test/browsertype-launch-server.spec.ts

@@ -17,7 +17,7 @@

 import { it, expect, describe } from './fixtures';

-describe('lauch server', (suite, { wire }) => {
+describe('launch server', (suite, { wire }) => {


Thought I fix that while I'm there 😄

JoelEinbinder · 2020-11-04T20:09:57Z

test/browsertype-launch-server.spec.ts

@@ -62,4 +62,17 @@ describe('lauch server', (suite, { wire }) => {
    expect(result['exitCode']).toBe(0);
    expect(result['signal']).toBe(null);
  });
+
+  it('should allow using an existing cdp endpoint', async ({ browserType, server}) => {


This test don't really prove that the cdp websocket works. @dgozman should we add another bot that runs all the tests via cdp?

Or we can just have a smoke test that.

launches a browser server with --remote-debugging-port

launches a second browser server that connects to the first via cdpWebSocketEndpoint

connect to that second browser server and create a page and eval 1+1=2 in there.

Yeah, doing that was my first instinct :-) I had the impression the other tests were a bit shallow so I opted to just test the basics. Happy to change this to a more full-fledged test.

Just wondering about the best way to get the webSocketDebuggerUrl from /json/version (there doesn't seem to be an http client around)

You can do await page.evaluate(() => fetch(url)) and use the browser 😄

my attempt with vanilla http 😄

@JoelEinbinder added an additional smoke test to ensure the cdp websocket works :-)

berstend · 2020-11-04T20:10:32Z

I think mapping upgrade to onRequest in the testserver has side-effects, I made a dedicated onUpgrade handler

dgozman · 2020-11-04T20:48:17Z

src/client/browserType.ts


 export interface BrowserServerLauncher {
  launchServer(options?: LaunchServerOptions): Promise<BrowserServer>;
 }

 export interface BrowserServer {
-  process(): ChildProcess;
+  process(): ChildProcess | undefined;


Please do not change this signature, it's not backwards-compatible. Instead, throw an error from process() when running in "connect over cdp" mode.

dgozman · 2020-11-04T20:51:48Z

src/browserServerImpl.ts

@@ -93,11 +96,13 @@ export class BrowserServerImpl extends EventEmitter implements BrowserServer {
  }

  async close(): Promise<void> {
-    await this._browser._options.browserProcess.close();
+    if (this._browser._options.browserProcess)


I think the right way is to connect right here, in the BrowserServerImpl, instead of going all the way to _innerLaunch and back.

Now that I think about this more: when connecting over existing cdp, all other launch parameters are actually ignored. This hints at using a different method, something like chromium.connectServer that only takes the websocket url. WDYT?

I agree, when connecting to an existing cdp websocket virtually all of the launch code isn't needed. Also the flag feels a bit shoehorned into there, so it might be cleaner to keep it separate. If promoting this feature to it's own top-level API is fine then I'm happy to work on the changes 😄

Added a dedicated API implementation for comparison

dgozman · 2020-11-04T20:53:03Z

test/browsertype-launch-server.spec.ts

@@ -62,4 +62,17 @@ describe('lauch server', (suite, { wire }) => {
    expect(result['exitCode']).toBe(0);
    expect(result['signal']).toBe(null);
  });
+
+  it('should allow using an existing cdp endpoint', async ({ browserType, server}) => {


You can do await page.evaluate(() => fetch(url)) and use the browser 😄

berstend · 2020-11-04T21:14:47Z

Thanks for all the valuable input and suggestions, will get back at it tomorrow. :-)

berstend · 2020-11-05T13:03:04Z

@dgozman I added a dedicated browserType.connectServer API as well, so we can see if we like it better:

Personally I think it's the better approach than cramping this into the existing .launchServer 😄

Note: I placed the meaty stuff in server/browserType.ts as I'd otherwise would need to re-import a bunch of things in src/browserServerImpl.ts :-)

dgozman

I definitely like the separate method more! Please give us some time to discuss the shape of this new API, and I'll get back to you with detailed PR review 😄

mxschmitt · 2021-02-17T09:20:33Z

@berstend seems like it was implemented in #5207

pavelfeldman · 2021-03-30T03:10:39Z

This is now obsolete and we have connectOverCDP, so closing!

berstend added 2 commits November 4, 2020 20:21

feat: add support for using existing cdp websocket in launchServer

0122986

Merge branch 'master' into add-cdp-endpoint-support

1b67b5c

fix: remove optional chaining

2e8f650

JoelEinbinder reviewed Nov 4, 2020

View reviewed changes

fix: switch to dedicated onUpgrade handler

fe4c9d3

JoelEinbinder reviewed Nov 4, 2020

View reviewed changes

dgozman reviewed Nov 4, 2020

View reviewed changes

berstend added 2 commits November 5, 2020 12:27

test: add smoke test for cdp websocket connection

e4f57a3

feat: add dedicated browserType.connectServer api

1f2e6cc

dgozman reviewed Nov 5, 2020

View reviewed changes

rinogo mentioned this pull request Nov 18, 2020

[BUG] Can't connect to running browserServer from another process #4467

Closed

Niek mentioned this pull request Dec 27, 2020

[REGRESSION]: chromium.connect does not work with vanilla CDP servers anymore #4054

Closed

pavelfeldman closed this Mar 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for using existing cdp websocket in launchServer #4344

feat: add support for using existing cdp websocket in launchServer #4344

berstend commented Nov 4, 2020 •

edited

Loading

ghost commented Nov 4, 2020 •

edited by ghost

Loading

berstend commented Nov 4, 2020 •

edited

Loading

JoelEinbinder Nov 4, 2020

berstend Nov 4, 2020

JoelEinbinder Nov 4, 2020

berstend Nov 4, 2020 •

edited

Loading

berstend Nov 4, 2020

dgozman Nov 4, 2020

berstend Nov 4, 2020 •

edited

Loading

berstend Nov 5, 2020

berstend commented Nov 4, 2020

dgozman Nov 4, 2020

dgozman Nov 4, 2020

berstend Nov 4, 2020

berstend Nov 5, 2020

dgozman Nov 4, 2020

berstend commented Nov 4, 2020

berstend commented Nov 5, 2020

dgozman left a comment

mxschmitt commented Feb 17, 2021

pavelfeldman commented Mar 30, 2021

feat: add support for using existing cdp websocket in launchServer #4344

feat: add support for using existing cdp websocket in launchServer #4344

Conversation

berstend commented Nov 4, 2020 • edited Loading

CDP

User-Agent

ghost commented Nov 4, 2020 • edited by ghost Loading

berstend commented Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berstend Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berstend Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berstend commented Nov 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berstend commented Nov 4, 2020

berstend commented Nov 5, 2020

dgozman left a comment

Choose a reason for hiding this comment

mxschmitt commented Feb 17, 2021

pavelfeldman commented Mar 30, 2021

berstend commented Nov 4, 2020 •

edited

Loading

ghost commented Nov 4, 2020 •

edited by ghost

Loading

berstend commented Nov 4, 2020 •

edited

Loading

berstend Nov 4, 2020 •

edited

Loading

berstend Nov 4, 2020 •

edited

Loading