Always Ignore SIGPIPE for NaCl both in SFI mode and non SFI mode.

Always ignore SIGPIPE for NaCl both in SFI mode and non SFI mode.

Currently, if there is an access to the pipe, whose other side is
already closed, SIGPIPE will be raised in some platform such as linux.
It is unexpected behavior, and causes some test failure.
This is what trusted plugin does in content/app/content_main_runner.cc.

TEST=Ran PPAPINaClPNaClNonSfiTest.Audio locally many times and the flakiness
was gone. Also run trybots.
BUG=371662
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271000

[hidehiko, 2014-05-15 06:43:25]

Hi Mark,

could you take a look?

[Mark Seaborn, 2014-05-15 16:21:24]

LGTM

https://codereview.chromium.org/288063004/diff/1/components/nacl/loader/nacl_...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://codereview.chromium.org/288063004/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_helper_linux.cc:94: // Always ignore SIGPIPE.  We
check the return value of write().
The "we" here is rather vague (and different from the "we" in the next line :-)
).

How about: Always ignore SIGPIPE, for consistency with other Chrome processes,
and because some IPC code, such as sync_socket_posix.cc, requires this.

https://codereview.chromium.org/288063004/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_helper_linux.cc:95: // We do this just before
seccomp-bpf is initialized.
Maybe omit "just", so that this doesn't become wrong if someone adds code after
this?

https://codereview.chromium.org/288063004/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_helper_linux.cc:96: CHECK(signal(SIGPIPE, SIG_IGN)
!= SIG_ERR);
You could use PCHECK to print errno if this fails.

[hidehiko, 2014-05-16 08:21:02]

Thank you for review. Submitting...

https://codereview.chromium.org/288063004/diff/1/components/nacl/loader/nacl_...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://codereview.chromium.org/288063004/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_helper_linux.cc:94: // Always ignore SIGPIPE.  We
check the return value of write().
On 2014/05/15 16:21:25, Mark Seaborn wrote:
> The "we" here is rather vague (and different from the "we" in the next line
:-)
> ).
> 
> How about: Always ignore SIGPIPE, for consistency with other Chrome processes,
> and because some IPC code, such as sync_socket_posix.cc, requires this.

Done.

https://codereview.chromium.org/288063004/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_helper_linux.cc:95: // We do this just before
seccomp-bpf is initialized.
On 2014/05/15 16:21:25, Mark Seaborn wrote:
> Maybe omit "just", so that this doesn't become wrong if someone adds code
after
> this?

Done.

https://codereview.chromium.org/288063004/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_helper_linux.cc:96: CHECK(signal(SIGPIPE, SIG_IGN)
!= SIG_ERR);
On 2014/05/15 16:21:25, Mark Seaborn wrote:
> You could use PCHECK to print errno if this fails.

Done.

[hidehiko, 2014-05-16 08:21:06]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-05-16 08:21:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hidehiko@chromium.org/288063004/40001

[commit-bot: I haz the power, 2014-05-16 10:32:39]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-16 12:22:55]

Change committed as 271000