0

Revert to manual ref-counting in mojo::edk::ChannelWin.

Changes to manual ref-counting in a recent change to the timing of
write-error notifications in ChannelWin missed that multiple write
operations may be in-progress at the same time, and so introduced
issues with the lifetime of the ChannelWin object if ShutDown()
while write operations were in-progress.

We also add some DCHECKs to sanity-check the pending-IO flags.

Bug: 821658, 816620
Change-Id: I69cea0a1ed080ca2920738f5212a25d5f5e63c42
Reviewed-on: https://chromium-review.googlesource.com/961941
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543162}
This commit is contained in:
Wez
2018-03-14 19:45:42 +00:00
committed by Commit Bot
parent a4caf65b35
commit 0021badf45

@@ -165,7 +165,8 @@ class ChannelWin : public Channel,
case ERROR_PIPE_CONNECTED: case ERROR_PIPE_CONNECTED:
break; break;
case ERROR_IO_PENDING: case ERROR_IO_PENDING:
is_connect_pending_ = this; is_connect_pending_ = true;
AddRef();
return; return;
case ERROR_NO_DATA: case ERROR_NO_DATA:
default: default:
@@ -221,7 +222,7 @@ class ChannelWin : public Channel,
OnError(Error::kDisconnected); OnError(Error::kDisconnected);
} else if (context == &connect_context_) { } else if (context == &connect_context_) {
DCHECK(is_connect_pending_); DCHECK(is_connect_pending_);
scoped_refptr<ChannelWin> self(std::move(is_connect_pending_)); is_connect_pending_ = false;
ReadMore(0); ReadMore(0);
base::AutoLock lock(write_lock_); base::AutoLock lock(write_lock_);
@@ -230,17 +231,18 @@ class ChannelWin : public Channel,
WriteNextNoLock(); WriteNextNoLock();
} }
} else if (context == &read_context_) { } else if (context == &read_context_) {
scoped_refptr<ChannelWin> self(std::move(is_read_pending_));
OnReadDone(static_cast<size_t>(bytes_transfered)); OnReadDone(static_cast<size_t>(bytes_transfered));
} else { } else {
CHECK(context == &write_context_); CHECK(context == &write_context_);
scoped_refptr<ChannelWin> self(std::move(is_write_pending_));
OnWriteDone(static_cast<size_t>(bytes_transfered)); OnWriteDone(static_cast<size_t>(bytes_transfered));
} }
// |this| may have been deleted by the time we reach here. Release();
} }
void OnReadDone(size_t bytes_read) { void OnReadDone(size_t bytes_read) {
DCHECK(is_read_pending_);
is_read_pending_ = false;
if (bytes_read > 0) { if (bytes_read > 0) {
size_t next_read_size = 0; size_t next_read_size = 0;
if (OnReadComplete(bytes_read, &next_read_size)) { if (OnReadComplete(bytes_read, &next_read_size)) {
@@ -283,6 +285,8 @@ class ChannelWin : public Channel,
} }
void ReadMore(size_t next_read_size_hint) { void ReadMore(size_t next_read_size_hint) {
DCHECK(!is_read_pending_);
size_t buffer_capacity = next_read_size_hint; size_t buffer_capacity = next_read_size_hint;
char* buffer = GetReadBuffer(&buffer_capacity); char* buffer = GetReadBuffer(&buffer_capacity);
DCHECK_GT(buffer_capacity, 0u); DCHECK_GT(buffer_capacity, 0u);
@@ -294,7 +298,8 @@ class ChannelWin : public Channel,
&read_context_.overlapped); &read_context_.overlapped);
if (ok || GetLastError() == ERROR_IO_PENDING) { if (ok || GetLastError() == ERROR_IO_PENDING) {
is_read_pending_ = this; is_read_pending_ = true;
AddRef();
} else { } else {
OnError(Error::kDisconnected); OnError(Error::kDisconnected);
} }
@@ -311,7 +316,7 @@ class ChannelWin : public Channel,
&write_context_.overlapped); &write_context_.overlapped);
if (ok || GetLastError() == ERROR_IO_PENDING) { if (ok || GetLastError() == ERROR_IO_PENDING) {
is_write_pending_ = this; AddRef();
return true; return true;
} }
return false; return false;
@@ -347,9 +352,8 @@ class ChannelWin : public Channel,
base::MessageLoopForIO::IOContext connect_context_; base::MessageLoopForIO::IOContext connect_context_;
base::MessageLoopForIO::IOContext read_context_; base::MessageLoopForIO::IOContext read_context_;
base::MessageLoopForIO::IOContext write_context_; base::MessageLoopForIO::IOContext write_context_;
scoped_refptr<ChannelWin> is_connect_pending_; bool is_connect_pending_ = false;
scoped_refptr<ChannelWin> is_read_pending_; bool is_read_pending_ = false;
scoped_refptr<ChannelWin> is_write_pending_;
// Protects |delay_writes_|, |reject_writes_| and |outgoing_messages_|. // Protects |delay_writes_|, |reject_writes_| and |outgoing_messages_|.
base::Lock write_lock_; base::Lock write_lock_;