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

This change does three things:
1. Checks the threading of MessagePipeReader using a ThreadChecker.
2. Eliminates races on |pending_send_error_| by using atomic ops.
3. Documents and removes checks which are no longer valid because Send()
   can be called on a non-IO thread.

BUG=522888,492867
TESTED=Enabled Mojo IPC channel and ran StatsTableBrowserTest.StartWithStatTable
       with tsan 100 times.
       Before: IPC-related races and DCHECK failures
       After: No IPC-related races and DCHECK failures.

Review URL: https://codereview.chromium.org/1318453002

Cr-Commit-Position: refs/heads/master@{#346967}

1 files changed, 22 insertions, 10 deletions

diff --git a/ipc/mojo/ipc_message_pipe_reader.cc b/ipc/mojo/ipc_message_pipe_reader.cc
index 2202a57..6609f26 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());
+  MojoResult pending_error = base::subtle::NoBarrier_Load(&pending_send_error_);
+  if (pending_error == MOJO_RESULT_OK)
     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.
+  CloseWithError(pending_error);
   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.