gfe-relnote: Do not include PUSH_PROMISE header block length when calling QuicSpdyStream::OnStreamHeaderList(). Not protected.
Before this CL, PUSH_PROMISE header block size is incorrectly added to
headers_payload_length_ or trailers_payload_length_, which is then passed to
QuicSpdyStream::OnStreamHeaderList(). This CL instead saves the payload_length
argument of OnHeadersFrameStart().
The |frame_len| argument of OnStreamHeaderList() is only used in a VLOG statement in QuicEventManagerStream::OnInitialHeadersComplete() and for incrementing |header_bytes_read_| in QuicSpdyClientStream::OnInitialHeadersComplete(). QuicSpdyClientStream::header_bytes_read() is only called from QuicTestClient. Therefore production code is not affected by this change.
PiperOrigin-RevId: 305281258
Change-Id: I65be9886574f849bf25fe0f1e5c05ee2c6a453a9
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc
index c376d01..06962b0 100644
--- a/quic/core/http/quic_spdy_stream.cc
+++ b/quic/core/http/quic_spdy_stream.cc
@@ -192,7 +192,6 @@
headers_decompressed_(false),
header_list_size_limit_exceeded_(false),
headers_payload_length_(0),
- trailers_payload_length_(0),
trailers_decompressed_(false),
trailers_consumed_(false),
http_decoder_visitor_(std::make_unique<HttpDecoderVisitor>(this)),
@@ -229,7 +228,6 @@
headers_decompressed_(false),
header_list_size_limit_exceeded_(false),
headers_payload_length_(0),
- trailers_payload_length_(0),
trailers_decompressed_(false),
trailers_consumed_(false),
http_decoder_visitor_(std::make_unique<HttpDecoderVisitor>(this)),
@@ -569,10 +567,7 @@
debug_visitor->OnHeadersDecoded(id(), headers);
}
- const QuicByteCount frame_length = headers_decompressed_
- ? trailers_payload_length_
- : headers_payload_length_;
- OnStreamHeaderList(/* fin = */ false, frame_length, headers);
+ OnStreamHeaderList(/* fin = */ false, headers_payload_length_, headers);
} else {
if (debug_visitor) {
debug_visitor->OnPushPromiseDecoded(id(), promised_stream_id, headers);
@@ -962,6 +957,8 @@
payload_length);
}
+ headers_payload_length_ = payload_length;
+
if (trailers_decompressed_) {
stream_delegate()->OnStreamError(
QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_SPDY_STREAM,
@@ -983,15 +980,6 @@
DCHECK(VersionUsesHttp3(transport_version()));
DCHECK(qpack_decoded_headers_accumulator_);
- // TODO(b/152518220): Save |payload_length| argument of OnHeadersFrameStart()
- // instead of accumulating payload length in |headers_payload_length_| or
- // |trailers_payload_length_|.
- if (headers_decompressed_) {
- trailers_payload_length_ += payload.length();
- } else {
- headers_payload_length_ += payload.length();
- }
-
qpack_decoded_headers_accumulator_->Decode(payload);
// |qpack_decoded_headers_accumulator_| is reset if an error is detected.
@@ -1040,7 +1028,7 @@
id(), push_id, header_block_length);
}
- // TODO(renjietang): Check max push id and handle errors.
+ // TODO(b/151749109): Check max push id and handle errors.
spdy_session_->OnPushPromise(id(), push_id);
sequencer()->MarkConsumed(body_manager_.OnNonBody(push_id_length));
diff --git a/quic/core/http/quic_spdy_stream.h b/quic/core/http/quic_spdy_stream.h
index abf3ef4..61bc1fb 100644
--- a/quic/core/http/quic_spdy_stream.h
+++ b/quic/core/http/quic_spdy_stream.h
@@ -300,10 +300,8 @@
// Contains a copy of the decompressed header (name, value) pairs until they
// are consumed via Readv.
QuicHeaderList header_list_;
- // Length of HEADERS frame payload.
+ // Length of most recently received HEADERS frame payload.
QuicByteCount headers_payload_length_;
- // Length of TRAILERS frame payload.
- QuicByteCount trailers_payload_length_;
// True if the trailers have been completely decompressed.
bool trailers_decompressed_;
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc
index 4f9aefa..f46d914 100644
--- a/quic/core/http/quic_spdy_stream_test.cc
+++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -144,7 +144,8 @@
QuicSpdySession* session,
bool should_process_data)
: QuicSpdyStream(id, session, BIDIRECTIONAL),
- should_process_data_(should_process_data) {}
+ should_process_data_(should_process_data),
+ headers_payload_length_(0) {}
~TestStream() override = default;
using QuicSpdyStream::set_ack_listener;
@@ -187,10 +188,20 @@
return QuicStream::sequencer();
}
+ void OnStreamHeaderList(bool fin,
+ size_t frame_len,
+ const QuicHeaderList& header_list) override {
+ headers_payload_length_ = frame_len;
+ QuicSpdyStream::OnStreamHeaderList(fin, frame_len, header_list);
+ }
+
+ size_t headers_payload_length() const { return headers_payload_length_; }
+
private:
bool should_process_data_;
spdy::SpdyHeaderBlock saved_headers_;
std::string data_;
+ size_t headers_payload_length_;
};
class TestSession : public MockQuicSpdySession {
@@ -391,6 +402,21 @@
return quiche::QuicheStrCat(headers_frame_header, payload);
}
+ // Construct PUSH_PROMISE frame with given payload.
+ std::string SerializePushPromiseFrame(PushId push_id,
+ quiche::QuicheStringPiece payload) {
+ PushPromiseFrame frame;
+ frame.push_id = push_id;
+ frame.headers = payload;
+ std::unique_ptr<char[]> push_promise_buffer;
+ QuicByteCount push_promise_frame_header_length =
+ HttpEncoder::SerializePushPromiseFrameWithOnlyPushId(
+ frame, &push_promise_buffer);
+ quiche::QuicheStringPiece push_promise_frame_header(
+ push_promise_buffer.get(), push_promise_frame_header_length);
+ return quiche::QuicheStrCat(push_promise_frame_header, payload);
+ }
+
std::string DataFrame(quiche::QuicheStringPiece payload) {
std::unique_ptr<char[]> data_buffer;
QuicByteCount data_frame_header_length =
@@ -2618,13 +2644,7 @@
std::string headers = EncodeQpackHeaders(pushed_headers);
const QuicStreamId push_id = 1;
- PushPromiseFrame push_promise;
- push_promise.push_id = push_id;
- push_promise.headers = headers;
- std::unique_ptr<char[]> buffer;
- uint64_t length = HttpEncoder::SerializePushPromiseFrameWithOnlyPushId(
- push_promise, &buffer);
- std::string data = std::string(buffer.get(), length) + headers;
+ std::string data = SerializePushPromiseFrame(push_id, headers);
QuicStreamFrame frame(stream_->id(), false, 0, data);
EXPECT_CALL(debug_visitor, OnPushPromiseFrameReceived(stream_->id(), push_id,
@@ -2633,11 +2653,40 @@
OnPushPromiseDecoded(stream_->id(), push_id,
AsHeaderList(pushed_headers)));
EXPECT_CALL(*session_,
- OnPromiseHeaderList(stream_->id(), push_promise.push_id,
- headers.length(), _));
+ OnPromiseHeaderList(stream_->id(), push_id, headers.length(), _));
stream_->OnStreamFrame(frame);
}
+// Regression test for b/152518220.
+TEST_P(QuicSpdyStreamTest,
+ OnStreamHeaderBlockArgumentDoesNotIncludePushedHeaderBlock) {
+ Initialize(kShouldProcessData);
+ if (!UsesHttp3()) {
+ return;
+ }
+
+ std::string pushed_headers = EncodeQpackHeaders({{"foo", "bar"}});
+ const QuicStreamId push_id = 1;
+ std::string push_promise_frame =
+ SerializePushPromiseFrame(push_id, pushed_headers);
+ QuicStreamOffset offset = 0;
+ QuicStreamFrame frame1(stream_->id(), /* fin = */ false, offset,
+ push_promise_frame);
+ offset += push_promise_frame.length();
+
+ EXPECT_CALL(*session_, OnPromiseHeaderList(stream_->id(), push_id,
+ pushed_headers.length(), _));
+ stream_->OnStreamFrame(frame1);
+
+ std::string headers =
+ EncodeQpackHeaders({{":method", "GET"}, {":path", "/"}});
+ std::string headers_frame = HeadersFrame(headers);
+ QuicStreamFrame frame2(stream_->id(), /* fin = */ false, offset,
+ headers_frame);
+ stream_->OnStreamFrame(frame2);
+ EXPECT_EQ(headers.length(), stream_->headers_payload_length());
+}
+
// Close connection if a DATA frame is received before a HEADERS frame.
TEST_P(QuicSpdyStreamTest, DataBeforeHeaders) {
if (!UsesHttp3()) {
