Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support for --remote-debugging-pipe transport #440

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

flotwig
Copy link

@flotwig flotwig commented Dec 29, 2020

Closes #381

@cyrus-and I see the conversation in #381 where you expressed that you don't believe pipe support is necessary. IMO, using --remote-debugging-pipe is simpler since you don't need to establish a TCP connection or implement network error handling, so here is a patch that enables a user to pass a process option to use stdio instead of websockets.

Passing process also sets local to true since there may not be a TCP port listening.

Thoughts?

Pre-merge checklist:

  • need to add tests - holding off to see if the approach here needs to change. I've manually tested this changeset and it works.
  • re-write StdioWrapper to not use puppeteer code?

P.S. this provides some motivation for #439 - since there is no default target when using the stdio transport

@@ -0,0 +1,87 @@
'use strict';

// Adapted from https://github.com/puppeteer/puppeteer/blob/7a2a41f2087b07e8ef1feaf3881bdcc3fd4922ca/src/PipeTransport.js
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is objectionable for some reason, this can be rewritten from scratch, it's not too complicated

// establish the WebSocket connection and start processing user commands
_connectToWebSocket() {
_createStdioWrapper() {
const stdio = new StdioWrapper(this.process.stdio[3], this.process.stdio[4]);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i also considered having the user pass the writeStream and readStream directly, but figured that there is not a use case for it

@cyrus-and
Copy link
Owner

IMO, using --remote-debugging-pipe is simpler since you don't need to establish a TCP connection or implement network error handling, so here is a patch that enables a user to pass a process option to use stdio instead of websockets.

Yeah, I still don't see why we should need this. The argument that it is simpler is a weak one IMHO since the WebSocket approach is already implemented and well tested, and on the consumer side is just regular exception handling, which BTW you can't never avoid.

What is the real advantage here? The only thing I can think of is that by using the pipe transport you can provide isolation from other users, meaning that only the user that started Chrome can access the CDP, while with the WebSocket approach anyone reaching the endpoint can do that. But this something that can be easily worked around, if really needed, with containerisation.

@flotwig
Copy link
Author

flotwig commented Jan 7, 2021

What is the real advantage here? The only thing I can think of is that by using the pipe transport you can provide isolation from other users, meaning that only the user that started Chrome can access the CDP, while with the WebSocket approach anyone reaching the endpoint can do that. But this something that can be easily worked around, if really needed, with containerisation.

Well, for one, using stdio lets you isolate CDP without containerization.

It also avoids the need for either (1) detecting the port Chrome chooses with --remote-debugging-port=0 or (2) praying that the random port you chose for --remote-debugging-port is free.

Additionally, I believe Playwright only exposes the CDP implementation in WebKit via stdio, not via port. So this PR is required to use this library with Playwright's patched binaries.

I think stdio transport merits adding, even if you don't think it's the best way to do things, because it's part of CDP as implemented in Chrome. IMO, chrome-remote-interface should support the full extent of CDP's capabilities, including stdio transport.

But if you feel differently, that chrome-remote-interface's goal is not to 100% support CDP in all forms, that's totally your choice. In which case, I'll either just use my fork or use some other library that supports stdio transport, so no worries.

@cyrus-and cyrus-and force-pushed the master branch 2 times, most recently from 62bd9f8 to 21e01d6 Compare April 13, 2023 22:09
@Kle0s
Copy link

Kle0s commented May 5, 2024

Is this PR still alive?

@flotwig
Copy link
Author

flotwig commented May 5, 2024

@Kle0s the changes in this PR work correctly if you'd like to use my fork, but my impression is that @cyrus-and doesn't wish to merge them upstream due to a perceived lack of benefit.

@Kle0s
Copy link

Kle0s commented May 5, 2024

@flotwig That's why I also raised another reason to support it - the extension installation in runtime seems to require it according to https://peter.sh/experiments/chromium-command-line-switches/#enable-unsafe-extension-debugging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--remote-debugging-pipe support
3 participants