Make writing Prioritized Element ID and Element Dependency ID optional when handling priority. These fields are specified to be absent when priority type or dependency type is ROOT_OF_TREE. On the decoding side, the absent fields will be parsed as 0 for default value. Those fields won't be used when later the frame is processed. gfe-relnote: v99 only, not flag protected. PiperOrigin-RevId: 254072432 Change-Id: I336e06b39d89ff01507e54818dafa67923e96e4c
diff --git a/quic/core/http/http_decoder.cc b/quic/core/http/http_decoder.cc index f08d2ff..4513f04 100644 --- a/quic/core/http/http_decoder.cc +++ b/quic/core/http/http_decoder.cc
@@ -519,11 +519,13 @@ frame->dependency_type = static_cast<PriorityElementType>(ExtractBits(flags, 2, 4)); frame->exclusive = flags % 2 == 1; - if (!reader->ReadVarInt62(&frame->prioritized_element_id)) { + if (frame->prioritized_type != ROOT_OF_TREE && + !reader->ReadVarInt62(&frame->prioritized_element_id)) { RaiseError(QUIC_INTERNAL_ERROR, "Unable to read prioritized_element_id"); return false; } - if (!reader->ReadVarInt62(&frame->element_dependency_id)) { + if (frame->dependency_type != ROOT_OF_TREE && + !reader->ReadVarInt62(&frame->element_dependency_id)) { RaiseError(QUIC_INTERNAL_ERROR, "Unable to read element_dependency_id"); return false; }
diff --git a/quic/core/http/http_decoder_test.cc b/quic/core/http/http_decoder_test.cc index b99689f..c25d086 100644 --- a/quic/core/http/http_decoder_test.cc +++ b/quic/core/http/http_decoder_test.cc
@@ -343,6 +343,26 @@ EXPECT_EQ(QUIC_NO_ERROR, decoder_.error()); EXPECT_EQ("", decoder_.error_detail()); + char input2[] = {// type (PRIORITY) + 0x2, + // length + 0x2, + // root of tree, root of tree, exclusive + 0xf1, + // weight + 0xFF}; + PriorityFrame frame2; + frame2.prioritized_type = ROOT_OF_TREE; + frame2.dependency_type = ROOT_OF_TREE; + frame2.exclusive = true; + frame2.weight = 0xFF; + + EXPECT_CALL(visitor_, OnPriorityFrame(frame2)); + EXPECT_EQ(QUIC_ARRAYSIZE(input2), + decoder_.ProcessInput(input2, QUIC_ARRAYSIZE(input2))); + EXPECT_EQ(QUIC_NO_ERROR, decoder_.error()); + EXPECT_EQ("", decoder_.error_detail()); + // Test on the situation when the visitor wants to stop processing. EXPECT_CALL(visitor_, OnPriorityFrame(frame)).WillOnce(Return(false)); EXPECT_EQ(0u, decoder_.ProcessInput(input, QUIC_ARRAYSIZE(input)));
diff --git a/quic/core/http/http_encoder.cc b/quic/core/http/http_encoder.cc index 4505dc7..2b8be8e 100644 --- a/quic/core/http/http_encoder.cc +++ b/quic/core/http/http_encoder.cc
@@ -3,6 +3,7 @@ // found in the LICENSE file. #include "net/third_party/quiche/src/quic/core/http/http_encoder.h" + #include "net/third_party/quiche/src/quic/core/quic_data_writer.h" namespace quic { @@ -43,6 +44,8 @@ static const size_t kPriorityWeightLength = 1; // Length of a priority frame's first byte. static const size_t kPriorityFirstByteLength = 1; +// The bit that indicates Priority frame is exclusive. +static const uint8_t kPriorityExclusiveBit = 1; } // namespace @@ -95,8 +98,12 @@ std::unique_ptr<char[]>* output) { QuicByteCount payload_length = kPriorityFirstByteLength + - QuicDataWriter::GetVarInt62Len(priority.prioritized_element_id) + - QuicDataWriter::GetVarInt62Len(priority.element_dependency_id) + + (priority.prioritized_type == ROOT_OF_TREE + ? 0 + : QuicDataWriter::GetVarInt62Len(priority.prioritized_element_id)) + + (priority.dependency_type == ROOT_OF_TREE + ? 0 + : QuicDataWriter::GetVarInt62Len(priority.element_dependency_id)) + kPriorityWeightLength; QuicByteCount total_length = GetTotalLength(payload_length, HttpFrameType::PRIORITY); @@ -111,16 +118,14 @@ } // Set the first byte of the payload. - uint8_t bits = 0; - bits = SetPriorityFields(bits, priority.prioritized_type, true); - bits = SetPriorityFields(bits, priority.dependency_type, false); + uint8_t firstByte = 0; + firstByte = SetPriorityFields(firstByte, priority.prioritized_type, true); + firstByte = SetPriorityFields(firstByte, priority.dependency_type, false); if (priority.exclusive) { - bits |= 1; + firstByte |= kPriorityExclusiveBit; } - if (writer.WriteUInt8(bits) && - writer.WriteVarInt62(priority.prioritized_element_id) && - writer.WriteVarInt62(priority.element_dependency_id) && + if (writer.WriteUInt8(firstByte) && MaybeWriteIds(priority, &writer) && writer.WriteUInt8(priority.weight)) { return total_length; } @@ -282,4 +287,27 @@ payload_length; } +bool HttpEncoder::MaybeWriteIds(const PriorityFrame& priority, + QuicDataWriter* writer) { + if (priority.prioritized_type != ROOT_OF_TREE) { + if (!writer->WriteVarInt62(priority.prioritized_element_id)) { + return false; + } + } else { + DCHECK_EQ(0u, priority.prioritized_element_id) + << "Prioritized element id should be 0 when prioritized type is " + "ROOT_OF_TREE"; + } + if (priority.dependency_type != ROOT_OF_TREE) { + if (!writer->WriteVarInt62(priority.element_dependency_id)) { + return false; + } + } else { + DCHECK_EQ(0u, priority.element_dependency_id) + << "Element dependency id should be 0 when dependency type is " + "ROOT_OF_TREE"; + } + return true; +} + } // namespace quic
diff --git a/quic/core/http/http_encoder.h b/quic/core/http/http_encoder.h index a3c1f4c..12c5bab 100644 --- a/quic/core/http/http_encoder.h +++ b/quic/core/http/http_encoder.h
@@ -76,6 +76,9 @@ QuicByteCount GetTotalLength(QuicByteCount payload_length, HttpFrameType type); + + // Write prioritized element id and element dependency id if needed. + bool MaybeWriteIds(const PriorityFrame& priority, QuicDataWriter* writer); }; } // namespace quic
diff --git a/quic/core/http/http_encoder_test.cc b/quic/core/http/http_encoder_test.cc index 6a029d8..d0696ed 100644 --- a/quic/core/http/http_encoder_test.cc +++ b/quic/core/http/http_encoder_test.cc
@@ -3,6 +3,7 @@ // found in the LICENSE file. #include "net/third_party/quiche/src/quic/core/http/http_encoder.h" + #include "net/third_party/quiche/src/quic/platform/api/quic_arraysize.h" #include "net/third_party/quiche/src/quic/platform/api/quic_test.h" #include "net/third_party/quiche/src/quic/test_tools/quic_test_utils.h" @@ -68,6 +69,45 @@ EXPECT_EQ(QUIC_ARRAYSIZE(output), length); CompareCharArraysWithHexError("PRIORITY", buffer.get(), length, output, QUIC_ARRAYSIZE(output)); + + PriorityFrame priority2; + priority2.prioritized_type = ROOT_OF_TREE; + priority2.dependency_type = REQUEST_STREAM; + priority2.exclusive = true; + priority2.element_dependency_id = 0x04; + priority2.weight = 0xFF; + char output2[] = {// type (PRIORIRTY) + 0x2, + // length + 0x3, + // root of tree, request stream, exclusive + 0xc1, + // element_dependency_id + 0x04, + // weight + 0xff}; + length = encoder_.SerializePriorityFrame(priority2, &buffer); + EXPECT_EQ(QUIC_ARRAYSIZE(output2), length); + CompareCharArraysWithHexError("PRIORITY", buffer.get(), length, output2, + QUIC_ARRAYSIZE(output2)); + + PriorityFrame priority3; + priority3.prioritized_type = ROOT_OF_TREE; + priority3.dependency_type = ROOT_OF_TREE; + priority3.exclusive = true; + priority3.weight = 0xFF; + char output3[] = {// type (PRIORITY) + 0x2, + // length + 0x2, + // root of tree, root of tree, exclusive + 0xf1, + // weight + 0xff}; + length = encoder_.SerializePriorityFrame(priority3, &buffer); + EXPECT_EQ(QUIC_ARRAYSIZE(output3), length); + CompareCharArraysWithHexError("PRIORITY", buffer.get(), length, output3, + QUIC_ARRAYSIZE(output3)); } TEST_F(HttpEncoderTest, SerializeCancelPushFrame) {
diff --git a/quic/core/http/http_frames.h b/quic/core/http/http_frames.h index f1aa7ff..ff59614 100644 --- a/quic/core/http/http_frames.h +++ b/quic/core/http/http_frames.h
@@ -53,12 +53,12 @@ }; struct PriorityFrame { - PriorityElementType prioritized_type; - PriorityElementType dependency_type; - bool exclusive; - uint64_t prioritized_element_id; - uint64_t element_dependency_id; - uint8_t weight; + PriorityElementType prioritized_type = REQUEST_STREAM; + PriorityElementType dependency_type = REQUEST_STREAM; + bool exclusive = false; + uint64_t prioritized_element_id = 0; + uint64_t element_dependency_id = 0; + uint8_t weight = 0; bool operator==(const PriorityFrame& rhs) const { return prioritized_type == rhs.prioritized_type &&