ChannelMojo: Let MessagePipeReader wait "peer closed" signal as well.

ChannelMojo: Let MessagePipeReader wait "peer closed" signal as well.

This CL adds MOJO_SIGNAL_PEER_CLOSED for the MessagePipeReader's
waiting bits. Without this, PRECONDITION_FAILED error can be notified
at the end of the channel lifetime, and the browser process misinterprets
it as a renderer crash, which results sadface screen.

This rarely happens and is reproduced only with some underpowered CPU
with excessive pressure. So we cannot have any reliable test.

BUG=466814
R=viettrungluu@chromium.org

Committed: https://crrev.com/29d88220e88d21deaf6856b7ded300fa06ab94d3
Cr-Commit-Position: refs/heads/master@{#322094}

[Hajime Morrita, 2015-03-23 17:21:39]

Trung: PTAL this and https://codereview.chromium.org/1021393002/ ?

[Hajime Morrita, 2015-03-24 00:23:49]

Trung: Ping? This blocks ChannelMojo, and I cannot move this without this.

[viettrungluu, 2015-03-24 21:03:26]

https://codereview.chromium.org/1023043002/diff/1/ipc/mojo/ipc_message_pipe_r...
File ipc/mojo/ipc_message_pipe_reader.cc (right):

https://codereview.chromium.org/1023043002/diff/1/ipc/mojo/ipc_message_pipe_r...
ipc/mojo/ipc_message_pipe_reader.cc:166: MOJO_HANDLE_SIGNAL_READABLE |
MOJO_HANDLE_SIGNAL_PEER_CLOSED);
So if it gets signaled due to "peer closed" ... the result would still be
"MOJO_RESULT_OK" in PipeIsReady() ... which then sends you to
ReadMessagesThenWait(), etc.

I guess that's OK, but it's still a little weird.

https://codereview.chromium.org/1023043002/diff/1/ipc/mojo/ipc_message_pipe_r...
ipc/mojo/ipc_message_pipe_reader.cc:170: if (result !=
MOJO_RESULT_ALREADY_EXISTS) {
It seems to me that without adding "peer closed" above, you'd get "precondition
failed" here.

https://codereview.chromium.org/1023043002/diff/1/ipc/mojo/ipc_message_pipe_r...
ipc/mojo/ipc_message_pipe_reader.cc:171: if (result != MOJO_RESULT_OK) {
And then end up down this path. Why doesn't this work?

https://codereview.chromium.org/1023043002/diff/1/ipc/mojo/ipc_message_pipe_r...
ipc/mojo/ipc_message_pipe_reader.cc:184: if (wait_result != MOJO_RESULT_ABORTED)
{
It seems unlikely to me that you should ever see MOJO_RESULT_ABORTED, since it
probably indicates a race condition.

https://codereview.chromium.org/1023043002/diff/1/ipc/mojo/ipc_message_pipe_r...
ipc/mojo/ipc_message_pipe_reader.cc:185: // FAILED_PRECONDITION happens every
time the peer is dead so
And with your change, you shouldn't get "failed precondition" here any longer.

[Hajime Morrita, 2015-03-24 21:12:57]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2015-03-24 21:13:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1023043002/20001

[commit-bot: I haz the power, 2015-03-24 21:13:31]

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

[commit-bot: I haz the power, 2015-03-24 21:13:32]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Hajime Morrita, 2015-03-24 21:13:37]

Thanks for the review!
PTAL at  the updated CL?

https://codereview.chromium.org/1023043002/diff/1/ipc/mojo/ipc_message_pipe_r...
File ipc/mojo/ipc_message_pipe_reader.cc (right):

https://codereview.chromium.org/1023043002/diff/1/ipc/mojo/ipc_message_pipe_r...
ipc/mojo/ipc_message_pipe_reader.cc:171: if (result != MOJO_RESULT_OK) {
On 2015/03/24 21:03:25, viettrungluu wrote:
> And then end up down this path. Why doesn't this work?
The issue isn't real crash. My understanding is that closing in
this way makes the rowser process think this renderer is crashed, 
instead of closing the channel due to the error.
The channel has to be closed by the browser side, it looks.

https://codereview.chromium.org/1023043002/diff/1/ipc/mojo/ipc_message_pipe_r...
ipc/mojo/ipc_message_pipe_reader.cc:184: if (wait_result != MOJO_RESULT_ABORTED)
{
On 2015/03/24 21:03:26, viettrungluu wrote:
> It seems unlikely to me that you should ever see MOJO_RESULT_ABORTED, since it
> probably indicates a race condition.
This has been here from the beginning but is probably just a leftover
from my trial-and-error at that stage. Let me turn this to CHECK()
just in case.

https://codereview.chromium.org/1023043002/diff/1/ipc/mojo/ipc_message_pipe_r...
ipc/mojo/ipc_message_pipe_reader.cc:185: // FAILED_PRECONDITION happens every
time the peer is dead so
On 2015/03/24 21:03:25, viettrungluu wrote:
> And with your change, you shouldn't get "failed precondition" here any longer.

Right. Removing.

[viettrungluu, 2015-03-24 21:16:08]

lgtm

[Hajime Morrita, 2015-03-24 21:20:41]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2015-03-24 21:21:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1023043002/20001

[commit-bot: I haz the power, 2015-03-24 23:52:39]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-24 23:53:41]

Patchset 2 (id:??) landed as
https://crrev.com/29d88220e88d21deaf6856b7ded300fa06ab94d3
Cr-Commit-Position: refs/heads/master@{#322094}

[tkent, 2015-03-25 04:38:37]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1035633002/ by tkent@chromium.org.

The reason for reverting is: speculative revert for "Too many open files" on
various Mac bots.
e.g.
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.9/builds/...
.

[tkent, 2015-03-25 06:01:35]

On 2015/03/25 04:38:37, tkent wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/1035633002/ by mailto:tkent@chromium.org.
> 
> The reason for reverting is: speculative revert for "Too many open files" on
> various Mac bots.
> e.g.
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.9/builds/...
> .

The problem was not only in Mac bots.  browser_tests on Windows, Linux, and
Android, content_browsertests on Windows and Linux, etc.