Deprecate --gfe2_reloadable_flag_quic_parse_accept_ch_frame. PiperOrigin-RevId: 359827183 Change-Id: I91f8989e4943055d1aed7c96baca8e2dd8086a54
diff --git a/quic/core/http/http_decoder.cc b/quic/core/http/http_decoder.cc index fe33a81..95f5079 100644 --- a/quic/core/http/http_decoder.cc +++ b/quic/core/http/http_decoder.cc
@@ -238,13 +238,7 @@ continue_processing = visitor_->OnPriorityUpdateFrameStart(header_length); break; case static_cast<uint64_t>(HttpFrameType::ACCEPT_CH): - if (GetQuicReloadableFlag(quic_parse_accept_ch_frame)) { - QUIC_RELOADABLE_FLAG_COUNT(quic_parse_accept_ch_frame); - continue_processing = visitor_->OnAcceptChFrameStart(header_length); - } else { - continue_processing = visitor_->OnUnknownFrameStart( - current_frame_type_, header_length, current_frame_length_); - } + continue_processing = visitor_->OnAcceptChFrameStart(header_length); break; default: continue_processing = visitor_->OnUnknownFrameStart( @@ -381,13 +375,9 @@ break; } case static_cast<uint64_t>(HttpFrameType::ACCEPT_CH): { - if (GetQuicReloadableFlag(quic_parse_accept_ch_frame)) { - // TODO(bnc): Avoid buffering if the entire frame is present, and - // instead parse directly out of |reader|. - BufferFramePayload(reader); - } else { - continue_processing = HandleUnknownFramePayload(reader); - } + // TODO(bnc): Avoid buffering if the entire frame is present, and + // instead parse directly out of |reader|. + BufferFramePayload(reader); break; } default: { @@ -499,18 +489,14 @@ break; } case static_cast<uint64_t>(HttpFrameType::ACCEPT_CH): { - if (GetQuicReloadableFlag(quic_parse_accept_ch_frame)) { - // TODO(bnc): Avoid buffering if the entire frame is present, and - // instead parse directly out of |reader|. - AcceptChFrame frame; - QuicDataReader reader(buffer_.data(), current_frame_length_); - if (!ParseAcceptChFrame(&reader, &frame)) { - return false; - } - continue_processing = visitor_->OnAcceptChFrame(frame); - } else { - continue_processing = visitor_->OnUnknownFrameEnd(); + // TODO(bnc): Avoid buffering if the entire frame is present, and + // instead parse directly out of |reader|. + AcceptChFrame frame; + QuicDataReader reader(buffer_.data(), current_frame_length_); + if (!ParseAcceptChFrame(&reader, &frame)) { + return false; } + continue_processing = visitor_->OnAcceptChFrame(frame); break; } default:
diff --git a/quic/core/http/http_decoder_test.cc b/quic/core/http/http_decoder_test.cc index 1be5084..4f518a4 100644 --- a/quic/core/http/http_decoder_test.cc +++ b/quic/core/http/http_decoder_test.cc
@@ -867,11 +867,43 @@ "\x04" // length "\x05" // valid stream id "foo", // superfluous data - "Superfluous data in GOAWAY frame."}}; + "Superfluous data in GOAWAY frame."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x01" // length + "\x40", // first byte of two-byte varint origin length + "Unable to read ACCEPT_CH origin."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x01" // length + "\x05", // valid origin length but no origin string + "Unable to read ACCEPT_CH origin."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x04" // length + "\x05" // valid origin length + "foo", // payload ends before origin ends + "Unable to read ACCEPT_CH origin."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x04" // length + "\x03" // valid origin length + "foo", // payload ends at end of origin: no value + "Unable to read ACCEPT_CH value."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x05" // length + "\x03" // valid origin length + "foo" // payload ends at end of origin: no value + "\x40", // first byte of two-byte varint value length + "Unable to read ACCEPT_CH value."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x08" // length + "\x03" // valid origin length + "foo" // origin + "\x05" // valid value length + "bar", // payload ends before value ends + "Unable to read ACCEPT_CH value."}}; for (const auto& test_data : kTestData) { { HttpDecoder decoder(&visitor_); + EXPECT_CALL(visitor_, OnAcceptChFrameStart(_)).Times(AnyNumber()); EXPECT_CALL(visitor_, OnError(&decoder)); absl::string_view input(test_data.input); @@ -881,6 +913,7 @@ } { HttpDecoder decoder(&visitor_); + EXPECT_CALL(visitor_, OnAcceptChFrameStart(_)).Times(AnyNumber()); EXPECT_CALL(visitor_, OnError(&decoder)); absl::string_view input(test_data.input); @@ -1194,10 +1227,6 @@ } TEST_F(HttpDecoderTest, AcceptChFrame) { - if (!GetQuicReloadableFlag(quic_parse_accept_ch_frame)) { - return; - } - InSequence s; std::string input1 = absl::HexStringToBytes( "4089" // type (ACCEPT_CH) @@ -1272,74 +1301,6 @@ EXPECT_EQ("", decoder_.error_detail()); } -TEST_F(HttpDecoderTest, CorruptAcceptChFrame) { - if (!GetQuicReloadableFlag(quic_parse_accept_ch_frame)) { - // TODO(bnc): merge this test into HttpDecoderTest.CorruptFrame when - // deprecating flag. - return; - } - - EXPECT_CALL(visitor_, OnAcceptChFrameStart(_)).Times(AnyNumber()); - - struct { - const char* const input; - const char* const error_message; - } kTestData[] = {{"\x40\x89" // type (ACCEPT_CH) - "\x01" // length - "\x40", // first byte of two-byte varint origin length - "Unable to read ACCEPT_CH origin."}, - {"\x40\x89" // type (ACCEPT_CH) - "\x01" // length - "\x05", // valid origin length but no origin string - "Unable to read ACCEPT_CH origin."}, - {"\x40\x89" // type (ACCEPT_CH) - "\x04" // length - "\x05" // valid origin length - "foo", // payload ends before origin ends - "Unable to read ACCEPT_CH origin."}, - {"\x40\x89" // type (ACCEPT_CH) - "\x04" // length - "\x03" // valid origin length - "foo", // payload ends at end of origin: no value - "Unable to read ACCEPT_CH value."}, - {"\x40\x89" // type (ACCEPT_CH) - "\x05" // length - "\x03" // valid origin length - "foo" // payload ends at end of origin: no value - "\x40", // first byte of two-byte varint value length - "Unable to read ACCEPT_CH value."}, - {"\x40\x89" // type (ACCEPT_CH) - "\x08" // length - "\x03" // valid origin length - "foo" // origin - "\x05" // valid value length - "bar", // payload ends before value ends - "Unable to read ACCEPT_CH value."}}; - - for (const auto& test_data : kTestData) { - { - HttpDecoder decoder(&visitor_); - EXPECT_CALL(visitor_, OnError(&decoder)); - - absl::string_view input(test_data.input); - decoder.ProcessInput(input.data(), input.size()); - EXPECT_THAT(decoder.error(), IsError(QUIC_HTTP_FRAME_ERROR)); - EXPECT_EQ(test_data.error_message, decoder.error_detail()); - } - { - HttpDecoder decoder(&visitor_); - EXPECT_CALL(visitor_, OnError(&decoder)); - - absl::string_view input(test_data.input); - for (auto c : input) { - decoder.ProcessInput(&c, 1); - } - EXPECT_THAT(decoder.error(), IsError(QUIC_HTTP_FRAME_ERROR)); - EXPECT_EQ(test_data.error_message, decoder.error_detail()); - } - } -} - TEST_F(HttpDecoderTest, DecodeSettings) { std::string input = absl::HexStringToBytes( "04" // type (SETTINGS)
diff --git a/quic/core/http/quic_receive_control_stream.cc b/quic/core/http/quic_receive_control_stream.cc index 35d5902..ffc38a0 100644 --- a/quic/core/http/quic_receive_control_stream.cc +++ b/quic/core/http/quic_receive_control_stream.cc
@@ -263,8 +263,7 @@ frame_type == HttpFrameType::PUSH_PROMISE) || (spdy_session()->perspective() == Perspective::IS_CLIENT && frame_type == HttpFrameType::MAX_PUSH_ID) || - (GetQuicReloadableFlag(quic_parse_accept_ch_frame) && - spdy_session()->perspective() == Perspective::IS_SERVER && + (spdy_session()->perspective() == Perspective::IS_SERVER && frame_type == HttpFrameType::ACCEPT_CH)) { stream_delegate()->OnStreamError( QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM,
diff --git a/quic/core/http/quic_receive_control_stream_test.cc b/quic/core/http/quic_receive_control_stream_test.cc index 3e4532f..491255d 100644 --- a/quic/core/http/quic_receive_control_stream_test.cc +++ b/quic/core/http/quic_receive_control_stream_test.cc
@@ -404,8 +404,7 @@ "4089" // type (ACCEPT_CH) "00"); // length - if (GetQuicReloadableFlag(quic_parse_accept_ch_frame) && - perspective() == Perspective::IS_SERVER) { + if (perspective() == Perspective::IS_SERVER) { EXPECT_CALL(*connection_, CloseConnection( QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM, @@ -449,23 +448,17 @@ "4089" // type (ACCEPT_CH) "00"); // length - if (GetQuicReloadableFlag(quic_parse_accept_ch_frame)) { - if (perspective() == Perspective::IS_CLIENT) { - EXPECT_CALL(debug_visitor, OnAcceptChFrameReceived(_)); - } else { - EXPECT_CALL(*connection_, - CloseConnection( - QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM, - "Invalid frame type 137 received on control stream.", _)) - .WillOnce( - Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); - EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _, _)); - EXPECT_CALL(session_, OnConnectionClosed(_, _)); - } + if (perspective() == Perspective::IS_CLIENT) { + EXPECT_CALL(debug_visitor, OnAcceptChFrameReceived(_)); } else { - EXPECT_CALL(debug_visitor, - OnUnknownFrameReceived(id, /* frame_type = */ 0x89, - /* payload_length = */ 0)); + EXPECT_CALL(*connection_, + CloseConnection( + QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM, + "Invalid frame type 137 received on control stream.", _)) + .WillOnce( + Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); + EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _, _)); + EXPECT_CALL(session_, OnConnectionClosed(_, _)); } receive_control_stream_->OnStreamFrame(
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc index b266633..3985769 100644 --- a/quic/core/http/quic_spdy_session_test.cc +++ b/quic/core/http/quic_spdy_session_test.cc
@@ -3203,10 +3203,6 @@ return; } - if (!GetQuicReloadableFlag(quic_parse_accept_ch_frame)) { - return; - } - StrictMock<MockHttp3DebugVisitor> debug_visitor; session_.set_debug_visitor(&debug_visitor); @@ -3265,11 +3261,9 @@ "03" // length of value "626172"); // value "bar" - if (GetQuicReloadableFlag(quic_parse_accept_ch_frame)) { - AcceptChFrame expected_accept_ch_frame{{{"foo", "bar"}}}; - EXPECT_CALL(debug_visitor, - OnAcceptChFrameReceivedViaAlps(expected_accept_ch_frame)); - } + AcceptChFrame expected_accept_ch_frame{{{"foo", "bar"}}}; + EXPECT_CALL(debug_visitor, + OnAcceptChFrameReceivedViaAlps(expected_accept_ch_frame)); auto error = session_.OnAlpsData( reinterpret_cast<const uint8_t*>(serialized_accept_ch_frame.data()),
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h index 40f8502..d2a66e0 100644 --- a/quic/core/quic_flags_list.h +++ b/quic/core/quic_flags_list.h
@@ -46,7 +46,6 @@ QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_fix_on_stream_reset, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_fix_willing_and_able_to_write2, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_h3_datagram, false) -QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_parse_accept_ch_frame, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_pass_path_response_to_validator, false) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_require_handshake_confirmation, false) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_send_goaway_with_connection_close, true)