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

ipc/mojo/ipc_message_pipe_reader.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ipc/mojo/ipc_message_pipe_reader.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/location.h"
 #include "base/logging.h"
@@ skipping 143 matching lines @@
 
 void MessagePipeReader::ReadMessagesThenWait() {
   while (true) {
     ReadAvailableMessages();
     if (!pipe_.is_valid())
       break;
     // |Wait()| is safe to call only after all messages are read.
     // If can fail with |MOJO_RESULT_ALREADY_EXISTS| otherwise.
     // Also, we don't use MOJO_HANDLE_SIGNAL_WRITABLE here, expecting buffer in
     // MessagePipe.
-    MojoResult result =
-        async_waiter_->Wait(pipe_.get().value(), MOJO_HANDLE_SIGNAL_READABLE);
+    MojoResult result = async_waiter_->Wait(
+        pipe_.get().value(),
+        MOJO_HANDLE_SIGNAL_READABLE | MOJO_HANDLE_SIGNAL_PEER_CLOSED);

[viettrungluu, 2015/03/24 21:03:25]

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.

     // If the result is |MOJO_RESULT_ALREADY_EXISTS|, there could be messages
     // that have been arrived after the last |ReadAvailableMessages()|.
     // We have to consume then and retry in that case.
     if (result != MOJO_RESULT_ALREADY_EXISTS) {

[viettrungluu, 2015/03/24 21:03:25]

It seems to me that without adding "peer closed" above, you'd get "precondition
failed" here.

       if (result != MOJO_RESULT_OK) {

[viettrungluu, 2015/03/24 21:03:25]

And then end up down this path. Why doesn't this work?

[Hajime Morrita, 2015/03/24 21:13:36]

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.

         LOG(ERROR) << "Failed to wait on the pipe. Result is " << result;
         OnPipeError(result);
         Close();
       }
 
       break;
     }
   }
 }
 
 void MessagePipeReader::PipeIsReady(MojoResult wait_result) {
   if (wait_result != MOJO_RESULT_OK) {
     if (wait_result != MOJO_RESULT_ABORTED) {

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

It seems unlikely to me that you should ever see MOJO_RESULT_ABORTED, since it
probably indicates a race condition.

[Hajime Morrita, 2015/03/24 21:13:36]

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.

       // FAILED_PRECONDITION happens every time the peer is dead so

[viettrungluu, 2015/03/24 21:03:25]

And with your change, you shouldn't get "failed precondition" here any longer.

[Hajime Morrita, 2015/03/24 21:13:36]

Right. Removing.

       // it isn't worth polluting the log message.
       LOG_IF(WARNING, wait_result != MOJO_RESULT_FAILED_PRECONDITION)
           << "Pipe got error from the waiter. Closing: " << wait_result;
       OnPipeError(wait_result);
     }
 
     Close();
     return;
   }
 
   ReadMessagesThenWait();
 }
 
 void MessagePipeReader::DelayedDeleter::operator()(
     MessagePipeReader* ptr) const {
   ptr->Close();
   base::MessageLoopProxy::current()->PostTask(
       FROM_HERE, base::Bind(&DeleteNow, ptr));
 }
 
 }  // namespace internal
 }  // namespace IPC