Ensure QuicChromiumPacketWriter isn't still tracked when deleted. #4691
Conversation
This moves the WeakPtr instantiation that detects object destruction closer to the evaluation. b/388316262 b/388616504
Attempt to address increased crashes in the QuicChromiumPacketWriter destructor. The QuicChromiumPacketWriter destructor crashes seem to indicate that there is a double free. The destructor is called from both the QuicConnection destructor and during QuicConnection::MigratePath. A double free appears to only be possible after MigratePath is called with the same writer as currently used, but on a connection that is not connected. The increase possibly means a race between a connection teardown and migration was made more likely to occur as a result of recent performance improvements. This adds a check and pointer reset that should avoid having a stale QuicChromiumPacketWriter pointer in the QuicConnection. b/388316262
| @@ -2808,12 +2808,14 @@ void QuicConnection::ProcessUdpPacket(const QuicSocketAddress& self_address, | |||
| } | |||
|
|
|||
| void QuicConnection::OnBlockedWriterCanWrite() { | |||
| writer_->SetWritable(); | |||
| OnCanWrite(); | |||
| if (writer_) { | |||
There was a problem hiding this comment.
tiny style inconsistency nitpick: Above you are writing out writer != nullptr but here you do a simple boolean check. I don't think it matters either way, but would be good to keep consistent style.
There was a problem hiding this comment.
I followed the pre-existing pattern already used elsewhere in this file for consistency
| @@ -411,7 +411,7 @@ void QuicConnection::InstallInitialCrypters(QuicConnectionId connection_id) { | |||
|
|
|||
| QuicConnection::~QuicConnection() { | |||
| QUICHE_DCHECK_GE(stats_.max_egress_mtu, long_term_mtu_); | |||
| if (owns_writer_) { | |||
| if (writer_ != nullptr && owns_writer_) { | |||
| delete writer_; | |||
There was a problem hiding this comment.
Should it do owns_writer_ = false; here as well just in case ?
There was a problem hiding this comment.
I considered doing that, but elsewhere that boolean is only ever used when the pointer is not nullptr.
| struct Socket::ReadPacketResult* read_packet = read_results_.packets; | ||
| for (int p = 0; p < read_results_.result; ++p, ++read_packet) { | ||
| if (read_packet->result <= 0) { | ||
| continue; | ||
| } | ||
| quic::QuicReceivedPacket packet(read_packet->buffer, read_packet->result, | ||
| clock_->ApproximateNow()); | ||
| auto self = weak_factory_.GetWeakPtr(); |
There was a problem hiding this comment.
It shouldn't matter where you call this. Does the weak pointer need to be invalidated before the QuicChromiumPacketReader instance gets destroyed? If you so, you can call weak_factory_.InvalidateWeakPtrs().
Every time the weak pointer is treated like a bool, the WeakReference is checked to see if it's still valid.
Attempt to address increased crashes in the QuicChromiumPacketWriter
destructor.
The QuicChromiumPacketWriter destructor crashes seem to indicate that
there is a double free. The destructor is called from both the
QuicConnection destructor and during QuicConnection::MigratePath.
A double free appears to only be possible after MigratePath is called
with the same writer as currently used, but on a connection that is
not connected.
The increase possibly means a race between a connection teardown and
migration was made more likely to occur as a result of recent
performance improvements.
This adds a check and pointer reset that should avoid having a stale
QuicChromiumPacketWriter pointer in the QuicConnection.
This also moves the WeakPtr instantiation that detects object destruction
closer to the evaluation.
b/388316262
b/388616504