Deprecate --gfe2_reloadable_flag_quic_error_on_http3_push.
PiperOrigin-RevId: 384535586
diff --git a/quic/core/http/http_decoder.cc b/quic/core/http/http_decoder.cc
index de0ae53..92a2e38 100644
--- a/quic/core/http/http_decoder.cc
+++ b/quic/core/http/http_decoder.cc
@@ -33,15 +33,9 @@
remaining_frame_length_(0),
current_type_field_length_(0),
remaining_type_field_length_(0),
- current_push_id_length_(0),
- remaining_push_id_length_(0),
error_(QUIC_NO_ERROR),
- error_detail_(""),
- error_on_http3_push_(GetQuicReloadableFlag(quic_error_on_http3_push)) {
+ error_detail_("") {
QUICHE_DCHECK(visitor_);
- if (error_on_http3_push_) {
- QUIC_RELOADABLE_FLAG_COUNT(quic_error_on_http3_push);
- }
}
HttpDecoder::~HttpDecoder() {}
@@ -177,17 +171,15 @@
return false;
}
- if (error_on_http3_push_) {
- if (current_frame_type_ ==
- static_cast<uint64_t>(HttpFrameType::CANCEL_PUSH)) {
- RaiseError(QUIC_HTTP_FRAME_ERROR, "CANCEL_PUSH frame received.");
- return false;
- }
- if (current_frame_type_ ==
- static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE)) {
- RaiseError(QUIC_HTTP_FRAME_ERROR, "PUSH_PROMISE frame received.");
- return false;
- }
+ if (current_frame_type_ ==
+ static_cast<uint64_t>(HttpFrameType::CANCEL_PUSH)) {
+ RaiseError(QUIC_HTTP_FRAME_ERROR, "CANCEL_PUSH frame received.");
+ return false;
+ }
+ if (current_frame_type_ ==
+ static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE)) {
+ RaiseError(QUIC_HTTP_FRAME_ERROR, "PUSH_PROMISE frame received.");
+ return false;
}
state_ = STATE_READING_FRAME_LENGTH;
@@ -256,27 +248,13 @@
visitor_->OnHeadersFrameStart(header_length, current_frame_length_);
break;
case static_cast<uint64_t>(HttpFrameType::CANCEL_PUSH):
- if (error_on_http3_push_) {
- QUICHE_NOTREACHED();
- break;
- }
+ QUICHE_NOTREACHED();
break;
case static_cast<uint64_t>(HttpFrameType::SETTINGS):
continue_processing = visitor_->OnSettingsFrameStart(header_length);
break;
case static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE):
- if (error_on_http3_push_) {
- QUICHE_NOTREACHED();
- break;
- }
- // This edge case needs to be handled here, because ReadFramePayload()
- // does not get called if |current_frame_length_| is zero.
- if (current_frame_length_ == 0) {
- RaiseError(QUIC_HTTP_FRAME_ERROR,
- "PUSH_PROMISE frame with empty payload.");
- return false;
- }
- continue_processing = visitor_->OnPushPromiseFrameStart(header_length);
+ QUICHE_NOTREACHED();
break;
case static_cast<uint64_t>(HttpFrameType::GOAWAY):
break;
@@ -330,11 +308,7 @@
break;
}
case static_cast<uint64_t>(HttpFrameType::CANCEL_PUSH): {
- if (error_on_http3_push_) {
- QUICHE_NOTREACHED();
- } else {
- continue_processing = BufferOrParsePayload(reader);
- }
+ QUICHE_NOTREACHED();
break;
}
case static_cast<uint64_t>(HttpFrameType::SETTINGS): {
@@ -342,72 +316,7 @@
break;
}
case static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE): {
- if (error_on_http3_push_) {
- QUICHE_NOTREACHED();
- break;
- }
- PushId push_id;
- if (current_frame_length_ == remaining_frame_length_) {
- // A new Push Promise frame just arrived.
- QUICHE_DCHECK_EQ(0u, current_push_id_length_);
- current_push_id_length_ = reader->PeekVarInt62Length();
- if (current_push_id_length_ > remaining_frame_length_) {
- RaiseError(QUIC_HTTP_FRAME_ERROR,
- "Unable to read PUSH_PROMISE push_id.");
- return false;
- }
- if (current_push_id_length_ > reader->BytesRemaining()) {
- // Not all bytes of push id is present yet, buffer push id.
- QUICHE_DCHECK_EQ(0u, remaining_push_id_length_);
- remaining_push_id_length_ = current_push_id_length_;
- BufferPushId(reader);
- break;
- }
- bool success = reader->ReadVarInt62(&push_id);
- QUICHE_DCHECK(success);
- remaining_frame_length_ -= current_push_id_length_;
- if (!visitor_->OnPushPromiseFramePushId(
- push_id, current_push_id_length_,
- current_frame_length_ - current_push_id_length_)) {
- continue_processing = false;
- current_push_id_length_ = 0;
- break;
- }
- current_push_id_length_ = 0;
- } else if (remaining_push_id_length_ > 0) {
- // Waiting for more bytes on push id.
- BufferPushId(reader);
- if (remaining_push_id_length_ != 0) {
- break;
- }
- QuicDataReader push_id_reader(push_id_buffer_.data(),
- current_push_id_length_);
-
- bool success = push_id_reader.ReadVarInt62(&push_id);
- QUICHE_DCHECK(success);
- if (!visitor_->OnPushPromiseFramePushId(
- push_id, current_push_id_length_,
- current_frame_length_ - current_push_id_length_)) {
- continue_processing = false;
- current_push_id_length_ = 0;
- break;
- }
- current_push_id_length_ = 0;
- }
-
- // Read Push Promise headers.
- QUICHE_DCHECK_LT(remaining_frame_length_, current_frame_length_);
- QuicByteCount bytes_to_read = std::min<QuicByteCount>(
- remaining_frame_length_, reader->BytesRemaining());
- if (bytes_to_read == 0) {
- break;
- }
- absl::string_view payload;
- bool success = reader->ReadStringPiece(&payload, bytes_to_read);
- QUICHE_DCHECK(success);
- QUICHE_DCHECK(!payload.empty());
- continue_processing = visitor_->OnPushPromiseFramePayload(payload);
- remaining_frame_length_ -= payload.length();
+ QUICHE_NOTREACHED();
break;
}
case static_cast<uint64_t>(HttpFrameType::GOAWAY): {
@@ -455,13 +364,7 @@
break;
}
case static_cast<uint64_t>(HttpFrameType::CANCEL_PUSH): {
- if (error_on_http3_push_) {
- QUICHE_NOTREACHED();
- } else {
- // If frame payload is not empty, FinishParsing() is skipped.
- QUICHE_DCHECK_EQ(0u, current_frame_length_);
- continue_processing = BufferOrParsePayload(reader);
- }
+ QUICHE_NOTREACHED();
break;
}
case static_cast<uint64_t>(HttpFrameType::SETTINGS): {
@@ -471,11 +374,7 @@
break;
}
case static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE): {
- if (error_on_http3_push_) {
- QUICHE_NOTREACHED();
- } else {
- continue_processing = visitor_->OnPushPromiseFrameEnd();
- }
+ QUICHE_NOTREACHED();
break;
}
case static_cast<uint64_t>(HttpFrameType::GOAWAY): {
@@ -588,22 +487,8 @@
switch (current_frame_type_) {
case static_cast<uint64_t>(HttpFrameType::CANCEL_PUSH): {
- if (error_on_http3_push_) {
- QUICHE_NOTREACHED();
- return false;
- }
- CancelPushFrame frame;
- if (!reader->ReadVarInt62(&frame.push_id)) {
- RaiseError(QUIC_HTTP_FRAME_ERROR,
- "Unable to read CANCEL_PUSH push_id.");
- return false;
- }
- if (!reader->IsDoneReading()) {
- RaiseError(QUIC_HTTP_FRAME_ERROR,
- "Superfluous data in CANCEL_PUSH frame.");
- return false;
- }
- return visitor_->OnCancelPushFrame(frame);
+ QUICHE_NOTREACHED();
+ return false;
}
case static_cast<uint64_t>(HttpFrameType::SETTINGS): {
SettingsFrame frame;
@@ -681,19 +566,6 @@
remaining_type_field_length_ -= bytes_to_read;
}
-void HttpDecoder::BufferPushId(QuicDataReader* reader) {
- QUICHE_DCHECK_LE(remaining_push_id_length_, current_frame_length_);
- QuicByteCount bytes_to_read = std::min<QuicByteCount>(
- reader->BytesRemaining(), remaining_push_id_length_);
- bool success =
- reader->ReadBytes(push_id_buffer_.data() + current_push_id_length_ -
- remaining_push_id_length_,
- bytes_to_read);
- QUICHE_DCHECK(success);
- remaining_push_id_length_ -= bytes_to_read;
- remaining_frame_length_ -= bytes_to_read;
-}
-
void HttpDecoder::RaiseError(QuicErrorCode error, std::string error_detail) {
state_ = STATE_ERROR;
error_ = error;
@@ -792,17 +664,11 @@
QuicByteCount HttpDecoder::MaxFrameLength(uint64_t frame_type) {
switch (frame_type) {
- case static_cast<uint64_t>(HttpFrameType::CANCEL_PUSH):
- // TODO(b/171463363): Remove.
- return sizeof(PushId);
case static_cast<uint64_t>(HttpFrameType::SETTINGS):
// This limit is arbitrary.
return 1024 * 1024;
case static_cast<uint64_t>(HttpFrameType::GOAWAY):
return VARIABLE_LENGTH_INTEGER_LENGTH_8;
- case static_cast<uint64_t>(HttpFrameType::MAX_PUSH_ID):
- // TODO(b/171463363): Remove.
- return sizeof(PushId);
case static_cast<uint64_t>(HttpFrameType::PRIORITY_UPDATE_REQUEST_STREAM):
// This limit is arbitrary.
return 1024 * 1024;
diff --git a/quic/core/http/http_decoder.h b/quic/core/http/http_decoder.h
index 4d415b6..7cbee7a 100644
--- a/quic/core/http/http_decoder.h
+++ b/quic/core/http/http_decoder.h
@@ -231,10 +231,6 @@
// Buffers any remaining frame type field from |reader| into |type_buffer_|.
void BufferFrameType(QuicDataReader* reader);
- // Buffers at most |remaining_push_id_length_| from |reader| to
- // |push_id_buffer_|. TODO(b/171463363): Remove.
- void BufferPushId(QuicDataReader* reader);
-
// Sets |error_| and |error_detail_| accordingly.
void RaiseError(QuicErrorCode error, std::string error_detail);
@@ -279,12 +275,6 @@
QuicByteCount current_type_field_length_;
// Remaining length that's needed for the frame's type field.
QuicByteCount remaining_type_field_length_;
- // Length of PUSH_PROMISE frame's push id.
- // TODO(b/171463363): Remove.
- QuicByteCount current_push_id_length_;
- // Remaining length that's needed for PUSH_PROMISE frame's push id field.
- // TODO(b/171463363): Remove.
- QuicByteCount remaining_push_id_length_;
// Last error.
QuicErrorCode error_;
// The issue which caused |error_|
@@ -295,13 +285,6 @@
std::array<char, sizeof(uint64_t)> length_buffer_;
// Remaining unparsed type field data.
std::array<char, sizeof(uint64_t)> type_buffer_;
- // Remaining unparsed push id data.
- // TODO(b/171463363): Remove.
- std::array<char, sizeof(uint64_t)> push_id_buffer_;
-
- // Latched value of
- // gfe2_reloadable_flag_quic_error_on_http3_push.
- const bool error_on_http3_push_;
};
} // namespace quic
diff --git a/quic/core/http/http_decoder_test.cc b/quic/core/http/http_decoder_test.cc
index bae0dc0..d01cfa7 100644
--- a/quic/core/http/http_decoder_test.cc
+++ b/quic/core/http/http_decoder_test.cc
@@ -248,144 +248,24 @@
"01" // length
"01"); // Push Id
- if (GetQuicReloadableFlag(quic_error_on_http3_push)) {
- EXPECT_CALL(visitor_, OnError(&decoder_));
- EXPECT_EQ(1u, ProcessInput(input));
- EXPECT_THAT(decoder_.error(), IsError(QUIC_HTTP_FRAME_ERROR));
- EXPECT_EQ("CANCEL_PUSH frame received.", decoder_.error_detail());
- return;
- }
-
- // Visitor pauses processing.
- EXPECT_CALL(visitor_, OnCancelPushFrame(CancelPushFrame({1})))
- .WillOnce(Return(false));
- EXPECT_EQ(input.size(), ProcessInputWithGarbageAppended(input));
- EXPECT_THAT(decoder_.error(), IsQuicNoError());
- EXPECT_EQ("", decoder_.error_detail());
-
- // Process the full frame.
- EXPECT_CALL(visitor_, OnCancelPushFrame(CancelPushFrame({1})));
- EXPECT_EQ(input.size(), ProcessInput(input));
- EXPECT_THAT(decoder_.error(), IsQuicNoError());
- EXPECT_EQ("", decoder_.error_detail());
-
- // Process the frame incrementally.
- EXPECT_CALL(visitor_, OnCancelPushFrame(CancelPushFrame({1})));
- ProcessInputCharByChar(input);
- EXPECT_THAT(decoder_.error(), IsQuicNoError());
- EXPECT_EQ("", decoder_.error_detail());
+ EXPECT_CALL(visitor_, OnError(&decoder_));
+ EXPECT_EQ(1u, ProcessInput(input));
+ EXPECT_THAT(decoder_.error(), IsError(QUIC_HTTP_FRAME_ERROR));
+ EXPECT_EQ("CANCEL_PUSH frame received.", decoder_.error_detail());
}
TEST_F(HttpDecoderTest, PushPromiseFrame) {
InSequence s;
std::string input =
- absl::StrCat(absl::HexStringToBytes("05" // type (PUSH PROMISE)
- "0f" // length
- "C000000000000101"), // push id 257
- "Headers"); // headers
+ absl::StrCat(absl::HexStringToBytes("05" // type (PUSH PROMISE)
+ "08" // length
+ "1f"), // push id 31
+ "Headers"); // headers
- if (GetQuicReloadableFlag(quic_error_on_http3_push)) {
- EXPECT_CALL(visitor_, OnError(&decoder_));
- EXPECT_EQ(1u, ProcessInput(input));
- EXPECT_THAT(decoder_.error(), IsError(QUIC_HTTP_FRAME_ERROR));
- EXPECT_EQ("PUSH_PROMISE frame received.", decoder_.error_detail());
- return;
- }
-
- // Visitor pauses processing.
- EXPECT_CALL(visitor_, OnPushPromiseFrameStart(2)).WillOnce(Return(false));
- EXPECT_CALL(visitor_, OnPushPromiseFramePushId(257, 8, 7))
- .WillOnce(Return(false));
- absl::string_view remaining_input(input);
- QuicByteCount processed_bytes =
- ProcessInputWithGarbageAppended(remaining_input);
- EXPECT_EQ(2u, processed_bytes);
- remaining_input = remaining_input.substr(processed_bytes);
- processed_bytes = ProcessInputWithGarbageAppended(remaining_input);
- EXPECT_EQ(8u, processed_bytes);
- remaining_input = remaining_input.substr(processed_bytes);
-
- EXPECT_CALL(visitor_, OnPushPromiseFramePayload(absl::string_view("Headers")))
- .WillOnce(Return(false));
- processed_bytes = ProcessInputWithGarbageAppended(remaining_input);
- EXPECT_EQ(remaining_input.size(), processed_bytes);
-
- EXPECT_CALL(visitor_, OnPushPromiseFrameEnd()).WillOnce(Return(false));
- EXPECT_EQ(0u, ProcessInputWithGarbageAppended(""));
- EXPECT_THAT(decoder_.error(), IsQuicNoError());
- EXPECT_EQ("", decoder_.error_detail());
-
- // Process the full frame.
- EXPECT_CALL(visitor_, OnPushPromiseFrameStart(2));
- EXPECT_CALL(visitor_, OnPushPromiseFramePushId(257, 8, 7));
- EXPECT_CALL(visitor_,
- OnPushPromiseFramePayload(absl::string_view("Headers")));
- EXPECT_CALL(visitor_, OnPushPromiseFrameEnd());
- EXPECT_EQ(input.size(), ProcessInput(input));
- EXPECT_THAT(decoder_.error(), IsQuicNoError());
- EXPECT_EQ("", decoder_.error_detail());
-
- // Process the frame incrementally.
- EXPECT_CALL(visitor_, OnPushPromiseFrameStart(2));
- EXPECT_CALL(visitor_, OnPushPromiseFramePushId(257, 8, 7));
- EXPECT_CALL(visitor_, OnPushPromiseFramePayload(absl::string_view("H")));
- EXPECT_CALL(visitor_, OnPushPromiseFramePayload(absl::string_view("e")));
- EXPECT_CALL(visitor_, OnPushPromiseFramePayload(absl::string_view("a")));
- EXPECT_CALL(visitor_, OnPushPromiseFramePayload(absl::string_view("d")));
- EXPECT_CALL(visitor_, OnPushPromiseFramePayload(absl::string_view("e")));
- EXPECT_CALL(visitor_, OnPushPromiseFramePayload(absl::string_view("r")));
- EXPECT_CALL(visitor_, OnPushPromiseFramePayload(absl::string_view("s")));
- EXPECT_CALL(visitor_, OnPushPromiseFrameEnd());
- ProcessInputCharByChar(input);
- EXPECT_THAT(decoder_.error(), IsQuicNoError());
- EXPECT_EQ("", decoder_.error_detail());
-
- // Process push id incrementally and append headers with last byte of push id.
- EXPECT_CALL(visitor_, OnPushPromiseFrameStart(2));
- EXPECT_CALL(visitor_, OnPushPromiseFramePushId(257, 8, 7));
- EXPECT_CALL(visitor_,
- OnPushPromiseFramePayload(absl::string_view("Headers")));
- EXPECT_CALL(visitor_, OnPushPromiseFrameEnd());
- ProcessInputCharByChar(input.substr(0, 9));
- EXPECT_EQ(8u, ProcessInput(input.substr(9)));
- EXPECT_THAT(decoder_.error(), IsQuicNoError());
- EXPECT_EQ("", decoder_.error_detail());
-}
-
-TEST_F(HttpDecoderTest, CorruptPushPromiseFrame) {
- if (GetQuicReloadableFlag(quic_error_on_http3_push)) {
- return;
- }
-
- InSequence s;
-
- std::string input = absl::HexStringToBytes(
- "05" // type (PUSH_PROMISE)
- "01" // length
- "40"); // first byte of two-byte varint push id
-
- {
- HttpDecoder decoder(&visitor_);
- EXPECT_CALL(visitor_, OnPushPromiseFrameStart(2));
- EXPECT_CALL(visitor_, OnError(&decoder));
-
- decoder.ProcessInput(input.data(), input.size());
-
- EXPECT_THAT(decoder.error(), IsError(QUIC_HTTP_FRAME_ERROR));
- EXPECT_EQ("Unable to read PUSH_PROMISE push_id.", decoder.error_detail());
- }
- {
- HttpDecoder decoder(&visitor_);
- EXPECT_CALL(visitor_, OnPushPromiseFrameStart(2));
- EXPECT_CALL(visitor_, OnError(&decoder));
-
- for (auto c : input) {
- decoder.ProcessInput(&c, 1);
- }
-
- EXPECT_THAT(decoder.error(), IsError(QUIC_HTTP_FRAME_ERROR));
- EXPECT_EQ("Unable to read PUSH_PROMISE push_id.", decoder.error_detail());
- }
+ EXPECT_CALL(visitor_, OnError(&decoder_));
+ EXPECT_EQ(1u, ProcessInput(input));
+ EXPECT_THAT(decoder_.error(), IsError(QUIC_HTTP_FRAME_ERROR));
+ EXPECT_EQ("PUSH_PROMISE frame received.", decoder_.error_detail());
}
TEST_F(HttpDecoderTest, MaxPushId) {
@@ -749,55 +629,11 @@
EXPECT_EQ("", decoder_.error_detail());
}
-TEST_F(HttpDecoderTest, PushPromiseFrameNoHeaders) {
- if (GetQuicReloadableFlag(quic_error_on_http3_push)) {
- return;
- }
-
- InSequence s;
- std::string input = absl::HexStringToBytes(
- "05" // type (PUSH_PROMISE)
- "01" // length
- "01"); // Push Id
-
- // Visitor pauses processing.
- EXPECT_CALL(visitor_, OnPushPromiseFrameStart(2));
- EXPECT_CALL(visitor_, OnPushPromiseFramePushId(1, 1, 0))
- .WillOnce(Return(false));
- EXPECT_EQ(input.size(), ProcessInputWithGarbageAppended(input));
-
- EXPECT_CALL(visitor_, OnPushPromiseFrameEnd()).WillOnce(Return(false));
- EXPECT_EQ(0u, ProcessInputWithGarbageAppended(""));
- EXPECT_THAT(decoder_.error(), IsQuicNoError());
- EXPECT_EQ("", decoder_.error_detail());
-
- // Process the full frame.
- EXPECT_CALL(visitor_, OnPushPromiseFrameStart(2));
- EXPECT_CALL(visitor_, OnPushPromiseFramePushId(1, 1, 0));
- EXPECT_CALL(visitor_, OnPushPromiseFrameEnd());
- EXPECT_EQ(input.size(), ProcessInput(input));
- EXPECT_THAT(decoder_.error(), IsQuicNoError());
- EXPECT_EQ("", decoder_.error_detail());
-
- // Process the frame incrementally.
- EXPECT_CALL(visitor_, OnPushPromiseFrameStart(2));
- EXPECT_CALL(visitor_, OnPushPromiseFramePushId(1, 1, 0));
- EXPECT_CALL(visitor_, OnPushPromiseFrameEnd());
- ProcessInputCharByChar(input);
- EXPECT_THAT(decoder_.error(), IsQuicNoError());
- EXPECT_EQ("", decoder_.error_detail());
-}
-
TEST_F(HttpDecoderTest, MalformedFrameWithOverlyLargePayload) {
- if (GetQuicReloadableFlag(quic_error_on_http3_push)) {
- return;
- }
-
std::string input = absl::HexStringToBytes(
- "03" // type (CANCEL_PUSH)
- "10" // length
- "15"); // malformed payload
- // Process the full frame.
+ "07" // type (GOAWAY)
+ "10"); // length exceeding the maximum possible length for GOAWAY frame
+ // Process all data at once.
EXPECT_CALL(visitor_, OnError(&decoder_));
EXPECT_EQ(2u, ProcessInput(input));
EXPECT_THAT(decoder_.error(), IsError(QUIC_HTTP_FRAME_TOO_LARGE));
@@ -866,193 +702,87 @@
}
TEST_F(HttpDecoderTest, CorruptFrame) {
- if (GetQuicReloadableFlag(quic_error_on_http3_push)) {
- InSequence s;
+ InSequence s;
- struct {
- const char* const input;
- const char* const error_message;
- } kTestData[] = {{"\x0D" // type (MAX_PUSH_ID)
- "\x01" // length
- "\x40", // first byte of two-byte varint push id
- "Unable to read MAX_PUSH_ID push_id."},
- {"\x0D" // type (MAX_PUSH_ID)
- "\x04" // length
- "\x05" // valid push id
- "foo", // superfluous data
- "Superfluous data in MAX_PUSH_ID frame."},
- {"\x07" // type (GOAWAY)
- "\x01" // length
- "\x40", // first byte of two-byte varint stream id
- "Unable to read GOAWAY ID."},
- {"\x07" // type (GOAWAY)
- "\x04" // length
- "\x05" // valid stream id
- "foo", // superfluous data
- "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."}};
+ struct {
+ const char* const input;
+ const char* const error_message;
+ } kTestData[] = {{"\x0D" // type (MAX_PUSH_ID)
+ "\x01" // length
+ "\x40", // first byte of two-byte varint push id
+ "Unable to read MAX_PUSH_ID push_id."},
+ {"\x0D" // type (MAX_PUSH_ID)
+ "\x04" // length
+ "\x05" // valid push id
+ "foo", // superfluous data
+ "Superfluous data in MAX_PUSH_ID frame."},
+ {"\x07" // type (GOAWAY)
+ "\x01" // length
+ "\x40", // first byte of two-byte varint stream id
+ "Unable to read GOAWAY ID."},
+ {"\x07" // type (GOAWAY)
+ "\x04" // length
+ "\x05" // valid stream id
+ "foo", // superfluous data
+ "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));
+ 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);
- 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_, OnAcceptChFrameStart(_)).Times(AnyNumber());
- 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());
- }
+ 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());
}
- } else {
- InSequence s;
+ {
+ HttpDecoder decoder(&visitor_);
+ EXPECT_CALL(visitor_, OnAcceptChFrameStart(_)).Times(AnyNumber());
+ EXPECT_CALL(visitor_, OnError(&decoder));
- struct {
- const char* const input;
- const char* const error_message;
- } kTestData[] = {{"\x03" // type (CANCEL_PUSH)
- "\x01" // length
- "\x40", // first byte of two-byte varint push id
- "Unable to read CANCEL_PUSH push_id."},
- {"\x03" // type (CANCEL_PUSH)
- "\x04" // length
- "\x05" // valid push id
- "foo", // superfluous data
- "Superfluous data in CANCEL_PUSH frame."},
- {"\x0D" // type (MAX_PUSH_ID)
- "\x01" // length
- "\x40", // first byte of two-byte varint push id
- "Unable to read MAX_PUSH_ID push_id."},
- {"\x0D" // type (MAX_PUSH_ID)
- "\x04" // length
- "\x05" // valid push id
- "foo", // superfluous data
- "Superfluous data in MAX_PUSH_ID frame."},
- {"\x07" // type (GOAWAY)
- "\x01" // length
- "\x40", // first byte of two-byte varint stream id
- "Unable to read GOAWAY ID."},
- {"\x07" // type (GOAWAY)
- "\x04" // length
- "\x05" // valid stream id
- "foo", // superfluous data
- "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);
- decoder.ProcessInput(input.data(), input.size());
- EXPECT_THAT(decoder.error(), IsError(QUIC_HTTP_FRAME_ERROR));
- EXPECT_EQ(test_data.error_message, decoder.error_detail());
+ absl::string_view input(test_data.input);
+ for (auto c : input) {
+ decoder.ProcessInput(&c, 1);
}
- {
- HttpDecoder decoder(&visitor_);
- EXPECT_CALL(visitor_, OnAcceptChFrameStart(_)).Times(AnyNumber());
- 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());
- }
+ EXPECT_THAT(decoder.error(), IsError(QUIC_HTTP_FRAME_ERROR));
+ EXPECT_EQ(test_data.error_message, decoder.error_detail());
}
}
}
-TEST_F(HttpDecoderTest, EmptyCancelPushFrame) {
- if (GetQuicReloadableFlag(quic_error_on_http3_push)) {
- return;
- }
-
- std::string input = absl::HexStringToBytes(
- "03" // type (CANCEL_PUSH)
- "00"); // frame length
-
- EXPECT_CALL(visitor_, OnError(&decoder_));
- EXPECT_EQ(input.size(), ProcessInput(input));
- EXPECT_THAT(decoder_.error(), IsError(QUIC_HTTP_FRAME_ERROR));
- EXPECT_EQ("Unable to read CANCEL_PUSH push_id.", decoder_.error_detail());
-}
-
TEST_F(HttpDecoderTest, EmptySettingsFrame) {
std::string input = absl::HexStringToBytes(
"04" // type (SETTINGS)
@@ -1068,22 +798,6 @@
EXPECT_EQ("", decoder_.error_detail());
}
-// Regression test for https://crbug.com/1001823.
-TEST_F(HttpDecoderTest, EmptyPushPromiseFrame) {
- if (GetQuicReloadableFlag(quic_error_on_http3_push)) {
- return;
- }
-
- std::string input = absl::HexStringToBytes(
- "05" // type (PUSH_PROMISE)
- "00"); // frame length
-
- EXPECT_CALL(visitor_, OnError(&decoder_));
- EXPECT_EQ(input.size(), ProcessInput(input));
- EXPECT_THAT(decoder_.error(), IsError(QUIC_HTTP_FRAME_ERROR));
- EXPECT_EQ("PUSH_PROMISE frame with empty payload.", decoder_.error_detail());
-}
-
TEST_F(HttpDecoderTest, EmptyGoAwayFrame) {
std::string input = absl::HexStringToBytes(
"07" // type (GOAWAY)
diff --git a/quic/core/http/quic_receive_control_stream_test.cc b/quic/core/http/quic_receive_control_stream_test.cc
index 169af21..ae7a0d8 100644
--- a/quic/core/http/quic_receive_control_stream_test.cc
+++ b/quic/core/http/quic_receive_control_stream_test.cc
@@ -308,12 +308,7 @@
"00"); // push ID
QuicStreamFrame frame(receive_control_stream_->id(), false, 1,
push_promise_frame);
- EXPECT_CALL(
- *connection_,
- CloseConnection(GetQuicReloadableFlag(quic_error_on_http3_push)
- ? QUIC_HTTP_FRAME_ERROR
- : QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM,
- _, _))
+ EXPECT_CALL(*connection_, CloseConnection(QUIC_HTTP_FRAME_ERROR, _, _))
.WillOnce(
Invoke(connection_, &MockQuicConnection::ReallyCloseConnection));
EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _, _));
@@ -384,21 +379,10 @@
"01" // payload length
"01"); // push ID
- if (GetQuicReloadableFlag(quic_error_on_http3_push)) {
- EXPECT_CALL(*connection_, CloseConnection(QUIC_HTTP_FRAME_ERROR,
- "CANCEL_PUSH frame received.", _))
- .WillOnce(
- Invoke(connection_, &MockQuicConnection::ReallyCloseConnection));
- } else {
- EXPECT_CALL(
- *connection_,
- CloseConnection(QUIC_HTTP_MISSING_SETTINGS_FRAME,
- "First frame received on control stream is type "
- "3, but it must be SETTINGS.",
- _))
- .WillOnce(
- Invoke(connection_, &MockQuicConnection::ReallyCloseConnection));
- }
+ EXPECT_CALL(*connection_, CloseConnection(QUIC_HTTP_FRAME_ERROR,
+ "CANCEL_PUSH frame received.", _))
+ .WillOnce(
+ Invoke(connection_, &MockQuicConnection::ReallyCloseConnection));
EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _, _));
EXPECT_CALL(session_, OnConnectionClosed(_, _));
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc
index 7821be7..5d5c71a 100644
--- a/quic/core/http/quic_spdy_session_test.cc
+++ b/quic/core/http/quic_spdy_session_test.cc
@@ -2927,18 +2927,13 @@
"00"); // push ID
QuicStreamFrame data3(receive_control_stream_id, /* fin = */ false, offset,
cancel_push_frame);
- if (GetQuicReloadableFlag(quic_error_on_http3_push)) {
- EXPECT_CALL(*connection_, CloseConnection(QUIC_HTTP_FRAME_ERROR,
- "CANCEL_PUSH frame received.", _))
- .WillOnce(
- Invoke(connection_, &MockQuicConnection::ReallyCloseConnection));
- EXPECT_CALL(*connection_,
- SendConnectionClosePacket(QUIC_HTTP_FRAME_ERROR, _,
- "CANCEL_PUSH frame received."));
- } else {
- // CANCEL_PUSH is ignored.
- EXPECT_CALL(debug_visitor, OnCancelPushFrameReceived(_));
- }
+ EXPECT_CALL(*connection_, CloseConnection(QUIC_HTTP_FRAME_ERROR,
+ "CANCEL_PUSH frame received.", _))
+ .WillOnce(
+ Invoke(connection_, &MockQuicConnection::ReallyCloseConnection));
+ EXPECT_CALL(*connection_,
+ SendConnectionClosePacket(QUIC_HTTP_FRAME_ERROR, _,
+ "CANCEL_PUSH frame received."));
session_.OnStreamFrame(data3);
}
@@ -3134,18 +3129,13 @@
"00"); // push ID
QuicStreamFrame data3(receive_control_stream_id, /* fin = */ false, offset,
cancel_push_frame);
- if (GetQuicReloadableFlag(quic_error_on_http3_push)) {
- EXPECT_CALL(*connection_, CloseConnection(QUIC_HTTP_FRAME_ERROR,
- "CANCEL_PUSH frame received.", _))
- .WillOnce(
- Invoke(connection_, &MockQuicConnection::ReallyCloseConnection));
- EXPECT_CALL(*connection_,
- SendConnectionClosePacket(QUIC_HTTP_FRAME_ERROR, _,
- "CANCEL_PUSH frame received."));
- } else {
- // CANCEL_PUSH is ignored.
- EXPECT_CALL(debug_visitor, OnCancelPushFrameReceived(_));
- }
+ EXPECT_CALL(*connection_, CloseConnection(QUIC_HTTP_FRAME_ERROR,
+ "CANCEL_PUSH frame received.", _))
+ .WillOnce(
+ Invoke(connection_, &MockQuicConnection::ReallyCloseConnection));
+ EXPECT_CALL(*connection_,
+ SendConnectionClosePacket(QUIC_HTTP_FRAME_ERROR, _,
+ "CANCEL_PUSH frame received."));
session_.OnStreamFrame(data3);
}
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc
index 038e0f6..c19dc3c 100644
--- a/quic/core/http/quic_spdy_stream_test.cc
+++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -455,33 +455,6 @@
return absl::StrCat(headers_frame_header, payload);
}
- // Construct PUSH_PROMISE frame with given payload.
- // TODO(b/171463363): Remove.
- std::string SerializePushPromiseFrame(PushId push_id,
- absl::string_view headers) {
- const QuicByteCount payload_length =
- QuicDataWriter::GetVarInt62Len(push_id) + headers.length();
-
- const QuicByteCount length_without_headers =
- QuicDataWriter::GetVarInt62Len(
- static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE)) +
- QuicDataWriter::GetVarInt62Len(payload_length) +
- QuicDataWriter::GetVarInt62Len(push_id);
-
- std::string push_promise_frame(length_without_headers, '\0');
- QuicDataWriter writer(length_without_headers, &*push_promise_frame.begin());
-
- QUICHE_CHECK(writer.WriteVarInt62(
- static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE)));
- QUICHE_CHECK(writer.WriteVarInt62(payload_length));
- QUICHE_CHECK(writer.WriteVarInt62(push_id));
- QUICHE_CHECK_EQ(0u, writer.remaining());
-
- absl::StrAppend(&push_promise_frame, headers);
-
- return push_promise_frame;
- }
-
std::string DataFrame(absl::string_view payload) {
QuicBuffer header = HttpEncoder::SerializeDataFrameHeader(
payload.length(), SimpleBufferAllocator::Get());
@@ -2766,71 +2739,6 @@
EXPECT_EQ(unknown_frame4.size(), NewlyConsumedBytes());
}
-// TODO(b/171463363): Remove.
-TEST_P(QuicSpdyStreamTest, PushPromiseOnDataStream) {
- Initialize(kShouldProcessData);
- if (GetQuicReloadableFlag(quic_error_on_http3_push)) {
- return;
- }
- if (!UsesHttp3()) {
- return;
- }
-
- StrictMock<MockHttp3DebugVisitor> debug_visitor;
- session_->set_debug_visitor(&debug_visitor);
-
- SpdyHeaderBlock pushed_headers;
- pushed_headers["foo"] = "bar";
- std::string headers = EncodeQpackHeaders(pushed_headers);
-
- const QuicStreamId push_id = 1;
- std::string data = SerializePushPromiseFrame(push_id, headers);
- QuicStreamFrame frame(stream_->id(), false, 0, data);
-
- EXPECT_CALL(debug_visitor, OnPushPromiseFrameReceived(stream_->id(), push_id,
- headers.length()));
- EXPECT_CALL(debug_visitor,
- OnPushPromiseDecoded(stream_->id(), push_id,
- AsHeaderList(pushed_headers)));
- EXPECT_CALL(*session_,
- OnPromiseHeaderList(stream_->id(), push_id, headers.length(), _));
- stream_->OnStreamFrame(frame);
-}
-
-// Regression test for b/152518220.
-// TODO(b/171463363): Remove.
-TEST_P(QuicSpdyStreamTest,
- OnStreamHeaderBlockArgumentDoesNotIncludePushedHeaderBlock) {
- Initialize(kShouldProcessData);
- if (GetQuicReloadableFlag(quic_error_on_http3_push)) {
- return;
- }
- if (!UsesHttp3()) {
- return;
- }
-
- std::string pushed_headers = EncodeQpackHeaders({{"foo", "bar"}});
- const QuicStreamId push_id = 1;
- std::string push_promise_frame =
- SerializePushPromiseFrame(push_id, pushed_headers);
- QuicStreamOffset offset = 0;
- QuicStreamFrame frame1(stream_->id(), /* fin = */ false, offset,
- push_promise_frame);
- offset += push_promise_frame.length();
-
- EXPECT_CALL(*session_, OnPromiseHeaderList(stream_->id(), push_id,
- pushed_headers.length(), _));
- stream_->OnStreamFrame(frame1);
-
- std::string headers =
- EncodeQpackHeaders({{":method", "GET"}, {":path", "/"}});
- std::string headers_frame = HeadersFrame(headers);
- QuicStreamFrame frame2(stream_->id(), /* fin = */ false, offset,
- headers_frame);
- stream_->OnStreamFrame(frame2);
- EXPECT_EQ(headers.length(), stream_->headers_payload_length());
-}
-
// Close connection if a DATA frame is received before a HEADERS frame.
TEST_P(QuicSpdyStreamTest, DataBeforeHeaders) {
if (!UsesHttp3()) {
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h
index 9f1d854..693053a 100644
--- a/quic/core/quic_flags_list.h
+++ b/quic/core/quic_flags_list.h
@@ -93,8 +93,6 @@
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_send_path_response2, true)
// If true, set burst token to 2 in cwnd bootstrapping experiment.
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_conservative_bursts, false)
-// If true, signal error in HttpDecoder upon receiving a PUSH_PROMISE or CANCEL_PUSH frame.
-QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_error_on_http3_push, true)
// If true, stop resetting ideal_next_packet_send_time_ in pacing sender.
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_donot_reset_ideal_next_packet_send_time, false)
// If true, time_wait_list can support multiple connection IDs.
