Commit 7e2c7b2f authored by sergeyu's avatar sergeyu Committed by Commit bot

Fix BufferedSocketWriter not to return any results from Write().

Previously BufferedSocketWriter was returning false from Write() if
the write fails synchronously. That's redundant because
BufferedSocketWriter also calls a callback after the failed write.

This also fixes a bug in Write() - it was using is_closed() method after
calling the callback, which is not allowed because the object may be
deleted by the callback. That problem was found by tests added in
https://codereview.chromium.org/1258323003 . The bug was introduced
recently in https://codereview.chromium.org/1197853003 .

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

Cr-Commit-Position: refs/heads/master@{#341229}
parent 96bb0fdb
......@@ -57,7 +57,7 @@ void BufferedSocketWriter::Init(
write_failed_callback_ = write_failed_callback;
}
bool BufferedSocketWriter::Write(
void BufferedSocketWriter::Write(
const scoped_refptr<net::IOBufferWithSize>& data,
const base::Closure& done_task) {
DCHECK(thread_checker_.CalledOnValidThread());
......@@ -65,14 +65,12 @@ bool BufferedSocketWriter::Write(
// Don't write after error.
if (is_closed())
return false;
return;
queue_.push_back(new PendingPacket(
new net::DrainableIOBuffer(data.get(), data->size()), done_task));
DoWrite();
return !is_closed();
}
bool BufferedSocketWriter::is_closed() {
......
......@@ -42,9 +42,8 @@ class BufferedSocketWriter {
void Init(const WriteCallback& write_callback,
const WriteFailedCallback& write_failed_callback);
// Puts a new data chunk in the buffer. Returns false if writing has stopped
// because of an error.
bool Write(const scoped_refptr<net::IOBufferWithSize>& buffer,
// Puts a new data chunk in the buffer.
void Write(const scoped_refptr<net::IOBufferWithSize>& buffer,
const base::Closure& done_task);
// Returns true when there is data waiting to be written.
......
......@@ -84,7 +84,7 @@ class ChannelMultiplexer::MuxChannel {
// Called by MuxSocket.
void OnSocketDestroyed();
bool DoWrite(scoped_ptr<MultiplexPacket> packet,
void DoWrite(scoped_ptr<MultiplexPacket> packet,
const base::Closure& done_task);
int DoRead(const scoped_refptr<net::IOBuffer>& buffer, int buffer_len);
......@@ -182,7 +182,7 @@ void ChannelMultiplexer::MuxChannel::OnSocketDestroyed() {
socket_ = nullptr;
}
bool ChannelMultiplexer::MuxChannel::DoWrite(
void ChannelMultiplexer::MuxChannel::DoWrite(
scoped_ptr<MultiplexPacket> packet,
const base::Closure& done_task) {
packet->set_channel_id(send_id_);
......@@ -190,7 +190,7 @@ bool ChannelMultiplexer::MuxChannel::DoWrite(
packet->set_channel_name(name_);
id_sent_ = true;
}
return multiplexer_->DoWrite(packet.Pass(), done_task);
multiplexer_->DoWrite(packet.Pass(), done_task);
}
int ChannelMultiplexer::MuxChannel::DoRead(
......@@ -256,14 +256,9 @@ int ChannelMultiplexer::MuxSocket::Write(
packet->mutable_data()->assign(buffer->data(), size);
write_pending_ = true;
bool result = channel_->DoWrite(packet.Pass(), base::Bind(
channel_->DoWrite(packet.Pass(), base::Bind(
&ChannelMultiplexer::MuxSocket::OnWriteComplete, AsWeakPtr()));
if (!result) {
// Cannot complete the write, e.g. if the connection has been terminated.
return net::ERR_FAILED;
}
// OnWriteComplete() might be called above synchronously.
if (write_pending_) {
DCHECK(write_callback_.is_null());
......@@ -465,9 +460,9 @@ void ChannelMultiplexer::OnIncomingPacket(scoped_ptr<MultiplexPacket> packet,
channel->OnIncomingPacket(packet.Pass(), done_task);
}
bool ChannelMultiplexer::DoWrite(scoped_ptr<MultiplexPacket> packet,
void ChannelMultiplexer::DoWrite(scoped_ptr<MultiplexPacket> packet,
const base::Closure& done_task) {
return writer_.Write(SerializeAndFrameMessage(*packet), done_task);
writer_.Write(SerializeAndFrameMessage(*packet), done_task);
}
} // namespace protocol
......
......@@ -56,7 +56,7 @@ class ChannelMultiplexer : public StreamChannelFactory {
const base::Closure& done_task);
// Called by MuxChannel.
bool DoWrite(scoped_ptr<MultiplexPacket> packet,
void DoWrite(scoped_ptr<MultiplexPacket> packet,
const base::Closure& done_task);
// Factory used to create |base_channel_|. Set to nullptr once creation is
......
......@@ -254,18 +254,15 @@ TEST_F(ChannelMultiplexerTest, WriteFailSync) {
MockSocketCallback cb1;
MockSocketCallback cb2;
EXPECT_CALL(cb1, OnDone(net::ERR_FAILED));
EXPECT_CALL(cb2, OnDone(net::ERR_FAILED));
EXPECT_CALL(cb1, OnDone(_))
.Times(0);
EXPECT_CALL(cb2, OnDone(_))
.Times(0);
EXPECT_EQ(net::ERR_FAILED,
EXPECT_EQ(net::ERR_IO_PENDING,
host_socket1_->Write(buf.get(),
buf->size(),
base::Bind(&MockSocketCallback::OnDone,
base::Unretained(&cb1))));
EXPECT_EQ(net::ERR_FAILED,
EXPECT_EQ(net::ERR_IO_PENDING,
host_socket2_->Write(buf.get(),
buf->size(),
base::Bind(&MockSocketCallback::OnDone,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment