Adds a command-line flag indicating an open and listening socket to

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}
Committed: https://chromium.googlesource.com/chromium/src/+/cce7c0a2dcdbcb203d207baa4a01f24df5a594e5

[Raul Vera, 2017-04-13 02:01:08]

The CQ bit was checked by rvera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 02:02:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Raul Vera, 2017-04-13 02:17:08]

rvera@chromium.org changed reviewers:
+ dvallet@chromium.org, pfeldman@chromium.org, skyostil@chromium.org

[Raul Vera, 2017-04-13 02:17:09]

Hi,

Here is the fix for crbug/624837, which was undertaken more as a starter project
than for its importance. I've asked David for a local review, Sami for owners
and because he reported the bug, and Pavel because it touches devtools. 

Pavel, feel free to delegate this as appropriate. It's a pretty small change in
the scheme of things. 

Most of the design decisions are documented in the bug. Note that this is for
Posix and headless only, to keep things simple while handling the most pressing
use case.

I tested using a small Go program which I've attached to the bug, because I'm
told that I shouldn't commit Go.

I look forward to your comments.

Raul

[Raul Vera, 2017-04-13 02:25:39]

rvera@chromium.org changed required reviewers:
+ skyostil@chromium.org

[commit-bot: I haz the power, 2017-04-13 02:41:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-13 02:41:44]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Raul Vera, 2017-04-13 02:56:12]

The CQ bit was checked by rvera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 02:56:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dvallet, 2017-04-13 03:44:29]

LGTM

https://codereview.chromium.org/2810353003/diff/20001/headless/app/headless_s...
File headless/app/headless_shell.cc (right):

https://codereview.chromium.org/2810353003/diff/20001/headless/app/headless_s...
headless/app/headless_shell.cc:401: if
(command_line.HasSwitch(switches::kRemoteDebuggingPort) &&
You could add a non POSIX check here to ensure that they not try and set a
socketFd

https://codereview.chromium.org/2810353003/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_devtools.cc (right):

https://codereview.chromium.org/2810353003/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_devtools.cc:46: return socket;
nit: since you are returning here, I would remove the else block.
Like so, 
if (socket_fd == 0) {
...
  return socket;
}
#if defined(OS_POSIX)
std::unique_ptr<net::TCPServerSocket> tsock(
...
return std::unique_ptr<net::ServerSocket>();

[commit-bot: I haz the power, 2017-04-13 03:54:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-13 03:54:12]

Dry run: This issue passed the CQ dry run.

[Raul Vera, 2017-04-13 04:13:46]

On 2017/04/13 at 03:44:29, dvallet wrote:
> LGTM
> 
>
https://codereview.chromium.org/2810353003/diff/20001/headless/app/headless_s...
> File headless/app/headless_shell.cc (right):
> 
>
https://codereview.chromium.org/2810353003/diff/20001/headless/app/headless_s...
> headless/app/headless_shell.cc:401: if
(command_line.HasSwitch(switches::kRemoteDebuggingPort) &&
> You could add a non POSIX check here to ensure that they not try and set a
socketFd

Done.

> 
>
https://codereview.chromium.org/2810353003/diff/20001/headless/lib/browser/he...
> File headless/lib/browser/headless_devtools.cc (right):
> 
>
https://codereview.chromium.org/2810353003/diff/20001/headless/lib/browser/he...
> headless/lib/browser/headless_devtools.cc:46: return socket;
> nit: since you are returning here, I would remove the else block.
> Like so, 
> if (socket_fd == 0) {
> ...
>   return socket;
> }
> #if defined(OS_POSIX)
> std::unique_ptr<net::TCPServerSocket> tsock(
> ...
> return std::unique_ptr<net::ServerSocket>();

Done.

Thanks.

[Sami, 2017-04-13 09:34:29]

Thanks! lgtm with a couple of minor suggestions.

https://codereview.chromium.org/2810353003/diff/40001/headless/app/headless_s...
File headless/app/headless_shell.cc (right):

https://codereview.chromium.org/2810353003/diff/40001/headless/app/headless_s...
headless/app/headless_shell.cc:403: LOG(ERROR) << "remote-debugging-socket can't
be set on non-Posix systems";
nit: Start with a capital letter please.

https://codereview.chromium.org/2810353003/diff/40001/headless/app/headless_s...
headless/app/headless_shell.cc:409: LOG(ERROR) << "remote-debugging-port and
remote-debugging-socket "
Ditto.

https://codereview.chromium.org/2810353003/diff/40001/headless/app/headless_s...
headless/app/headless_shell.cc:502:
builder.EnableDevToolsServer(base::checked_cast<uint16_t>(parsed_fd));
Should we be casting to size_t instead since that's what we're checking above?

https://codereview.chromium.org/2810353003/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_devtools.cc (right):

https://codereview.chromium.org/2810353003/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_devtools.cc:41: if (socket_fd == 0) {
nit: we generally prefer !socket_fd instead of comparing to zero

https://codereview.chromium.org/2810353003/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_devtools.cc:68: size_t socket_fd;
nit: socket_fd_;

[Raul Vera, 2017-04-18 00:47:25]

Thanks for the review, Sami.

https://codereview.chromium.org/2810353003/diff/40001/headless/app/headless_s...
File headless/app/headless_shell.cc (right):

https://codereview.chromium.org/2810353003/diff/40001/headless/app/headless_s...
headless/app/headless_shell.cc:403: LOG(ERROR) << "remote-debugging-socket can't
be set on non-Posix systems";
On 2017/04/13 at 09:34:28, Sami wrote:
> nit: Start with a capital letter please.

Done. I did this on purpose because the name of the argument isn't capitalised,
but I'm not that fussed.

https://codereview.chromium.org/2810353003/diff/40001/headless/app/headless_s...
headless/app/headless_shell.cc:409: LOG(ERROR) << "remote-debugging-port and
remote-debugging-socket "
On 2017/04/13 at 09:34:28, Sami wrote:
> Ditto.

Done.

https://codereview.chromium.org/2810353003/diff/40001/headless/app/headless_s...
headless/app/headless_shell.cc:502:
builder.EnableDevToolsServer(base::checked_cast<uint16_t>(parsed_fd));
On 2017/04/13 at 09:34:28, Sami wrote:
> Should we be casting to size_t instead since that's what we're checking above?

Done. Converted from uint16_t to size_t and missed this one. Thanks.

https://codereview.chromium.org/2810353003/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_devtools.cc (right):

https://codereview.chromium.org/2810353003/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_devtools.cc:41: if (socket_fd == 0) {
On 2017/04/13 at 09:34:29, Sami wrote:
> nit: we generally prefer !socket_fd instead of comparing to zero

Done.

https://codereview.chromium.org/2810353003/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_devtools.cc:68: size_t socket_fd;
On 2017/04/13 at 09:34:29, Sami wrote:
> nit: socket_fd_;

Done. Thanks.

[Raul Vera, 2017-04-20 05:15:17]

The CQ bit was checked by rvera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 05:15:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 05:44:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 05:44:45]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Raul Vera, 2017-05-02 22:55:25]

The CQ bit was checked by rvera@chromium.org to run a CQ dry run

[Raul Vera, 2017-05-02 22:55:33]

The CQ bit was unchecked by rvera@chromium.org

[Raul Vera, 2017-05-02 22:55:50]

The CQ bit was checked by rvera@chromium.org

[Raul Vera, 2017-05-02 22:55:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
dvallet@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2810353003/#ps100001
(title: "Changed method call due to change in dependent CL.")

[commit-bot: I haz the power, 2017-05-02 22:56:17]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 23:00:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 23:00:25]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Raul Vera, 2017-05-03 00:04:24]

The CQ bit was checked by rvera@chromium.org

[Raul Vera, 2017-05-03 00:04:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
dvallet@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2810353003/#ps120001
(title: "Merge branch 'master' of https://chromium.googlesource.com/chromium/src
into headlessport")

[commit-bot: I haz the power, 2017-05-03 00:05:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 01:09:03]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493769864850390,
"parent_rev": "d3b61aeab646c01a5e0a921a580e603f160678e3", "commit_rev":
"cce7c0a2dcdbcb203d207baa4a01f24df5a594e5"}

[commit-bot: I haz the power, 2017-05-03 01:09:11]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/cce7c0a2dcdbcb203d207baa4a01...
==========

[commit-bot: I haz the power, 2017-05-03 01:09:12]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/cce7c0a2dcdbcb203d207baa4a01...