Notify QuicPacketWriter that socket has closed
Previously, when the socket returns ENOBUFS (i.e. socket does not have
enough buffer space), we retry writing the packet to the socket
asynchronously. However, before we actually write, the socket can
become closed due to an network idle timeout.
To fix the issue of writing to an invalidated socket, we need to notify
the writer's socket that it has been closed, when the connection is
closed and all the sockets are closed.
Bug: 1383390
Change-Id: I8b6aa598465c4001cccb22515e2fd44b94fc307d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4570128
Commit-Queue: Nidhi Jaju <nidhijaju@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1149662}
diff --git a/net/quic/quic_chromium_client_session.cc b/net/quic/quic_chromium_client_session.cc
index 5ef9de1..7502daf 100644
--- a/net/quic/quic_chromium_client_session.cc
+++ b/net/quic/quic_chromium_client_session.cc
@@ -2129,9 +2129,18 @@
}
CHECK_EQ(sockets_.size(), packet_readers_.size());
+ bool socket_found_in_writer = false;
for (auto& socket : sockets_) {
socket->Close();
+ // If a writer exists that was not destroyed when the connection migrated,
+ // then that writer may not be notified that its socket has been closed.
+ // We know that the writer is a QuicChromiumPacketWriter since the packet
+ // writer is set with the same type originally.
+ socket_found_in_writer |=
+ static_cast<QuicChromiumPacketWriter*>(connection()->writer())
+ ->OnSocketClosed(std::move(socket));
}
+ CHECK(socket_found_in_writer);
DCHECK(!HasActiveRequestStreams());
CloseAllHandles(ERR_UNEXPECTED);
CancelAllRequests(ERR_CONNECTION_CLOSED);
diff --git a/net/quic/quic_chromium_packet_writer.cc b/net/quic/quic_chromium_packet_writer.cc
index a2b92e0..da272cc2 100644
--- a/net/quic/quic_chromium_packet_writer.cc
+++ b/net/quic/quic_chromium_packet_writer.cc
@@ -145,6 +145,9 @@
quic::WriteResult QuicChromiumPacketWriter::WritePacketToSocketImpl() {
base::TimeTicks now = base::TimeTicks::Now();
+ // When the connection is closed, the socket is cleaned up. If socket is
+ // invalidated, packets should not be written to the socket.
+ CHECK(socket_);
int rv = socket_->Write(packet_.get(), packet_->size(), write_callback_,
kTrafficAnnotation);
@@ -182,9 +185,12 @@
void QuicChromiumPacketWriter::RetryPacketAfterNoBuffers() {
DCHECK_GT(retry_count_, 0);
- quic::WriteResult result = WritePacketToSocketImpl();
- if (result.error_code != ERR_IO_PENDING)
- OnWriteComplete(result.error_code);
+ if (socket_) {
+ quic::WriteResult result = WritePacketToSocketImpl();
+ if (result.error_code != ERR_IO_PENDING) {
+ OnWriteComplete(result.error_code);
+ }
+ }
}
bool QuicChromiumPacketWriter::IsWriteBlocked() const {
@@ -278,4 +284,13 @@
return quic::WriteResult(quic::WRITE_STATUS_OK, 0);
}
+bool QuicChromiumPacketWriter::OnSocketClosed(
+ std::unique_ptr<DatagramClientSocket> socket) {
+ if (socket_.get() == socket.get()) {
+ socket_ = nullptr;
+ return true;
+ }
+ return false;
+}
+
} // namespace net
diff --git a/net/quic/quic_chromium_packet_writer.h b/net/quic/quic_chromium_packet_writer.h
index 4846f9a..a60c168 100644
--- a/net/quic/quic_chromium_packet_writer.h
+++ b/net/quic/quic_chromium_packet_writer.h
@@ -109,6 +109,10 @@
void OnWriteComplete(int rv);
+ // If the writer has enqueued a task to retry, OnSocketClosed() must be called
+ // when the socket is closed to avoid using an invalid socket.
+ bool OnSocketClosed(std::unique_ptr<DatagramClientSocket> socket);
+
private:
void SetPacket(const char* buffer, size_t buf_len);
bool MaybeRetryAfterWriteError(int rv);
