Fix QUIC_BUG in FlushAckFrame to check whether frames is empty.
Protected by FLAGS_quic_reloadable_flag_quic_single_ack_in_packet2.
PiperOrigin-RevId: 349583801
Change-Id: I75958100168a1c77c8661d217224f6584fac284c
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index 9342bd6..3ea815b 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -3927,8 +3927,8 @@
bool send_ack = error != QUIC_PACKET_WRITE_ERROR &&
!uber_received_packet_manager_.IsAckFrameEmpty(
QuicUtils::GetPacketNumberSpace(encryption_level_));
- if (GetQuicReloadableFlag(quic_single_ack_in_packet)) {
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_single_ack_in_packet, 1, 2);
+ if (GetQuicReloadableFlag(quic_single_ack_in_packet2)) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_single_ack_in_packet2, 1, 2);
send_ack = !packet_creator_.has_ack() && send_ack;
}
if (send_ack) {
@@ -3974,8 +3974,8 @@
bool send_ack = error != QUIC_PACKET_WRITE_ERROR &&
!uber_received_packet_manager_.IsAckFrameEmpty(
QuicUtils::GetPacketNumberSpace(encryption_level_));
- if (GetQuicReloadableFlag(quic_single_ack_in_packet)) {
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_single_ack_in_packet, 2, 2);
+ if (GetQuicReloadableFlag(quic_single_ack_in_packet2)) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_single_ack_in_packet2, 2, 2);
send_ack = !packet_creator_.has_ack() && send_ack;
}
if (send_ack) {
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index 1baf44a..dc434a5 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -12750,7 +12750,7 @@
ProcessFramesPacketWithAddresses(frames, kSelfAddress, kPeerAddress,
ENCRYPTION_FORWARD_SECURE);
ASSERT_FALSE(writer_->ack_frames().empty());
- if (GetQuicReloadableFlag(quic_single_ack_in_packet)) {
+ if (GetQuicReloadableFlag(quic_single_ack_in_packet2)) {
EXPECT_EQ(1u, writer_->ack_frames().size());
} else {
EXPECT_EQ(2u, writer_->ack_frames().size());
@@ -13070,6 +13070,33 @@
EXPECT_FALSE(connection_.connected());
}
+// Regresstion test for b/175685916
+TEST_P(QuicConnectionTest, TryToFlushAckWithAckQueued) {
+ if (!version().HasIetfQuicFrames()) {
+ return;
+ }
+ SetQuicReloadableFlag(quic_can_send_ack_frequency, true);
+ SetQuicReloadableFlag(quic_single_ack_in_packet2, true);
+ set_perspective(Perspective::IS_SERVER);
+
+ QuicConfig config;
+ QuicConfigPeer::SetReceivedMinAckDelayMs(&config, /*min_ack_delay_ms=*/1);
+ EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _));
+ connection_.SetFromConfig(config);
+ connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
+ connection_.OnHandshakeComplete();
+ QuicPacketCreatorPeer::SetPacketNumber(creator_, 200);
+
+ EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(1);
+ ProcessDataPacketAtLevel(1, !kHasStopWaiting, ENCRYPTION_FORWARD_SECURE);
+ // Sending ACK_FREQUENCY bundles ACK. QuicConnectionPeer::SendPing
+ // will try to bundle ACK but there is no pending ACK.
+ EXPECT_CALL(visitor_, SendAckFrequency(_))
+ .WillOnce(Invoke(¬ifier_,
+ &SimpleSessionNotifier::WriteOrBufferAckFrequency));
+ QuicConnectionPeer::SendPing(&connection_);
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h
index 1397140..27383de 100644
--- a/quic/core/quic_flags_list.h
+++ b/quic/core/quic_flags_list.h
@@ -55,7 +55,7 @@
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_send_path_response, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_send_timestamps, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_send_version_negotiation_for_short_connection_ids, true)
-QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_single_ack_in_packet, false)
+QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_single_ack_in_packet2, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_split_up_send_rst_2, true)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_start_peer_migration_earlier, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_testonly_default_false, false)
diff --git a/quic/core/quic_packet_creator.cc b/quic/core/quic_packet_creator.cc
index 0ebfc30..7dbe37a 100644
--- a/quic/core/quic_packet_creator.cc
+++ b/quic/core/quic_packet_creator.cc
@@ -1423,7 +1423,10 @@
bool QuicPacketCreator::FlushAckFrame(const QuicFrames& frames) {
QUIC_BUG_IF(!flusher_attached_) << "Packet flusher is not attached when "
"generator tries to send ACK frame.";
- QUIC_BUG_IF(GetQuicReloadableFlag(quic_single_ack_in_packet) && has_ack())
+ // MaybeBundleAckOpportunistically could be called nestedly when sending a
+ // control frame causing another control frame to be sent.
+ QUIC_BUG_IF(GetQuicReloadableFlag(quic_single_ack_in_packet2) &&
+ !frames.empty() && has_ack())
<< "Trying to flush " << frames << " when there is ACK queued";
for (const auto& frame : frames) {
DCHECK(frame.type == ACK_FRAME || frame.type == STOP_WAITING_FRAME);
diff --git a/quic/test_tools/simple_session_notifier.cc b/quic/test_tools/simple_session_notifier.cc
index ccb784f..1a75a4c 100644
--- a/quic/test_tools/simple_session_notifier.cc
+++ b/quic/test_tools/simple_session_notifier.cc
@@ -125,6 +125,24 @@
WriteBufferedControlFrames();
}
+void SimpleSessionNotifier::WriteOrBufferAckFrequency(
+ const QuicAckFrequencyFrame& ack_frequency_frame) {
+ QUIC_DVLOG(1) << "Writing ACK_FREQUENCY";
+ const bool had_buffered_data =
+ HasBufferedStreamData() || HasBufferedControlFrames();
+ QuicControlFrameId control_frame_id = ++last_control_frame_id_;
+ control_frames_.emplace_back((
+ QuicFrame(new QuicAckFrequencyFrame(control_frame_id,
+ /*sequence_number=*/control_frame_id,
+ ack_frequency_frame.packet_tolerance,
+ ack_frequency_frame.max_ack_delay))));
+ if (had_buffered_data) {
+ QUIC_DLOG(WARNING) << "Connection is write blocked";
+ return;
+ }
+ WriteBufferedControlFrames();
+}
+
void SimpleSessionNotifier::NeuterUnencryptedData() {
if (QuicVersionUsesCryptoFrames(connection_->transport_version())) {
for (const auto& interval : crypto_bytes_transferred_[ENCRYPTION_INITIAL]) {
diff --git a/quic/test_tools/simple_session_notifier.h b/quic/test_tools/simple_session_notifier.h
index ac2f18a..5f7d4ae 100644
--- a/quic/test_tools/simple_session_notifier.h
+++ b/quic/test_tools/simple_session_notifier.h
@@ -35,6 +35,10 @@
// Tries to write PING.
void WriteOrBufferPing();
+ // Tries to write ACK_FREQUENCY.
+ void WriteOrBufferAckFrequency(
+ const QuicAckFrequencyFrame& ack_frequency_frame);
+
// Tries to write CRYPTO data and returns the number of bytes written.
size_t WriteCryptoData(EncryptionLevel level,
QuicByteCount data_length,
