Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1050)

Unified Diff: ipc/mojo/ipc_message_pipe_reader.cc

Issue 1318453002: Fix races with MessagePipeReader due to the Mojo IPC channel being thread-safe. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Add bug reference. Created 5 years, 4 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « ipc/mojo/ipc_message_pipe_reader.h ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
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..6609f26bd0fbe305e0189f3ca3011e597f861a23 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.
« no previous file with comments | « ipc/mojo/ipc_message_pipe_reader.h ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698