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,
