Fix races with MessagePipeReader due to the Mojo IPC channel being thread-safe.

ipc/mojo/ipc_message_pipe_reader.cc

Index: ipc/mojo/ipc_message_pipe_reader.cc
diff --git a/ipc/mojo/ipc_message_pipe_reader.cc b/ipc/mojo/ipc_message_pipe_reader.cc
index 2202a575307a6d1cb1ee0e35e76e84af7b505fb8..be6005d5efa331576ceca5fe10e57a4d59cc4bc1 100644
--- a/ipc/mojo/ipc_message_pipe_reader.cc
+++ b/ipc/mojo/ipc_message_pipe_reader.cc
@@ -19,6 +19,7 @@ namespace internal {
 MessagePipeReader::MessagePipeReader(mojo::ScopedMessagePipeHandle handle,
                                      MessagePipeReader::Delegate* delegate)
     : pipe_(handle.Pass()),
+      handle_copy_(pipe_.get().value()),
       delegate_(delegate),
       async_waiter_(
           new AsyncHandleWaiter(base::Bind(&MessagePipeReader::PipeIsReady,
@@ -27,40 +28,45 @@ MessagePipeReader::MessagePipeReader(mojo::ScopedMessagePipeHandle handle,
 }
 
 MessagePipeReader::~MessagePipeReader() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   // The pipe should be closed before deletion.
   CHECK(!IsValid());
-  DCHECK_EQ(pending_send_error_, MOJO_RESULT_OK);
 }
 
 void MessagePipeReader::Close() {
-  // All pending errors should be signaled before Close().
-  DCHECK_EQ(pending_send_error_, MOJO_RESULT_OK);
+  DCHECK(thread_checker_.CalledOnValidThread());
   async_waiter_.reset();
   pipe_.reset();
   OnPipeClosed();
 }
 
 void MessagePipeReader::CloseWithError(MojoResult error) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   OnPipeError(error);
   Close();
 }
 
 void MessagePipeReader::CloseWithErrorIfPending() {
-  if (pending_send_error_ == MOJO_RESULT_OK)
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (pending_send_error() == MOJO_RESULT_OK)

[Alexander Potapenko, 2015/08/25 11:22:34]

I think the read of pending_send_error_ here must be an acquire load.

[Anand Mistry (off Chromium), 2015/08/25 12:16:08]

I don't think so. There are no other memory accesses that need to be ordered
between threads. |pending_send_error_| is the only variable that is shared
between threads (well, and |handle_copy_| which is const). Everything else is
only accessed on the IO thread.

     return;
-  MojoResult error = pending_send_error_;
-  pending_send_error_ = MOJO_RESULT_OK;
-  CloseWithError(error);
+  // NOTE: This races with Send(), and therefore the value of
+  // pending_send_error() can change. In any case, it won't be MOJO_RESULT_OK,
+  // so it's fine.
+  CloseWithError(pending_send_error());

[Alexander Potapenko, 2015/08/25 11:22:34]

Why not reuse the value returned by pending_send_error() before?

[Anand Mistry (off Chromium), 2015/08/26 03:43:19]

Done.

   return;
 }
 
 void MessagePipeReader::CloseWithErrorLater(MojoResult error) {
-  pending_send_error_ = error;
+  DCHECK_NE(error, MOJO_RESULT_OK);
+  // NOTE: No assumptions about the value of |pending_send_error_| or whether or
+  // not the error has been signaled can be made. If Send() is called
+  // immediately before Close() and errors, it's possible for the error to not
+  // be signaled.
+  base::subtle::NoBarrier_Store(&pending_send_error_, error);
 }
 
 bool MessagePipeReader::Send(scoped_ptr<Message> message) {
-  DCHECK(IsValid());
-
   TRACE_EVENT_WITH_FLOW0(TRACE_DISABLED_BY_DEFAULT("ipc.flow"),
                          "MessagePipeReader::Send",
                          message->flags(),
@@ -111,6 +117,7 @@ void MessagePipeReader::OnMessageReceived() {
 }
 
 void MessagePipeReader::OnPipeClosed() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (!delegate_)
     return;
   delegate_->OnPipeClosed(this);
@@ -118,12 +125,14 @@ void MessagePipeReader::OnPipeClosed() {
 }
 
 void MessagePipeReader::OnPipeError(MojoResult error) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (!delegate_)
     return;
   delegate_->OnPipeError(this);
 }
 
 MojoResult MessagePipeReader::ReadMessageBytes() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(handle_buffer_.empty());
 
   uint32_t num_bytes = static_cast<uint32_t>(data_buffer_.size());
@@ -153,6 +162,7 @@ MojoResult MessagePipeReader::ReadMessageBytes() {
 }
 
 void MessagePipeReader::ReadAvailableMessages() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   while (pipe_.is_valid()) {
     MojoResult read_result = ReadMessageBytes();
     if (read_result == MOJO_RESULT_SHOULD_WAIT)
@@ -171,6 +181,7 @@ void MessagePipeReader::ReadAvailableMessages() {
 }
 
 void MessagePipeReader::ReadMessagesThenWait() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   while (true) {
     ReadAvailableMessages();
     if (!pipe_.is_valid())
@@ -197,6 +208,7 @@ void MessagePipeReader::ReadMessagesThenWait() {
 }
 
 void MessagePipeReader::PipeIsReady(MojoResult wait_result) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   CloseWithErrorIfPending();
   if (!IsValid()) {
     // There was a pending error and it closed the pipe.