For Linux, create a ZygoteForkDelegate that uses the nacl_helper executable

A ZygoteForkDelegate for Native Client.

BUG=nativeclient:480
TEST=native client chrome linux tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93021

[Brad Chen, 2011-07-18 19:04:25]

As noted in my earlier mail I've broken my earlier CL into three pieces. This is
the third piece, changes to chrome/. It's the same code as the last review,
except for a bit of memory management code in chrome/app/chrome_main.cc to try
to appease the valgrind bots.

I suspect that I won't be able to use the commit-bot with this due to the "A +"
status of nacl_listener.cc. I'm also somewhat concerned about whether this will
work on ARM, as I had a failure earlier but the logs are no longer available.

Note: Bots are red because I didn't know about the -r option to git-try. I'm in
the process of rectifying that...

[agl, 2011-07-18 21:19:35]

LGTM.

I pretty superficial review as I believe that I, and several others, have seen
this before.

http://codereview.chromium.org/7395024/diff/22/chrome/app/chrome_main.cc
File chrome/app/chrome_main.cc (right):

http://codereview.chromium.org/7395024/diff/22/chrome/app/chrome_main.cc#newc...
chrome/app/chrome_main.cc:463: NaClForkDelegate* nacldelegate = new
NaClForkDelegate();
s/nacldelegate/nacl_delegate/

http://codereview.chromium.org/7395024/diff/22/chrome/app/chrome_main.cc#newc...
chrome/app/chrome_main.cc:466: free(nacldelegate);
|delete nacldelegate| and no need for the if(), delete is a no-op on a NULL
pointer. Could also consider using a small scope and a scoped_ptr.

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

http://codereview.chromium.org/7395024/diff/22/chrome/common/nacl_types.h#new...
chrome/common/nacl_types.h:10: #include "base/native_library.h"  // for HANDLE
and OS_POSIX
I think this should be removed. Unless the types are used in the header file
itself, the header can be included in .cc files.

http://codereview.chromium.org/7395024/diff/22/chrome/common/nacl_types.h#new...
chrome/common/nacl_types.h:23: #endif
I don't believe that we have any platforms where OS_WIN and OS_POSIX are both
true, so the previous #elif was correct.

http://codereview.chromium.org/7395024/diff/22/chrome/nacl/nacl_fork_delegate...
File chrome/nacl/nacl_fork_delegate_linux.cc (right):

http://codereview.chromium.org/7395024/diff/22/chrome/nacl/nacl_fork_delegate...
chrome/nacl/nacl_fork_delegate_linux.cc:22: NaClForkDelegate::NaClForkDelegate()
: ready_(false),
nit: unless constructor init lists fit on the same line, they should on
subsequent lines, with the ':' indented by four spaces.

http://codereview.chromium.org/7395024/diff/22/chrome/nacl/nacl_fork_delegate...
chrome/nacl/nacl_fork_delegate_linux.cc:63: const ssize_t nread = read(fds[0],
buf, sizeof(buf));
HANDLE_EINTR around read()

http://codereview.chromium.org/7395024/diff/22/chrome/nacl/nacl_fork_delegate...
chrome/nacl/nacl_fork_delegate_linux.cc:97: int nread = read(fd_, &naclchild,
sizeof(naclchild));
HANDLE_EINTR

[Brad Chen, 2011-07-18 22:40:29]

All fixed except for one below...

http://codereview.chromium.org/7395024/diff/22/chrome/app/chrome_main.cc
File chrome/app/chrome_main.cc (right):

http://codereview.chromium.org/7395024/diff/22/chrome/app/chrome_main.cc#newc...
chrome/app/chrome_main.cc:463: NaClForkDelegate* nacldelegate = new
NaClForkDelegate();
On 2011/07/18 21:19:35, agl wrote:
> s/nacldelegate/nacl_delegate/

Done.

http://codereview.chromium.org/7395024/diff/22/chrome/app/chrome_main.cc#newc...
chrome/app/chrome_main.cc:466: free(nacldelegate);
On 2011/07/18 21:19:35, agl wrote:
> |delete nacldelegate| and no need for the if(), delete is a no-op on a NULL
> pointer. Could also consider using a small scope and a scoped_ptr.

Done.

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

http://codereview.chromium.org/7395024/diff/22/chrome/common/nacl_types.h#new...
chrome/common/nacl_types.h:10: #include "base/native_library.h"  // for HANDLE
and OS_POSIX
Just to double check: given that HANDLE and OS_POSIX are used in this file I
would have thought the include belongs in this file.

On 2011/07/18 21:19:35, agl wrote:
> I think this should be removed. Unless the types are used in the header file
> itself, the header can be included in .cc files.

http://codereview.chromium.org/7395024/diff/22/chrome/common/nacl_types.h#new...
chrome/common/nacl_types.h:23: #endif
On 2011/07/18 21:19:35, agl wrote:
> I don't believe that we have any platforms where OS_WIN and OS_POSIX are both
> true, so the previous #elif was correct.

Done.

http://codereview.chromium.org/7395024/diff/22/chrome/nacl/nacl_fork_delegate...
File chrome/nacl/nacl_fork_delegate_linux.cc (right):

http://codereview.chromium.org/7395024/diff/22/chrome/nacl/nacl_fork_delegate...
chrome/nacl/nacl_fork_delegate_linux.cc:22: NaClForkDelegate::NaClForkDelegate()
: ready_(false),
On 2011/07/18 21:19:35, agl wrote:
> nit: unless constructor init lists fit on the same line, they should on
> subsequent lines, with the ':' indented by four spaces.

Done.

http://codereview.chromium.org/7395024/diff/22/chrome/nacl/nacl_fork_delegate...
chrome/nacl/nacl_fork_delegate_linux.cc:63: const ssize_t nread = read(fds[0],
buf, sizeof(buf));
On 2011/07/18 21:19:35, agl wrote:
> HANDLE_EINTR around read()

Done.

http://codereview.chromium.org/7395024/diff/22/chrome/nacl/nacl_fork_delegate...
chrome/nacl/nacl_fork_delegate_linux.cc:97: int nread = read(fd_, &naclchild,
sizeof(naclchild));
On 2011/07/18 21:19:35, agl wrote:
> HANDLE_EINTR

Done.

[agl, 2011-07-18 22:56:35]

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

http://codereview.chromium.org/7395024/diff/22/chrome/common/nacl_types.h#new...
chrome/common/nacl_types.h:10: #include "base/native_library.h"  // for HANDLE
and OS_POSIX
On 2011/07/18 22:40:29, Brad Chen wrote:
> Just to double check: given that HANDLE and OS_POSIX are used in this file I
> would have thought the include belongs in this file.

#include "build/build_config.h" for OS_* defines.

#if defined(OS_WIN)
#include <windows.h>
#endif

for HANDLE.

[Brad Chen, 2011-07-18 23:05:48]

Thanks for the reviews. The tree looks a bit crazy right now so I may hold off
until early tomorrow to commit this.

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

http://codereview.chromium.org/7395024/diff/22/chrome/common/nacl_types.h#new...
chrome/common/nacl_types.h:10: #include "base/native_library.h"  // for HANDLE
and OS_POSIX
On 2011/07/18 22:56:35, agl wrote:
> On 2011/07/18 22:40:29, Brad Chen wrote:
> > Just to double check: given that HANDLE and OS_POSIX are used in this file I
> > would have thought the include belongs in this file.
> 
> #include "build/build_config.h" for OS_* defines.
> 
> #if defined(OS_WIN)
> #include <windows.h>
> #endif
> 
> for HANDLE.

Done.

[commit-bot: I haz the power, 2011-07-19 15:08:23]

Can't process patch for file chrome/nacl/nacl_listener.cc.
A +