Remove unused HTTP/3 server push related code. Remove unused methods QuicSpdyStream::WritePushPromise() and QuicSendControlStream::SendMaxPushIdFrame(). Remove unused code from HttpEncoder to serialize HTTP/3 frames PUSH_PROMISE, CANCEL_PUSH, MAX_PUSH_ID. When used in tests, either hardcode frame or copy implementation to test helper methods. Add more TODOs for this bug. PiperOrigin-RevId: 366286729 Change-Id: I62826989ddb2784cdcebb8c33f85f5edb15ed1db
diff --git a/quic/core/http/http_encoder.cc b/quic/core/http/http_encoder.cc index 804a2bf..1fa215c 100644 --- a/quic/core/http/http_encoder.cc +++ b/quic/core/http/http_encoder.cc
@@ -76,27 +76,6 @@ } // static -QuicByteCount HttpEncoder::SerializeCancelPushFrame( - const CancelPushFrame& cancel_push, - std::unique_ptr<char[]>* output) { - QuicByteCount payload_length = - QuicDataWriter::GetVarInt62Len(cancel_push.push_id); - QuicByteCount total_length = - GetTotalLength(payload_length, HttpFrameType::CANCEL_PUSH); - - output->reset(new char[total_length]); - QuicDataWriter writer(total_length, output->get()); - - if (WriteFrameHeader(payload_length, HttpFrameType::CANCEL_PUSH, &writer) && - writer.WriteVarInt62(cancel_push.push_id)) { - return total_length; - } - QUIC_DLOG(ERROR) - << "Http encoder failed when attempting to serialize cancel push frame."; - return 0; -} - -// static QuicByteCount HttpEncoder::SerializeSettingsFrame( const SettingsFrame& settings, std::unique_ptr<char[]>* output) { @@ -134,32 +113,6 @@ } // static -QuicByteCount HttpEncoder::SerializePushPromiseFrameWithOnlyPushId( - const PushPromiseFrame& push_promise, - std::unique_ptr<char[]>* output) { - QuicByteCount payload_length = - QuicDataWriter::GetVarInt62Len(push_promise.push_id) + - push_promise.headers.length(); - // GetTotalLength() is not used because headers will not be serialized. - QuicByteCount total_length = - QuicDataWriter::GetVarInt62Len(payload_length) + - QuicDataWriter::GetVarInt62Len( - static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE)) + - QuicDataWriter::GetVarInt62Len(push_promise.push_id); - - output->reset(new char[total_length]); - QuicDataWriter writer(total_length, output->get()); - - if (WriteFrameHeader(payload_length, HttpFrameType::PUSH_PROMISE, &writer) && - writer.WriteVarInt62(push_promise.push_id)) { - return total_length; - } - QUIC_DLOG(ERROR) - << "Http encoder failed when attempting to serialize push promise frame."; - return 0; -} - -// static QuicByteCount HttpEncoder::SerializeGoAwayFrame( const GoAwayFrame& goaway, std::unique_ptr<char[]>* output) { @@ -180,27 +133,6 @@ } // static -QuicByteCount HttpEncoder::SerializeMaxPushIdFrame( - const MaxPushIdFrame& max_push_id, - std::unique_ptr<char[]>* output) { - QuicByteCount payload_length = - QuicDataWriter::GetVarInt62Len(max_push_id.push_id); - QuicByteCount total_length = - GetTotalLength(payload_length, HttpFrameType::MAX_PUSH_ID); - - output->reset(new char[total_length]); - QuicDataWriter writer(total_length, output->get()); - - if (WriteFrameHeader(payload_length, HttpFrameType::MAX_PUSH_ID, &writer) && - writer.WriteVarInt62(max_push_id.push_id)) { - return total_length; - } - QUIC_DLOG(ERROR) - << "Http encoder failed when attempting to serialize max push id frame."; - return 0; -} - -// static QuicByteCount HttpEncoder::SerializePriorityUpdateFrame( const PriorityUpdateFrame& priority_update, std::unique_ptr<char[]>* output) {
diff --git a/quic/core/http/http_encoder.h b/quic/core/http/http_encoder.h index 64a2160..b6cb9ad 100644 --- a/quic/core/http/http_encoder.h +++ b/quic/core/http/http_encoder.h
@@ -32,38 +32,16 @@ QuicByteCount payload_length, std::unique_ptr<char[]>* output); - // Serializes a CANCEL_PUSH frame into a new buffer stored in |output|. - // Returns the length of the buffer on success, or 0 otherwise. - // TODO(b/171463363): Remove. - static QuicByteCount SerializeCancelPushFrame( - const CancelPushFrame& cancel_push, - std::unique_ptr<char[]>* output); - // Serializes a SETTINGS frame into a new buffer stored in |output|. // Returns the length of the buffer on success, or 0 otherwise. static QuicByteCount SerializeSettingsFrame(const SettingsFrame& settings, std::unique_ptr<char[]>* output); - // Serializes the header and push_id of a PUSH_PROMISE frame into a new buffer - // stored in |output|. Returns the length of the buffer on success, or 0 - // otherwise. - // TODO(b/171463363): Remove. - static QuicByteCount SerializePushPromiseFrameWithOnlyPushId( - const PushPromiseFrame& push_promise, - std::unique_ptr<char[]>* output); - // Serializes a GOAWAY frame into a new buffer stored in |output|. // Returns the length of the buffer on success, or 0 otherwise. static QuicByteCount SerializeGoAwayFrame(const GoAwayFrame& goaway, std::unique_ptr<char[]>* output); - // Serializes a MAX_PUSH frame into a new buffer stored in |output|. - // Returns the length of the buffer on success, or 0 otherwise. - // TODO(b/171463363): Remove. - static QuicByteCount SerializeMaxPushIdFrame( - const MaxPushIdFrame& max_push_id, - std::unique_ptr<char[]>* output); - // Serializes a PRIORITY_UPDATE frame into a new buffer stored in |output|. // Returns the length of the buffer on success, or 0 otherwise. static QuicByteCount SerializePriorityUpdateFrame(
diff --git a/quic/core/http/http_encoder_test.cc b/quic/core/http/http_encoder_test.cc index ad25cc4..c59dd85 100644 --- a/quic/core/http/http_encoder_test.cc +++ b/quic/core/http/http_encoder_test.cc
@@ -39,22 +39,6 @@ output, ABSL_ARRAYSIZE(output)); } -TEST(HttpEncoderTest, SerializeCancelPushFrame) { - CancelPushFrame cancel_push; - cancel_push.push_id = 0x01; - char output[] = {// type (CANCEL_PUSH) - 0x03, - // length - 0x1, - // Push Id - 0x01}; - std::unique_ptr<char[]> buffer; - uint64_t length = HttpEncoder::SerializeCancelPushFrame(cancel_push, &buffer); - EXPECT_EQ(ABSL_ARRAYSIZE(output), length); - quiche::test::CompareCharArraysWithHexError( - "CANCEL_PUSH", buffer.get(), length, output, ABSL_ARRAYSIZE(output)); -} - TEST(HttpEncoderTest, SerializeSettingsFrame) { SettingsFrame settings; settings.values[1] = 2; @@ -83,24 +67,6 @@ output, ABSL_ARRAYSIZE(output)); } -TEST(HttpEncoderTest, SerializePushPromiseFrameWithOnlyPushId) { - PushPromiseFrame push_promise; - push_promise.push_id = 0x01; - push_promise.headers = "Headers"; - char output[] = {// type (PUSH_PROMISE) - 0x05, - // length - 0x8, - // Push Id - 0x01}; - std::unique_ptr<char[]> buffer; - uint64_t length = HttpEncoder::SerializePushPromiseFrameWithOnlyPushId( - push_promise, &buffer); - EXPECT_EQ(ABSL_ARRAYSIZE(output), length); - quiche::test::CompareCharArraysWithHexError( - "PUSH_PROMISE", buffer.get(), length, output, ABSL_ARRAYSIZE(output)); -} - TEST(HttpEncoderTest, SerializeGoAwayFrame) { GoAwayFrame goaway; goaway.id = 0x1; @@ -117,22 +83,6 @@ output, ABSL_ARRAYSIZE(output)); } -TEST(HttpEncoderTest, SerializeMaxPushIdFrame) { - MaxPushIdFrame max_push_id; - max_push_id.push_id = 0x1; - char output[] = {// type (MAX_PUSH_ID) - 0x0D, - // length - 0x1, - // Push Id - 0x01}; - std::unique_ptr<char[]> buffer; - uint64_t length = HttpEncoder::SerializeMaxPushIdFrame(max_push_id, &buffer); - EXPECT_EQ(ABSL_ARRAYSIZE(output), length); - quiche::test::CompareCharArraysWithHexError( - "MAX_PUSH_ID", buffer.get(), length, output, ABSL_ARRAYSIZE(output)); -} - TEST(HttpEncoderTest, SerializePriorityUpdateFrame) { PriorityUpdateFrame priority_update1; priority_update1.prioritized_element_type = REQUEST_STREAM;
diff --git a/quic/core/http/http_frames.h b/quic/core/http/http_frames.h index 21e2acc..0d55857 100644 --- a/quic/core/http/http_frames.h +++ b/quic/core/http/http_frames.h
@@ -103,6 +103,7 @@ // // The PUSH_PROMISE frame (type=0x05) is used to carry a request header // set from server to client, as in HTTP/2. +// TODO(b/171463363): Remove. struct QUIC_EXPORT_PRIVATE PushPromiseFrame { PushId push_id; absl::string_view headers;
diff --git a/quic/core/http/quic_receive_control_stream_test.cc b/quic/core/http/quic_receive_control_stream_test.cc index 491255d..cc54442 100644 --- a/quic/core/http/quic_receive_control_stream_test.cc +++ b/quic/core/http/quic_receive_control_stream_test.cc
@@ -303,14 +303,12 @@ } TEST_P(QuicReceiveControlStreamTest, PushPromiseOnControlStreamShouldClose) { - PushPromiseFrame push_promise; - push_promise.push_id = 0x01; - push_promise.headers = "Headers"; - std::unique_ptr<char[]> buffer; - uint64_t length = HttpEncoder::SerializePushPromiseFrameWithOnlyPushId( - push_promise, &buffer); - QuicStreamFrame frame(receive_control_stream_->id(), false, 1, buffer.get(), - length); + std::string push_promise_frame = absl::HexStringToBytes( + "05" // PUSH_PROMISE + "01" // length + "00"); // push ID + QuicStreamFrame frame(receive_control_stream_->id(), false, 1, + push_promise_frame); EXPECT_CALL( *connection_, CloseConnection(QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM, _, _))
diff --git a/quic/core/http/quic_send_control_stream.cc b/quic/core/http/quic_send_control_stream.cc index 3334e52..79555e0 100644 --- a/quic/core/http/quic_send_control_stream.cc +++ b/quic/core/http/quic_send_control_stream.cc
@@ -104,24 +104,6 @@ nullptr); } -void QuicSendControlStream::SendMaxPushIdFrame(PushId max_push_id) { - QUICHE_DCHECK_EQ(Perspective::IS_CLIENT, session()->perspective()); - QuicConnection::ScopedPacketFlusher flusher(session()->connection()); - MaybeSendSettingsFrame(); - - MaxPushIdFrame frame; - frame.push_id = max_push_id; - if (spdy_session_->debug_visitor()) { - spdy_session_->debug_visitor()->OnMaxPushIdFrameSent(frame); - } - - std::unique_ptr<char[]> buffer; - QuicByteCount frame_length = - HttpEncoder::SerializeMaxPushIdFrame(frame, &buffer); - WriteOrBufferData(absl::string_view(buffer.get(), frame_length), - /*fin = */ false, nullptr); -} - void QuicSendControlStream::SendGoAway(QuicStreamId id) { QuicConnection::ScopedPacketFlusher flusher(session()->connection()); MaybeSendSettingsFrame();
diff --git a/quic/core/http/quic_send_control_stream.h b/quic/core/http/quic_send_control_stream.h index c5e18d1..face806 100644 --- a/quic/core/http/quic_send_control_stream.h +++ b/quic/core/http/quic_send_control_stream.h
@@ -37,11 +37,6 @@ // first frame sent on this stream. void MaybeSendSettingsFrame(); - // Send a MAX_PUSH_ID frame on this stream, and a SETTINGS frame beforehand if - // one has not been already sent. Must only be called for a client. - // TODO(b/171463363): Remove. - void SendMaxPushIdFrame(PushId max_push_id); - // Send a PRIORITY_UPDATE frame on this stream, and a SETTINGS frame // beforehand if one has not been already sent. void WritePriorityUpdate(const PriorityUpdateFrame& priority_update);
diff --git a/quic/core/http/quic_spdy_session.h b/quic/core/http/quic_spdy_session.h index 1ae1ebc..cc99b5b 100644 --- a/quic/core/http/quic_spdy_session.h +++ b/quic/core/http/quic_spdy_session.h
@@ -78,9 +78,11 @@ virtual void OnAcceptChFrameReceivedViaAlps(const AcceptChFrame& /*frame*/) {} // Incoming HTTP/3 frames on the control stream. + // TODO(b/171463363): Remove. virtual void OnCancelPushFrameReceived(const CancelPushFrame& /*frame*/) {} virtual void OnSettingsFrameReceived(const SettingsFrame& /*frame*/) = 0; virtual void OnGoAwayFrameReceived(const GoAwayFrame& /*frame*/) {} + // TODO(b/171463363): Remove. virtual void OnMaxPushIdFrameReceived(const MaxPushIdFrame& /*frame*/) {} virtual void OnPriorityUpdateFrameReceived( const PriorityUpdateFrame& /*frame*/) {} @@ -94,10 +96,12 @@ QuicByteCount /*compressed_headers_length*/) {} virtual void OnHeadersDecoded(QuicStreamId /*stream_id*/, QuicHeaderList /*headers*/) {} + // TODO(b/171463363): Remove. virtual void OnPushPromiseFrameReceived(QuicStreamId /*stream_id*/, QuicStreamId /*push_id*/, QuicByteCount /*compressed_headers_length*/) {} + // TODO(b/171463363): Remove. virtual void OnPushPromiseDecoded(QuicStreamId /*stream_id*/, QuicStreamId /*push_id*/, QuicHeaderList /*headers*/) {} @@ -110,6 +114,7 @@ // Outgoing HTTP/3 frames on the control stream. virtual void OnSettingsFrameSent(const SettingsFrame& /*frame*/) = 0; virtual void OnGoAwayFrameSent(QuicStreamId /*stream_id*/) {} + // TODO(b/171463363): Remove. virtual void OnMaxPushIdFrameSent(const MaxPushIdFrame& /*frame*/) {} virtual void OnPriorityUpdateFrameSent(const PriorityUpdateFrame& /*frame*/) { } @@ -120,6 +125,7 @@ virtual void OnHeadersFrameSent( QuicStreamId /*stream_id*/, const spdy::SpdyHeaderBlock& /*header_block*/) {} + // TODO(b/171463363): Remove. virtual void OnPushPromiseFrameSent( QuicStreamId /*stream_id*/, QuicStreamId @@ -313,6 +319,7 @@ void OnCompressedFrameSize(size_t frame_len); // Called when a PUSH_PROMISE frame has been received. + // TODO(b/171463363): Remove. void OnPushPromise(spdy::SpdyStreamId stream_id, spdy::SpdyStreamId promised_stream_id); @@ -330,6 +337,7 @@ // server. It must only be called if a MAX_PUSH_ID frame is received. // Returns whether |max_push_id| is greater than or equal to current // |max_push_id_|. + // TODO(b/171463363): Remove. bool OnMaxPushIdFrame(PushId max_push_id); // TODO(b/171463363): Remove. @@ -628,6 +636,7 @@ // after encryption is established, the push ID in the most recently sent // MAX_PUSH_ID frame. // Once set, never goes back to unset. + // TODO(b/171463363): Remove. absl::optional<PushId> max_push_id_; // Not owned by the session.
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc index 0a174c8..1f27146 100644 --- a/quic/core/http/quic_spdy_session_test.cc +++ b/quic/core/http/quic_spdy_session_test.cc
@@ -483,13 +483,27 @@ return std::string(priority_buffer.get(), priority_frame_length); } + // TODO(b/171463363): Remove. std::string SerializeMaxPushIdFrame(PushId push_id) { - MaxPushIdFrame max_push_id_frame; - max_push_id_frame.push_id = push_id; - std::unique_ptr<char[]> buffer; - QuicByteCount frame_length = - HttpEncoder::SerializeMaxPushIdFrame(max_push_id_frame, &buffer); - return std::string(buffer.get(), frame_length); + const QuicByteCount payload_length = + QuicDataWriter::GetVarInt62Len(push_id); + + const QuicByteCount total_length = + QuicDataWriter::GetVarInt62Len( + static_cast<uint64_t>(HttpFrameType::MAX_PUSH_ID)) + + QuicDataWriter::GetVarInt62Len(payload_length) + + QuicDataWriter::GetVarInt62Len(push_id); + + std::string max_push_id_frame(total_length, '\0'); + QuicDataWriter writer(total_length, max_push_id_frame.data()); + + QUICHE_CHECK(writer.WriteVarInt62( + static_cast<uint64_t>(HttpFrameType::MAX_PUSH_ID))); + QUICHE_CHECK(writer.WriteVarInt62(payload_length)); + QUICHE_CHECK(writer.WriteVarInt62(push_id)); + QUICHE_CHECK_EQ(0u, writer.remaining()); + + return max_push_id_frame; } QuicStreamId StreamCountToId(QuicStreamCount stream_count, @@ -1823,6 +1837,7 @@ } } +// TODO(b/171463363): Remove. TEST_P(QuicSpdySessionTestServer, ReduceMaxPushId) { if (!VersionUsesHttp3(transport_version())) { return; @@ -2914,12 +2929,12 @@ EXPECT_CALL(debug_visitor, OnSettingsFrameReceived(_)); session_.OnStreamFrame(data2); - CancelPushFrame cancel_push{/* push_id = */ 0}; - std::unique_ptr<char[]> buffer; - auto frame_length = - HttpEncoder::SerializeCancelPushFrame(cancel_push, &buffer); + std::string cancel_push_frame = absl::HexStringToBytes( + "03" // CANCEL_PUSH + "01" // length + "00"); // push ID QuicStreamFrame data3(receive_control_stream_id, /* fin = */ false, offset, - absl::string_view(buffer.get(), frame_length)); + cancel_push_frame); EXPECT_CALL(debug_visitor, OnCancelPushFrameReceived(_)); session_.OnStreamFrame(data3); } @@ -3125,12 +3140,12 @@ EXPECT_CALL(debug_visitor, OnSettingsFrameReceived(_)); session_.OnStreamFrame(data2); - CancelPushFrame cancel_push{/* push_id = */ 0}; - std::unique_ptr<char[]> buffer; - auto frame_length = - HttpEncoder::SerializeCancelPushFrame(cancel_push, &buffer); + std::string cancel_push_frame = absl::HexStringToBytes( + "03" // CANCEL_PUSH + "01" // length + "00"); // push ID QuicStreamFrame data3(receive_control_stream_id, /* fin = */ false, offset, - absl::string_view(buffer.get(), frame_length)); + cancel_push_frame); EXPECT_CALL(debug_visitor, OnCancelPushFrameReceived(_)); session_.OnStreamFrame(data3); }
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc index 1a03885..00c631b 100644 --- a/quic/core/http/quic_spdy_stream.cc +++ b/quic/core/http/quic_spdy_stream.cc
@@ -377,35 +377,6 @@ return bytes_written; } -void QuicSpdyStream::WritePushPromise(const PushPromiseFrame& frame) { - QUICHE_DCHECK(VersionUsesHttp3(transport_version())); - std::unique_ptr<char[]> push_promise_frame_with_id; - const size_t push_promise_frame_length = - HttpEncoder::SerializePushPromiseFrameWithOnlyPushId( - frame, &push_promise_frame_with_id); - - unacked_frame_headers_offsets_.Add(send_buffer().stream_offset(), - send_buffer().stream_offset() + - push_promise_frame_length + - frame.headers.length()); - - // Write Push Promise frame header and push id. - QUIC_DLOG(INFO) << ENDPOINT << "Stream " << id() - << " is writing Push Promise frame header of length " - << push_promise_frame_length << " , with promised id " - << frame.push_id; - WriteOrBufferData(absl::string_view(push_promise_frame_with_id.get(), - push_promise_frame_length), - /* fin = */ false, /* ack_listener = */ nullptr); - - // Write response headers. - QUIC_DLOG(INFO) << ENDPOINT << "Stream " << id() - << " is writing Push Promise request header of length " - << frame.headers.length(); - WriteOrBufferData(frame.headers, /* fin = */ false, - /* ack_listener = */ nullptr); -} - QuicConsumedData QuicSpdyStream::WritevBody(const struct iovec* iov, int count, bool fin) {
diff --git a/quic/core/http/quic_spdy_stream.h b/quic/core/http/quic_spdy_stream.h index 523b105..ef3c846 100644 --- a/quic/core/http/quic_spdy_stream.h +++ b/quic/core/http/quic_spdy_stream.h
@@ -135,10 +135,6 @@ spdy::SpdyHeaderBlock trailer_block, QuicReferenceCountedPointer<QuicAckListenerInterface> ack_listener); - // Serializes |frame| and writes the encoded push promise data. - // TODO(b/171463363): Remove. - void WritePushPromise(const PushPromiseFrame& frame); - // Override to report newly acked bytes via ack_listener_. bool OnStreamFrameAcked(QuicStreamOffset offset, QuicByteCount data_length,
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc index dca6f92..6530618 100644 --- a/quic/core/http/quic_spdy_stream_test.cc +++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -11,6 +11,7 @@ #include "absl/base/macros.h" #include "absl/strings/escaping.h" +#include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "quic/core/crypto/null_encrypter.h" #include "quic/core/http/http_encoder.h" @@ -448,18 +449,30 @@ } // Construct PUSH_PROMISE frame with given payload. + // TODO(b/171463363): Remove. std::string SerializePushPromiseFrame(PushId push_id, - absl::string_view 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); - absl::string_view push_promise_frame_header( - push_promise_buffer.get(), push_promise_frame_header_length); - return absl::StrCat(push_promise_frame_header, payload); + absl::string_view headers) { + const QuicByteCount payload_length = + QuicDataWriter::GetVarInt62Len(push_id) + headers.length(); + + const QuicByteCount length_without_headers = + QuicDataWriter::GetVarInt62Len( + static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE)) + + QuicDataWriter::GetVarInt62Len(payload_length) + + QuicDataWriter::GetVarInt62Len(push_id); + + std::string push_promise_frame(length_without_headers, '\0'); + QuicDataWriter writer(length_without_headers, push_promise_frame.data()); + + QUICHE_CHECK(writer.WriteVarInt62( + static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE))); + QUICHE_CHECK(writer.WriteVarInt62(payload_length)); + QUICHE_CHECK(writer.WriteVarInt62(push_id)); + QUICHE_CHECK_EQ(0u, writer.remaining()); + + absl::StrAppend(&push_promise_frame, headers); + + return push_promise_frame; } std::string DataFrame(absl::string_view payload) { @@ -2758,6 +2771,7 @@ EXPECT_EQ(unknown_frame4.size(), NewlyConsumedBytes()); } +// TODO(b/171463363): Remove. TEST_P(QuicSpdyStreamTest, PushPromiseOnDataStream) { Initialize(kShouldProcessData); if (!UsesHttp3()) { @@ -2786,6 +2800,7 @@ } // Regression test for b/152518220. +// TODO(b/171463363): Remove. TEST_P(QuicSpdyStreamTest, OnStreamHeaderBlockArgumentDoesNotIncludePushedHeaderBlock) { Initialize(kShouldProcessData);