Skip to content

Commit 21d6e3f

Browse files
DavidSchinazicopybara-github
authored andcommitted
Unblock quic_stop_sending_legacy_version_info
We realized that a few tests were failing when the values of quic_stop_sending_legacy_version_info and quic_stop_parsing_legacy_version_info were different, so quic_stop_sending_legacy_version_info was blocked in cl/853491571. This CL fixes the test issue and reverts cl/853491571. We confirmed that tests pass by running all four combinations via: ``` blaze test //third_party/quic/core/crypto:transport_parameters_test --test_arg=--gfe2_restart_flag_quic_stop_parsing_legacy_version_info=true --test_arg=--gfe2_restart_flag_quic_stop_sending_legacy_version_info=true && blaze test //third_party/quic/core/crypto:transport_parameters_test --test_arg=--gfe2_restart_flag_quic_stop_parsing_legacy_version_info=true --test_arg=--gfe2_restart_flag_quic_stop_sending_legacy_version_info=false && blaze test //third_party/quic/core/crypto:transport_parameters_test --test_arg=--gfe2_restart_flag_quic_stop_parsing_legacy_version_info=false --test_arg=--gfe2_restart_flag_quic_stop_sending_legacy_version_info=true && blaze test //third_party/quic/core/crypto:transport_parameters_test --test_arg=--gfe2_restart_flag_quic_stop_parsing_legacy_version_info=false --test_arg=--gfe2_restart_flag_quic_stop_sending_legacy_version_info=false ``` PiperOrigin-RevId: 855471392
1 parent 022c607 commit 21d6e3f

File tree

2 files changed

+29
-13
lines changed

2 files changed

+29
-13
lines changed

quiche/common/quiche_feature_flags_list.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ QUICHE_FLAG(bool, quiche_reloadable_flag_reset_content_length_status_when_removi
7272
QUICHE_FLAG(bool, quiche_restart_flag_quic_dispatcher_close_connection_on_invalid_ack, false, false, "An invalid ack is an ack that the peer sent for a packet that was not sent by the dispatcher. If true, the dispatcher will close the connection if it receives an invalid ack.")
7373
QUICHE_FLAG(bool, quiche_restart_flag_quic_shed_tls_handshake_config, false, false, "If true, QUIC connections will call SSL_set_shed_handshake_config to drop BoringSSL handshake state after the handshake finishes in order to save memory.")
7474
QUICHE_FLAG(bool, quiche_restart_flag_quic_stop_parsing_legacy_version_info, false, false, "If true, disable parsing the legacy version information transport parameter.")
75-
QUICHE_FLAG(bool, quiche_restart_flag_quic_stop_sending_legacy_version_info, false, false, "If true, disable sending the legacy version information transport parameter.")
75+
QUICHE_FLAG(bool, quiche_restart_flag_quic_stop_sending_legacy_version_info, false, true, "If true, disable sending the legacy version information transport parameter.")
7676
QUICHE_FLAG(bool, quiche_restart_flag_quic_support_release_time_for_gso, false, false, "If true, QuicGsoBatchWriter will support release time if it is available and the process has the permission to do so.")
7777
QUICHE_FLAG(bool, quiche_restart_flag_quic_testonly_default_false, false, false, "A testonly restart flag that will always default to false.")
7878
QUICHE_FLAG(bool, quiche_restart_flag_quic_testonly_default_true, true, true, "A testonly restart flag that will always default to true.")

quiche/quic/core/crypto/transport_parameters_test.cc

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,8 @@ const auto kCustomParameter1 =
5252
const char* kCustomParameter1Value = "foo";
5353
const auto kCustomParameter2 =
5454
static_cast<TransportParameters::TransportParameterId>(0xff34);
55+
const auto kLegacyVersionInfoParameter =
56+
static_cast<TransportParameters::TransportParameterId>(0x4752);
5557
const char* kCustomParameter2Value = "bar";
5658

5759
const char kFakeGoogleHandshakeMessage[] =
@@ -325,8 +327,7 @@ TEST_P(TransportParametersTest, CopyConstructor) {
325327
TEST_P(TransportParametersTest, RoundTripClient) {
326328
TransportParameters orig_params;
327329
orig_params.perspective = Perspective::IS_CLIENT;
328-
if (!GetQuicRestartFlag(quic_stop_parsing_legacy_version_info) ||
329-
!GetQuicRestartFlag(quic_stop_sending_legacy_version_info)) {
330+
if (!GetQuicRestartFlag(quic_stop_sending_legacy_version_info)) {
330331
orig_params.legacy_version_information =
331332
CreateFakeLegacyVersionInformationClient();
332333
}
@@ -373,14 +374,18 @@ TEST_P(TransportParametersTest, RoundTripClient) {
373374
<< error_details;
374375
EXPECT_TRUE(error_details.empty());
375376
RemoveGreaseParameters(&new_params);
377+
if (GetQuicRestartFlag(quic_stop_parsing_legacy_version_info) &&
378+
!GetQuicRestartFlag(quic_stop_sending_legacy_version_info)) {
379+
orig_params.legacy_version_information.reset();
380+
new_params.custom_parameters.erase(kLegacyVersionInfoParameter);
381+
}
376382
EXPECT_EQ(new_params, orig_params);
377383
}
378384

379385
TEST_P(TransportParametersTest, RoundTripServer) {
380386
TransportParameters orig_params;
381387
orig_params.perspective = Perspective::IS_SERVER;
382-
if (!GetQuicRestartFlag(quic_stop_parsing_legacy_version_info) ||
383-
!GetQuicRestartFlag(quic_stop_sending_legacy_version_info)) {
388+
if (!GetQuicRestartFlag(quic_stop_sending_legacy_version_info)) {
384389
orig_params.legacy_version_information =
385390
CreateFakeLegacyVersionInformationServer();
386391
}
@@ -423,6 +428,11 @@ TEST_P(TransportParametersTest, RoundTripServer) {
423428
<< error_details;
424429
EXPECT_TRUE(error_details.empty());
425430
RemoveGreaseParameters(&new_params);
431+
if (GetQuicRestartFlag(quic_stop_parsing_legacy_version_info) &&
432+
!GetQuicRestartFlag(quic_stop_sending_legacy_version_info)) {
433+
orig_params.legacy_version_information.reset();
434+
new_params.custom_parameters.erase(kLegacyVersionInfoParameter);
435+
}
426436
EXPECT_EQ(new_params, orig_params);
427437
}
428438

@@ -654,8 +664,7 @@ TEST_P(TransportParametersTest, ParseClientParams) {
654664
<< error_details;
655665
EXPECT_TRUE(error_details.empty());
656666
EXPECT_EQ(Perspective::IS_CLIENT, new_params.perspective);
657-
if (!GetQuicRestartFlag(quic_stop_parsing_legacy_version_info) ||
658-
!GetQuicRestartFlag(quic_stop_sending_legacy_version_info)) {
667+
if (!GetQuicRestartFlag(quic_stop_parsing_legacy_version_info)) {
659668
ASSERT_TRUE(new_params.legacy_version_information.has_value());
660669
EXPECT_EQ(kFakeVersionLabel,
661670
new_params.legacy_version_information.value().version);
@@ -921,8 +930,7 @@ TEST_P(TransportParametersTest, ParseServerParams) {
921930
<< error_details;
922931
EXPECT_TRUE(error_details.empty());
923932
EXPECT_EQ(Perspective::IS_SERVER, new_params.perspective);
924-
if (!GetQuicRestartFlag(quic_stop_parsing_legacy_version_info) ||
925-
!GetQuicRestartFlag(quic_stop_sending_legacy_version_info)) {
933+
if (!GetQuicRestartFlag(quic_stop_parsing_legacy_version_info)) {
926934
ASSERT_TRUE(new_params.legacy_version_information.has_value());
927935
EXPECT_EQ(kFakeVersionLabel,
928936
new_params.legacy_version_information.value().version);
@@ -1055,8 +1063,7 @@ TEST_P(TransportParametersTest, VeryLongCustomParameter) {
10551063
std::string custom_value(70000, '?');
10561064
TransportParameters orig_params;
10571065
orig_params.perspective = Perspective::IS_CLIENT;
1058-
if (!GetQuicRestartFlag(quic_stop_parsing_legacy_version_info) ||
1059-
!GetQuicRestartFlag(quic_stop_sending_legacy_version_info)) {
1066+
if (!GetQuicRestartFlag(quic_stop_sending_legacy_version_info)) {
10601067
orig_params.legacy_version_information =
10611068
CreateFakeLegacyVersionInformationClient();
10621069
}
@@ -1073,6 +1080,11 @@ TEST_P(TransportParametersTest, VeryLongCustomParameter) {
10731080
<< error_details;
10741081
EXPECT_TRUE(error_details.empty());
10751082
RemoveGreaseParameters(&new_params);
1083+
if (GetQuicRestartFlag(quic_stop_parsing_legacy_version_info) &&
1084+
!GetQuicRestartFlag(quic_stop_sending_legacy_version_info)) {
1085+
orig_params.legacy_version_information.reset();
1086+
new_params.custom_parameters.erase(kLegacyVersionInfoParameter);
1087+
}
10761088
EXPECT_EQ(new_params, orig_params);
10771089
}
10781090

@@ -1126,8 +1138,7 @@ TEST_P(TransportParametersTest, SerializationOrderIsRandom) {
11261138
TEST_P(TransportParametersTest, Degrease) {
11271139
TransportParameters orig_params;
11281140
orig_params.perspective = Perspective::IS_CLIENT;
1129-
if (!GetQuicRestartFlag(quic_stop_parsing_legacy_version_info) ||
1130-
!GetQuicRestartFlag(quic_stop_sending_legacy_version_info)) {
1141+
if (!GetQuicRestartFlag(quic_stop_sending_legacy_version_info)) {
11311142
orig_params.legacy_version_information =
11321143
CreateFakeLegacyVersionInformationClient();
11331144
}
@@ -1178,6 +1189,11 @@ TEST_P(TransportParametersTest, Degrease) {
11781189
EXPECT_NE(new_params, orig_params);
11791190

11801191
DegreaseTransportParameters(new_params);
1192+
if (GetQuicRestartFlag(quic_stop_parsing_legacy_version_info) &&
1193+
!GetQuicRestartFlag(quic_stop_sending_legacy_version_info)) {
1194+
orig_params.legacy_version_information.reset();
1195+
new_params.custom_parameters.erase(kLegacyVersionInfoParameter);
1196+
}
11811197
EXPECT_EQ(new_params, orig_params);
11821198
}
11831199

0 commit comments

Comments
 (0)