Simplify Http3DebugVisitor methods.
Pass frame arguments by const ref instead of value.
Combine multiple methods into one for DATA and unknown frames.
Add compressed header block size argument to OnPushPromiseFrameReceived().
Call OnMaxPushIdFrameReceived() and OnGoAwayFrameReceived() even if they cause
the connection to be closed to make debugging easier.
This is all motivated by renjietang's comments at https://crrev.com/c/2117316.
Bug is https://crbug.com/1062700.
gfe-relnote: n/a, Http3DebugVisitor not used in production.
PiperOrigin-RevId: 303427808
Change-Id: I91ee190aea66f565f91575379537fa50c62c9c58
diff --git a/quic/core/http/quic_receive_control_stream.cc b/quic/core/http/quic_receive_control_stream.cc
index fa9ae5c..efd980f 100644
--- a/quic/core/http/quic_receive_control_stream.cc
+++ b/quic/core/http/quic_receive_control_stream.cc
@@ -40,15 +40,15 @@
}
bool OnMaxPushIdFrame(const MaxPushIdFrame& frame) override {
+ if (stream_->spdy_session()->debug_visitor()) {
+ stream_->spdy_session()->debug_visitor()->OnMaxPushIdFrameReceived(frame);
+ }
+
if (stream_->spdy_session()->perspective() == Perspective::IS_CLIENT) {
OnWrongFrame("Max Push Id");
return false;
}
- if (stream_->spdy_session()->debug_visitor()) {
- stream_->spdy_session()->debug_visitor()->OnMaxPushIdFrameReceived(frame);
- }
-
// TODO(b/124216424): Signal error if received push ID is smaller than a
// previously received value.
stream_->spdy_session()->OnMaxPushIdFrame(frame.push_id);
@@ -57,15 +57,15 @@
bool OnGoAwayFrame(const GoAwayFrame& frame) override {
// TODO(bnc): Check if SETTINGS frame has been received.
+ if (stream_->spdy_session()->debug_visitor()) {
+ stream_->spdy_session()->debug_visitor()->OnGoAwayFrameReceived(frame);
+ }
+
if (stream_->spdy_session()->perspective() == Perspective::IS_SERVER) {
OnWrongFrame("Go Away");
return false;
}
- if (stream_->spdy_session()->debug_visitor()) {
- stream_->spdy_session()->debug_visitor()->OnGoAwayFrameReceived(frame);
- }
-
stream_->spdy_session()->OnHttp3GoAway(frame.stream_id);
return true;
}
@@ -144,31 +144,21 @@
bool OnUnknownFrameStart(uint64_t frame_type,
QuicByteCount /* header_length */,
- QuicByteCount /* payload_length */) override {
+ QuicByteCount payload_length) override {
if (stream_->spdy_session()->debug_visitor()) {
- stream_->spdy_session()->debug_visitor()->OnUnknownFrameStart(
- stream_->id(), frame_type);
+ stream_->spdy_session()->debug_visitor()->OnUnknownFrameReceived(
+ stream_->id(), frame_type, payload_length);
}
return stream_->OnUnknownFrameStart();
}
- bool OnUnknownFramePayload(quiche::QuicheStringPiece payload) override {
- if (stream_->spdy_session()->debug_visitor()) {
- stream_->spdy_session()->debug_visitor()->OnUnknownFramePayload(
- stream_->id(), payload.length());
- }
-
+ bool OnUnknownFramePayload(quiche::QuicheStringPiece /* payload */) override {
// Ignore unknown frame types.
return true;
}
bool OnUnknownFrameEnd() override {
- if (stream_->spdy_session()->debug_visitor()) {
- stream_->spdy_session()->debug_visitor()->OnUnknownFrameEnd(
- stream_->id());
- }
-
// Ignore unknown frame types.
return true;
}
diff --git a/quic/core/http/quic_receive_control_stream_test.cc b/quic/core/http/quic_receive_control_stream_test.cc
index 77692a5..d9583ae 100644
--- a/quic/core/http/quic_receive_control_stream_test.cc
+++ b/quic/core/http/quic_receive_control_stream_test.cc
@@ -281,12 +281,12 @@
QuicStreamFrame frame(receive_control_stream_->id(), false, 1, data);
EXPECT_FALSE(session_.http3_goaway_received());
+ EXPECT_CALL(debug_visitor, OnGoAwayFrameReceived(goaway));
+
if (perspective() == Perspective::IS_SERVER) {
EXPECT_CALL(
*connection_,
CloseConnection(QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM, _, _));
- } else {
- EXPECT_CALL(debug_visitor, OnGoAwayFrameReceived(goaway));
}
receive_control_stream_->OnStreamFrame(frame);
@@ -363,10 +363,8 @@
"03" // payload length
"666f6f"); // payload "foo"
- EXPECT_CALL(debug_visitor, OnUnknownFrameStart(id, /* frame_type = */ 0x21));
- EXPECT_CALL(debug_visitor,
- OnUnknownFramePayload(id, /* payload_length = */ 3));
- EXPECT_CALL(debug_visitor, OnUnknownFrameEnd(id));
+ EXPECT_CALL(debug_visitor, OnUnknownFrameReceived(id, /* frame_type = */ 0x21,
+ /* payload_length = */ 3));
receive_control_stream_->OnStreamFrame(
QuicStreamFrame(id, /* fin = */ false,
/* offset = */ 1 + settings_frame.size(), unknown_frame));
diff --git a/quic/core/http/quic_spdy_session.h b/quic/core/http/quic_spdy_session.h
index b0ce12a..93fa09d 100644
--- a/quic/core/http/quic_spdy_session.h
+++ b/quic/core/http/quic_spdy_session.h
@@ -82,40 +82,40 @@
virtual void OnPeerQpackDecoderStreamCreated(QuicStreamId /*stream_id*/) = 0;
// Incoming HTTP/3 frames on the control stream.
- virtual void OnCancelPushFrameReceived(CancelPushFrame /*frame*/) {}
+ virtual void OnCancelPushFrameReceived(const CancelPushFrame& /*frame*/) {}
virtual void OnSettingsFrameReceived(const SettingsFrame& /*frame*/) = 0;
- virtual void OnGoAwayFrameReceived(GoAwayFrame /*frame*/) {}
- virtual void OnMaxPushIdFrameReceived(MaxPushIdFrame /*frame*/) {}
- virtual void OnPriorityUpdateFrameReceived(PriorityUpdateFrame /*frame*/) {}
+ virtual void OnGoAwayFrameReceived(const GoAwayFrame& /*frame*/) {}
+ virtual void OnMaxPushIdFrameReceived(const MaxPushIdFrame& /*frame*/) {}
+ virtual void OnPriorityUpdateFrameReceived(
+ const PriorityUpdateFrame& /*frame*/) {}
// Incoming HTTP/3 frames on request or push streams.
- virtual void OnDataFrameStart(QuicStreamId /*stream_id*/) {}
- virtual void OnDataFramePayload(QuicStreamId /*stream_id*/,
- QuicByteCount /*payload_fragment_length*/) {}
- virtual void OnDataFrameEnd(QuicStreamId /*stream_id*/) {}
- virtual void OnHeadersFrameReceived(QuicStreamId /*stream_id*/,
- QuicByteCount /*payload_length*/) {}
+ virtual void OnDataFrameReceived(QuicStreamId /*stream_id*/,
+ QuicByteCount /*payload_length*/) {}
+ virtual void OnHeadersFrameReceived(
+ QuicStreamId /*stream_id*/,
+ QuicByteCount /*compressed_headers_length*/) {}
virtual void OnHeadersDecoded(QuicStreamId /*stream_id*/,
QuicHeaderList /*headers*/) {}
virtual void OnPushPromiseFrameReceived(QuicStreamId /*stream_id*/,
- QuicStreamId /*push_id*/) {}
+ QuicStreamId /*push_id*/,
+ QuicByteCount
+ /*compressed_headers_length*/) {}
virtual void OnPushPromiseDecoded(QuicStreamId /*stream_id*/,
QuicStreamId /*push_id*/,
QuicHeaderList /*headers*/) {}
// Incoming HTTP/3 frames of unknown type on any stream.
- virtual void OnUnknownFrameStart(QuicStreamId /*stream_id*/,
- uint64_t /*frame_type*/) {}
- virtual void OnUnknownFramePayload(
- QuicStreamId /*stream_id*/,
- QuicByteCount /*payload_fragment_length*/) {}
- virtual void OnUnknownFrameEnd(QuicStreamId /*stream_id*/) {}
+ virtual void OnUnknownFrameReceived(QuicStreamId /*stream_id*/,
+ uint64_t /*frame_type*/,
+ QuicByteCount /*payload_length*/) {}
// Outgoing HTTP/3 frames on the control stream.
virtual void OnSettingsFrameSent(const SettingsFrame& /*frame*/) = 0;
virtual void OnGoAwayFrameSent(QuicStreamId /*stream_id*/) {}
- virtual void OnMaxPushIdFrameSent(MaxPushIdFrame /*frame*/) {}
- virtual void OnPriorityUpdateFrameSent(PriorityUpdateFrame /*frame*/) {}
+ virtual void OnMaxPushIdFrameSent(const MaxPushIdFrame& /*frame*/) {}
+ virtual void OnPriorityUpdateFrameSent(const PriorityUpdateFrame& /*frame*/) {
+ }
// Outgoing HTTP/3 frames on request or push streams.
virtual void OnDataFrameSent(QuicStreamId /*stream_id*/,
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc
index 573ab90..c376d01 100644
--- a/quic/core/http/quic_spdy_stream.cc
+++ b/quic/core/http/quic_spdy_stream.cc
@@ -867,8 +867,13 @@
}
bool QuicSpdyStream::OnDataFrameStart(QuicByteCount header_length,
- QuicByteCount /*payload_length*/) {
+ QuicByteCount payload_length) {
DCHECK(VersionUsesHttp3(transport_version()));
+
+ if (spdy_session_->debug_visitor()) {
+ spdy_session_->debug_visitor()->OnDataFrameReceived(id(), payload_length);
+ }
+
if (!headers_decompressed_ || trailers_decompressed_) {
stream_delegate()->OnStreamError(
QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_SPDY_STREAM,
@@ -876,10 +881,6 @@
return false;
}
- if (spdy_session_->debug_visitor()) {
- spdy_session_->debug_visitor()->OnDataFrameStart(id());
- }
-
sequencer()->MarkConsumed(body_manager_.OnNonBody(header_length));
return true;
@@ -888,10 +889,6 @@
bool QuicSpdyStream::OnDataFramePayload(quiche::QuicheStringPiece payload) {
DCHECK(VersionUsesHttp3(transport_version()));
- if (spdy_session_->debug_visitor()) {
- spdy_session_->debug_visitor()->OnDataFramePayload(id(), payload.length());
- }
-
body_manager_.OnBody(payload);
return true;
@@ -900,10 +897,6 @@
bool QuicSpdyStream::OnDataFrameEnd() {
DCHECK(VersionUsesHttp3(transport_version()));
- if (spdy_session_->debug_visitor()) {
- spdy_session_->debug_visitor()->OnDataFrameEnd(id());
- }
-
QUIC_DVLOG(1) << ENDPOINT
<< "Reaches the end of a data frame. Total bytes received are "
<< body_manager_.total_body_bytes_received();
@@ -960,10 +953,15 @@
}
bool QuicSpdyStream::OnHeadersFrameStart(QuicByteCount header_length,
- QuicByteCount /*payload_length*/) {
+ QuicByteCount payload_length) {
DCHECK(VersionUsesHttp3(transport_version()));
DCHECK(!qpack_decoded_headers_accumulator_);
+ if (spdy_session_->debug_visitor()) {
+ spdy_session_->debug_visitor()->OnHeadersFrameReceived(id(),
+ payload_length);
+ }
+
if (trailers_decompressed_) {
stream_delegate()->OnStreamError(
QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_SPDY_STREAM,
@@ -1009,18 +1007,6 @@
DCHECK(VersionUsesHttp3(transport_version()));
DCHECK(qpack_decoded_headers_accumulator_);
- if (spdy_session_->debug_visitor()) {
- if (spdy_session_->promised_stream_id() ==
- QuicUtils::GetInvalidStreamId(transport_version())) {
- spdy_session_->debug_visitor()->OnHeadersFrameReceived(
- id(), headers_decompressed_ ? trailers_payload_length_
- : headers_payload_length_);
- } else {
- spdy_session_->debug_visitor()->OnPushPromiseFrameReceived(
- id(), spdy_session_->promised_stream_id());
- }
- }
-
qpack_decoded_headers_accumulator_->EndHeaderBlock();
// If decoding is complete or an error is detected, then
@@ -1045,10 +1031,15 @@
bool QuicSpdyStream::OnPushPromiseFramePushId(
PushId push_id,
QuicByteCount push_id_length,
- QuicByteCount /*header_block_length*/) {
+ QuicByteCount header_block_length) {
DCHECK(VersionUsesHttp3(transport_version()));
DCHECK(!qpack_decoded_headers_accumulator_);
+ if (spdy_session_->debug_visitor()) {
+ spdy_session_->debug_visitor()->OnPushPromiseFrameReceived(
+ id(), push_id, header_block_length);
+ }
+
// TODO(renjietang): Check max push id and handle errors.
spdy_session_->OnPushPromise(id(), push_id);
sequencer()->MarkConsumed(body_manager_.OnNonBody(push_id_length));
@@ -1075,9 +1066,10 @@
bool QuicSpdyStream::OnUnknownFrameStart(uint64_t frame_type,
QuicByteCount header_length,
- QuicByteCount /*payload_length*/) {
+ QuicByteCount payload_length) {
if (spdy_session_->debug_visitor()) {
- spdy_session_->debug_visitor()->OnUnknownFrameStart(id(), frame_type);
+ spdy_session_->debug_visitor()->OnUnknownFrameReceived(id(), frame_type,
+ payload_length);
}
// Ignore unknown frames, but consume frame header.
@@ -1089,10 +1081,6 @@
}
bool QuicSpdyStream::OnUnknownFramePayload(quiche::QuicheStringPiece payload) {
- if (spdy_session_->debug_visitor()) {
- spdy_session_->debug_visitor()->OnUnknownFramePayload(id(), payload.size());
- }
-
// Ignore unknown frames, but consume frame payload.
QUIC_DVLOG(1) << ENDPOINT << "Discarding " << payload.size()
<< " bytes of payload of frame of unknown type.";
@@ -1101,10 +1089,6 @@
}
bool QuicSpdyStream::OnUnknownFrameEnd() {
- if (spdy_session_->debug_visitor()) {
- spdy_session_->debug_visitor()->OnUnknownFrameEnd(id());
- }
-
return true;
}
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc
index 09fb664..536cb12 100644
--- a/quic/core/http/quic_spdy_stream_test.cc
+++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -2148,12 +2148,13 @@
session_->qpack_decoder()->OnInsertWithoutNameReference("foo", "bar");
// HEADERS frame referencing first dynamic table entry.
- EXPECT_CALL(debug_visitor, OnHeadersFrameReceived(stream_->id(), _));
+ std::string encoded_headers = quiche::QuicheTextUtils::HexDecode("020080");
+ std::string headers = HeadersFrame(encoded_headers);
+ EXPECT_CALL(debug_visitor,
+ OnHeadersFrameReceived(stream_->id(), encoded_headers.length()));
// Decoder stream type and header acknowledgement.
EXPECT_CALL(*session_, WritevData(decoder_send_stream->id(), _, _, _, _, _));
EXPECT_CALL(debug_visitor, OnHeadersDecoded(stream_->id(), _));
- std::string headers =
- HeadersFrame(quiche::QuicheTextUtils::HexDecode("020080"));
stream_->OnStreamFrame(QuicStreamFrame(stream_->id(), false, 0, headers));
// Headers can be decoded immediately.
@@ -2165,10 +2166,8 @@
// DATA frame.
std::string data = DataFrame(kDataFramePayload);
- EXPECT_CALL(debug_visitor, OnDataFrameStart(stream_->id()));
EXPECT_CALL(debug_visitor,
- OnDataFramePayload(stream_->id(), strlen(kDataFramePayload)));
- EXPECT_CALL(debug_visitor, OnDataFrameEnd(stream_->id()));
+ OnDataFrameReceived(stream_->id(), strlen(kDataFramePayload)));
stream_->OnStreamFrame(QuicStreamFrame(stream_->id(), false, /* offset = */
headers.length(), data));
EXPECT_EQ(kDataFramePayload, stream_->data());
@@ -2177,12 +2176,13 @@
session_->qpack_decoder()->OnInsertWithoutNameReference("trailing", "foobar");
// Trailing HEADERS frame referencing second dynamic table entry.
- EXPECT_CALL(debug_visitor, OnHeadersFrameReceived(stream_->id(), _));
+ std::string encoded_trailers = quiche::QuicheTextUtils::HexDecode("030080");
+ std::string trailers = HeadersFrame(encoded_trailers);
+ EXPECT_CALL(debug_visitor,
+ OnHeadersFrameReceived(stream_->id(), encoded_trailers.length()));
// Header acknowledgement.
EXPECT_CALL(*session_, WritevData(decoder_send_stream->id(), _, _, _, _, _));
EXPECT_CALL(debug_visitor, OnHeadersDecoded(stream_->id(), _));
- std::string trailers =
- HeadersFrame(quiche::QuicheTextUtils::HexDecode("030080"));
stream_->OnStreamFrame(QuicStreamFrame(stream_->id(), true, /* offset = */
headers.length() + data.length(),
trailers));
@@ -2208,9 +2208,10 @@
session_->set_debug_visitor(&debug_visitor);
// HEADERS frame referencing first dynamic table entry.
- std::string headers =
- HeadersFrame(quiche::QuicheTextUtils::HexDecode("020080"));
- EXPECT_CALL(debug_visitor, OnHeadersFrameReceived(stream_->id(), _));
+ std::string encoded_headers = quiche::QuicheTextUtils::HexDecode("020080");
+ std::string headers = HeadersFrame(encoded_headers);
+ EXPECT_CALL(debug_visitor,
+ OnHeadersFrameReceived(stream_->id(), encoded_headers.length()));
stream_->OnStreamFrame(QuicStreamFrame(stream_->id(), false, 0, headers));
// Decoding is blocked because dynamic table entry has not been received yet.
@@ -2232,18 +2233,17 @@
// DATA frame.
std::string data = DataFrame(kDataFramePayload);
- EXPECT_CALL(debug_visitor, OnDataFrameStart(stream_->id()));
EXPECT_CALL(debug_visitor,
- OnDataFramePayload(stream_->id(), strlen(kDataFramePayload)));
- EXPECT_CALL(debug_visitor, OnDataFrameEnd(stream_->id()));
+ OnDataFrameReceived(stream_->id(), strlen(kDataFramePayload)));
stream_->OnStreamFrame(QuicStreamFrame(stream_->id(), false, /* offset = */
headers.length(), data));
EXPECT_EQ(kDataFramePayload, stream_->data());
// Trailing HEADERS frame referencing second dynamic table entry.
- std::string trailers =
- HeadersFrame(quiche::QuicheTextUtils::HexDecode("030080"));
- EXPECT_CALL(debug_visitor, OnHeadersFrameReceived(stream_->id(), _));
+ std::string encoded_trailers = quiche::QuicheTextUtils::HexDecode("030080");
+ std::string trailers = HeadersFrame(encoded_trailers);
+ EXPECT_CALL(debug_visitor,
+ OnHeadersFrameReceived(stream_->id(), encoded_trailers.length()));
stream_->OnStreamFrame(QuicStreamFrame(stream_->id(), true, /* offset = */
headers.length() + data.length(),
trailers));
@@ -2506,11 +2506,8 @@
session_->set_debug_visitor(&debug_visitor);
EXPECT_CALL(debug_visitor,
- OnUnknownFrameStart(stream_->id(), /* frame_type = */ 0x21));
- EXPECT_CALL(debug_visitor,
- OnUnknownFramePayload(stream_->id(), /* payload_length = */ 3));
- EXPECT_CALL(debug_visitor, OnUnknownFrameEnd(stream_->id()));
-
+ OnUnknownFrameReceived(stream_->id(), /* frame_type = */ 0x21,
+ /* payload_length = */ 3));
std::string unknown_frame = UnknownFrame(0x21, "foo");
OnStreamFrame(unknown_frame);
}
@@ -2623,8 +2620,8 @@
std::string data = std::string(buffer.get(), length) + headers;
QuicStreamFrame frame(stream_->id(), false, 0, data);
- EXPECT_CALL(debug_visitor,
- OnPushPromiseFrameReceived(stream_->id(), push_id));
+ EXPECT_CALL(debug_visitor, OnPushPromiseFrameReceived(stream_->id(), push_id,
+ headers.length()));
EXPECT_CALL(debug_visitor,
OnPushPromiseDecoded(stream_->id(), push_id,
AsHeaderList(pushed_headers)));
diff --git a/quic/test_tools/quic_test_utils.h b/quic/test_tools/quic_test_utils.h
index 224263f..473b632 100644
--- a/quic/test_tools/quic_test_utils.h
+++ b/quic/test_tools/quic_test_utils.h
@@ -812,31 +812,26 @@
MOCK_METHOD1(OnPeerQpackEncoderStreamCreated, void(QuicStreamId));
MOCK_METHOD1(OnPeerQpackDecoderStreamCreated, void(QuicStreamId));
- MOCK_METHOD1(OnCancelPushFrameReceived, void(CancelPushFrame));
+ MOCK_METHOD1(OnCancelPushFrameReceived, void(const CancelPushFrame&));
MOCK_METHOD1(OnSettingsFrameReceived, void(const SettingsFrame&));
- MOCK_METHOD1(OnGoAwayFrameReceived, void(GoAwayFrame));
- MOCK_METHOD1(OnMaxPushIdFrameReceived, void(MaxPushIdFrame));
- MOCK_METHOD1(OnPriorityUpdateFrameReceived, void(PriorityUpdateFrame));
+ MOCK_METHOD1(OnGoAwayFrameReceived, void(const GoAwayFrame&));
+ MOCK_METHOD1(OnMaxPushIdFrameReceived, void(const MaxPushIdFrame&));
+ MOCK_METHOD1(OnPriorityUpdateFrameReceived, void(const PriorityUpdateFrame&));
- MOCK_METHOD1(OnDataFrameStart, void(QuicStreamId));
- MOCK_METHOD2(OnDataFramePayload, void(QuicStreamId, QuicByteCount));
- MOCK_METHOD1(OnDataFrameEnd, void(QuicStreamId));
-
+ MOCK_METHOD2(OnDataFrameReceived, void(QuicStreamId, QuicByteCount));
MOCK_METHOD2(OnHeadersFrameReceived, void(QuicStreamId, QuicByteCount));
MOCK_METHOD2(OnHeadersDecoded, void(QuicStreamId, QuicHeaderList));
-
- MOCK_METHOD2(OnPushPromiseFrameReceived, void(QuicStreamId, QuicStreamId));
+ MOCK_METHOD3(OnPushPromiseFrameReceived,
+ void(QuicStreamId, QuicStreamId, QuicByteCount));
MOCK_METHOD3(OnPushPromiseDecoded,
void(QuicStreamId, QuicStreamId, QuicHeaderList));
-
- MOCK_METHOD2(OnUnknownFrameStart, void(QuicStreamId, uint64_t));
- MOCK_METHOD2(OnUnknownFramePayload, void(QuicStreamId, QuicByteCount));
- MOCK_METHOD1(OnUnknownFrameEnd, void(QuicStreamId));
+ MOCK_METHOD3(OnUnknownFrameReceived,
+ void(QuicStreamId, uint64_t, QuicByteCount));
MOCK_METHOD1(OnSettingsFrameSent, void(const SettingsFrame&));
MOCK_METHOD1(OnGoAwayFrameSent, void(QuicStreamId));
- MOCK_METHOD1(OnMaxPushIdFrameSent, void(MaxPushIdFrame));
- MOCK_METHOD1(OnPriorityUpdateFrameSent, void(PriorityUpdateFrame));
+ MOCK_METHOD1(OnMaxPushIdFrameSent, void(const MaxPushIdFrame&));
+ MOCK_METHOD1(OnPriorityUpdateFrameSent, void(const PriorityUpdateFrame&));
MOCK_METHOD2(OnDataFrameSent, void(QuicStreamId, QuicByteCount));
MOCK_METHOD2(OnHeadersFrameSent,
