Simplify QpackDecodedHeadersAccumulator API.
Fix handling of blocked headers that contain an error in the part that is
buffered and then processed when unblocked before reading the entire header
block is over, see https://crbug.com/1024263.
Simplify QpackDecodedHeadersAccumulator and Visitor API such that Visitor
methods are always called instead of Decode() and EndHeaderBlock() signalling
error in return value in the synchronous case.
This allows for a simpler QpackDecodedHeadersAccumulator, QuicSpdyStream, and
QpackRoundTripFuzzer implementation. Also it makes it easier to improve the
handling of headers exceeding the limit in a future CL.
gfe-relnote: n/a, change to QUIC v99-only code. Protected by existing disabled gfe2_reloadable_flag_quic_enable_version_99.
PiperOrigin-RevId: 280327626
Change-Id: Ifc60f6f530340ddb3f40e2861cc353e7abd89422
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc
index 08beb8c..fe990fc 100644
--- a/quic/core/http/quic_spdy_stream.cc
+++ b/quic/core/http/quic_spdy_stream.cc
@@ -537,20 +537,40 @@
}
void QuicSpdyStream::OnHeadersDecoded(QuicHeaderList headers) {
- blocked_on_decoding_headers_ = false;
- ProcessDecodedHeaders(headers);
- // Continue decoding HTTP/3 frames.
- OnDataAvailable();
+ qpack_decoded_headers_accumulator_.reset();
+
+ QuicSpdySession::LogHeaderCompressionRatioHistogram(
+ /* using_qpack = */ true,
+ /* is_sent = */ false, headers.compressed_header_bytes(),
+ headers.uncompressed_header_bytes());
+
+ if (spdy_session_->promised_stream_id() ==
+ QuicUtils::GetInvalidStreamId(session()->transport_version())) {
+ const QuicByteCount frame_length = headers_decompressed_
+ ? trailers_payload_length_
+ : headers_payload_length_;
+ OnStreamHeaderList(/* fin = */ false, frame_length, headers);
+ } else {
+ spdy_session_->OnHeaderList(headers);
+ }
+
+ if (blocked_on_decoding_headers_) {
+ blocked_on_decoding_headers_ = false;
+ // Continue decoding HTTP/3 frames.
+ OnDataAvailable();
+ }
}
-void QuicSpdyStream::OnHeaderDecodingError() {
+void QuicSpdyStream::OnHeaderDecodingError(QuicStringPiece error_message) {
+ qpack_decoded_headers_accumulator_.reset();
+
// TODO(b/124216424): Use HTTP_EXCESSIVE_LOAD instead if indicated by
// |qpack_decoded_headers_accumulator_|.
- std::string error_message = QuicStrCat(
- "Error during async decoding of ",
- headers_decompressed_ ? "trailers" : "headers", " on stream ", id(), ": ",
- qpack_decoded_headers_accumulator_->error_message());
- CloseConnectionWithDetails(QUIC_QPACK_DECOMPRESSION_FAILED, error_message);
+ std::string connection_close_error_message = QuicStrCat(
+ "Error decoding ", headers_decompressed_ ? "trailers" : "headers",
+ " on stream ", id(), ": ", error_message);
+ CloseConnectionWithDetails(QUIC_QPACK_DECOMPRESSION_FAILED,
+ connection_close_error_message);
}
void QuicSpdyStream::OnHeadersTooLarge() {
@@ -898,42 +918,26 @@
headers_payload_length_ += payload.length();
}
- const bool success = qpack_decoded_headers_accumulator_->Decode(payload);
-
+ qpack_decoded_headers_accumulator_->Decode(payload);
sequencer()->MarkConsumed(body_manager_.OnNonBody(payload.size()));
- if (!success) {
- std::string error_message =
- QuicStrCat("Error decompressing header block on stream ", id(), ": ",
- qpack_decoded_headers_accumulator_->error_message());
- CloseConnectionWithDetails(QUIC_QPACK_DECOMPRESSION_FAILED, error_message);
- return false;
- }
- return true;
+ // |qpack_decoded_headers_accumulator_| is reset if an error is detected.
+ return qpack_decoded_headers_accumulator_ != nullptr;
}
bool QuicSpdyStream::OnHeadersFrameEnd() {
DCHECK(VersionUsesHttp3(transport_version()));
DCHECK(qpack_decoded_headers_accumulator_);
- auto result = qpack_decoded_headers_accumulator_->EndHeaderBlock();
+ qpack_decoded_headers_accumulator_->EndHeaderBlock();
- if (result == QpackDecodedHeadersAccumulator::Status::kError) {
- std::string error_message =
- QuicStrCat("Error decompressing header block on stream ", id(), ": ",
- qpack_decoded_headers_accumulator_->error_message());
- CloseConnectionWithDetails(QUIC_QPACK_DECOMPRESSION_FAILED, error_message);
- return false;
- }
-
- if (result == QpackDecodedHeadersAccumulator::Status::kBlocked) {
+ // If decoding is complete or an error is detected, then
+ // |qpack_decoded_headers_accumulator_| is already reset.
+ if (qpack_decoded_headers_accumulator_) {
blocked_on_decoding_headers_ = true;
return false;
}
- DCHECK(result == QpackDecodedHeadersAccumulator::Status::kSuccess);
-
- ProcessDecodedHeaders(qpack_decoded_headers_accumulator_->quic_header_list());
return !sequencer()->IsClosed() && !reading_stopped();
}
@@ -996,24 +1000,6 @@
return true;
}
-void QuicSpdyStream::ProcessDecodedHeaders(const QuicHeaderList& headers) {
- QuicSpdySession::LogHeaderCompressionRatioHistogram(
- /* using_qpack = */ true,
- /* is_sent = */ false, headers.compressed_header_bytes(),
- headers.uncompressed_header_bytes());
-
- if (spdy_session_->promised_stream_id() ==
- QuicUtils::GetInvalidStreamId(session()->transport_version())) {
- const QuicByteCount frame_length = headers_decompressed_
- ? trailers_payload_length_
- : headers_payload_length_;
- OnStreamHeaderList(/* fin = */ false, frame_length, headers);
- } else {
- spdy_session_->OnHeaderList(headers);
- }
- qpack_decoded_headers_accumulator_.reset();
-}
-
size_t QuicSpdyStream::WriteHeadersImpl(
spdy::SpdyHeaderBlock header_block,
bool fin,
diff --git a/quic/core/http/quic_spdy_stream.h b/quic/core/http/quic_spdy_stream.h
index 317bfc5..a7a0939 100644
--- a/quic/core/http/quic_spdy_stream.h
+++ b/quic/core/http/quic_spdy_stream.h
@@ -213,7 +213,7 @@
// QpackDecodedHeadersAccumulator::Visitor implementation.
void OnHeadersDecoded(QuicHeaderList headers) override;
- void OnHeaderDecodingError() override;
+ void OnHeaderDecodingError(QuicStringPiece error_message) override;
protected:
// Called when the received headers are too large. By default this will
@@ -265,9 +265,6 @@
bool OnUnknownFramePayload(QuicStringPiece payload);
bool OnUnknownFrameEnd();
- // Called internally when headers are decoded.
- void ProcessDecodedHeaders(const QuicHeaderList& headers);
-
// Given the interval marked by [|offset|, |offset| + |data_length|), return
// the number of frame header bytes contained in it.
QuicByteCount GetNumFrameHeadersInInterval(QuicStreamOffset offset,
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc
index 0e6161b..af52992 100644
--- a/quic/core/http/quic_spdy_stream_test.cc
+++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -1840,11 +1840,10 @@
EXPECT_CALL(
*connection_,
- CloseConnection(
- QUIC_QPACK_DECOMPRESSION_FAILED,
- MatchesRegex("Error decompressing header block on stream \\d+: "
- "Incomplete header block."),
- _))
+ CloseConnection(QUIC_QPACK_DECOMPRESSION_FAILED,
+ MatchesRegex("Error decoding headers on stream \\d+: "
+ "Incomplete header block."),
+ _))
.WillOnce(
(Invoke([this](QuicErrorCode error, const std::string& error_details,
ConnectionCloseBehavior connection_close_behavior) {
@@ -1994,11 +1993,10 @@
EXPECT_CALL(
*connection_,
- CloseConnection(
- QUIC_QPACK_DECOMPRESSION_FAILED,
- MatchesRegex("Error during async decoding of headers on stream \\d+: "
- "Required Insert Count too large."),
- ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET));
+ CloseConnection(QUIC_QPACK_DECOMPRESSION_FAILED,
+ MatchesRegex("Error decoding headers on stream \\d+: "
+ "Required Insert Count too large."),
+ ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET));
// Deliver two dynamic table entries to decoder
// to trigger decoding of header block.
@@ -2054,13 +2052,12 @@
// Insert Count value advertised in the header block prefix.
EXPECT_FALSE(stream_->trailers_decompressed());
- EXPECT_CALL(*connection_,
- CloseConnection(
- QUIC_QPACK_DECOMPRESSION_FAILED,
- MatchesRegex(
- "Error during async decoding of trailers on stream \\d+: "
- "Required Insert Count too large."),
- ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET));
+ EXPECT_CALL(
+ *connection_,
+ CloseConnection(QUIC_QPACK_DECOMPRESSION_FAILED,
+ MatchesRegex("Error decoding trailers on stream \\d+: "
+ "Required Insert Count too large."),
+ ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET));
// Deliver second dynamic table entry to decoder
// to trigger decoding of trailing header block.
