gfe-relnote: In QUIC, call NeuterHandshakePackets() at most once per connection. Protected by gfe2_reloadable_flag_quic_neuter_handshake_packets_once2 which replaces gfe2_reloadable_flag_quic_neuter_handshake_packets_once.

Also fix the bug where RTO and PTO try to retransmit not in flight packets.

PiperOrigin-RevId: 279118870
Change-Id: I1235372277109c18fded6c8d162a2147424d3dbd

quic/core/frames/quic_stream_frame.cc

diff --git a/quic/core/frames/quic_stream_frame.cc b/quic/core/frames/quic_stream_frame.cc
index c7626d3..d0e65d2 100644
--- a/quic/core/frames/quic_stream_frame.cc
+++ b/quic/core/frames/quic_stream_frame.cc
@@ -49,4 +49,8 @@
          offset == rhs.offset;
 }
 
+bool QuicStreamFrame::operator!=(const QuicStreamFrame& rhs) const {
+  return !(*this == rhs);
+}
+
 }  // namespace quic

quic/core/frames/quic_stream_frame.h

diff --git a/quic/core/frames/quic_stream_frame.h b/quic/core/frames/quic_stream_frame.h
index 5e7439d..5c5323b 100644
--- a/quic/core/frames/quic_stream_frame.h
+++ b/quic/core/frames/quic_stream_frame.h
@@ -33,6 +33,8 @@
 
   bool operator==(const QuicStreamFrame& rhs) const;
 
+  bool operator!=(const QuicStreamFrame& rhs) const;
+
   bool fin;
   QuicPacketLength data_length;
   QuicStreamId stream_id;

quic/core/quic_sent_packet_manager.cc

diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc
index c8c870b..cdc5a1c 100644
--- a/quic/core/quic_sent_packet_manager.cc
+++ b/quic/core/quic_sent_packet_manager.cc
@@ -107,7 +107,7 @@
       pto_exponential_backoff_start_point_(0),
       pto_rttvar_multiplier_(4),
       neuter_handshake_packets_once_(
-          GetQuicReloadableFlag(quic_neuter_handshake_packets_once)) {
+          GetQuicReloadableFlag(quic_neuter_handshake_packets_once2)) {
   SetSendAlgorithm(congestion_control_type);
 }
 
@@ -342,7 +342,7 @@
 void QuicSentPacketManager::SetHandshakeConfirmed() {
   if (!neuter_handshake_packets_once_ || !handshake_confirmed_) {
     if (neuter_handshake_packets_once_) {
-      QUIC_RELOADABLE_FLAG_COUNT(quic_neuter_handshake_packets_once);
+      QUIC_RELOADABLE_FLAG_COUNT_N(quic_neuter_handshake_packets_once2, 1, 3);
     }
     handshake_confirmed_ = true;
     NeuterHandshakePackets();
@@ -454,7 +454,7 @@
 
 void QuicSentPacketManager::NeuterUnencryptedPackets() {
   QuicPacketNumber packet_number = unacked_packets_.GetLeastUnacked();
-  for (QuicUnackedPacketMap::const_iterator it = unacked_packets_.begin();
+  for (QuicUnackedPacketMap::iterator it = unacked_packets_.begin();
        it != unacked_packets_.end(); ++it, ++packet_number) {
     if (!it->retransmittable_frames.empty() &&
         it->encryption_level == ENCRYPTION_INITIAL) {
@@ -462,18 +462,31 @@
       // will be sent. The data has been abandoned in the cryto stream. Remove
       // it from in flight.
       unacked_packets_.RemoveFromInFlight(packet_number);
+      if (neuter_handshake_packets_once_) {
+        QUIC_RELOADABLE_FLAG_COUNT_N(quic_neuter_handshake_packets_once2, 2, 3);
+        it->state = NEUTERED;
+        DCHECK(!unacked_packets_.HasRetransmittableFrames(*it));
+      }
     }
   }
 }
 
 void QuicSentPacketManager::NeuterHandshakePackets() {
   QuicPacketNumber packet_number = unacked_packets_.GetLeastUnacked();
-  for (QuicUnackedPacketMap::const_iterator it = unacked_packets_.begin();
+  for (QuicUnackedPacketMap::iterator it = unacked_packets_.begin();
        it != unacked_packets_.end(); ++it, ++packet_number) {
     if (!it->retransmittable_frames.empty() &&
         unacked_packets_.GetPacketNumberSpace(it->encryption_level) ==
             HANDSHAKE_DATA) {
       unacked_packets_.RemoveFromInFlight(packet_number);
+      if (neuter_handshake_packets_once_) {
+        // Notify session that the data has been delivered (but do not notify
+        // send algorithm).
+        QUIC_RELOADABLE_FLAG_COUNT_N(quic_neuter_handshake_packets_once2, 3, 3);
+        it->state = NEUTERED;
+        unacked_packets_.NotifyFramesAcked(*it, QuicTime::Delta::Zero(),
+                                           QuicTime::Zero());
+      }
     }
   }
 }
@@ -765,6 +778,7 @@
     if (it->state == OUTSTANDING &&
         unacked_packets_.HasRetransmittableFrames(*it) &&
         pending_timer_transmission_count_ < max_rto_packets_) {
+      DCHECK(!neuter_handshake_packets_once_ || it->in_flight);
       retransmissions.push_back(packet_number);
       ++pending_timer_transmission_count_;
     }
@@ -797,6 +811,7 @@
        it != unacked_packets_.end(); ++it, ++packet_number) {
     if (it->state == OUTSTANDING &&
         unacked_packets_.HasRetransmittableFrames(*it)) {
+      DCHECK(!neuter_handshake_packets_once_ || it->in_flight);
       probing_packets.push_back(packet_number);
       if (probing_packets.size() == pending_timer_transmission_count_) {
         break;

quic/core/quic_sent_packet_manager.h

diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h
index 95b9ee2..67ded33 100644
--- a/quic/core/quic_sent_packet_manager.h
+++ b/quic/core/quic_sent_packet_manager.h
@@ -646,7 +646,7 @@
   // The multiplier of rttvar when calculating PTO timeout.
   int pto_rttvar_multiplier_;
 
-  // Latched value of quic_neuter_handshake_packets_once.
+  // Latched value of quic_neuter_handshake_packets_once2.
   const bool neuter_handshake_packets_once_;
 };
 

quic/core/quic_sent_packet_manager_test.cc

diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc
index 539ce97..1bd08d9 100644
--- a/quic/core/quic_sent_packet_manager_test.cc
+++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -1125,9 +1125,9 @@
 
   // Now neuter all unacked unencrypted packets, which occurs when the
   // connection goes forward secure.
-  manager_.NeuterUnencryptedPackets();
   EXPECT_CALL(notifier_, HasUnackedCryptoData()).WillRepeatedly(Return(false));
   EXPECT_CALL(notifier_, IsFrameOutstanding(_)).WillRepeatedly(Return(false));
+  manager_.NeuterUnencryptedPackets();
   EXPECT_FALSE(manager_.HasUnackedCryptoPackets());
   uint64_t unacked[] = {1, 2, 3};
   VerifyUnackedPackets(unacked, QUIC_ARRAYSIZE(unacked));
@@ -3055,6 +3055,58 @@
             manager_.GetRetransmissionTime());
 }
 
+// Regression test for b/143962153
+TEST_F(QuicSentPacketManagerTest, RtoNotInFlightPacket) {
+  QuicSentPacketManagerPeer::SetMaxTailLossProbes(&manager_, 2);
+  // Send SHLO.
+  QuicStreamFrame crypto_frame(1, false, 0, QuicStringPiece());
+  SendCryptoPacket(1);
+  // Send data packet.
+  SendDataPacket(2, ENCRYPTION_FORWARD_SECURE);
+
+  // Successfully decrypt a forward secure packet.
+  if (GetQuicReloadableFlag(quic_neuter_handshake_packets_once2)) {
+    EXPECT_CALL(notifier_, OnFrameAcked(_, _, _)).Times(1);
+  } else {
+    EXPECT_CALL(notifier_, OnFrameAcked(_, _, _)).Times(0);
+  }
+  manager_.SetHandshakeConfirmed();
+
+  // 1st TLP.
+  manager_.OnRetransmissionTimeout();
+  EXPECT_CALL(notifier_, RetransmitFrames(_, _))
+      .WillOnce(WithArgs<1>(Invoke([this](TransmissionType type) {
+        RetransmitDataPacket(3, type, ENCRYPTION_FORWARD_SECURE);
+      })));
+  manager_.MaybeRetransmitTailLossProbe();
+
+  // 2nd TLP.
+  manager_.OnRetransmissionTimeout();
+  EXPECT_CALL(notifier_, RetransmitFrames(_, _))
+      .WillOnce(WithArgs<1>(Invoke([this](TransmissionType type) {
+        RetransmitDataPacket(4, type, ENCRYPTION_FORWARD_SECURE);
+      })));
+  manager_.MaybeRetransmitTailLossProbe();
+
+  // RTO retransmits SHLO although it is not in flight.
+  size_t num_rto_packets = 2;
+  if (GetQuicReloadableFlag(quic_neuter_handshake_packets_once2)) {
+    num_rto_packets = 1;
+  }
+  EXPECT_CALL(notifier_, RetransmitFrames(_, _))
+      .Times(num_rto_packets)
+      .WillOnce(
+          WithArgs<0>(Invoke([this, &crypto_frame](const QuicFrames& frames) {
+            EXPECT_EQ(1u, frames.size());
+            if (GetQuicReloadableFlag(quic_neuter_handshake_packets_once2)) {
+              EXPECT_NE(crypto_frame, frames[0].stream_frame);
+            } else {
+              EXPECT_EQ(crypto_frame, frames[0].stream_frame);
+            }
+          })));
+  manager_.OnRetransmissionTimeout();
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace quic

quic/core/quic_types.h

diff --git a/quic/core/quic_types.h b/quic/core/quic_types.h
index 972e57c..06931dc 100644
--- a/quic/core/quic_types.h
+++ b/quic/core/quic_types.h
@@ -446,7 +446,7 @@
 };
 
 enum SentPacketState : uint8_t {
-  // The packet has been sent and waiting to be acked.
+  // The packet is in flight and waiting to be acked.
   OUTSTANDING,
   FIRST_PACKET_STATE = OUTSTANDING,
   // The packet was never sent.
@@ -455,6 +455,8 @@
   ACKED,
   // This packet is not expected to be acked.
   UNACKABLE,
+  // This packet has been delivered or unneeded.
+  NEUTERED,
 
   // States below are corresponding to retransmission types in TransmissionType.
 

quic/core/quic_utils.cc

diff --git a/quic/core/quic_utils.cc b/quic/core/quic_utils.cc
index a6f6577..eed0e37 100644
--- a/quic/core/quic_utils.cc
+++ b/quic/core/quic_utils.cc
@@ -161,6 +161,7 @@
     RETURN_STRING_LITERAL(NEVER_SENT);
     RETURN_STRING_LITERAL(ACKED);
     RETURN_STRING_LITERAL(UNACKABLE);
+    RETURN_STRING_LITERAL(NEUTERED);
     RETURN_STRING_LITERAL(HANDSHAKE_RETRANSMITTED);
     RETURN_STRING_LITERAL(LOST);
     RETURN_STRING_LITERAL(TLP_RETRANSMITTED);

quic/test_tools/simple_session_notifier.cc

diff --git a/quic/test_tools/simple_session_notifier.cc b/quic/test_tools/simple_session_notifier.cc
index 5ba12c6..9e67c89 100644
--- a/quic/test_tools/simple_session_notifier.cc
+++ b/quic/test_tools/simple_session_notifier.cc
@@ -126,8 +126,13 @@
 }
 
 void SimpleSessionNotifier::NeuterUnencryptedData() {
-  // TODO(nharper): Handle CRYPTO frame case.
   if (QuicVersionUsesCryptoFrames(connection_->transport_version())) {
+    for (const auto& interval : crypto_bytes_transferred_[ENCRYPTION_INITIAL]) {
+      QuicCryptoFrame crypto_frame(ENCRYPTION_INITIAL, interval.min(),
+                                   interval.max() - interval.min());
+      OnFrameAcked(QuicFrame(&crypto_frame), QuicTime::Delta::Zero(),
+                   QuicTime::Zero());
+    }
     return;
   }
   for (const auto& interval : crypto_bytes_transferred_[ENCRYPTION_INITIAL]) {