Send Set Dynamic Table Capacity instruction. Do not change dynamic table capacity in QpackHeaderTable::SetMaximumDynamicTableCapacity(), because according to https://quicwg.org/base-drafts/draft-ietf-quic-qpack.html#eviction, "The initial capacity of the dynamic table is zero.". Instead, add QpackEncoder::SetDynamicTableCapacity() to send Set Dynamic Table Capacity instruction. Roundtrip tests were passing because QpackHeaderTable::SetMaximumDynamicTableCapacity() incorrectly updated the capacity for both the encoder and decoder context. gfe-relnote: n/a, change to QUIC v99-only code. Protected by existing disabled gfe2_reloadable_flag_quic_enable_version_99. PiperOrigin-RevId: 266358801 Change-Id: I23f10f224139dee79e9305c42c85b75cba61a999
diff --git a/quic/core/http/quic_spdy_session.cc b/quic/core/http/quic_spdy_session.cc index 101b33d..73f63bb 100644 --- a/quic/core/http/quic_spdy_session.cc +++ b/quic/core/http/quic_spdy_session.cc
@@ -694,9 +694,13 @@ QUIC_DVLOG(1) << "SETTINGS_QPACK_MAX_TABLE_CAPACITY received with value " << value; - // TODO(b/112770235): Limit value to - // qpack_maximum_dynamic_table_capacity_. + // Communicate |value| to encoder, because it is used for encoding + // Required Insert Count. qpack_encoder_->SetMaximumDynamicTableCapacity(value); + // However, limit the dynamic table capacity to + // |qpack_maximum_dynamic_table_capacity_|. + qpack_encoder_->SetDynamicTableCapacity( + std::min(value, qpack_maximum_dynamic_table_capacity_)); break; case SETTINGS_MAX_HEADER_LIST_SIZE: QUIC_DVLOG(1) << "SETTINGS_MAX_HEADER_LIST_SIZE received with value "
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc index 3798461..e09cbf4 100644 --- a/quic/core/http/quic_spdy_stream_test.cc +++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -1921,6 +1921,7 @@ testing::InSequence s; Initialize(kShouldProcessData); + session_->qpack_decoder()->OnSetDynamicTableCapacity(1024); auto decoder_send_stream = QuicSpdySessionPeer::GetQpackDecoderSendStream(session_.get()); @@ -1983,6 +1984,7 @@ testing::InSequence s; Initialize(kShouldProcessData); + session_->qpack_decoder()->OnSetDynamicTableCapacity(1024); // HEADERS frame referencing first dynamic table entry. std::string headers = HeadersFrame(QuicTextUtils::HexDecode("020080")); @@ -2040,6 +2042,7 @@ } Initialize(kShouldProcessData); + session_->qpack_decoder()->OnSetDynamicTableCapacity(1024); // HEADERS frame only referencing entry with absolute index 0 but with // Required Insert Count = 2, which is incorrect. @@ -2078,6 +2081,7 @@ testing::InSequence s; Initialize(kShouldProcessData); + session_->qpack_decoder()->OnSetDynamicTableCapacity(1024); // HEADERS frame referencing first dynamic table entry. std::string headers = HeadersFrame(QuicTextUtils::HexDecode("020080"));
diff --git a/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc b/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc index 624eca5..a26a6c3 100644 --- a/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc +++ b/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc
@@ -45,6 +45,10 @@ encoder_.set_qpack_stream_sender_delegate(delegate); } + void SetDynamicTableCapacity(uint64_t maximum_dynamic_table_capacity) { + encoder_.SetDynamicTableCapacity(maximum_dynamic_table_capacity); + } + QpackStreamReceiver* decoder_stream_receiver() { return encoder_.decoder_stream_receiver(); } @@ -596,6 +600,10 @@ decoder.encoder_stream_receiver(), &provider); encoder.set_qpack_stream_sender_delegate(&encoder_stream_transmitter); + // Use a dynamic table as large as the peer allows. This sends data on the + // encoder stream, so it can only be done after delegate is set. + encoder.SetDynamicTableCapacity(maximum_dynamic_table_capacity); + // Transmit decoder stream data from encoder to decoder. DelayedStreamDataTransmitter decoder_stream_transmitter( encoder.decoder_stream_receiver(), &provider);
diff --git a/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc b/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc index 2a0a222..1d60660 100644 --- a/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc +++ b/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc
@@ -134,6 +134,8 @@ EXPECT_TRUE(accumulator_.Decode(QuicTextUtils::HexDecode("020080"))); EXPECT_EQ(Status::kBlocked, 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))); @@ -147,6 +149,8 @@ // Reference to dynamic table entry not yet received. EXPECT_TRUE(accumulator_.Decode(QuicTextUtils::HexDecode("020080"))); + // Set dynamic table capacity. + qpack_decoder_.OnSetDynamicTableCapacity(kMaxDynamicTableCapacity); // Adding dynamic table entry unblocks decoding. qpack_decoder_.OnInsertWithoutNameReference("foo", "bar");
diff --git a/quic/core/qpack/qpack_decoder_test.cc b/quic/core/qpack/qpack_decoder_test.cc index 0a4e5e2..ef711b9 100644 --- a/quic/core/qpack/qpack_decoder_test.cc +++ b/quic/core/qpack/qpack_decoder_test.cc
@@ -301,6 +301,7 @@ TEST_P(QpackDecoderTest, DynamicTable) { DecodeEncoderStreamData(QuicTextUtils::HexDecode( + "3fe107" // Set dynamic table capacity to 1024. "6294e703626172" // Add literal entry with name "foo" and value "bar". "80035a5a5a" // Add entry with name of dynamic table entry index 0 // (relative index) and value "ZZZ". @@ -382,6 +383,8 @@ } TEST_P(QpackDecoderTest, DecreasingDynamicTableCapacityEvictsEntries) { + // Set dynamic table capacity to 1024. + DecodeEncoderStreamData(QuicTextUtils::HexDecode("3fe107")); // Add literal entry with name "foo" and value "bar". DecodeEncoderStreamData(QuicTextUtils::HexDecode("6294e703626172")); @@ -431,6 +434,7 @@ OnEncoderStreamError(Eq("Invalid relative index."))); DecodeEncoderStreamData(QuicTextUtils::HexDecode( + "3fe107" // Set dynamic table capacity to 1024. "6294e703626172" // Add literal entry with name "foo" and value "bar". "8100")); // Address dynamic table entry with relative index 1. Such // entry does not exist. The most recently added and only @@ -442,6 +446,7 @@ OnEncoderStreamError(Eq("Invalid relative index."))); DecodeEncoderStreamData(QuicTextUtils::HexDecode( + "3fe107" // Set dynamic table capacity to 1024. "6294e703626172" // Add literal entry with name "foo" and value "bar". "01")); // Duplicate dynamic table entry with relative index 1. Such // entry does not exist. The most recently added and only @@ -458,6 +463,8 @@ TEST_P(QpackDecoderTest, InvalidDynamicEntryWhenBaseIsZero) { EXPECT_CALL(handler_, OnDecodingErrorDetected(Eq("Invalid relative index."))); + // Set dynamic table capacity to 1024. + DecodeEncoderStreamData(QuicTextUtils::HexDecode("3fe107")); // Add literal entry with name "foo" and value "bar". DecodeEncoderStreamData(QuicTextUtils::HexDecode("6294e703626172")); @@ -477,6 +484,8 @@ } TEST_P(QpackDecoderTest, InvalidDynamicEntryByRelativeIndex) { + // Set dynamic table capacity to 1024. + DecodeEncoderStreamData(QuicTextUtils::HexDecode("3fe107")); // Add literal entry with name "foo" and value "bar". DecodeEncoderStreamData(QuicTextUtils::HexDecode("6294e703626172")); @@ -585,6 +594,8 @@ // Maximum dynamic table capacity is 1024. // MaxEntries is 1024 / 32 = 32. + // Set dynamic table capacity to 1024. + DecodeEncoderStreamData(QuicTextUtils::HexDecode("3fe107")); // Add literal entry with name "foo" and a 600 byte long value. This will fit // in the dynamic table once but not twice. DecodeEncoderStreamData( @@ -611,6 +622,8 @@ } TEST_P(QpackDecoderTest, NonZeroRequiredInsertCountButNoDynamicEntries) { + // Set dynamic table capacity to 1024. + DecodeEncoderStreamData(QuicTextUtils::HexDecode("3fe107")); // Add literal entry with name "foo" and value "bar". DecodeEncoderStreamData(QuicTextUtils::HexDecode("6294e703626172")); @@ -624,6 +637,8 @@ } TEST_P(QpackDecoderTest, AddressEntryNotAllowedByRequiredInsertCount) { + // Set dynamic table capacity to 1024. + DecodeEncoderStreamData(QuicTextUtils::HexDecode("3fe107")); // Add literal entry with name "foo" and value "bar". DecodeEncoderStreamData(QuicTextUtils::HexDecode("6294e703626172")); @@ -680,6 +695,8 @@ } TEST_P(QpackDecoderTest, PromisedRequiredInsertCountLargerThanActual) { + // Set dynamic table capacity to 1024. + DecodeEncoderStreamData(QuicTextUtils::HexDecode("3fe107")); // Add literal entry with name "foo" and value "bar". DecodeEncoderStreamData(QuicTextUtils::HexDecode("6294e703626172")); // Duplicate entry twice so that decoding of header blocks with Required @@ -748,6 +765,8 @@ EXPECT_CALL(decoder_stream_sender_delegate_, WriteStreamData(Eq(kHeaderAcknowledgement))); + // Set dynamic table capacity to 1024. + DecodeEncoderStreamData(QuicTextUtils::HexDecode("3fe107")); // Add literal entry with name "foo" and value "bar". DecodeEncoderStreamData(QuicTextUtils::HexDecode("6294e703626172")); } @@ -767,6 +786,10 @@ // 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_);
diff --git a/quic/core/qpack/qpack_encoder.cc b/quic/core/qpack/qpack_encoder.cc index 370887a..d1736f8 100644 --- a/quic/core/qpack/qpack_encoder.cc +++ b/quic/core/qpack/qpack_encoder.cc
@@ -318,11 +318,15 @@ void QpackEncoder::SetMaximumDynamicTableCapacity( uint64_t maximum_dynamic_table_capacity) { - // TODO(b/112770235): Send set dynamic table capacity instruction on encoder - // stream. header_table_.SetMaximumDynamicTableCapacity(maximum_dynamic_table_capacity); } +void QpackEncoder::SetDynamicTableCapacity(uint64_t dynamic_table_capacity) { + encoder_stream_sender_.SendSetDynamicTableCapacity(dynamic_table_capacity); + bool success = header_table_.SetDynamicTableCapacity(dynamic_table_capacity); + DCHECK(success); +} + void QpackEncoder::SetMaximumBlockedStreams(uint64_t maximum_blocked_streams) { maximum_blocked_streams_ = maximum_blocked_streams; }
diff --git a/quic/core/qpack/qpack_encoder.h b/quic/core/qpack/qpack_encoder.h index 993b76e..4aa69c1 100644 --- a/quic/core/qpack/qpack_encoder.h +++ b/quic/core/qpack/qpack_encoder.h
@@ -53,12 +53,17 @@ std::string EncodeHeaderList(QuicStreamId stream_id, const spdy::SpdyHeaderBlock& header_list); - // Set maximum capacity of dynamic table, measured in bytes. - // Called when SETTINGS_QPACK_MAX_TABLE_CAPACITY is received. - // Sends set dynamic table capacity instruction on encoder stream. - // TODO(b/112770235): Actually send set dynamic table capacity instruction. + // Set maximum dynamic table capacity to |maximum_dynamic_table_capacity|, + // measured in bytes. Called when SETTINGS_QPACK_MAX_TABLE_CAPACITY is + // received. Encoder needs to know this value so that it can calculate + // MaxEntries, used as a modulus to encode Required Insert Count. void SetMaximumDynamicTableCapacity(uint64_t maximum_dynamic_table_capacity); + // Set dynamic table capacity to |dynamic_table_capacity|. + // |dynamic_table_capacity| must not exceed maximum dynamic table capacity. + // Also sends Set Dynamic Table Capacity instruction on encoder stream. + void SetDynamicTableCapacity(uint64_t dynamic_table_capacity); + // Set maximum number of blocked streams. // Called when SETTINGS_QPACK_BLOCKED_STREAMS is received. void SetMaximumBlockedStreams(uint64_t maximum_blocked_streams);
diff --git a/quic/core/qpack/qpack_encoder_test.cc b/quic/core/qpack/qpack_encoder_test.cc index f23cb8b..0da6f3e 100644 --- a/quic/core/qpack/qpack_encoder_test.cc +++ b/quic/core/qpack/qpack_encoder_test.cc
@@ -11,6 +11,8 @@ #include "net/third_party/quiche/src/quic/core/qpack/qpack_utils.h" #include "net/third_party/quiche/src/quic/platform/api/quic_test.h" #include "net/third_party/quiche/src/quic/platform/api/quic_text_utils.h" +#include "net/third_party/quiche/src/quic/test_tools/qpack_encoder_peer.h" +#include "net/third_party/quiche/src/quic/test_tools/qpack_header_table_peer.h" using ::testing::_; using ::testing::Eq; @@ -182,8 +184,13 @@ } TEST_F(QpackEncoderTest, DynamicTable) { - encoder_.SetMaximumDynamicTableCapacity(4096); encoder_.SetMaximumBlockedStreams(1); + encoder_.SetMaximumDynamicTableCapacity(4096); + + // Set Dynamic Table Capacity instruction. + EXPECT_CALL(encoder_stream_sender_delegate_, + WriteStreamData(Eq(QuicTextUtils::HexDecode("3fe11f")))); + encoder_.SetDynamicTableCapacity(4096); spdy::SpdyHeaderBlock header_list; header_list["foo"] = "bar"; @@ -214,8 +221,13 @@ // There is no room in the dynamic table after inserting the first entry. TEST_F(QpackEncoderTest, SmallDynamicTable) { - encoder_.SetMaximumDynamicTableCapacity(QpackEntry::Size("foo", "bar")); encoder_.SetMaximumBlockedStreams(1); + encoder_.SetMaximumDynamicTableCapacity(QpackEntry::Size("foo", "bar")); + + // Set Dynamic Table Capacity instruction. + EXPECT_CALL(encoder_stream_sender_delegate_, + WriteStreamData(Eq(QuicTextUtils::HexDecode("3f07")))); + encoder_.SetDynamicTableCapacity(QpackEntry::Size("foo", "bar")); spdy::SpdyHeaderBlock header_list; header_list["foo"] = "bar"; @@ -243,8 +255,13 @@ } TEST_F(QpackEncoderTest, BlockedStream) { - encoder_.SetMaximumDynamicTableCapacity(4096); encoder_.SetMaximumBlockedStreams(1); + encoder_.SetMaximumDynamicTableCapacity(4096); + + // Set Dynamic Table Capacity instruction. + EXPECT_CALL(encoder_stream_sender_delegate_, + WriteStreamData(Eq(QuicTextUtils::HexDecode("3fe11f")))); + encoder_.SetDynamicTableCapacity(4096); spdy::SpdyHeaderBlock header_list1; header_list1["foo"] = "bar"; @@ -362,6 +379,10 @@ maximum_dynamic_table_capacity += QpackEntry::Size("one", "foo"); encoder_.SetMaximumDynamicTableCapacity(maximum_dynamic_table_capacity); + // Set Dynamic Table Capacity instruction. + EXPECT_CALL(encoder_stream_sender_delegate_, WriteStreamData(_)); + encoder_.SetDynamicTableCapacity(maximum_dynamic_table_capacity); + // Insert ten entries into the dynamic table. EXPECT_CALL(encoder_stream_sender_delegate_, WriteStreamData(_)).Times(10); @@ -399,6 +420,21 @@ Encode(header_list3)); } +TEST_F(QpackEncoderTest, DynamicTableCapacityLessThanMaximum) { + encoder_.SetMaximumDynamicTableCapacity(1024); + + // Set Dynamic Table Capacity instruction. + EXPECT_CALL(encoder_stream_sender_delegate_, + WriteStreamData(Eq(QuicTextUtils::HexDecode("3e")))); + encoder_.SetDynamicTableCapacity(30); + + QpackHeaderTable* header_table = QpackEncoderPeer::header_table(&encoder_); + + EXPECT_EQ(1024u, + QpackHeaderTablePeer::maximum_dynamic_table_capacity(header_table)); + EXPECT_EQ(30u, QpackHeaderTablePeer::dynamic_table_capacity(header_table)); +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/qpack/qpack_header_table.cc b/quic/core/qpack/qpack_header_table.cc index 3c9296d..bb28f72 100644 --- a/quic/core/qpack/qpack_header_table.cc +++ b/quic/core/qpack/qpack_header_table.cc
@@ -184,7 +184,6 @@ DCHECK_EQ(0u, maximum_dynamic_table_capacity_); DCHECK_EQ(0u, max_entries_); - dynamic_table_capacity_ = maximum_dynamic_table_capacity; maximum_dynamic_table_capacity_ = maximum_dynamic_table_capacity; max_entries_ = maximum_dynamic_table_capacity / 32; }
diff --git a/quic/core/qpack/qpack_header_table_test.cc b/quic/core/qpack/qpack_header_table_test.cc index 592d2cb..00f36e5 100644 --- a/quic/core/qpack/qpack_header_table_test.cc +++ b/quic/core/qpack/qpack_header_table_test.cc
@@ -30,6 +30,7 @@ QpackHeaderTableTest() { table_.SetMaximumDynamicTableCapacity( kMaximumDynamicTableCapacityForTesting); + table_.SetDynamicTableCapacity(kMaximumDynamicTableCapacityForTesting); } ~QpackHeaderTableTest() override = default; @@ -371,6 +372,7 @@ const uint64_t dynamic_table_capacity = 100; QpackHeaderTable table; table.SetMaximumDynamicTableCapacity(dynamic_table_capacity); + EXPECT_TRUE(table.SetDynamicTableCapacity(dynamic_table_capacity)); // Empty table can take an entry up to its capacity. EXPECT_EQ(dynamic_table_capacity, @@ -455,7 +457,9 @@ TEST_F(QpackHeaderTableTest, DrainingIndex) { QpackHeaderTable table; - table.SetMaximumDynamicTableCapacity(4 * QpackEntry::Size("foo", "bar")); + table.SetMaximumDynamicTableCapacity(kMaximumDynamicTableCapacityForTesting); + EXPECT_TRUE( + table.SetDynamicTableCapacity(4 * QpackEntry::Size("foo", "bar"))); // Empty table: no draining entry. EXPECT_EQ(0u, table.draining_index(0.0));
diff --git a/quic/test_tools/qpack_header_table_peer.cc b/quic/test_tools/qpack_header_table_peer.cc index 37c2961..bb18731 100644 --- a/quic/test_tools/qpack_header_table_peer.cc +++ b/quic/test_tools/qpack_header_table_peer.cc
@@ -10,6 +10,12 @@ namespace test { // static +uint64_t QpackHeaderTablePeer::dynamic_table_capacity( + const QpackHeaderTable* header_table) { + return header_table->dynamic_table_capacity_; +} + +// static uint64_t QpackHeaderTablePeer::maximum_dynamic_table_capacity( const QpackHeaderTable* header_table) { return header_table->maximum_dynamic_table_capacity_;
diff --git a/quic/test_tools/qpack_header_table_peer.h b/quic/test_tools/qpack_header_table_peer.h index a73859c..cbf3f44 100644 --- a/quic/test_tools/qpack_header_table_peer.h +++ b/quic/test_tools/qpack_header_table_peer.h
@@ -17,6 +17,7 @@ public: QpackHeaderTablePeer() = delete; + static uint64_t dynamic_table_capacity(const QpackHeaderTable* header_table); static uint64_t maximum_dynamic_table_capacity( const QpackHeaderTable* header_table); };