Let quic::serializedpacket to own the frames and optionally the encrypted_buffer. no behavior change. This change removes quic::ClearSerializedPacket() and quic::OwningSerializedPacketPointer. Their functionality have be moved to the destructor of quic::SerializedPacket. To simplify ownership, I changed quic::SerializedPacket to move-only and changed many functions in QuicPacketCreator and QuicConnection to take a SerializedPacket object instead of a pointer. The optional ownership of encrypted_buffer is expressed using the newly added SerializedPacket.release_encrypted_buffer function. Currently only connectivity probing packets are setting it. In the next flag-protected change, I'll use it to free writer-allocated buffers. PiperOrigin-RevId: 311381784 Change-Id: Icea678c488c4f2af1397ce82ecdf715b3d9f5407
diff --git a/quic/core/quic_coalesced_packet.cc b/quic/core/quic_coalesced_packet.cc index 6393f5d..c18369e 100644 --- a/quic/core/quic_coalesced_packet.cc +++ b/quic/core/quic_coalesced_packet.cc
@@ -90,9 +90,6 @@ for (auto& packet : encrypted_buffers_) { packet.clear(); } - if (initial_packet_ != nullptr) { - ClearSerializedPacket(initial_packet_.get()); - } initial_packet_ = nullptr; }
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 61119ad..1832092 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -2299,7 +2299,7 @@ // Termination packets are encrypted and saved, so don't exit early. const bool is_termination_packet = IsTerminationPacket(*packet); QuicPacketNumber packet_number = packet->packet_number; - QuicPacketLength encrypted_length = packet->encrypted_length; + const QuicPacketLength encrypted_length = packet->encrypted_length; // Termination packets are eventually owned by TimeWaitListManager. // Others are deleted at the end of this call. if (is_termination_packet) { @@ -2422,8 +2422,8 @@ // When MSG_TOO_BIG is returned, the system typically knows what the // actual MTU is, so there is no need to probe further. // TODO(wub): Reduce max packet size to a safe default, or the actual MTU. - QUIC_DVLOG(1) << ENDPOINT << " MTU probe packet too big, size:" - << packet->encrypted_length + QUIC_DVLOG(1) << ENDPOINT + << " MTU probe packet too big, size:" << encrypted_length << ", long_term_mtu_:" << long_term_mtu_; mtu_discoverer_.Disable(); mtu_discovery_alarm_->Cancel(); @@ -2487,7 +2487,7 @@ if (EnforceAntiAmplificationLimit()) { // Include bytes sent even if they are not in flight. - bytes_sent_before_address_validation_ += packet->encrypted_length; + bytes_sent_before_address_validation_ += encrypted_length; } const bool in_flight = sent_packet_manager_.OnPacketSent( @@ -2623,8 +2623,8 @@ return writer_->GetNextWriteLocation(self_address().host(), peer_address()); } -void QuicConnection::OnSerializedPacket(SerializedPacket* serialized_packet) { - if (serialized_packet->encrypted_buffer == nullptr) { +void QuicConnection::OnSerializedPacket(SerializedPacket serialized_packet) { + if (serialized_packet.encrypted_buffer == nullptr) { // We failed to serialize the packet, so close the connection. // Specify that the close is silent, that no packet be sent, so no infinite // loop here. @@ -2643,14 +2643,14 @@ return; } - if (serialized_packet->retransmittable_frames.empty()) { + if (serialized_packet.retransmittable_frames.empty()) { // Increment consecutive_num_packets_with_no_retransmittable_frames_ if // this packet is a new transmission with no retransmittable frames. ++consecutive_num_packets_with_no_retransmittable_frames_; } else { consecutive_num_packets_with_no_retransmittable_frames_ = 0; } - SendOrQueuePacket(serialized_packet); + SendOrQueuePacket(std::move(serialized_packet)); } void QuicConnection::OnUnrecoverableError(QuicErrorCode error, @@ -2709,14 +2709,9 @@ kAlarmGranularity); } -void QuicConnection::SendOrQueuePacket(SerializedPacket* packet) { +void QuicConnection::SendOrQueuePacket(SerializedPacket packet) { // The caller of this function is responsible for checking CanWrite(). - if (packet->encrypted_buffer == nullptr) { - QUIC_BUG << "packet.encrypted_buffer == nullptr in to SendOrQueuePacket"; - return; - } - WritePacket(packet); - ClearSerializedPacket(packet); + WritePacket(&packet); } void QuicConnection::OnPingTimeout() { @@ -3568,7 +3563,7 @@ << "Sending path probe packet for connection_id = " << server_connection_id_; - OwningSerializedPacketPointer probing_packet; + std::unique_ptr<SerializedPacket> probing_packet; if (!version().HasIetfQuicFrames()) { // Non-IETF QUIC, generate a padded ping regardless of whether this is a // request or a response.
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h index 08f44ff..6586fb2 100644 --- a/quic/core/quic_connection.h +++ b/quic/core/quic_connection.h
@@ -588,7 +588,7 @@ IsHandshake handshake) override; const QuicFrames MaybeBundleAckOpportunistically() override; char* GetPacketBuffer() override; - void OnSerializedPacket(SerializedPacket* packet) override; + void OnSerializedPacket(SerializedPacket packet) override; void OnUnrecoverableError(QuicErrorCode error, const std::string& error_details) override; @@ -971,7 +971,7 @@ // Send a packet to the peer, and takes ownership of the packet if the packet // cannot be written immediately. - virtual void SendOrQueuePacket(SerializedPacket* packet); + virtual void SendOrQueuePacket(SerializedPacket packet); // Called after a packet is received from a new effective peer address and is // decrypted. Starts validation of effective peer's address change. Calls
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index ffe43c6..a5a0a59 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -674,7 +674,7 @@ serialized_packet.retransmittable_frames.push_back( QuicFrame(QuicPingFrame())); } - OnSerializedPacket(&serialized_packet); + OnSerializedPacket(std::move(serialized_packet)); } QuicConsumedData SaveAndSendStreamData(QuicStreamId id, @@ -1160,6 +1160,15 @@ } } + QuicFrame MakeCryptoFrame() const { + if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { + return QuicFrame(new QuicCryptoFrame(crypto_frame_)); + } + return QuicFrame(QuicStreamFrame( + QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, + 0u, quiche::QuicheStringPiece())); + } + void ProcessFramePacket(QuicFrame frame) { ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); } @@ -1506,7 +1515,7 @@ return ConstructPacket(header, frames); } - OwningSerializedPacketPointer ConstructProbingPacket() { + std::unique_ptr<SerializedPacket> ConstructProbingPacket() { if (VersionHasIetfQuicFrames(version().transport_version)) { QuicPathFrameBuffer payload = { {0xde, 0xad, 0xbe, 0xef, 0xba, 0xdc, 0x0f, 0xfe}}; @@ -1780,17 +1789,13 @@ EXPECT_EQ(Perspective::IS_CLIENT, connection_.perspective()); EXPECT_TRUE(connection_.connected()); - QuicFrame frame; if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { - frame = QuicFrame(&crypto_frame_); EXPECT_CALL(visitor_, OnCryptoFrame(_)); } else { - frame = QuicFrame(QuicStreamFrame( - QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, - 0u, quiche::QuicheStringPiece())); EXPECT_CALL(visitor_, OnStreamFrame(_)); } - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); // Cause change in self_address. QuicIpAddress host; host.FromString("1.1.1.1"); @@ -1800,7 +1805,8 @@ } else { EXPECT_CALL(visitor_, OnStreamFrame(_)); } - ProcessFramePacketWithAddresses(frame, self_address, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), self_address, + kPeerAddress); EXPECT_TRUE(connection_.connected()); } @@ -1811,24 +1817,21 @@ EXPECT_EQ(Perspective::IS_SERVER, connection_.perspective()); EXPECT_TRUE(connection_.connected()); - QuicFrame frame; if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { - frame = QuicFrame(&crypto_frame_); EXPECT_CALL(visitor_, OnCryptoFrame(_)); } else { - frame = QuicFrame(QuicStreamFrame( - QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, - 0u, quiche::QuicheStringPiece())); EXPECT_CALL(visitor_, OnStreamFrame(_)); } - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); // Cause change in self_address. QuicIpAddress host; host.FromString("1.1.1.1"); QuicSocketAddress self_address(host, 123); EXPECT_CALL(visitor_, AllowSelfAddressChange()).WillOnce(Return(false)); EXPECT_CALL(visitor_, OnConnectionClosed(_, _)); - ProcessFramePacketWithAddresses(frame, self_address, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), self_address, + kPeerAddress); EXPECT_FALSE(connection_.connected()); TestConnectionCloseQuicErrorCode(QUIC_ERROR_MIGRATING_ADDRESS); } @@ -1840,29 +1843,27 @@ EXPECT_EQ(Perspective::IS_SERVER, connection_.perspective()); EXPECT_TRUE(connection_.connected()); - QuicFrame frame; if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { - frame = QuicFrame(&crypto_frame_); EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(3); } else { - frame = QuicFrame(QuicStreamFrame( - QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, - 0u, quiche::QuicheStringPiece())); EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(3); } QuicIpAddress host; host.FromString("1.1.1.1"); QuicSocketAddress self_address1(host, 443); - ProcessFramePacketWithAddresses(frame, self_address1, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), self_address1, + kPeerAddress); // Cause self_address change to mapped Ipv4 address. QuicIpAddress host2; host2.FromString(quiche::QuicheStrCat( "::ffff:", connection_.self_address().host().ToString())); QuicSocketAddress self_address2(host2, connection_.self_address().port()); - ProcessFramePacketWithAddresses(frame, self_address2, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), self_address2, + kPeerAddress); EXPECT_TRUE(connection_.connected()); // self_address change back to Ipv4 address. - ProcessFramePacketWithAddresses(frame, self_address1, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), self_address1, + kPeerAddress); EXPECT_TRUE(connection_.connected()); } @@ -1877,21 +1878,17 @@ QuicConnectionPeer::SetEffectivePeerAddress(&connection_, QuicSocketAddress()); - QuicFrame frame; if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { - frame = QuicFrame(&crypto_frame_); EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber()); } else { - frame = QuicFrame(QuicStreamFrame( - QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, - 0u, quiche::QuicheStringPiece())); EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber()); } QuicPacketCreatorPeer::SetPacketNumber(&peer_creator_, 5); const QuicSocketAddress kNewPeerAddress = QuicSocketAddress(QuicIpAddress::Loopback6(), /*port=*/23456); - ProcessFramePacketWithAddresses(frame, kSelfAddress, kNewPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kNewPeerAddress); EXPECT_EQ(kNewPeerAddress, connection_.peer_address()); EXPECT_EQ(kNewPeerAddress, connection_.effective_peer_address()); @@ -1899,7 +1896,8 @@ QuicPacketCreatorPeer::SetPacketNumber(&peer_creator_, 4); // This is an old packet, do not migrate. EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0); - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); EXPECT_EQ(kNewPeerAddress, connection_.peer_address()); EXPECT_EQ(kNewPeerAddress, connection_.effective_peer_address()); } @@ -1917,17 +1915,13 @@ QuicSocketAddress()); EXPECT_FALSE(connection_.effective_peer_address().IsInitialized()); - QuicFrame frame; if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { - frame = QuicFrame(&crypto_frame_); EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber()); } else { - frame = QuicFrame(QuicStreamFrame( - QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, - 0u, quiche::QuicheStringPiece())); EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber()); } - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); EXPECT_EQ(kPeerAddress, connection_.peer_address()); EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); @@ -1936,7 +1930,8 @@ const QuicSocketAddress kNewPeerAddress = QuicSocketAddress(QuicIpAddress::Loopback6(), /*port=*/23456); EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(1); - ProcessFramePacketWithAddresses(frame, kSelfAddress, kNewPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kNewPeerAddress); EXPECT_EQ(kNewPeerAddress, connection_.peer_address()); EXPECT_EQ(kNewPeerAddress, connection_.effective_peer_address()); } @@ -1956,17 +1951,13 @@ QuicSocketAddress(QuicIpAddress::Loopback6(), /*port=*/43210); connection_.ReturnEffectivePeerAddressForNextPacket(kEffectivePeerAddress); - QuicFrame frame; if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { - frame = QuicFrame(&crypto_frame_); EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber()); } else { - frame = QuicFrame(QuicStreamFrame( - QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, - 0u, quiche::QuicheStringPiece())); EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber()); } - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); EXPECT_EQ(kPeerAddress, connection_.peer_address()); EXPECT_EQ(kEffectivePeerAddress, connection_.effective_peer_address()); @@ -1976,7 +1967,8 @@ QuicSocketAddress(QuicIpAddress::Loopback6(), /*port=*/54321); connection_.ReturnEffectivePeerAddressForNextPacket(kNewEffectivePeerAddress); EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(1); - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); EXPECT_EQ(kPeerAddress, connection_.peer_address()); EXPECT_EQ(kNewEffectivePeerAddress, connection_.effective_peer_address()); @@ -2005,7 +1997,8 @@ connection_.ReturnEffectivePeerAddressForNextPacket( kNewerEffectivePeerAddress); EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(1); - ProcessFramePacketWithAddresses(frame, kSelfAddress, kFinalPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kFinalPeerAddress); EXPECT_EQ(kFinalPeerAddress, connection_.peer_address()); EXPECT_EQ(kNewerEffectivePeerAddress, connection_.effective_peer_address()); EXPECT_EQ(PORT_CHANGE, connection_.active_effective_peer_migration_type()); @@ -2019,7 +2012,8 @@ kNewestEffectivePeerAddress); EXPECT_CALL(visitor_, OnConnectionMigration(IPV6_TO_IPV4_CHANGE)).Times(1); EXPECT_CALL(*send_algorithm_, OnConnectionMigration()).Times(1); - ProcessFramePacketWithAddresses(frame, kSelfAddress, kFinalPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kFinalPeerAddress); EXPECT_EQ(kFinalPeerAddress, connection_.peer_address()); EXPECT_EQ(kNewestEffectivePeerAddress, connection_.effective_peer_address()); EXPECT_EQ(IPV6_TO_IPV4_CHANGE, @@ -2039,17 +2033,13 @@ QuicSocketAddress()); EXPECT_FALSE(connection_.effective_peer_address().IsInitialized()); - QuicFrame frame; if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { - frame = QuicFrame(&crypto_frame_); EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber()); } else { - frame = QuicFrame(QuicStreamFrame( - QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, - 0u, quiche::QuicheStringPiece())); EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber()); } - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); EXPECT_EQ(kPeerAddress, connection_.peer_address()); EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); @@ -2058,7 +2048,7 @@ // Process a padded PING or PATH CHALLENGE packet with no peer address change // on server side will be ignored. - OwningSerializedPacketPointer probing_packet = ConstructProbingPacket(); + std::unique_ptr<SerializedPacket> probing_packet = ConstructProbingPacket(); std::unique_ptr<QuicReceivedPacket> received(ConstructReceivedPacket( QuicEncryptedPacket(probing_packet->encrypted_buffer, @@ -2141,17 +2131,13 @@ QuicSocketAddress()); EXPECT_FALSE(connection_.effective_peer_address().IsInitialized()); - QuicFrame frame; if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { - frame = QuicFrame(&crypto_frame_); EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber()); } else { - frame = QuicFrame(QuicStreamFrame( - QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, - 0u, quiche::QuicheStringPiece())); EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber()); } - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); EXPECT_EQ(kPeerAddress, connection_.peer_address()); EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); @@ -2168,7 +2154,7 @@ const QuicSocketAddress kNewPeerAddress = QuicSocketAddress(QuicIpAddress::Loopback6(), /*port=*/23456); - OwningSerializedPacketPointer probing_packet = ConstructProbingPacket(); + std::unique_ptr<SerializedPacket> probing_packet = ConstructProbingPacket(); std::unique_ptr<QuicReceivedPacket> received(ConstructReceivedPacket( QuicEncryptedPacket(probing_packet->encrypted_buffer, probing_packet->encrypted_length), @@ -2186,7 +2172,8 @@ // Process another packet with the old peer address on server side will not // start peer migration. EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0); - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); EXPECT_EQ(kPeerAddress, connection_.peer_address()); EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); } @@ -2205,17 +2192,13 @@ QuicSocketAddress()); EXPECT_FALSE(connection_.effective_peer_address().IsInitialized()); - QuicFrame frame; if (GetParam().version.UsesCryptoFrames()) { - frame = QuicFrame(&crypto_frame_); EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber()); } else { - frame = QuicFrame(QuicStreamFrame( - QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, - 0u, quiche::QuicheStringPiece())); EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber()); } - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); EXPECT_EQ(kPeerAddress, connection_.peer_address()); EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); @@ -2269,7 +2252,8 @@ } else { EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0); } - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); EXPECT_EQ(kPeerAddress, connection_.peer_address()); EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); } @@ -2287,18 +2271,14 @@ QuicSocketAddress()); EXPECT_FALSE(connection_.effective_peer_address().IsInitialized()); - QuicFrame frame; if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { - frame = QuicFrame(&crypto_frame_); EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber()); } else { - frame = QuicFrame(QuicStreamFrame( - QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, - 0u, quiche::QuicheStringPiece())); EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber()); } QuicPacketCreatorPeer::SetPacketNumber(&peer_creator_, 5); - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); EXPECT_EQ(kPeerAddress, connection_.peer_address()); EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); @@ -2320,7 +2300,7 @@ const QuicSocketAddress kNewPeerAddress = QuicSocketAddress(QuicIpAddress::Loopback6(), /*port=*/23456); - OwningSerializedPacketPointer probing_packet = ConstructProbingPacket(); + std::unique_ptr<SerializedPacket> probing_packet = ConstructProbingPacket(); std::unique_ptr<QuicReceivedPacket> received(ConstructReceivedPacket( QuicEncryptedPacket(probing_packet->encrypted_buffer, probing_packet->encrypted_length), @@ -2349,17 +2329,13 @@ QuicSocketAddress()); EXPECT_FALSE(connection_.effective_peer_address().IsInitialized()); - QuicFrame frame; if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { - frame = QuicFrame(&crypto_frame_); EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber()); } else { - frame = QuicFrame(QuicStreamFrame( - QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, - 0u, quiche::QuicheStringPiece())); EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber()); } - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); EXPECT_EQ(kPeerAddress, connection_.peer_address()); EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); @@ -2377,7 +2353,7 @@ const QuicSocketAddress kNewPeerAddress = QuicSocketAddress(QuicIpAddress::Loopback6(), /*port=*/23456); - OwningSerializedPacketPointer probing_packet = ConstructProbingPacket(); + std::unique_ptr<SerializedPacket> probing_packet = ConstructProbingPacket(); std::unique_ptr<QuicReceivedPacket> received(ConstructReceivedPacket( QuicEncryptedPacket(probing_packet->encrypted_buffer, probing_packet->encrypted_length), @@ -2390,7 +2366,8 @@ // side will start peer migration. EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(1); - ProcessFramePacketWithAddresses(frame, kSelfAddress, kNewPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kNewPeerAddress); EXPECT_EQ(kNewPeerAddress, connection_.peer_address()); EXPECT_EQ(kNewPeerAddress, connection_.effective_peer_address()); } @@ -2408,17 +2385,13 @@ QuicSocketAddress()); EXPECT_FALSE(connection_.effective_peer_address().IsInitialized()); - QuicFrame frame; if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { - frame = QuicFrame(&crypto_frame_); EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber()); } else { - frame = QuicFrame(QuicStreamFrame( - QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, - 0u, quiche::QuicheStringPiece())); EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber()); } - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); EXPECT_EQ(kPeerAddress, connection_.peer_address()); EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); @@ -2427,7 +2400,7 @@ EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0); EXPECT_CALL(visitor_, OnPacketReceived(_, _, false)).Times(1); - OwningSerializedPacketPointer probing_packet = ConstructProbingPacket(); + std::unique_ptr<SerializedPacket> probing_packet = ConstructProbingPacket(); std::unique_ptr<QuicReceivedPacket> received(ConstructReceivedPacket( QuicEncryptedPacket(probing_packet->encrypted_buffer, probing_packet->encrypted_length), @@ -2461,17 +2434,13 @@ QuicSocketAddress()); EXPECT_FALSE(connection_.effective_peer_address().IsInitialized()); - QuicFrame frame; if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { - frame = QuicFrame(&crypto_frame_); EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber()); } else { - frame = QuicFrame(QuicStreamFrame( - QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, - 0u, quiche::QuicheStringPiece())); EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber()); } - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); EXPECT_EQ(kPeerAddress, connection_.peer_address()); EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); @@ -2489,7 +2458,7 @@ const QuicSocketAddress kNewSelfAddress = QuicSocketAddress(QuicIpAddress::Loopback6(), /*port=*/23456); - OwningSerializedPacketPointer probing_packet = ConstructProbingPacket(); + std::unique_ptr<SerializedPacket> probing_packet = ConstructProbingPacket(); std::unique_ptr<QuicReceivedPacket> received(ConstructReceivedPacket( QuicEncryptedPacket(probing_packet->encrypted_buffer, probing_packet->encrypted_length), @@ -2517,17 +2486,13 @@ QuicSocketAddress()); EXPECT_FALSE(connection_.effective_peer_address().IsInitialized()); - QuicFrame frame; if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { - frame = QuicFrame(&crypto_frame_); EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber()); } else { - frame = QuicFrame(QuicStreamFrame( - QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, - 0u, quiche::QuicheStringPiece())); EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber()); } - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); EXPECT_EQ(kPeerAddress, connection_.peer_address()); EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); @@ -2536,7 +2501,8 @@ const QuicSocketAddress kNewPeerAddress = QuicSocketAddress(QuicIpAddress::Loopback6(), /*port=*/23456); EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0); - ProcessFramePacketWithAddresses(frame, kSelfAddress, kNewPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kNewPeerAddress); EXPECT_EQ(kNewPeerAddress, connection_.peer_address()); EXPECT_EQ(kNewPeerAddress, connection_.effective_peer_address()); } @@ -7382,31 +7348,31 @@ EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_)); - QuicGoAwayFrame goaway; - goaway.last_good_stream_id = 1; - goaway.error_code = QUIC_PEER_GOING_AWAY; - goaway.reason_phrase = "Going away."; + QuicGoAwayFrame* goaway = new QuicGoAwayFrame(); + goaway->last_good_stream_id = 1; + goaway->error_code = QUIC_PEER_GOING_AWAY; + goaway->reason_phrase = "Going away."; EXPECT_CALL(visitor_, OnGoAway(_)); - ProcessGoAwayPacket(&goaway); + ProcessGoAwayPacket(goaway); } TEST_P(QuicConnectionTest, WindowUpdate) { EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_)); - QuicWindowUpdateFrame window_update; - window_update.stream_id = 3; - window_update.max_data = 1234; + QuicWindowUpdateFrame* window_update = new QuicWindowUpdateFrame(); + window_update->stream_id = 3; + window_update->max_data = 1234; EXPECT_CALL(visitor_, OnWindowUpdateFrame(_)); - ProcessFramePacket(QuicFrame(&window_update)); + ProcessFramePacket(QuicFrame(window_update)); } TEST_P(QuicConnectionTest, Blocked) { EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_)); - QuicBlockedFrame blocked; - blocked.stream_id = 3; + QuicBlockedFrame* blocked = new QuicBlockedFrame(); + blocked->stream_id = 3; EXPECT_CALL(visitor_, OnBlockedFrame(_)); - ProcessFramePacket(QuicFrame(&blocked)); + ProcessFramePacket(QuicFrame(blocked)); EXPECT_EQ(1u, connection_.GetStats().blocked_frames_received); EXPECT_EQ(0u, connection_.GetStats().blocked_frames_sent); } @@ -7684,11 +7650,11 @@ EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_)); // Send a WINDOW_UPDATE frame. - QuicWindowUpdateFrame window_update; - window_update.stream_id = 3; - window_update.max_data = 1234; + QuicWindowUpdateFrame* window_update = new QuicWindowUpdateFrame(); + window_update->stream_id = 3; + window_update->max_data = 1234; EXPECT_CALL(visitor_, OnWindowUpdateFrame(_)); - ProcessFramePacket(QuicFrame(&window_update)); + ProcessFramePacket(QuicFrame(window_update)); // Ensure that this has caused the ACK alarm to be set. QuicAlarm* ack_alarm = QuicConnectionPeer::GetAckAlarm(&connection_); @@ -7699,10 +7665,10 @@ EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_)); // Send a BLOCKED frame. - QuicBlockedFrame blocked; - blocked.stream_id = 3; + QuicBlockedFrame* blocked = new QuicBlockedFrame(); + blocked->stream_id = 3; EXPECT_CALL(visitor_, OnBlockedFrame(_)); - ProcessFramePacket(QuicFrame(&blocked)); + ProcessFramePacket(QuicFrame(blocked)); // Ensure that this has caused the ACK alarm to be set. QuicAlarm* ack_alarm = QuicConnectionPeer::GetAckAlarm(&connection_); @@ -9304,17 +9270,13 @@ return; } set_perspective(Perspective::IS_SERVER); - QuicFrame frame; if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { - frame = QuicFrame(&crypto_frame_); EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(1); } else { - frame = QuicFrame(QuicStreamFrame( - QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, - 0u, quiche::QuicheStringPiece())); EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(1); } - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); // Let connection process a Google QUIC packet. peer_framer_.set_version_for_tests( @@ -9625,20 +9587,16 @@ /*transport_close_frame_type=*/0)); // Received 2 packets. - QuicFrame frame; if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { - frame = QuicFrame(&crypto_frame_); EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber()); } else { - frame = QuicFrame(QuicStreamFrame( - QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, - 0u, quiche::QuicheStringPiece())); EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber()); } - ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kPeerAddress); QuicAlarm* ack_alarm = QuicConnectionPeer::GetAckAlarm(&connection_); EXPECT_TRUE(ack_alarm->IsSet()); - ProcessFramePacketWithAddresses(QuicFrame(connection_close_frame.get()), + ProcessFramePacketWithAddresses(QuicFrame(connection_close_frame.release()), kSelfAddress, kPeerAddress); // Verify ack alarm is not set. EXPECT_FALSE(ack_alarm->IsSet());
diff --git a/quic/core/quic_dispatcher.cc b/quic/core/quic_dispatcher.cc index ffea9ff..c0d3970 100644 --- a/quic/core/quic_dispatcher.cc +++ b/quic/core/quic_dispatcher.cc
@@ -61,14 +61,11 @@ ~PacketCollector() override = default; // QuicPacketCreator::DelegateInterface methods: - void OnSerializedPacket(SerializedPacket* serialized_packet) override { + void OnSerializedPacket(SerializedPacket serialized_packet) override { // Make a copy of the serialized packet to send later. packets_.emplace_back( - new QuicEncryptedPacket(CopyBuffer(*serialized_packet), - serialized_packet->encrypted_length, true)); - serialized_packet->encrypted_buffer = nullptr; - DeleteFrames(&(serialized_packet->retransmittable_frames)); - serialized_packet->retransmittable_frames.clear(); + new QuicEncryptedPacket(CopyBuffer(serialized_packet), + serialized_packet.encrypted_length, true)); } char* GetPacketBuffer() override { @@ -571,35 +568,35 @@ QuicDispatcher::QuicPacketFate QuicDispatcher::ValidityChecks( const ReceivedPacketInfo& packet_info) { if (!packet_info.version_flag) { - // The Android network conformance test contains a UDP test that sends a - // 12-byte packet with the following format: - // - 0x0c (public flags: 8-byte connection ID, 1-byte packet number) - // - randomized 8-byte connection ID - // - 0x01 (1-byte packet number) - // - 0x00 (private flags) - // - 0x07 (PING frame). - // That packet is invalid and we would normally drop it but in order to - // unblock this conformance testing we have the following workaround that - // will be removed once the fixed test is deployed. - // TODO(b/139691956) Remove this workaround once fixed test is deployed. - if (!GetQuicReloadableFlag( - quic_remove_android_conformance_test_workaround) && - packet_info.packet.length() == 12 && - packet_info.packet.data()[0] == 0x0c && - packet_info.packet.data()[9] == 0x01 && - packet_info.packet.data()[10] == 0x00 && - packet_info.packet.data()[11] == 0x07) { - QUIC_DLOG(INFO) << "Received Android UDP network conformance test " - "packet with connection ID " - << packet_info.destination_connection_id; - // Respond with a public reset that the test will know how to parse - // then return kFateDrop to stop processing of this packet. - time_wait_list_manager()->SendPublicReset( - packet_info.self_address, packet_info.peer_address, - packet_info.destination_connection_id, - /*ietf_quic=*/false, GetPerPacketContext()); - return kFateDrop; - } + // The Android network conformance test contains a UDP test that sends a + // 12-byte packet with the following format: + // - 0x0c (public flags: 8-byte connection ID, 1-byte packet number) + // - randomized 8-byte connection ID + // - 0x01 (1-byte packet number) + // - 0x00 (private flags) + // - 0x07 (PING frame). + // That packet is invalid and we would normally drop it but in order to + // unblock this conformance testing we have the following workaround that + // will be removed once the fixed test is deployed. + // TODO(b/139691956) Remove this workaround once fixed test is deployed. + if (!GetQuicReloadableFlag( + quic_remove_android_conformance_test_workaround) && + packet_info.packet.length() == 12 && + packet_info.packet.data()[0] == 0x0c && + packet_info.packet.data()[9] == 0x01 && + packet_info.packet.data()[10] == 0x00 && + packet_info.packet.data()[11] == 0x07) { + QUIC_DLOG(INFO) << "Received Android UDP network conformance test " + "packet with connection ID " + << packet_info.destination_connection_id; + // Respond with a public reset that the test will know how to parse + // then return kFateDrop to stop processing of this packet. + time_wait_list_manager()->SendPublicReset( + packet_info.self_address, packet_info.peer_address, + packet_info.destination_connection_id, + /*ietf_quic=*/false, GetPerPacketContext()); + return kFateDrop; + } QUIC_DLOG(INFO) << "Packet without version arrived for unknown connection ID "
diff --git a/quic/core/quic_packet_creator.cc b/quic/core/quic_packet_creator.cc index a9d7076..0ae78d2 100644 --- a/quic/core/quic_packet_creator.cc +++ b/quic/core/quic_packet_creator.cc
@@ -460,7 +460,7 @@ SerializedPacket packet(std::move(packet_)); ClearPacket(); RemoveSoftMaxPacketLength(); - delegate_->OnSerializedPacket(&packet); + delegate_->OnSerializedPacket(std::move(packet)); } void QuicPacketCreator::ClearPacket() { @@ -768,7 +768,7 @@ return encrypted; } -OwningSerializedPacketPointer +std::unique_ptr<SerializedPacket> QuicPacketCreator::SerializeConnectivityProbingPacket() { QUIC_BUG_IF(VersionHasIetfQuicFrames(framer_->transport_version())) << "Must not be version 99 to serialize padded ping connectivity probe"; @@ -791,17 +791,20 @@ kMaxOutgoingPacketSize, buffer.get()); DCHECK(encrypted_length); - OwningSerializedPacketPointer serialize_packet(new SerializedPacket( + std::unique_ptr<SerializedPacket> serialize_packet(new SerializedPacket( header.packet_number, header.packet_number_length, buffer.release(), encrypted_length, /*has_ack=*/false, /*has_stop_waiting=*/false)); + serialize_packet->release_encrypted_buffer = [](const char* p) { + delete[] p; + }; serialize_packet->encryption_level = packet_.encryption_level; serialize_packet->transmission_type = NOT_RETRANSMISSION; return serialize_packet; } -OwningSerializedPacketPointer +std::unique_ptr<SerializedPacket> QuicPacketCreator::SerializePathChallengeConnectivityProbingPacket( QuicPathFrameBuffer* payload) { QUIC_BUG_IF(!VersionHasIetfQuicFrames(framer_->transport_version())) @@ -827,17 +830,21 @@ kMaxOutgoingPacketSize, buffer.get()); DCHECK(encrypted_length); - OwningSerializedPacketPointer serialize_packet(new SerializedPacket( - header.packet_number, header.packet_number_length, buffer.release(), - encrypted_length, /*has_ack=*/false, /*has_stop_waiting=*/false)); + std::unique_ptr<SerializedPacket> serialize_packet( + new SerializedPacket(header.packet_number, header.packet_number_length, + buffer.release(), encrypted_length, + /*has_ack=*/false, /*has_stop_waiting=*/false)); + serialize_packet->release_encrypted_buffer = [](const char* p) { + delete[] p; + }; serialize_packet->encryption_level = packet_.encryption_level; serialize_packet->transmission_type = NOT_RETRANSMISSION; return serialize_packet; } -OwningSerializedPacketPointer +std::unique_ptr<SerializedPacket> QuicPacketCreator::SerializePathResponseConnectivityProbingPacket( const QuicCircularDeque<QuicPathFrameBuffer>& payloads, const bool is_padded) { @@ -864,10 +871,14 @@ kMaxOutgoingPacketSize, buffer.get()); DCHECK(encrypted_length); - OwningSerializedPacketPointer serialize_packet(new SerializedPacket( - header.packet_number, header.packet_number_length, buffer.release(), - encrypted_length, /*has_ack=*/false, /*has_stop_waiting=*/false)); + std::unique_ptr<SerializedPacket> serialize_packet( + new SerializedPacket(header.packet_number, header.packet_number_length, + buffer.release(), encrypted_length, + /*has_ack=*/false, /*has_stop_waiting=*/false)); + serialize_packet->release_encrypted_buffer = [](const char* p) { + delete[] p; + }; serialize_packet->encryption_level = packet_.encryption_level; serialize_packet->transmission_type = NOT_RETRANSMISSION;
diff --git a/quic/core/quic_packet_creator.h b/quic/core/quic_packet_creator.h index fbe046f..7695587 100644 --- a/quic/core/quic_packet_creator.h +++ b/quic/core/quic_packet_creator.h
@@ -44,10 +44,9 @@ // packet. If return nullptr, QuicPacketCreator will serialize on a stack // buffer. virtual char* GetPacketBuffer() = 0; - // Called when a packet is serialized. Delegate does not take the ownership - // of |serialized_packet|, but takes ownership of any frames it removes - // from |packet.retransmittable_frames|. - virtual void OnSerializedPacket(SerializedPacket* serialized_packet) = 0; + // Called when a packet is serialized. Delegate take the ownership of + // |serialized_packet|. + virtual void OnSerializedPacket(SerializedPacket serialized_packet) = 0; // Called when an unrecoverable error is encountered. virtual void OnUnrecoverableError(QuicErrorCode error, @@ -210,19 +209,20 @@ const ParsedQuicVersionVector& supported_versions); // Creates a connectivity probing packet for versions prior to version 99. - OwningSerializedPacketPointer SerializeConnectivityProbingPacket(); + std::unique_ptr<SerializedPacket> SerializeConnectivityProbingPacket(); // Create connectivity probing request and response packets using PATH // CHALLENGE and PATH RESPONSE frames, respectively, for version 99/IETF QUIC. // SerializePathChallengeConnectivityProbingPacket will pad the packet to be // MTU bytes long. - OwningSerializedPacketPointer SerializePathChallengeConnectivityProbingPacket( - QuicPathFrameBuffer* payload); + std::unique_ptr<SerializedPacket> + SerializePathChallengeConnectivityProbingPacket(QuicPathFrameBuffer* payload); // If |is_padded| is true then SerializePathResponseConnectivityProbingPacket // will pad the packet to be MTU bytes long, else it will not pad the packet. // |payloads| is cleared. - OwningSerializedPacketPointer SerializePathResponseConnectivityProbingPacket( + std::unique_ptr<SerializedPacket> + SerializePathResponseConnectivityProbingPacket( const QuicCircularDeque<QuicPathFrameBuffer>& payloads, const bool is_padded); @@ -458,8 +458,8 @@ // Serializes all frames which have been added and adds any which should be // retransmitted to packet_.retransmittable_frames. All frames must fit into // a single packet. - // Fails if |buffer_len| isn't long enough for the encrypted packet. - void SerializePacket(char* encrypted_buffer, size_t buffer_len); + // Fails if |encrypted_buffer_len| isn't long enough for the encrypted packet. + void SerializePacket(char* encrypted_buffer, size_t encrypted_buffer_len); // Called after a new SerialiedPacket is created to call the delegate's // OnSerializedPacket and reset state.
diff --git a/quic/core/quic_packet_creator_test.cc b/quic/core/quic_packet_creator_test.cc index b11417d..d1beea7 100644 --- a/quic/core/quic_packet_creator_test.cc +++ b/quic/core/quic_packet_creator_test.cc
@@ -138,28 +138,16 @@ class QuicPacketCreatorTest : public QuicTestWithParam<TestParams> { public: - void ClearSerializedPacketForTests(SerializedPacket* serialized_packet) { - if (serialized_packet == nullptr) { - return; - } - ClearSerializedPacket(serialized_packet); + void ClearSerializedPacketForTests(SerializedPacket /*serialized_packet*/) { + // serialized packet self-clears on destruction. } - void SaveSerializedPacket(SerializedPacket* serialized_packet) { - if (serialized_packet == nullptr) { - return; - } - delete[] serialized_packet_.encrypted_buffer; - serialized_packet_ = *serialized_packet; - serialized_packet_.encrypted_buffer = CopyBuffer(*serialized_packet); - serialized_packet->retransmittable_frames.clear(); + void SaveSerializedPacket(SerializedPacket serialized_packet) { + serialized_packet_.reset(CopySerializedPacket( + serialized_packet, &allocator_, /*copy_buffer=*/true)); } - void DeleteSerializedPacket() { - delete[] serialized_packet_.encrypted_buffer; - serialized_packet_.encrypted_buffer = nullptr; - ClearSerializedPacket(&serialized_packet_); - } + void DeleteSerializedPacket() { serialized_packet_ = nullptr; } protected: QuicPacketCreatorTest() @@ -173,8 +161,7 @@ Perspective::IS_CLIENT, connection_id_.length()), data_("foo"), - creator_(connection_id_, &client_framer_, &delegate_, &producer_), - serialized_packet_(creator_.NoPacket()) { + creator_(connection_id_, &client_framer_, &delegate_, &producer_) { EXPECT_CALL(delegate_, GetPacketBuffer()).WillRepeatedly(Return(nullptr)); creator_.SetEncrypter(ENCRYPTION_INITIAL, std::make_unique<NullEncrypter>( Perspective::IS_CLIENT)); @@ -208,10 +195,7 @@ } } - ~QuicPacketCreatorTest() override { - delete[] serialized_packet_.encrypted_buffer; - ClearSerializedPacket(&serialized_packet_); - } + ~QuicPacketCreatorTest() override {} SerializedPacket SerializeAllFrames(const QuicFrames& frames) { SerializedPacket packet = QuicPacketCreatorPeer::SerializeAllFrames( @@ -297,7 +281,7 @@ std::string data_; struct iovec iov_; TestPacketCreator creator_; - SerializedPacket serialized_packet_; + std::unique_ptr<SerializedPacket> serialized_packet_; SimpleDataProducer producer_; SimpleBufferAllocator allocator_; }; @@ -354,12 +338,12 @@ } TEST_P(QuicPacketCreatorTest, SerializeConnectionClose) { - QuicConnectionCloseFrame frame(creator_.transport_version(), QUIC_NO_ERROR, - "error", - /*transport_close_frame_type=*/0); + QuicConnectionCloseFrame* frame = new QuicConnectionCloseFrame( + creator_.transport_version(), QUIC_NO_ERROR, "error", + /*transport_close_frame_type=*/0); QuicFrames frames; - frames.push_back(QuicFrame(&frame)); + frames.push_back(QuicFrame(frame)); SerializedPacket serialized = SerializeAllFrames(frames); EXPECT_EQ(ENCRYPTION_INITIAL, serialized.encryption_level); ASSERT_EQ(QuicPacketNumber(1u), serialized.packet_number); @@ -489,7 +473,7 @@ EXPECT_CALL(delegate_, OnSerializedPacket(_)) .WillOnce(Invoke(this, &QuicPacketCreatorTest::SaveSerializedPacket)); creator_.FlushCurrentPacket(); - ASSERT_TRUE(serialized_packet_.encrypted_buffer); + ASSERT_TRUE(serialized_packet_->encrypted_buffer); DeleteSerializedPacket(); } } @@ -537,7 +521,7 @@ EXPECT_LT(0u, bytes_consumed); } creator_.FlushCurrentPacket(); - ASSERT_TRUE(serialized_packet_.encrypted_buffer); + ASSERT_TRUE(serialized_packet_->encrypted_buffer); // If there is not enough space in the packet to fit a padding frame // (1 byte) and to expand the stream frame (another 2 bytes) the packet // will not be padded. @@ -546,9 +530,9 @@ !QuicVersionUsesCryptoFrames(client_framer_.transport_version())) || client_framer_.version().CanSendCoalescedPackets()) { EXPECT_EQ(kDefaultMaxPacketSize - bytes_free, - serialized_packet_.encrypted_length); + serialized_packet_->encrypted_length); } else { - EXPECT_EQ(kDefaultMaxPacketSize, serialized_packet_.encrypted_length); + EXPECT_EQ(kDefaultMaxPacketSize, serialized_packet_->encrypted_length); } DeleteSerializedPacket(); } @@ -578,12 +562,12 @@ size_t bytes_consumed = frame.stream_frame.data_length; EXPECT_LT(0u, bytes_consumed); creator_.FlushCurrentPacket(); - ASSERT_TRUE(serialized_packet_.encrypted_buffer); + ASSERT_TRUE(serialized_packet_->encrypted_buffer); if (bytes_free > 0) { EXPECT_EQ(kDefaultMaxPacketSize - bytes_free, - serialized_packet_.encrypted_length); + serialized_packet_->encrypted_length); } else { - EXPECT_EQ(kDefaultMaxPacketSize, serialized_packet_.encrypted_length); + EXPECT_EQ(kDefaultMaxPacketSize, serialized_packet_->encrypted_length); } DeleteSerializedPacket(); } @@ -951,7 +935,7 @@ creator_.set_encryption_level(level); - OwningSerializedPacketPointer encrypted; + std::unique_ptr<SerializedPacket> encrypted; if (VersionHasIetfQuicFrames(creator_.transport_version())) { QuicPathFrameBuffer payload = { {0xde, 0xad, 0xbe, 0xef, 0xba, 0xdc, 0x0f, 0xfe}}; @@ -994,7 +978,7 @@ creator_.set_encryption_level(level); - OwningSerializedPacketPointer encrypted( + std::unique_ptr<SerializedPacket> encrypted( creator_.SerializePathChallengeConnectivityProbingPacket(&payload)); { InSequence s; @@ -1027,7 +1011,7 @@ QuicCircularDeque<QuicPathFrameBuffer> payloads; payloads.push_back(payload0); - OwningSerializedPacketPointer encrypted( + std::unique_ptr<SerializedPacket> encrypted( creator_.SerializePathResponseConnectivityProbingPacket(payloads, true)); { @@ -1061,7 +1045,7 @@ QuicCircularDeque<QuicPathFrameBuffer> payloads; payloads.push_back(payload0); - OwningSerializedPacketPointer encrypted( + std::unique_ptr<SerializedPacket> encrypted( creator_.SerializePathResponseConnectivityProbingPacket(payloads, false)); { @@ -1096,7 +1080,7 @@ payloads.push_back(payload0); payloads.push_back(payload1); - OwningSerializedPacketPointer encrypted( + std::unique_ptr<SerializedPacket> encrypted( creator_.SerializePathResponseConnectivityProbingPacket(payloads, true)); { @@ -1133,7 +1117,7 @@ payloads.push_back(payload0); payloads.push_back(payload1); - OwningSerializedPacketPointer encrypted( + std::unique_ptr<SerializedPacket> encrypted( creator_.SerializePathResponseConnectivityProbingPacket(payloads, false)); { @@ -1171,7 +1155,7 @@ payloads.push_back(payload1); payloads.push_back(payload2); - OwningSerializedPacketPointer encrypted( + std::unique_ptr<SerializedPacket> encrypted( creator_.SerializePathResponseConnectivityProbingPacket(payloads, true)); { @@ -1211,7 +1195,7 @@ payloads.push_back(payload1); payloads.push_back(payload2); - OwningSerializedPacketPointer encrypted( + std::unique_ptr<SerializedPacket> encrypted( creator_.SerializePathResponseConnectivityProbingPacket(payloads, false)); InSequence s; @@ -1364,7 +1348,6 @@ } ProcessPacket(serialized); EXPECT_EQ(GetParam().version_serialization, header.version_flag); - DeleteFrames(&frames_); } TEST_P(QuicPacketCreatorTest, SerializeFrameShortData) { @@ -1405,7 +1388,6 @@ } ProcessPacket(serialized); EXPECT_EQ(GetParam().version_serialization, header.version_flag); - DeleteFrames(&frames_); } TEST_P(QuicPacketCreatorTest, ConsumeDataLargerThanOneStreamFrame) { @@ -1491,13 +1473,14 @@ EXPECT_FALSE(creator_.AddFrame(QuicFrame(&ack_frame), NOT_RETRANSMISSION)); // Ensure the packet is successfully created. - ASSERT_TRUE(serialized_packet_.encrypted_buffer); - ASSERT_FALSE(serialized_packet_.retransmittable_frames.empty()); - const QuicFrames& retransmittable = serialized_packet_.retransmittable_frames; + ASSERT_TRUE(serialized_packet_->encrypted_buffer); + ASSERT_FALSE(serialized_packet_->retransmittable_frames.empty()); + const QuicFrames& retransmittable = + serialized_packet_->retransmittable_frames; ASSERT_EQ(1u, retransmittable.size()); EXPECT_EQ(STREAM_FRAME, retransmittable[0].type); - EXPECT_TRUE(serialized_packet_.has_ack); - EXPECT_EQ(QuicPacketNumber(10u), serialized_packet_.largest_acked); + EXPECT_TRUE(serialized_packet_->has_ack); + EXPECT_EQ(QuicPacketNumber(10u), serialized_packet_->largest_acked); DeleteSerializedPacket(); EXPECT_FALSE(creator_.HasPendingFrames()); @@ -1536,9 +1519,10 @@ EXPECT_EQ(4u, num_bytes_consumed); // Ensure the packet is successfully created. - ASSERT_TRUE(serialized_packet_.encrypted_buffer); - ASSERT_FALSE(serialized_packet_.retransmittable_frames.empty()); - const QuicFrames& retransmittable = serialized_packet_.retransmittable_frames; + ASSERT_TRUE(serialized_packet_->encrypted_buffer); + ASSERT_FALSE(serialized_packet_->retransmittable_frames.empty()); + const QuicFrames& retransmittable = + serialized_packet_->retransmittable_frames; ASSERT_EQ(1u, retransmittable.size()); EXPECT_EQ(STREAM_FRAME, retransmittable[0].type); DeleteSerializedPacket(); @@ -1568,8 +1552,8 @@ EXPECT_EQ(1u, num_bytes_consumed); // Check that a packet is created. - ASSERT_TRUE(serialized_packet_.encrypted_buffer); - ASSERT_FALSE(serialized_packet_.retransmittable_frames.empty()); + ASSERT_TRUE(serialized_packet_->encrypted_buffer); + ASSERT_FALSE(serialized_packet_->retransmittable_frames.empty()); { InSequence s; EXPECT_CALL(framer_visitor_, OnPacket()); @@ -1583,7 +1567,7 @@ } EXPECT_CALL(framer_visitor_, OnPacketComplete()); } - ProcessPacket(serialized_packet_); + ProcessPacket(*serialized_packet_); } TEST_P(QuicPacketCreatorTest, AddUnencryptedStreamDataClosesConnection) { @@ -1672,7 +1656,7 @@ EXPECT_CALL(framer_visitor_, OnPacketComplete()); } // Packet only contains padding. - ProcessPacket(serialized_packet_); + ProcessPacket(*serialized_packet_); } EXPECT_EQ(0u, creator_.pending_padding_bytes()); } @@ -1754,9 +1738,8 @@ /*needs_full_padding=*/true, NOT_RETRANSMISSION, &frame)); EXPECT_CALL(delegate_, OnSerializedPacket(_)) - .WillOnce(Invoke([expected_buffer](SerializedPacket* serialized_packet) { - EXPECT_EQ(expected_buffer, serialized_packet->encrypted_buffer); - ClearSerializedPacket(serialized_packet); + .WillOnce(Invoke([expected_buffer](SerializedPacket serialized_packet) { + EXPECT_EQ(expected_buffer, serialized_packet.encrypted_buffer); })); creator_.FlushCurrentPacket(); } @@ -1871,7 +1854,7 @@ EXPECT_CALL(delegate_, OnSerializedPacket(_)) .WillOnce(Invoke(this, &QuicPacketCreatorTest::SaveSerializedPacket)); creator_.FlushCurrentPacket(); - ASSERT_TRUE(serialized_packet_.encrypted_buffer); + ASSERT_TRUE(serialized_packet_->encrypted_buffer); DeleteSerializedPacket(); } } @@ -1987,18 +1970,18 @@ .WillOnce(Invoke(this, &QuicPacketCreatorTest::SaveSerializedPacket)); EXPECT_TRUE(creator_.AddFrame(ack_frame, LOSS_RETRANSMISSION)); - ASSERT_FALSE(serialized_packet_.encrypted_buffer); + ASSERT_EQ(serialized_packet_, nullptr); EXPECT_TRUE(creator_.AddFrame(stream_frame, RTO_RETRANSMISSION)); - ASSERT_FALSE(serialized_packet_.encrypted_buffer); + ASSERT_EQ(serialized_packet_, nullptr); EXPECT_TRUE(creator_.AddFrame(padding_frame, TLP_RETRANSMISSION)); creator_.FlushCurrentPacket(); - ASSERT_TRUE(serialized_packet_.encrypted_buffer); + ASSERT_TRUE(serialized_packet_->encrypted_buffer); // The last retransmittable frame on packet is a stream frame, the packet's // transmission type should be the same as the stream frame's. - EXPECT_EQ(serialized_packet_.transmission_type, RTO_RETRANSMISSION); + EXPECT_EQ(serialized_packet_->transmission_type, RTO_RETRANSMISSION); DeleteSerializedPacket(); } @@ -2053,7 +2036,6 @@ quiche::test::CompareCharArraysWithHexError( "retry token", header.retry_token.data(), header.retry_token.length(), retry_token_bytes, sizeof(retry_token_bytes)); - DeleteFrames(&frames_); } TEST_P(QuicPacketCreatorTest, GetConnectionId) { @@ -2149,7 +2131,7 @@ EXPECT_CALL(framer_visitor_, OnStreamFrame(_)); EXPECT_CALL(framer_visitor_, OnStreamFrame(_)); EXPECT_CALL(framer_visitor_, OnPacketComplete()); - ProcessPacket(serialized_packet_); + ProcessPacket(*serialized_packet_); } TEST_P(QuicPacketCreatorTest, SaveNonRetransmittableFrames) { @@ -2330,7 +2312,7 @@ (), (override)); MOCK_METHOD(char*, GetPacketBuffer, (), (override)); - MOCK_METHOD(void, OnSerializedPacket, (SerializedPacket*), (override)); + MOCK_METHOD(void, OnSerializedPacket, (SerializedPacket), (override)); MOCK_METHOD(void, OnUnrecoverableError, (QuicErrorCode, const std::string&), @@ -2498,18 +2480,13 @@ creator_.AttachPacketFlusher(); } - ~QuicPacketCreatorMultiplePacketsTest() override { - for (SerializedPacket& packet : packets_) { - delete[] packet.encrypted_buffer; - ClearSerializedPacket(&packet); - } - } + ~QuicPacketCreatorMultiplePacketsTest() override {} - void SavePacket(SerializedPacket* packet) { - packet->encrypted_buffer = CopyBuffer(*packet); - packets_.push_back(*packet); - packet->encrypted_buffer = nullptr; - packet->retransmittable_frames.clear(); + void SavePacket(SerializedPacket packet) { + DCHECK(packet.release_encrypted_buffer == nullptr); + packet.encrypted_buffer = CopyBuffer(packet); + packet.release_encrypted_buffer = [](const char* p) { delete[] p; }; + packets_.push_back(std::move(packet)); } protected: @@ -2994,7 +2971,7 @@ contents.num_stream_frames = 1; CheckPacketContains(contents, 0); EXPECT_FALSE(packets_.empty()); - SerializedPacket packet = packets_.back(); + SerializedPacket& packet = packets_.back(); EXPECT_TRUE(!packet.retransmittable_frames.empty()); EXPECT_EQ(LOSS_RETRANSMISSION, packet.transmission_type); EXPECT_EQ(STREAM_FRAME, packet.retransmittable_frames.front().type); @@ -3024,7 +3001,7 @@ contents.num_stream_frames = 1; CheckPacketContains(contents, 0); EXPECT_FALSE(packets_.empty()); - SerializedPacket packet = packets_.back(); + SerializedPacket& packet = packets_.back(); EXPECT_TRUE(!packet.retransmittable_frames.empty()); EXPECT_EQ(STREAM_FRAME, packet.retransmittable_frames.front().type); const QuicStreamFrame& stream_frame = @@ -3067,7 +3044,7 @@ EXPECT_FALSE(creator_.HasPendingRetransmittableFrames()); EXPECT_FALSE(packets_.empty()); - SerializedPacket packet = packets_.back(); + SerializedPacket& packet = packets_.back(); EXPECT_TRUE(!packet.retransmittable_frames.empty()); EXPECT_EQ(STREAM_FRAME, packet.retransmittable_frames.front().type); const QuicStreamFrame& stream_frame = @@ -3096,7 +3073,7 @@ EXPECT_FALSE(creator_.HasPendingRetransmittableFrames()); EXPECT_FALSE(packets_.empty()); - SerializedPacket packet = packets_.back(); + SerializedPacket& packet = packets_.back(); EXPECT_TRUE(!packet.retransmittable_frames.empty()); EXPECT_EQ(STREAM_FRAME, packet.retransmittable_frames.front().type); const QuicStreamFrame& stream_frame = @@ -3443,7 +3420,7 @@ GenerateConnectivityProbingPacket) { delegate_.SetCanWriteAnything(); - OwningSerializedPacketPointer probing_packet; + std::unique_ptr<SerializedPacket> probing_packet; if (VersionHasIetfQuicFrames(framer_.transport_version())) { QuicPathFrameBuffer payload = { {0xde, 0xad, 0xbe, 0xef, 0xba, 0xdc, 0x0f, 0xfe}};
diff --git a/quic/core/quic_packets.cc b/quic/core/quic_packets.cc index 4123c27..69575b9 100644 --- a/quic/core/quic_packets.cc +++ b/quic/core/quic_packets.cc
@@ -465,15 +465,8 @@ transmission_type(NOT_RETRANSMISSION), has_ack_frame_copy(false) {} -SerializedPacket::SerializedPacket(const SerializedPacket& other) = default; - -SerializedPacket& SerializedPacket::operator=(const SerializedPacket& other) = - default; - SerializedPacket::SerializedPacket(SerializedPacket&& other) - : encrypted_buffer(other.encrypted_buffer), - encrypted_length(other.encrypted_length), - has_crypto_handshake(other.has_crypto_handshake), + : has_crypto_handshake(other.has_crypto_handshake), num_padding_bytes(other.num_padding_bytes), packet_number(other.packet_number), packet_number_length(other.packet_number_length), @@ -483,23 +476,58 @@ transmission_type(other.transmission_type), largest_acked(other.largest_acked), has_ack_frame_copy(other.has_ack_frame_copy) { - retransmittable_frames.swap(other.retransmittable_frames); - nonretransmittable_frames.swap(other.nonretransmittable_frames); + if (this != &other) { + if (release_encrypted_buffer && encrypted_buffer != nullptr) { + release_encrypted_buffer(encrypted_buffer); + } + encrypted_buffer = other.encrypted_buffer; + encrypted_length = other.encrypted_length; + release_encrypted_buffer = std::move(other.release_encrypted_buffer); + other.release_encrypted_buffer = nullptr; + + retransmittable_frames.swap(other.retransmittable_frames); + nonretransmittable_frames.swap(other.nonretransmittable_frames); + } } -SerializedPacket::~SerializedPacket() {} +SerializedPacket::~SerializedPacket() { + if (release_encrypted_buffer && encrypted_buffer != nullptr) { + release_encrypted_buffer(encrypted_buffer); + } + + if (!retransmittable_frames.empty()) { + DeleteFrames(&retransmittable_frames); + } + for (auto& frame : nonretransmittable_frames) { + if (!has_ack_frame_copy && frame.type == ACK_FRAME) { + // Do not delete ack frame if the packet does not own a copy of it. + continue; + } + DeleteFrame(&frame); + } +} SerializedPacket* CopySerializedPacket(const SerializedPacket& serialized, QuicBufferAllocator* allocator, bool copy_buffer) { - SerializedPacket* copy = new SerializedPacket(serialized); + SerializedPacket* copy = new SerializedPacket( + serialized.packet_number, serialized.packet_number_length, + serialized.encrypted_buffer, serialized.encrypted_length, + serialized.has_ack, serialized.has_stop_waiting); + copy->has_crypto_handshake = serialized.has_crypto_handshake; + copy->num_padding_bytes = serialized.num_padding_bytes; + copy->encryption_level = serialized.encryption_level; + copy->transmission_type = serialized.transmission_type; + copy->largest_acked = serialized.largest_acked; + if (copy_buffer) { copy->encrypted_buffer = CopyBuffer(serialized); + copy->release_encrypted_buffer = [](const char* p) { delete[] p; }; } // Copy underlying frames. copy->retransmittable_frames = CopyQuicFrames(allocator, serialized.retransmittable_frames); - copy->nonretransmittable_frames.clear(); + DCHECK(copy->nonretransmittable_frames.empty()); for (const auto& frame : serialized.nonretransmittable_frames) { if (frame.type == ACK_FRAME) { copy->has_ack_frame_copy = true; @@ -509,27 +537,8 @@ return copy; } -void ClearSerializedPacket(SerializedPacket* serialized_packet) { - if (!serialized_packet->retransmittable_frames.empty()) { - DeleteFrames(&serialized_packet->retransmittable_frames); - } - for (auto& frame : serialized_packet->nonretransmittable_frames) { - if (!serialized_packet->has_ack_frame_copy && frame.type == ACK_FRAME) { - // Do not delete ack frame if the packet does not own a copy of it. - continue; - } - DeleteFrame(&frame); - } - serialized_packet->nonretransmittable_frames.clear(); - serialized_packet->encrypted_buffer = nullptr; - serialized_packet->encrypted_length = 0; - serialized_packet->largest_acked.Clear(); -} - char* CopyBuffer(const SerializedPacket& packet) { - char* dst_buffer = new char[packet.encrypted_length]; - memcpy(dst_buffer, packet.encrypted_buffer, packet.encrypted_length); - return dst_buffer; + return CopyBuffer(packet.encrypted_buffer, packet.encrypted_length); } char* CopyBuffer(const char* encrypted_buffer,
diff --git a/quic/core/quic_packets.h b/quic/core/quic_packets.h index e8e1931..04522e8 100644 --- a/quic/core/quic_packets.h +++ b/quic/core/quic_packets.h
@@ -357,6 +357,13 @@ bool owns_header_buffer_; }; +// SerializedPacket contains information of a serialized(encrypted) packet. +// +// WARNING: +// +// If you add a member field to this class, please make sure it is properly +// copied in |CopySerializedPacket|. +// struct QUIC_EXPORT_PRIVATE SerializedPacket { SerializedPacket(QuicPacketNumber packet_number, QuicPacketNumberLength packet_number_length, @@ -364,14 +371,20 @@ QuicPacketLength encrypted_length, bool has_ack, bool has_stop_waiting); - SerializedPacket(const SerializedPacket& other); - SerializedPacket& operator=(const SerializedPacket& other); + + // Copy constructor & assignment are deleted. Use |CopySerializedPacket| to + // make a copy. + SerializedPacket(const SerializedPacket& other) = delete; + SerializedPacket& operator=(const SerializedPacket& other) = delete; SerializedPacket(SerializedPacket&& other); ~SerializedPacket(); - // Not owned. + // Not owned if |release_encrypted_buffer| is nullptr. Otherwise it is + // released by |release_encrypted_buffer| on destruction. const char* encrypted_buffer; QuicPacketLength encrypted_length; + std::function<void(const char*)> release_encrypted_buffer; + QuicFrames retransmittable_frames; QuicFrames nonretransmittable_frames; IsHandshake has_crypto_handshake; @@ -401,10 +414,6 @@ QuicBufferAllocator* allocator, bool copy_buffer); -// Deletes and clears all the frames and the packet from serialized packet. -QUIC_EXPORT_PRIVATE void ClearSerializedPacket( - SerializedPacket* serialized_packet); - // Allocates a new char[] of size |packet.encrypted_length| and copies in // |packet.encrypted_buffer|. QUIC_EXPORT_PRIVATE char* CopyBuffer(const SerializedPacket& packet); @@ -413,21 +422,6 @@ QUIC_EXPORT_PRIVATE char* CopyBuffer(const char* encrypted_buffer, QuicPacketLength encrypted_length); -struct QUIC_EXPORT_PRIVATE SerializedPacketDeleter { - void operator()(SerializedPacket* packet) { - if (packet->encrypted_buffer != nullptr) { - delete[] packet->encrypted_buffer; - } - delete packet; - } -}; - -// On destruction, OwningSerializedPacketPointer deletes a packet's (on-heap) -// encrypted_buffer before deleting the (also on-heap) packet itself. -// TODO(wub): Maybe delete retransmittable_frames too? -typedef std::unique_ptr<SerializedPacket, SerializedPacketDeleter> - OwningSerializedPacketPointer; - // Context for an incoming packet. struct QUIC_EXPORT_PRIVATE QuicPerPacketContext { virtual ~QuicPerPacketContext() {}
diff --git a/quic/core/quic_packets_test.cc b/quic/core/quic_packets_test.cc index 1ccaabc..b2bccbe 100644 --- a/quic/core/quic_packets_test.cc +++ b/quic/core/quic_packets_test.cc
@@ -106,10 +106,6 @@ CopySerializedPacket(packet, &allocator, /*copy_buffer=*/false)); EXPECT_EQ(packet.encrypted_buffer, copy2->encrypted_buffer); EXPECT_EQ(1000u, copy2->encrypted_length); - ClearSerializedPacket(&packet); - delete[] copy->encrypted_buffer; - ClearSerializedPacket(copy.get()); - ClearSerializedPacket(copy2.get()); } } // namespace
diff --git a/quic/test_tools/quic_packet_creator_peer.cc b/quic/test_tools/quic_packet_creator_peer.cc index 1b5eb53..44fed88 100644 --- a/quic/test_tools/quic_packet_creator_peer.cc +++ b/quic/test_tools/quic_packet_creator_peer.cc
@@ -119,14 +119,14 @@ } // static -OwningSerializedPacketPointer +std::unique_ptr<SerializedPacket> QuicPacketCreatorPeer::SerializeConnectivityProbingPacket( QuicPacketCreator* creator) { return creator->SerializeConnectivityProbingPacket(); } // static -OwningSerializedPacketPointer +std::unique_ptr<SerializedPacket> QuicPacketCreatorPeer::SerializePathChallengeConnectivityProbingPacket( QuicPacketCreator* creator, QuicPathFrameBuffer* payload) {
diff --git a/quic/test_tools/quic_packet_creator_peer.h b/quic/test_tools/quic_packet_creator_peer.h index 60eb811..2dc9413 100644 --- a/quic/test_tools/quic_packet_creator_peer.h +++ b/quic/test_tools/quic_packet_creator_peer.h
@@ -50,9 +50,9 @@ const QuicFrames& frames, char* buffer, size_t buffer_len); - static OwningSerializedPacketPointer SerializeConnectivityProbingPacket( + static std::unique_ptr<SerializedPacket> SerializeConnectivityProbingPacket( QuicPacketCreator* creator); - static OwningSerializedPacketPointer + static std::unique_ptr<SerializedPacket> SerializePathChallengeConnectivityProbingPacket(QuicPacketCreator* creator, QuicPathFrameBuffer* payload);
diff --git a/quic/test_tools/quic_test_utils.cc b/quic/test_tools/quic_test_utils.cc index 00a57e7..7d88ccd 100644 --- a/quic/test_tools/quic_test_utils.cc +++ b/quic/test_tools/quic_test_utils.cc
@@ -560,14 +560,14 @@ PacketSavingConnection::~PacketSavingConnection() {} -void PacketSavingConnection::SendOrQueuePacket(SerializedPacket* packet) { +void PacketSavingConnection::SendOrQueuePacket(SerializedPacket packet) { encrypted_packets_.push_back(std::make_unique<QuicEncryptedPacket>( - CopyBuffer(*packet), packet->encrypted_length, true)); + CopyBuffer(packet), packet.encrypted_length, true)); clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(10)); // Transfer ownership of the packet to the SentPacketManager and the // ack notifier to the AckNotifierManager. QuicConnectionPeer::GetSentPacketManager(this)->OnPacketSent( - packet, clock_.ApproximateNow(), NOT_RETRANSMISSION, + &packet, clock_.ApproximateNow(), NOT_RETRANSMISSION, HAS_RETRANSMITTABLE_DATA); }
diff --git a/quic/test_tools/quic_test_utils.h b/quic/test_tools/quic_test_utils.h index f7e98ae..7d1d9db 100644 --- a/quic/test_tools/quic_test_utils.h +++ b/quic/test_tools/quic_test_utils.h
@@ -736,7 +736,7 @@ ~PacketSavingConnection() override; - void SendOrQueuePacket(SerializedPacket* packet) override; + void SendOrQueuePacket(SerializedPacket packet) override; std::vector<std::unique_ptr<QuicEncryptedPacket>> encrypted_packets_; MockClock clock_; @@ -1444,7 +1444,7 @@ ~MockPacketCreatorDelegate() override; MOCK_METHOD(char*, GetPacketBuffer, (), (override)); - MOCK_METHOD(void, OnSerializedPacket, (SerializedPacket*), (override)); + MOCK_METHOD(void, OnSerializedPacket, (SerializedPacket), (override)); MOCK_METHOD(void, OnUnrecoverableError, (QuicErrorCode, const std::string&),