Remove the version check and TODO in `QuicFramer::TryDecryptInitialPacketDispatcher`. As it turns out, `QuicFramer::ParsePublicHeaderDispatcherShortHeaderLengthUnknown` supports RFCv2 just fine, the problem is in the test I added in cl/638463298. PiperOrigin-RevId: 702468882
diff --git a/quiche/quic/core/http/end_to_end_test.cc b/quiche/quic/core/http/end_to_end_test.cc index a2043dd..208fe89 100644 --- a/quiche/quic/core/http/end_to_end_test.cc +++ b/quiche/quic/core/http/end_to_end_test.cc
@@ -1219,24 +1219,15 @@ QuicConnection* server_connection = GetServerConnection(); ASSERT_NE(server_connection, nullptr); const QuicConnectionStats& server_stats = server_connection->GetStats(); - - if (version_ != ParsedQuicVersion::RFCv2()) { - EXPECT_EQ(server_stats.packets_sent_by_dispatcher, 1u); - } else { - EXPECT_EQ(server_stats.packets_sent_by_dispatcher, 0u); - } + EXPECT_EQ(server_stats.packets_sent_by_dispatcher, 1u); const QuicDispatcherStats& dispatcher_stats = GetDispatcherStats(); // The first CHLO packet is enqueued, the second causes session to be created. EXPECT_EQ(dispatcher_stats.packets_processed_with_unknown_cid, 2u); EXPECT_EQ(dispatcher_stats.packets_enqueued_early, 1u); EXPECT_EQ(dispatcher_stats.packets_enqueued_chlo, 0u); + EXPECT_EQ(dispatcher_stats.packets_sent, 1u); - if (version_ != ParsedQuicVersion::RFCv2()) { - EXPECT_EQ(dispatcher_stats.packets_sent, 1u); - } else { - EXPECT_EQ(dispatcher_stats.packets_sent, 0u); - } server_thread_->Resume(); } @@ -1270,25 +1261,12 @@ server_thread_->ScheduleAndWaitForCompletion([&] { const QuicDispatcherStats& dispatcher_stats = GetDispatcherStats(); EXPECT_EQ(dispatcher_stats.sessions_created, 1u); - - if (version_ != ParsedQuicVersion::RFCv2()) { - // 2 CHLO packets are enqueued, but only the 1st caused a dispatcher ACK. - EXPECT_EQ(dispatcher_stats.packets_sent, 1u); - EXPECT_EQ(dispatcher_stats.packets_processed_with_unknown_cid, 2u); - EXPECT_EQ(dispatcher_stats.packets_enqueued_early, 1u); - EXPECT_EQ(dispatcher_stats.packets_enqueued_chlo, 0u); - EXPECT_DEBUG_EQ( - dispatcher_stats.packets_processed_with_replaced_cid_in_store, 1u); - } else { - EXPECT_EQ(dispatcher_stats.packets_sent, 0u); - // 4 CHLO packets are sent by client, 1 of them is lost in client_writer_. - EXPECT_EQ(dispatcher_stats.packets_processed_with_unknown_cid, 3u); - // Packet 1 and its retransmission are enqueued early. - EXPECT_EQ(dispatcher_stats.packets_enqueued_early, 2u); - EXPECT_EQ(dispatcher_stats.packets_enqueued_chlo, 0u); - EXPECT_DEBUG_EQ( - dispatcher_stats.packets_processed_with_replaced_cid_in_store, 0u); - } + EXPECT_EQ(dispatcher_stats.packets_sent, 1u); + EXPECT_EQ(dispatcher_stats.packets_processed_with_unknown_cid, 2u); + EXPECT_EQ(dispatcher_stats.packets_enqueued_early, 1u); + EXPECT_EQ(dispatcher_stats.packets_enqueued_chlo, 0u); + EXPECT_DEBUG_EQ( + dispatcher_stats.packets_processed_with_replaced_cid_in_store, 1u); }); } @@ -1328,13 +1306,8 @@ EXPECT_EQ(dispatcher_stats.packets_enqueued_chlo, 1u); EXPECT_EQ(dispatcher_stats.packets_enqueued_early, 1u); EXPECT_EQ(dispatcher_stats.packets_processed_with_unknown_cid, 2u); - - if (version_ != ParsedQuicVersion::RFCv2()) { - // 2 CHLO packets are enqueued, but only the 1st caused a dispatcher ACK. - EXPECT_EQ(dispatcher_stats.packets_sent, 1u); - } else { - EXPECT_EQ(dispatcher_stats.packets_sent, 0u); - } + // 2 CHLO packets are enqueued, but only the 1st caused a dispatcher ACK. + EXPECT_EQ(dispatcher_stats.packets_sent, 1u); EXPECT_EQ(dispatcher_stats.sessions_created, 0u); GetDispatcher()->ProcessBufferedChlos(1); @@ -1366,12 +1339,7 @@ QuicConnection* server_connection = GetServerConnection(); ASSERT_NE(server_connection, nullptr); const QuicConnectionStats& server_stats = server_connection->GetStats(); - - if (version_ != ParsedQuicVersion::RFCv2()) { - EXPECT_EQ(server_stats.packets_sent_by_dispatcher, 2u); - } else { - EXPECT_EQ(server_stats.packets_sent_by_dispatcher, 0u); - } + EXPECT_EQ(server_stats.packets_sent_by_dispatcher, 2u); const QuicDispatcherStats& dispatcher_stats = GetDispatcherStats(); // The first and second CHLO packets are enqueued, the third causes session to @@ -1379,12 +1347,7 @@ EXPECT_EQ(dispatcher_stats.packets_processed_with_unknown_cid, 3u); EXPECT_EQ(dispatcher_stats.packets_enqueued_early, 2u); EXPECT_EQ(dispatcher_stats.packets_enqueued_chlo, 0u); - - if (version_ != ParsedQuicVersion::RFCv2()) { - EXPECT_EQ(dispatcher_stats.packets_sent, 2u); - } else { - EXPECT_EQ(dispatcher_stats.packets_sent, 0u); - } + EXPECT_EQ(dispatcher_stats.packets_sent, 2u); server_thread_->Resume(); } @@ -1413,12 +1376,7 @@ QuicConnection* server_connection = GetServerConnection(); ASSERT_NE(server_connection, nullptr); const QuicConnectionStats& server_stats = server_connection->GetStats(); - - if (version_ != ParsedQuicVersion::RFCv2()) { - EXPECT_EQ(server_stats.packets_sent_by_dispatcher, 1u); - } else { - EXPECT_EQ(server_stats.packets_sent_by_dispatcher, 0u); - } + EXPECT_EQ(server_stats.packets_sent_by_dispatcher, 1u); const QuicDispatcherStats& dispatcher_stats = GetDispatcherStats(); // The first and second CHLO packets are enqueued, the third causes session to @@ -1426,12 +1384,7 @@ EXPECT_EQ(dispatcher_stats.packets_processed_with_unknown_cid, 3u); EXPECT_EQ(dispatcher_stats.packets_enqueued_early, 2u); EXPECT_EQ(dispatcher_stats.packets_enqueued_chlo, 0u); - - if (version_ != ParsedQuicVersion::RFCv2()) { - EXPECT_EQ(dispatcher_stats.packets_sent, 1u); - } else { - EXPECT_EQ(dispatcher_stats.packets_sent, 0u); - } + EXPECT_EQ(dispatcher_stats.packets_sent, 1u); server_thread_->Resume(); } @@ -1466,24 +1419,13 @@ const QuicDispatcherStats& dispatcher_stats = GetDispatcherStats(); EXPECT_EQ(dispatcher_stats.sessions_created, 1u); - if (version_ != ParsedQuicVersion::RFCv2()) { - // Packet 1 and Packet 2's retransmission caused dispatcher ACKs. - EXPECT_EQ(dispatcher_stats.packets_sent, 2u); - EXPECT_EQ(dispatcher_stats.packets_processed_with_unknown_cid, 3u); - EXPECT_EQ(dispatcher_stats.packets_enqueued_early, 2u); - EXPECT_EQ(dispatcher_stats.packets_enqueued_chlo, 0u); - EXPECT_DEBUG_EQ( - dispatcher_stats.packets_processed_with_replaced_cid_in_store, 2u); - } else { - EXPECT_EQ(dispatcher_stats.packets_sent, 0u); - // 6 CHLO packets are sent by client, 2 of them are lost in client_writer. - EXPECT_EQ(dispatcher_stats.packets_processed_with_unknown_cid, 4u); - // Packet 1 and packet 1 & 2's retransmissions are enqueued early. - EXPECT_EQ(dispatcher_stats.packets_enqueued_early, 3u); - EXPECT_EQ(dispatcher_stats.packets_enqueued_chlo, 0u); - EXPECT_DEBUG_EQ( - dispatcher_stats.packets_processed_with_replaced_cid_in_store, 0u); - } + // Packet 1 and Packet 2's retransmission caused dispatcher ACKs. + EXPECT_EQ(dispatcher_stats.packets_sent, 2u); + EXPECT_EQ(dispatcher_stats.packets_processed_with_unknown_cid, 3u); + EXPECT_EQ(dispatcher_stats.packets_enqueued_early, 2u); + EXPECT_EQ(dispatcher_stats.packets_enqueued_chlo, 0u); + EXPECT_DEBUG_EQ( + dispatcher_stats.packets_processed_with_replaced_cid_in_store, 2u); }); }
diff --git a/quiche/quic/core/quic_framer.cc b/quiche/quic/core/quic_framer.cc index 6418968..32a54dd 100644 --- a/quiche/quic/core/quic_framer.cc +++ b/quiche/quic/core/quic_framer.cc
@@ -6493,12 +6493,6 @@ QUICHE_DCHECK(packet_number != nullptr); packet_number->reset(); - // TODO(wub): Remove the version check once RFCv2 is supported by - // ParsePublicHeaderDispatcherShortHeaderLengthUnknown. - if (version != ParsedQuicVersion::RFCv1() && - version != ParsedQuicVersion::Draft29()) { - return QUIC_NO_ERROR; - } if (packet.length() == 0 || format != IETF_QUIC_LONG_HEADER_PACKET || !VersionHasIetfQuicFrames(version.transport_version) || long_packet_type != INITIAL) {
diff --git a/quiche/quic/core/quic_framer_test.cc b/quiche/quic/core/quic_framer_test.cc index c0d27d9..a11b638 100644 --- a/quiche/quic/core/quic_framer_test.cc +++ b/quiche/quic/core/quic_framer_test.cc
@@ -13285,6 +13285,9 @@ } TEST_P(QuicFramerTest, DispatcherParseClientInitialPacketNumber) { + if (!version_.HasIetfQuicFrames()) { + return; + } // clang-format off PacketFragments packet = { // Type (Long header, INITIAL, 2B packet number) @@ -13314,6 +13317,7 @@ }; // clang-format on + ReviseFirstByteByVersion(packet); SetDecrypterLevel(ENCRYPTION_INITIAL); std::unique_ptr<QuicEncryptedPacket> encrypted( AssemblePacketFromFragments(packet)); @@ -13336,10 +13340,6 @@ &destination_connection_id, &source_connection_id, &retry_token, &detailed_error, generator)); EXPECT_EQ(parsed_version, version_); - if (parsed_version != ParsedQuicVersion::RFCv1() && - parsed_version != ParsedQuicVersion::Draft29()) { - return; - } EXPECT_EQ(format, IETF_QUIC_LONG_HEADER_PACKET); EXPECT_EQ(destination_connection_id.length(), 8); EXPECT_EQ(long_packet_type, INITIAL); @@ -13363,7 +13363,7 @@ TEST_P(QuicFramerTest, DispatcherParseClientInitialPacketNumberFromCoalescedPacket) { - if (!QuicVersionHasLongHeaderLengths(framer_.transport_version())) { + if (!version_.HasIetfQuicFrames()) { return; } SetDecrypterLevel(ENCRYPTION_INITIAL); @@ -13457,10 +13457,6 @@ &destination_connection_id, &source_connection_id, &retry_token, &detailed_error, generator)); EXPECT_EQ(parsed_version, version_); - if (parsed_version != ParsedQuicVersion::RFCv1() && - parsed_version != ParsedQuicVersion::Draft29()) { - return; - } EXPECT_EQ(format, IETF_QUIC_LONG_HEADER_PACKET); EXPECT_EQ(destination_connection_id.length(), 8); EXPECT_EQ(long_packet_type, INITIAL);