Open socket for NaCl GDB debug stub in browser process.

NaCl: Bind TCP socket for the GDB debug stub in the browser process

This allows the debug stub to work without "--no-sandbox".

This change is only for POSIX since Windows requires very different code.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=3007
TEST= launch NaCl page in chrome with --enable-nacl-debug but without --no-sandbox
try to connect nacl gdb.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=156784

[Mark Seaborn, 2012-09-10 15:39:45]

What's the plan for making this work on Windows?

http://codereview.chromium.org/10928050/diff/7001/chrome/browser/nacl_host/na...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/10928050/diff/7001/chrome/browser/nacl_host/na...
chrome/browser/nacl_host/nacl_process_host.cc:639: SocketDescriptor s =
net::TCPListenSocket::CreateAndBind("127.0.0.1", 4014);
Nit: Can you make 4014 a named constant?  And comment that this is what NaCl
happened to choose for its debug stub TCP port, but that otherwise it's
arbitrary.

http://codereview.chromium.org/10928050/diff/7001/chrome/browser/nacl_host/na...
chrome/browser/nacl_host/nacl_process_host.cc:641: LOG(ERROR) << "Failed to
listen debug stub socket";
Missing a preposition.  Maybe "listen() failed on debug stub socket".

http://codereview.chromium.org/10928050/diff/7001/chrome/browser/nacl_host/na...
chrome/browser/nacl_host/nacl_process_host.cc:676: if (enable_debug_stub_) {
Nit: can you put this after the Mac shared memory block, because it's less
essential?

http://codereview.chromium.org/10928050/diff/7001/chrome/browser/nacl_host/na...
File chrome/browser/nacl_host/nacl_process_host.h (right):

http://codereview.chromium.org/10928050/diff/7001/chrome/browser/nacl_host/na...
chrome/browser/nacl_host/nacl_process_host.h:96: // sandbox turned on during
debugging.
Please disambiguate "debugging" in comments so that it's clear to readers that
you're referring to the GDB debug stub.

Maybe: "Create bound TCP socket in the browser process so that the GDB debug
stub will work when the Chrome sandbox is enabled."

http://codereview.chromium.org/10928050/diff/7001/chrome/common/nacl_types.cc
File chrome/common/nacl_types.cc (right):

http://codereview.chromium.org/10928050/diff/7001/chrome/common/nacl_types.cc...
chrome/common/nacl_types.cc:6: #include "net/base/tcp_listen_socket.h"
You don't need to add this, do you?

http://codereview.chromium.org/10928050/diff/7001/chrome/common/nacl_types.cc...
chrome/common/nacl_types.cc:13: 
Nit: don't add an empty line here

http://codereview.chromium.org/10928050/diff/7001/chrome/common/nacl_types.h
File chrome/common/nacl_types.h (right):

http://codereview.chromium.org/10928050/diff/7001/chrome/common/nacl_types.h#...
chrome/common/nacl_types.h:17: #include "net/base/tcp_listen_socket.h"
Unconditional #includes should go in the same block as build_config.h, right?

http://codereview.chromium.org/10928050/diff/7001/chrome/common/nacl_types.h#...
chrome/common/nacl_types.h:60: FileDescriptor debug_stub_server_socket;
Nit: can you put this after 'handles'?

http://codereview.chromium.org/10928050/diff/7001/net/base/tcp_listen_socket.h
File net/base/tcp_listen_socket.h (right):

http://codereview.chromium.org/10928050/diff/7001/net/base/tcp_listen_socket....
net/base/tcp_listen_socket.h:25: // Get raw socket descriptor binded to ip:port.
'binded' -> 'bound'

[halyavin, 2012-09-11 11:16:11]

https://chromiumcodereview.appspot.com/10928050/diff/7001/chrome/browser/nacl...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://chromiumcodereview.appspot.com/10928050/diff/7001/chrome/browser/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:639: SocketDescriptor s =
net::TCPListenSocket::CreateAndBind("127.0.0.1", 4014);
On 2012/09/10 15:39:45, Mark Seaborn wrote:
> Nit: Can you make 4014 a named constant?  And comment that this is what NaCl
> happened to choose for its debug stub TCP port, but that otherwise it's
> arbitrary.

Done.

https://chromiumcodereview.appspot.com/10928050/diff/7001/chrome/browser/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:641: LOG(ERROR) << "Failed to
listen debug stub socket";
On 2012/09/10 15:39:45, Mark Seaborn wrote:
> Missing a preposition.  Maybe "listen() failed on debug stub socket".

Done.

https://chromiumcodereview.appspot.com/10928050/diff/7001/chrome/browser/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:676: if (enable_debug_stub_) {
On 2012/09/10 15:39:45, Mark Seaborn wrote:
> Nit: can you put this after the Mac shared memory block, because it's less
> essential?

Done.

https://chromiumcodereview.appspot.com/10928050/diff/7001/chrome/browser/nacl...
File chrome/browser/nacl_host/nacl_process_host.h (right):

https://chromiumcodereview.appspot.com/10928050/diff/7001/chrome/browser/nacl...
chrome/browser/nacl_host/nacl_process_host.h:96: // sandbox turned on during
debugging.
On 2012/09/10 15:39:45, Mark Seaborn wrote:
> Please disambiguate "debugging" in comments so that it's clear to readers that
> you're referring to the GDB debug stub.
> 
> Maybe: "Create bound TCP socket in the browser process so that the GDB debug
> stub will work when the Chrome sandbox is enabled."

Done.

https://chromiumcodereview.appspot.com/10928050/diff/7001/chrome/common/nacl_...
File chrome/common/nacl_types.cc (right):

https://chromiumcodereview.appspot.com/10928050/diff/7001/chrome/common/nacl_...
chrome/common/nacl_types.cc:6: #include "net/base/tcp_listen_socket.h"
On 2012/09/10 15:39:45, Mark Seaborn wrote:
> You don't need to add this, do you?

Done.

https://chromiumcodereview.appspot.com/10928050/diff/7001/chrome/common/nacl_...
chrome/common/nacl_types.cc:13: 
On 2012/09/10 15:39:45, Mark Seaborn wrote:
> Nit: don't add an empty line here

Done.

https://chromiumcodereview.appspot.com/10928050/diff/7001/chrome/common/nacl_...
File chrome/common/nacl_types.h (right):

https://chromiumcodereview.appspot.com/10928050/diff/7001/chrome/common/nacl_...
chrome/common/nacl_types.h:17: #include "net/base/tcp_listen_socket.h"
On 2012/09/10 15:39:45, Mark Seaborn wrote:
> Unconditional #includes should go in the same block as build_config.h, right?
This is left from my experiments. I don't need this include at all now, so I
deleted it.

https://chromiumcodereview.appspot.com/10928050/diff/7001/chrome/common/nacl_...
chrome/common/nacl_types.h:60: FileDescriptor debug_stub_server_socket;
On 2012/09/10 15:39:45, Mark Seaborn wrote:
> Nit: can you put this after 'handles'?

Done.

https://chromiumcodereview.appspot.com/10928050/diff/7001/net/base/tcp_listen...
File net/base/tcp_listen_socket.h (right):

https://chromiumcodereview.appspot.com/10928050/diff/7001/net/base/tcp_listen...
net/base/tcp_listen_socket.h:25: // Get raw socket descriptor binded to ip:port.
On 2012/09/10 15:39:45, Mark Seaborn wrote:
> 'binded' -> 'bound'

Done.

[halyavin, 2012-09-11 11:17:41]

szym: I need approval for net/base/tcp_listen_socket.cc change.
sky: I need approval from owner of chrome/common files.

[sky, 2012-09-11 16:07:20]

LGTM

[szym, 2012-09-11 16:17:24]

http://codereview.chromium.org/10928050/diff/4011/chrome/browser/nacl_host/na...
File chrome/browser/nacl_host/nacl_process_host.h (right):

http://codereview.chromium.org/10928050/diff/4011/chrome/browser/nacl_host/na...
chrome/browser/nacl_host/nacl_process_host.h:20: #include
"net/base/tcp_listen_socket.h"
Don't include this. Instead, forward-declare SocketDescriptor. You're including
this anyway in the .cc file.

http://codereview.chromium.org/10928050/diff/4011/net/base/tcp_listen_socket.h
File net/base/tcp_listen_socket.h (right):

http://codereview.chromium.org/10928050/diff/4011/net/base/tcp_listen_socket....
net/base/tcp_listen_socket.h:25: // Get raw socket descriptor bound to ip:port.
"raw socket" might mislead the reader to think the socket is "raw". I suggest
"raw TCP socket descriptor".

http://codereview.chromium.org/10928050/diff/4011/net/base/tcp_listen_socket....
net/base/tcp_listen_socket.h:26: static SocketDescriptor CreateAndBind(const
std::string& ip, int port);
I'm okay with that for now, but I think it'd be better if NaCL gdb actually used
a net::StreamListenSocket instead of just getting the SocketDescriptor using
net/.

http://codereview.chromium.org/10928050/diff/4011/net/base/tcp_listen_socket....
net/base/tcp_listen_socket.h:27: protected:
nit: Blank line before protected: section.

[halyavin, 2012-09-12 14:29:58]

https://chromiumcodereview.appspot.com/10928050/diff/4011/chrome/browser/nacl...
File chrome/browser/nacl_host/nacl_process_host.h (right):

https://chromiumcodereview.appspot.com/10928050/diff/4011/chrome/browser/nacl...
chrome/browser/nacl_host/nacl_process_host.h:20: #include
"net/base/tcp_listen_socket.h"
On 2012/09/11 16:17:24, szym wrote:
> Don't include this. Instead, forward-declare SocketDescriptor. You're
including
> this anyway in the .cc file.
SocketDescriptor is not a struct or a class, it can't be forward-declared.

https://chromiumcodereview.appspot.com/10928050/diff/4011/net/base/tcp_listen...
File net/base/tcp_listen_socket.h (right):

https://chromiumcodereview.appspot.com/10928050/diff/4011/net/base/tcp_listen...
net/base/tcp_listen_socket.h:25: // Get raw socket descriptor bound to ip:port.
On 2012/09/11 16:17:24, szym wrote:
> "raw socket" might mislead the reader to think the socket is "raw". I suggest
> "raw TCP socket descriptor".

Done.

https://chromiumcodereview.appspot.com/10928050/diff/4011/net/base/tcp_listen...
net/base/tcp_listen_socket.h:27: protected:
On 2012/09/11 16:17:24, szym wrote:
> nit: Blank line before protected: section.

Done.

[szym, 2012-09-12 14:49:06]

lgtm

[Mark Seaborn, 2012-09-13 18:18:43]

LGTM

[Mark Seaborn, 2012-09-13 18:19:04]

Oops, I meant to send this comment:

http://codereview.chromium.org/10928050/diff/3004/chrome/browser/nacl_host/na...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/10928050/diff/3004/chrome/browser/nacl_host/na...
chrome/browser/nacl_host/nacl_process_host.cc:637: // TCP port we chosen for
NaCl debug stub. It can be any other number.
'chose'

[halyavin, 2012-09-13 19:03:30]

https://chromiumcodereview.appspot.com/10928050/diff/3004/chrome/browser/nacl...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://chromiumcodereview.appspot.com/10928050/diff/3004/chrome/browser/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:637: // TCP port we chosen for
NaCl debug stub. It can be any other number.
On 2012/09/13 18:19:04, Mark Seaborn wrote:
> 'chose'

Done.

[commit-bot: I haz the power, 2012-09-14 07:29:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/halyavin@google.com/10928050/8011