Over to you in case you have patches available.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/93a0240a75dafc41691457d253e090ac4d66506e

commit 93a0240a75dafc41691457d253e090ac4d66506e
Author: altimin <altimin@chromium.org>
Date: Wed Aug 03 16:26:43 2016

[headless]

Public API changes:

- [incompatible] Removed default browser context from HeadlessBrowser and HeadlessBrowser::CreateWebContents methods. Now BrowserContext should be created explicitly.
- [incompatible] ProtocolHandlers cannot be set in HeadlessBrowser::Options due to being move-only and not reusable across different BrowserContexts.
- Added more configurable options to HeadlessBrowserContext.
- Now user data directory can be set.
- HeadlessBrowser::Shutdown now closes all WebContents.
- All WebContents associated with a HeadlessBrowserContext are now deleted when user deletes a BrowserContext.

Internal library changes:
- Added HeadlessBrowserContextOptions class, allowing for more customization for BrowserContext and for fallback to default browser-wide settings from HeadlessBrowser::Options.
- Removed SetOptionsForTesting. Now BrowserContext should be created for this purposes.

Misc:
- Made linter happy.

BUG= 546953 , 624837 

Review-Url: https://codereview.chromium.org/2181413002
Cr-Commit-Position: refs/heads/master@{#409527}

[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/BUILD.gn
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/app/headless_shell.cc
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/app/headless_shell_switches.cc
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/app/headless_shell_switches.h
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/browser/headless_browser_context_impl.cc
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/browser/headless_browser_context_impl.h
[add] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/browser/headless_browser_context_options.cc
[add] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/browser/headless_browser_context_options.h
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/browser/headless_browser_impl.cc
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/browser/headless_browser_impl.h
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/browser/headless_browser_main_parts.cc
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/browser/headless_browser_main_parts.h
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/browser/headless_content_browser_client.cc
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/browser/headless_devtools.cc
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/browser/headless_devtools.h
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/browser/headless_devtools_manager_delegate.cc
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/browser/headless_devtools_manager_delegate.h
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/browser/headless_url_request_context_getter.cc
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/browser/headless_url_request_context_getter.h
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/browser/headless_web_contents_impl.cc
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/browser/headless_web_contents_impl.h
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/embedder_mojo_browsertest.cc
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/headless_browser_browsertest.cc
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/headless_browser_context_browsertest.cc
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/headless_devtools_client_browsertest.cc
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/lib/headless_web_contents_browsertest.cc
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/public/headless_browser.cc
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/public/headless_browser.h
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/public/headless_browser_context.h
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/public/headless_web_contents.h
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/test/headless_browser_test.cc
[modify] https://crrev.com/93a0240a75dafc41691457d253e090ac4d66506e/headless/test/headless_browser_test.h

In case it's useful, the workaround for now is to pass --user-data-dir=/some/path to Headless Shell and wait for a file called DevToolsActivePort to get written to that directory. After that, you can connect to the chosen port.

rvera@: May I ask what the plan for fixing this is?

Hi Sami,

I'm glad you asked. I am certainly planning to include you among the reviewers.

The plan is as follows:

* Build a repeatable way to measure the delay. This is done. I have a small Go program that execs chrome and times a loop that attempts to connect. Current results on my machine are ~1s for chrome, ~.8s for headless, ~.4s for content_shell. I have attached the program as a file below.

* Modify each of the respective implementing classes of DevToolsSocketFactory to contain a static member to hold a socket and a static method to open it, basically taking the guts out of CreateForHTTPServer and moving it into the static method.

* Change CreateForHTTPServer to simply return that previously opened socket, ensuring that ownership ends up identical to how it is now.

* Call the static methods from as early as possible in main, after forking to ensure we only do it once in the browser, in order to open the port and be listening as soon as possible. Connections should then be possible and just sit pending in the socket until the http handler accept loop picks one up. From my investigation and consultations so far, this should allow a client to connect essentially immediately, causing only the first request to sit pending and take a while, rather than the connection being refused for that time.

* Measure improvement with the above program.

I think this will work, and it isn't a large change, so I am hoping to have a CL out for review today or tomorrow. It does change the contract of the socket factory a bit, so I intend to document that thoroughly. If it turns out that these factories are actually called more than once, I'll add code to make them reusable, returning the above socket only the first time and behaving as now for further calls. As far as I can tell, however, they are called only once.

Please let me know what you think. In particular, if you can think of a reason why this won't work (threads?), I'd be very interested.

Deleted: latch.go 1.8 KB

latch.go 1.8 KB View Download

Here are three consecutive runs on my Linux machine. The first run was right after a reboot, so it appears to have spent much more time initializing system stuff that is already warmed up on the subsequent runs.

rvera@fuertote:~/chromium/src$ latch
2017/03/29 13:30:14 chrome connected after  5.146212999  seconds
2017/03/29 13:30:15 headless connected after  1.021295724  seconds
2017/03/29 13:30:16 content_shell connected after  0.593167049  seconds
rvera@fuertote:~/chromium/src$ latch
2017/03/29 13:30:23 chrome connected after  1.215715957  seconds
2017/03/29 13:30:24 headless connected after  1.002533723  seconds
2017/03/29 13:30:25 content_shell connected after  0.851272695  seconds
rvera@fuertote:~/chromium/src$ latch
2017/03/29 13:30:28 chrome connected after  1.212845699  seconds
2017/03/29 13:30:29 headless connected after  1.058090477  seconds
2017/03/29 13:30:30 content_shell connected after  0.888557738  seconds

Thanks, that sounds reasonable. FWIW, when I originally opened this bug I thought we could also support socket activation[1] which removes any possibility of a race condition from connecting. It sounds like we could extend your idea to support that too if necessary.

[1] http://0pointer.de/blog/projects/socket-activation.html

I implemented my plan for headless, as that's the simplest and also the clearest use case. It worked and didn't work.

It worked in that the port is clearly opened earlier and the connection is made much earlier.

It didn't work in that it still refuses the connection for a very long time (~half a second). It cut ~50% of the time, but the client still has to poll, so this doesn't solve the problem.

Clearly, solving the problem will require opening the port before execing chrome, i.e. something like socket activation. I've looked at systemd (thanks for the link, Sami), and at launchd on the Mac. They are very different and each require linking in a new library. The systemd code would be relatively easy to reimplement, as the protocol is quite simple (basically two environment variables set by the parent process), but I don't think it's worth the effort. I looked at how Windows passes sockets, and the only thing I found is a way to pass the actual socket via IPC, using yet a third library (SPI). I could not find anything about inheriting open sockets from a parent process on Windows.

For now then, I propose adding a command-line switch to pass the fd (call it debug_socket_fd), for use with fork/exec on linux and mac only. Fully enabling systemd, launchd, and Windows SPI compatibility will remain out of scope. The effort involved seems too big without a clear use case not met by a command-line switch.

The following semantics seem appropriate:
If --debug_socket_fd is present, --remote_debug_port must NOT be present.
If --debug_socket_fd is present, but adopting the socket fails, the default debug port will be opened and an appropriate message logged. (Or we could exit with an error. I'd rather fail open, though.)

Suggestions, comments, alternatives welcome. I will start implementing on Monday 3 April, Sydney time.

Adding an option to pass in an opened socket fd sounds good. I'm kind of wondering why systemd didn't adopt that option, but I may be missing something. Hopefully it should be possible to implement a systemd-compatible wrapper on top of that.

Systemd and launchd both rely on the child process inheriting the table of open sockets from its parent. The only thing the apis have to do is identify the relevant fds to the child, who otherwise doesn't know what they are. systemd does this by ensuring that they _are_ the fds immediately after stdin, stdout, and stderr, and just telling the child that a) it happened, and b) how many there are, via environment variables. launchd involves an actual connection and protocol back to the launchd process to register with launchd, retrieve an array of already-open fds, set up callbacks for kernel events when connections arrive on the fds, etc. I don't fully understand why it's so heavyweight, but I suspect it's a permissions issue. Launchd has root permissions and the child process may not. That's just a guess though.

My suspicion is that systemd does what it does in order to support an array of fds without complicated command-line semantics, and to avoid any possible collision with the client's command-line arguments. It might be hard to provide an api that is both easy to implement and doesn't mess with the existing command-line parsing of any arbitrary program.

In our case, we are the child so we want to do something simple that works in as many environments as possible, so a command-line flag seems reasonable. We could easily enough implement systemd compatibility for linux only, but that complicates the callers a bit (ensuring that the relevant socket is the _first_ after stderr and setting environment variables for the exec call instead of a single command-line flag), for not much gain. I guess I don't see the use case for systemd compatibility per se.

Sorry if this is long winded, but I'm also taking the opportunity to document my findings here a bit.

Thanks, that's a useful summary actually.

This is now implemented, working, and out for review. Some final notes:

The name of command-line flag is 
remote-debugging-socket-fd
for alignment with and alphabetical proximity to
remote-debugging-port.

In the end failing open didn't look like such a great idea, as nowhere else does this happen. If the socket can't be adopted, devtools is disabled, the same as if opening a requested port fails.

I've also attached the latest version of the test program, latch.go. A successful run gives this output:

rvera@fuertote:~/chromium/src$ latch
2017/04/13 12:08:03 

Starting headless_shell with an inherited fd for timing
2017/04/13 12:08:03 Timing connection
2017/04/13 12:08:03 headless_shell connected after  0.000184303  seconds using  1  tries
2017/04/13 12:08:03 Got json response from headless_shell &{200 OK 200 HTTP/1.1 1 1 map[Content-Length:[362] Content-Type:[application/json; charset=UTF-8]] 0xc420150040 362 [] false false map[] 0xc42000a800 <nil>}

There are two CLs implementing the fix, one for a small TCP stack change and one for the headless changes. Both are linked to this bug so should appear here once they land.

Deleted: latch.go 5.8 KB

latch.go 5.8 KB View Download

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/26f0a139eaac0d4554cd83c9b736724d2c6553be

commit 26f0a139eaac0d4554cd83c9b736724d2c6553be
Author: rvera <rvera@chromium.org>
Date: Tue May 02 22:25:44 2017

Adds a method to TCPServerSocket to adopt a socket.
This is used by headless to adopt a socket opened by a parent process
for remote debugging.

In order to keep the interfaces as regular as possible, this also renames and adds a few methods to the underlying TCPSocketWin, TCPSocketPosix, and SocketPosix classes.

The method rename resulted in bluetooth code changing, so ortuno@ is added as an owner reviewer for the bluetooth code.

Also adds tests for the Adopt[C|Unc]onnectedSocket methods. The server method is just a single line, so there is no explicit test for it.

The Dependent Patchset below uses this new method to
adopt the socket as the devtools remote-debug server connection.

BUG= 624837 

Review-Url: https://codereview.chromium.org/2815993002
Cr-Commit-Position: refs/heads/master@{#468793}

[modify] https://crrev.com/26f0a139eaac0d4554cd83c9b736724d2c6553be/device/bluetooth/bluetooth_socket_win.cc
[modify] https://crrev.com/26f0a139eaac0d4554cd83c9b736724d2c6553be/net/socket/socket_posix.cc
[modify] https://crrev.com/26f0a139eaac0d4554cd83c9b736724d2c6553be/net/socket/socket_posix.h
[modify] https://crrev.com/26f0a139eaac0d4554cd83c9b736724d2c6553be/net/socket/tcp_server_socket.cc
[modify] https://crrev.com/26f0a139eaac0d4554cd83c9b736724d2c6553be/net/socket/tcp_server_socket.h
[modify] https://crrev.com/26f0a139eaac0d4554cd83c9b736724d2c6553be/net/socket/tcp_socket_posix.cc
[modify] https://crrev.com/26f0a139eaac0d4554cd83c9b736724d2c6553be/net/socket/tcp_socket_posix.h
[modify] https://crrev.com/26f0a139eaac0d4554cd83c9b736724d2c6553be/net/socket/tcp_socket_unittest.cc
[modify] https://crrev.com/26f0a139eaac0d4554cd83c9b736724d2c6553be/net/socket/tcp_socket_win.cc
[modify] https://crrev.com/26f0a139eaac0d4554cd83c9b736724d2c6553be/net/socket/tcp_socket_win.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cce7c0a2dcdbcb203d207baa4a01f24df5a594e5

commit cce7c0a2dcdbcb203d207baa4a01f24df5a594e5
Author: rvera <rvera@chromium.org>
Date: Wed May 03 01:08:59 2017

Adds a command-line flag indicating an open and listening socket to
use for the remote-debugging connection, for Posix and headless only.
This has been tested using a small program written in Go, which is
attached to the bug.

BUG= 624837 

Review-Url: https://codereview.chromium.org/2810353003
Cr-Commit-Position: refs/heads/master@{#468851}

[modify] https://crrev.com/cce7c0a2dcdbcb203d207baa4a01f24df5a594e5/headless/app/headless_shell.cc
[modify] https://crrev.com/cce7c0a2dcdbcb203d207baa4a01f24df5a594e5/headless/app/headless_shell_switches.cc
[modify] https://crrev.com/cce7c0a2dcdbcb203d207baa4a01f24df5a594e5/headless/app/headless_shell_switches.h
[modify] https://crrev.com/cce7c0a2dcdbcb203d207baa4a01f24df5a594e5/headless/lib/browser/headless_browser_main_parts.cc
[modify] https://crrev.com/cce7c0a2dcdbcb203d207baa4a01f24df5a594e5/headless/lib/browser/headless_devtools.cc
[modify] https://crrev.com/cce7c0a2dcdbcb203d207baa4a01f24df5a594e5/headless/public/headless_browser.cc
[modify] https://crrev.com/cce7c0a2dcdbcb203d207baa4a01f24df5a594e5/headless/public/headless_browser.h

Ok. This is now fixed, though for Posix and headless only. The command-line flag is 

--remote-debugging-socket-fd=<fd of the open socket>

It's up to the process exec'ing headless chrome to provide the correct value. In Go, for example, the exec package provides the Cmd type for running processes, which contains an ExtraFiles slice member, and the fd is the index into that slice, plus 3 (for stdin, stdout, and stderr). See https://golang.org/pkg/os/exec/

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/43017673005af77aba9dc160a4c1ee4ee47076ff

commit 43017673005af77aba9dc160a4c1ee4ee47076ff
Author: rvera <rvera@chromium.org>
Date: Wed May 24 04:13:58 2017

Refactored adopting a socket for devtools into cleaner, more idiomatic code.

McGreevy because he suggested it. Sami for Owners.

BUG= 624837 

Review-Url: https://codereview.chromium.org/2882393002
Cr-Commit-Position: refs/heads/master@{#474157}

[modify] https://crrev.com/43017673005af77aba9dc160a4c1ee4ee47076ff/headless/lib/browser/headless_devtools.cc