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.
diff --git a/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc b/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc
index 1f06a43..bb89737 100644
--- a/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc
+++ b/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc
@@ -273,33 +273,13 @@
visitor_->OnHeaderBlockDecoded(stream_id_);
}
- void OnHeaderDecodingError() override {
- CHECK(false) << accumulator_.error_message();
+ void OnHeaderDecodingError(QuicStringPiece error_message) override {
+ CHECK(false) << error_message;
}
- void Decode(QuicStringPiece data) {
- const bool success = accumulator_.Decode(data);
- CHECK(success) << accumulator_.error_message();
- }
+ void Decode(QuicStringPiece data) { accumulator_.Decode(data); }
- void EndHeaderBlock() {
- QpackDecodedHeadersAccumulator::Status status =
- accumulator_.EndHeaderBlock();
-
- CHECK(status != QpackDecodedHeadersAccumulator::Status::kError)
- << accumulator_.error_message();
-
- if (status == QpackDecodedHeadersAccumulator::Status::kBlocked) {
- return;
- }
-
- CHECK(status == QpackDecodedHeadersAccumulator::Status::kSuccess);
-
- // Compare resulting header list to original.
- CHECK(expected_header_list_ == accumulator_.quic_header_list());
- // Might destroy |this|.
- visitor_->OnHeaderBlockDecoded(stream_id_);
- }
+ void EndHeaderBlock() { accumulator_.EndHeaderBlock(); }
private:
QuicStreamId stream_id_;
diff --git a/quic/core/qpack/qpack_decoded_headers_accumulator.cc b/quic/core/qpack/qpack_decoded_headers_accumulator.cc
index 9cce573..7c305ed 100644
--- a/quic/core/qpack/qpack_decoded_headers_accumulator.cc
+++ b/quic/core/qpack/qpack_decoded_headers_accumulator.cc
@@ -18,7 +18,6 @@
uncompressed_header_bytes_(0),
compressed_header_bytes_(0),
headers_decoded_(false),
- blocked_(false),
error_detected_(false) {
quic_header_list_.set_max_header_list_size(max_header_list_size);
quic_header_list_.OnHeaderBlockStart();
@@ -40,9 +39,8 @@
quic_header_list_.OnHeaderBlockEnd(uncompressed_header_bytes_,
compressed_header_bytes_);
- if (blocked_) {
- visitor_->OnHeadersDecoded(quic_header_list_);
- }
+ // Might destroy |this|.
+ visitor_->OnHeadersDecoded(std::move(quic_header_list_));
}
void QpackDecodedHeadersAccumulator::OnDecodingErrorDetected(
@@ -51,51 +49,24 @@
DCHECK(!headers_decoded_);
error_detected_ = true;
- // Copy error message to ensure it remains valid for the lifetime of |this|.
- error_message_.assign(error_message.data(), error_message.size());
-
- if (blocked_) {
- visitor_->OnHeaderDecodingError();
- }
+ // Might destroy |this|.
+ visitor_->OnHeaderDecodingError(error_message);
}
-bool QpackDecodedHeadersAccumulator::Decode(QuicStringPiece data) {
+void QpackDecodedHeadersAccumulator::Decode(QuicStringPiece data) {
DCHECK(!error_detected_);
compressed_header_bytes_ += data.size();
+ // Might destroy |this|.
decoder_->Decode(data);
-
- return !error_detected_;
}
-QpackDecodedHeadersAccumulator::Status
-QpackDecodedHeadersAccumulator::EndHeaderBlock() {
+void QpackDecodedHeadersAccumulator::EndHeaderBlock() {
DCHECK(!error_detected_);
DCHECK(!headers_decoded_);
+ // Might destroy |this|.
decoder_->EndHeaderBlock();
-
- if (error_detected_) {
- DCHECK(!headers_decoded_);
- return Status::kError;
- }
-
- if (headers_decoded_) {
- return Status::kSuccess;
- }
-
- blocked_ = true;
- return Status::kBlocked;
-}
-
-const QuicHeaderList& QpackDecodedHeadersAccumulator::quic_header_list() const {
- DCHECK(!error_detected_);
- return quic_header_list_;
-}
-
-QuicStringPiece QpackDecodedHeadersAccumulator::error_message() const {
- DCHECK(error_detected_);
- return error_message_;
}
} // namespace quic
diff --git a/quic/core/qpack/qpack_decoded_headers_accumulator.h b/quic/core/qpack/qpack_decoded_headers_accumulator.h
index 93c5621..460c189 100644
--- a/quic/core/qpack/qpack_decoded_headers_accumulator.h
+++ b/quic/core/qpack/qpack_decoded_headers_accumulator.h
@@ -20,23 +20,16 @@
// A class that creates and owns a QpackProgressiveDecoder instance, accumulates
// decoded headers in a QuicHeaderList, and keeps track of uncompressed and
-// compressed size so that it can be passed to QuicHeaderList::EndHeaderBlock().
+// compressed size so that it can be passed to
+// QuicHeaderList::OnHeaderBlockEnd().
class QUIC_EXPORT_PRIVATE QpackDecodedHeadersAccumulator
: public QpackProgressiveDecoder::HeadersHandlerInterface {
public:
- // Return value for EndHeaderBlock().
- enum class Status {
- // Headers have been successfully decoded.
- kSuccess,
- // An error has occurred.
- kError,
- // Decoding is blocked.
- kBlocked
- };
-
- // Visitor interface used for blocked decoding. Exactly one visitor method
- // will be called if EndHeaderBlock() returned kBlocked. No visitor method
- // will be called if EndHeaderBlock() returned any other value.
+ // Visitor interface to signal success or error.
+ // Exactly one method will be called.
+ // Methods may be called synchronously from Decode() and EndHeaderBlock(),
+ // or asynchronously.
+ // Method implementations are allowed to destroy |this|.
class QUIC_EXPORT_PRIVATE Visitor {
public:
virtual ~Visitor() = default;
@@ -45,7 +38,7 @@
virtual void OnHeadersDecoded(QuicHeaderList headers) = 0;
// Called when an error has occurred.
- virtual void OnHeaderDecodingError() = 0;
+ virtual void OnHeaderDecodingError(QuicStringPiece error_message) = 0;
};
QpackDecodedHeadersAccumulator(QuicStreamId id,
@@ -60,29 +53,15 @@
void OnDecodingCompleted() override;
void OnDecodingErrorDetected(QuicStringPiece error_message) override;
- // Decode payload data. Returns true on success, false on error.
+ // Decode payload data.
// Must not be called if an error has been detected.
// Must not be called after EndHeaderBlock().
- bool Decode(QuicStringPiece data);
+ void Decode(QuicStringPiece data);
// Signal end of HEADERS frame.
// Must not be called if an error has been detected.
// Must not be called more that once.
- // Returns kSuccess if headers can be readily decoded.
- // Returns kError if an error occurred.
- // Returns kBlocked if headers cannot be decoded at the moment, in which case
- // exactly one Visitor method will be called as soon as sufficient data
- // is received on the QPACK decoder stream.
- Status EndHeaderBlock();
-
- // Returns accumulated header list.
- const QuicHeaderList& quic_header_list() const;
-
- // Returns error message.
- // Must not be called unless an error has been detected.
- // TODO(b/124216424): Add accessor for error code, return HTTP_EXCESSIVE_LOAD
- // or HTTP_QPACK_DECOMPRESSION_FAILED.
- QuicStringPiece error_message() const;
+ void EndHeaderBlock();
private:
std::unique_ptr<QpackProgressiveDecoder> decoder_;
@@ -90,12 +69,10 @@
QuicHeaderList quic_header_list_;
size_t uncompressed_header_bytes_;
size_t compressed_header_bytes_;
- // Set to true when OnDecodingCompleted() is called.
+ // True if headers have been completedly and successfully decoded.
bool headers_decoded_;
- // Set to true when EndHeaderBlock() returns kBlocked.
- bool blocked_;
+ // An error is detected during decoding.
bool error_detected_;
- std::string error_message_;
};
} // namespace quic
diff --git a/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc b/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc
index 8a281ce..11d4289 100644
--- a/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc
+++ b/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc
@@ -12,11 +12,12 @@
#include "net/third_party/quiche/src/quic/test_tools/qpack/qpack_decoder_test_utils.h"
#include "net/third_party/quiche/src/quic/test_tools/qpack/qpack_test_utils.h"
+using ::testing::_;
using ::testing::ElementsAre;
using ::testing::Eq;
using ::testing::Pair;
+using ::testing::SaveArg;
using ::testing::StrictMock;
-using Status = quic::QpackDecodedHeadersAccumulator::Status;
namespace quic {
namespace test {
@@ -39,11 +40,11 @@
} // anonymous namespace
-class NoopVisitor : public QpackDecodedHeadersAccumulator::Visitor {
+class MockVisitor : public QpackDecodedHeadersAccumulator::Visitor {
public:
- ~NoopVisitor() override = default;
- void OnHeadersDecoded(QuicHeaderList /* headers */) override {}
- void OnHeaderDecodingError() override {}
+ ~MockVisitor() override = default;
+ MOCK_METHOD1(OnHeadersDecoded, void(QuicHeaderList headers));
+ MOCK_METHOD1(OnHeaderDecodingError, void(QuicStringPiece error_message));
};
class QpackDecodedHeadersAccumulatorTest : public QuicTest {
@@ -63,52 +64,64 @@
NoopEncoderStreamErrorDelegate encoder_stream_error_delegate_;
StrictMock<MockQpackStreamSenderDelegate> decoder_stream_sender_delegate_;
QpackDecoder qpack_decoder_;
- NoopVisitor visitor_;
+ StrictMock<MockVisitor> visitor_;
QpackDecodedHeadersAccumulator accumulator_;
};
// HEADERS frame payload must have a complete Header Block Prefix.
TEST_F(QpackDecodedHeadersAccumulatorTest, EmptyPayload) {
- EXPECT_EQ(Status::kError, accumulator_.EndHeaderBlock());
- EXPECT_EQ("Incomplete header data prefix.", accumulator_.error_message());
+ EXPECT_CALL(visitor_,
+ OnHeaderDecodingError(Eq("Incomplete header data prefix.")));
+ accumulator_.EndHeaderBlock();
}
// HEADERS frame payload must have a complete Header Block Prefix.
TEST_F(QpackDecodedHeadersAccumulatorTest, TruncatedHeaderBlockPrefix) {
- EXPECT_TRUE(accumulator_.Decode(QuicTextUtils::HexDecode("00")));
- EXPECT_EQ(Status::kError, accumulator_.EndHeaderBlock());
- EXPECT_EQ("Incomplete header data prefix.", accumulator_.error_message());
+ accumulator_.Decode(QuicTextUtils::HexDecode("00"));
+
+ EXPECT_CALL(visitor_,
+ OnHeaderDecodingError(Eq("Incomplete header data prefix.")));
+ accumulator_.EndHeaderBlock();
}
TEST_F(QpackDecodedHeadersAccumulatorTest, EmptyHeaderList) {
- EXPECT_TRUE(accumulator_.Decode(QuicTextUtils::HexDecode("0000")));
- EXPECT_EQ(Status::kSuccess, accumulator_.EndHeaderBlock());
+ std::string encoded_data(QuicTextUtils::HexDecode("0000"));
+ accumulator_.Decode(encoded_data);
- EXPECT_TRUE(accumulator_.quic_header_list().empty());
+ QuicHeaderList header_list;
+ EXPECT_CALL(visitor_, OnHeadersDecoded(_)).WillOnce(SaveArg<0>(&header_list));
+ accumulator_.EndHeaderBlock();
+
+ EXPECT_EQ(0u, header_list.uncompressed_header_bytes());
+ EXPECT_EQ(encoded_data.size(), header_list.compressed_header_bytes());
+ EXPECT_TRUE(header_list.empty());
}
// This payload is the prefix of a valid payload, but EndHeaderBlock() is called
// before it can be completely decoded.
TEST_F(QpackDecodedHeadersAccumulatorTest, TruncatedPayload) {
- EXPECT_TRUE(accumulator_.Decode(QuicTextUtils::HexDecode("00002366")));
- EXPECT_EQ(Status::kError, accumulator_.EndHeaderBlock());
- EXPECT_EQ("Incomplete header block.", accumulator_.error_message());
+ accumulator_.Decode(QuicTextUtils::HexDecode("00002366"));
+
+ EXPECT_CALL(visitor_, OnHeaderDecodingError(Eq("Incomplete header block.")));
+ accumulator_.EndHeaderBlock();
}
// This payload is invalid because it refers to a non-existing static entry.
TEST_F(QpackDecodedHeadersAccumulatorTest, InvalidPayload) {
- EXPECT_FALSE(accumulator_.Decode(QuicTextUtils::HexDecode("0000ff23ff24")));
- EXPECT_EQ("Static table entry not found.", accumulator_.error_message());
+ EXPECT_CALL(visitor_,
+ OnHeaderDecodingError(Eq("Static table entry not found.")));
+ accumulator_.Decode(QuicTextUtils::HexDecode("0000ff23ff24"));
}
TEST_F(QpackDecodedHeadersAccumulatorTest, Success) {
std::string encoded_data(QuicTextUtils::HexDecode("000023666f6f03626172"));
- EXPECT_TRUE(accumulator_.Decode(encoded_data));
- EXPECT_EQ(Status::kSuccess, accumulator_.EndHeaderBlock());
+ accumulator_.Decode(encoded_data);
- const QuicHeaderList& header_list = accumulator_.quic_header_list();
+ QuicHeaderList header_list;
+ EXPECT_CALL(visitor_, OnHeadersDecoded(_)).WillOnce(SaveArg<0>(&header_list));
+ accumulator_.EndHeaderBlock();
+
EXPECT_THAT(header_list, ElementsAre(Pair("foo", "bar")));
-
EXPECT_EQ(strlen("foo") + strlen("bar"),
header_list.uncompressed_header_bytes());
EXPECT_EQ(encoded_data.size(), header_list.compressed_header_bytes());
@@ -116,38 +129,51 @@
TEST_F(QpackDecodedHeadersAccumulatorTest, ExceedingLimit) {
// Total length of header list exceeds kMaxHeaderListSize.
- EXPECT_TRUE(accumulator_.Decode(QuicTextUtils::HexDecode(
+ std::string encoded_data(QuicTextUtils::HexDecode(
"0000" // header block prefix
"26666f6f626172" // header key: "foobar"
"7d61616161616161616161616161616161616161" // header value: 'a' 125 times
"616161616161616161616161616161616161616161616161616161616161616161616161"
"616161616161616161616161616161616161616161616161616161616161616161616161"
- "61616161616161616161616161616161616161616161616161616161616161616161")));
- EXPECT_EQ(Status::kSuccess, accumulator_.EndHeaderBlock());
+ "61616161616161616161616161616161616161616161616161616161616161616161"));
+ accumulator_.Decode(encoded_data);
+
+ QuicHeaderList header_list;
+ EXPECT_CALL(visitor_, OnHeadersDecoded(_)).WillOnce(SaveArg<0>(&header_list));
+ accumulator_.EndHeaderBlock();
// QuicHeaderList signals header list over limit by clearing it.
- EXPECT_TRUE(accumulator_.quic_header_list().empty());
+ EXPECT_EQ(0u, header_list.uncompressed_header_bytes());
+ EXPECT_EQ(0u, header_list.compressed_header_bytes());
+ EXPECT_TRUE(header_list.empty());
}
TEST_F(QpackDecodedHeadersAccumulatorTest, BlockedDecoding) {
// Reference to dynamic table entry not yet received.
- EXPECT_TRUE(accumulator_.Decode(QuicTextUtils::HexDecode("020080")));
- EXPECT_EQ(Status::kBlocked, accumulator_.EndHeaderBlock());
+ std::string encoded_data(QuicTextUtils::HexDecode("020080"));
+ accumulator_.Decode(encoded_data);
+ accumulator_.EndHeaderBlock();
// Set dynamic table capacity.
qpack_decoder_.OnSetDynamicTableCapacity(kMaxDynamicTableCapacity);
// Adding dynamic table entry unblocks decoding.
EXPECT_CALL(decoder_stream_sender_delegate_,
WriteStreamData(Eq(kHeaderAcknowledgement)));
+
+ QuicHeaderList header_list;
+ EXPECT_CALL(visitor_, OnHeadersDecoded(_)).WillOnce(SaveArg<0>(&header_list));
qpack_decoder_.OnInsertWithoutNameReference("foo", "bar");
- EXPECT_THAT(accumulator_.quic_header_list(), ElementsAre(Pair("foo", "bar")));
+ EXPECT_THAT(header_list, ElementsAre(Pair("foo", "bar")));
+ EXPECT_EQ(strlen("foo") + strlen("bar"),
+ header_list.uncompressed_header_bytes());
+ EXPECT_EQ(encoded_data.size(), header_list.compressed_header_bytes());
}
TEST_F(QpackDecodedHeadersAccumulatorTest,
BlockedDecodingUnblockedBeforeEndOfHeaderBlock) {
// Reference to dynamic table entry not yet received.
- EXPECT_TRUE(accumulator_.Decode(QuicTextUtils::HexDecode("020080")));
+ accumulator_.Decode(QuicTextUtils::HexDecode("020080"));
// Set dynamic table capacity.
qpack_decoder_.OnSetDynamicTableCapacity(kMaxDynamicTableCapacity);
@@ -157,11 +183,33 @@
// Rest of header block: same entry again.
EXPECT_CALL(decoder_stream_sender_delegate_,
WriteStreamData(Eq(kHeaderAcknowledgement)));
- EXPECT_TRUE(accumulator_.Decode(QuicTextUtils::HexDecode("80")));
- EXPECT_EQ(Status::kSuccess, accumulator_.EndHeaderBlock());
+ accumulator_.Decode(QuicTextUtils::HexDecode("80"));
- EXPECT_THAT(accumulator_.quic_header_list(),
- ElementsAre(Pair("foo", "bar"), Pair("foo", "bar")));
+ QuicHeaderList header_list;
+ EXPECT_CALL(visitor_, OnHeadersDecoded(_)).WillOnce(SaveArg<0>(&header_list));
+ accumulator_.EndHeaderBlock();
+
+ EXPECT_THAT(header_list, ElementsAre(Pair("foo", "bar"), Pair("foo", "bar")));
+}
+
+// Regression test for https://crbug.com/1024263.
+TEST_F(QpackDecodedHeadersAccumulatorTest,
+ BlockedDecodingUnblockedAndErrorBeforeEndOfHeaderBlock) {
+ // Required Insert Count higher than number of entries causes decoding to be
+ // blocked.
+ accumulator_.Decode(QuicTextUtils::HexDecode("0200"));
+ // Indexed Header Field instruction addressing dynamic table entry with
+ // relative index 0, absolute index 0.
+ accumulator_.Decode(QuicTextUtils::HexDecode("80"));
+ // Relative index larger than or equal to Base is invalid.
+ accumulator_.Decode(QuicTextUtils::HexDecode("81"));
+
+ // Set dynamic table capacity.
+ qpack_decoder_.OnSetDynamicTableCapacity(kMaxDynamicTableCapacity);
+
+ // Adding dynamic table entry unblocks decoding. Error is detected.
+ EXPECT_CALL(visitor_, OnHeaderDecodingError(Eq("Invalid relative index.")));
+ qpack_decoder_.OnInsertWithoutNameReference("foo", "bar");
}
} // namespace test
diff --git a/quic/core/qpack/qpack_decoder_test.cc b/quic/core/qpack/qpack_decoder_test.cc
index 5d05904..323a893 100644
--- a/quic/core/qpack/qpack_decoder_test.cc
+++ b/quic/core/qpack/qpack_decoder_test.cc
@@ -780,16 +780,15 @@
// entry with relative index 0, absolute index 0.
"d1")); // Static table entry with index 17.
+ // Set dynamic table capacity to 1024.
+ DecodeEncoderStreamData(QuicTextUtils::HexDecode("3fe107"));
+
// Add literal entry with name "foo" and value "bar". Decoding is now
// unblocked because dynamic table Insert Count reached the Required Insert
// Count of the header block. |handler_| methods are called immediately for
// the already consumed part of the header block.
EXPECT_CALL(handler_, OnHeaderDecoded(Eq("foo"), Eq("bar")));
EXPECT_CALL(handler_, OnHeaderDecoded(Eq(":method"), Eq("GET")));
-
- // Set dynamic table capacity to 1024.
- DecodeEncoderStreamData(QuicTextUtils::HexDecode("3fe107"));
- // Add literal entry with name "foo" and value "bar".
DecodeEncoderStreamData(QuicTextUtils::HexDecode("6294e703626172"));
Mock::VerifyAndClearExpectations(&handler_);
@@ -809,6 +808,29 @@
EndDecoding();
}
+// Regression test for https://crbug.com/1024263.
+TEST_P(QpackDecoderTest,
+ BlockedDecodingUnblockedAndErrorBeforeEndOfHeaderBlock) {
+ StartDecoding();
+ DecodeData(QuicTextUtils::HexDecode(
+ "0200" // Required Insert Count 1 and Delta Base 0.
+ // Base is 1 + 0 = 1.
+ "80" // Indexed Header Field instruction addressing dynamic table
+ // entry with relative index 0, absolute index 0.
+ "81")); // Relative index 1 is equal to Base, therefore invalid.
+
+ // Set dynamic table capacity to 1024.
+ DecodeEncoderStreamData(QuicTextUtils::HexDecode("3fe107"));
+
+ // Add literal entry with name "foo" and value "bar". Decoding is now
+ // unblocked because dynamic table Insert Count reached the Required Insert
+ // Count of the header block. |handler_| methods are called immediately for
+ // the already consumed part of the header block.
+ EXPECT_CALL(handler_, OnHeaderDecoded(Eq("foo"), Eq("bar")));
+ EXPECT_CALL(handler_, OnDecodingErrorDetected(Eq("Invalid relative index.")));
+ DecodeEncoderStreamData(QuicTextUtils::HexDecode("6294e703626172"));
+}
+
// Make sure that Required Insert Count is compared to Insert Count,
// not size of dynamic table.
TEST_P(QpackDecoderTest, BlockedDecodingAndEvictedEntries) {
