Fix flaky EndToEndTestWithTls.NoUndecryptablePackets
This test was failing on rare occasions because due to reordering the server would end up receiving a retransmission of the CHLO after it completed the handshake and discarded initial keys. Since QuicConnectionStats::undecryptable_packets_received had been added solely for the purpose of this test and isn't used elsewhere, we replaced it with undecryptable_packets_received_before_handshake_complete which is what we want for this test. Instead of testing for the absence of undecryptable packets at all, we now only test for them before the handshake. This still covers the regression that this test was created for.
gfe-relnote: change statistic that is only used by tests
PiperOrigin-RevId: 269444711
Change-Id: Ie846162b6bf7b65c8c3f7196b9cd58e29d45d892
diff --git a/quic/core/http/end_to_end_test.cc b/quic/core/http/end_to_end_test.cc
index 5845f78..dfaa9ec 100644
--- a/quic/core/http/end_to_end_test.cc
+++ b/quic/core/http/end_to_end_test.cc
@@ -972,18 +972,26 @@
EXPECT_EQ("200", client_->response_headers()->find(":status")->second);
}
-TEST_P(EndToEndTestWithTls, NoUndecryptablePackets) {
+// Regression test for a bug where we would always fail to decrypt the first
+// initial packet. Undecryptable packets can be seen after the handshake
+// is complete due to dropping the initial keys at that point, so we only test
+// for undecryptable packets before then.
+TEST_P(EndToEndTestWithTls, NoUndecryptablePacketsBeforeHandshakeComplete) {
ASSERT_TRUE(Initialize());
EXPECT_EQ(kFooResponseBody, client_->SendSynchronousRequest("/foo"));
EXPECT_EQ("200", client_->response_headers()->find(":status")->second);
QuicConnectionStats client_stats = GetClientConnection()->GetStats();
- EXPECT_EQ(0u, client_stats.undecryptable_packets_received);
+ EXPECT_EQ(
+ 0u,
+ client_stats.undecryptable_packets_received_before_handshake_complete);
server_thread_->Pause();
QuicConnectionStats server_stats = GetServerConnection()->GetStats();
- EXPECT_EQ(0u, server_stats.undecryptable_packets_received);
+ EXPECT_EQ(
+ 0u,
+ server_stats.undecryptable_packets_received_before_handshake_complete);
server_thread_->Resume();
}
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index 52ea3da..6f1b4d5 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -1702,7 +1702,9 @@
DCHECK(GetQuicRestartFlag(quic_framer_uses_undecryptable_upcall));
QUIC_RESTART_FLAG_COUNT_N(quic_framer_uses_undecryptable_upcall, 1, 7);
DCHECK(EncryptionLevelIsValid(decryption_level));
- ++stats_.undecryptable_packets_received;
+ if (encryption_level_ != ENCRYPTION_FORWARD_SECURE) {
+ ++stats_.undecryptable_packets_received_before_handshake_complete;
+ }
bool should_enqueue = true;
if (encryption_level_ == ENCRYPTION_FORWARD_SECURE) {
@@ -1793,7 +1795,9 @@
// because the CHLO or SHLO packet was lost.
if (framer_.error() == QUIC_DECRYPTION_FAILURE &&
!GetQuicRestartFlag(quic_framer_uses_undecryptable_upcall)) {
- ++stats_.undecryptable_packets_received;
+ if (encryption_level_ != ENCRYPTION_FORWARD_SECURE) {
+ ++stats_.undecryptable_packets_received_before_handshake_complete;
+ }
if (encryption_level_ != ENCRYPTION_FORWARD_SECURE &&
undecryptable_packets_.size() < max_undecryptable_packets_) {
QueueUndecryptablePacket(packet);
@@ -2821,7 +2825,9 @@
// because the CHLO or SHLO packet was lost.
if (framer_.error() == QUIC_DECRYPTION_FAILURE &&
!GetQuicRestartFlag(quic_framer_uses_undecryptable_upcall)) {
- ++stats_.undecryptable_packets_received;
+ if (encryption_level_ != ENCRYPTION_FORWARD_SECURE) {
+ ++stats_.undecryptable_packets_received_before_handshake_complete;
+ }
if (encryption_level_ != ENCRYPTION_FORWARD_SECURE &&
undecryptable_packets_.size() < max_undecryptable_packets_) {
QueueUndecryptablePacket(*packet);
diff --git a/quic/core/quic_connection_stats.cc b/quic/core/quic_connection_stats.cc
index 98859ac..c08f939 100644
--- a/quic/core/quic_connection_stats.cc
+++ b/quic/core/quic_connection_stats.cc
@@ -29,7 +29,7 @@
slowstart_duration(QuicTime::Delta::Zero()),
slowstart_start_time(QuicTime::Zero()),
packets_dropped(0),
- undecryptable_packets_received(0),
+ undecryptable_packets_received_before_handshake_complete(0),
crypto_retransmit_count(0),
loss_timeout_count(0),
tlp_count(0),
@@ -73,7 +73,8 @@
os << " slowstart_packets_lost: " << s.slowstart_packets_lost;
os << " slowstart_bytes_lost: " << s.slowstart_bytes_lost;
os << " packets_dropped: " << s.packets_dropped;
- os << " undecryptable_packets_received: " << s.undecryptable_packets_received;
+ os << " undecryptable_packets_received_before_handshake_complete: "
+ << s.undecryptable_packets_received_before_handshake_complete;
os << " crypto_retransmit_count: " << s.crypto_retransmit_count;
os << " loss_timeout_count: " << s.loss_timeout_count;
os << " tlp_count: " << s.tlp_count;
diff --git a/quic/core/quic_connection_stats.h b/quic/core/quic_connection_stats.h
index 56bb510..17a3a1e 100644
--- a/quic/core/quic_connection_stats.h
+++ b/quic/core/quic_connection_stats.h
@@ -67,8 +67,9 @@
QuicPacketCount packets_dropped; // Duplicate or less than least unacked.
- // Packets that failed to decrypt when they were first received.
- QuicPacketCount undecryptable_packets_received;
+ // Packets that failed to decrypt when they were first received,
+ // before the handshake was complete.
+ QuicPacketCount undecryptable_packets_received_before_handshake_complete;
size_t crypto_retransmit_count;
// Count of times the loss detection alarm fired. At least one packet should
