gfe-relnote: Refactor SetMaxAllowedPushId() method, add DCHECKs. Not protected. Split QuicSpdySession::SetMaxAllowedPushId() into two methods: OnMaxPushIdFrame() for the server and SetMaxPushId() for the client. Rename max_allowed_push_id_ to max_push_id_ for consistency with MAX_PUSH_ID frame. Add QUIC version and perspective restrictions to method comments and enforce them with DCHECKs. Also add DCHECK to QuicSendControlStream::SendMaxPushIdFrame(). PiperOrigin-RevId: 302687788 Change-Id: I043229016505a8642c353a1851402c6522a8dd1b
diff --git a/quic/core/http/end_to_end_test.cc b/quic/core/http/end_to_end_test.cc index 2bc4917..e5b30c1 100644 --- a/quic/core/http/end_to_end_test.cc +++ b/quic/core/http/end_to_end_test.cc
@@ -4193,7 +4193,7 @@ EXPECT_TRUE(client_->client()->WaitForCryptoHandshakeConfirmed()); static_cast<QuicSpdySession*>(client_->client()->session()) - ->SetMaxAllowedPushId(kMaxQuicStreamId); + ->SetMaxPushId(kMaxQuicStreamId); client_->SendSynchronousRequest("/foo");
diff --git a/quic/core/http/quic_receive_control_stream.cc b/quic/core/http/quic_receive_control_stream.cc index f9fdef9..7e74025 100644 --- a/quic/core/http/quic_receive_control_stream.cc +++ b/quic/core/http/quic_receive_control_stream.cc
@@ -49,7 +49,9 @@ stream_->spdy_session()->debug_visitor()->OnMaxPushIdFrameReceived(frame); } - stream_->spdy_session()->SetMaxAllowedPushId(frame.push_id); + // TODO(b/124216424): Signal error if received push ID is smaller than a + // previously received value. + stream_->spdy_session()->OnMaxPushIdFrame(frame.push_id); return true; }
diff --git a/quic/core/http/quic_send_control_stream.cc b/quic/core/http/quic_send_control_stream.cc index 0ea3d6a..64c306a 100644 --- a/quic/core/http/quic_send_control_stream.cc +++ b/quic/core/http/quic_send_control_stream.cc
@@ -116,6 +116,7 @@ } void QuicSendControlStream::SendMaxPushIdFrame(PushId max_push_id) { + DCHECK_EQ(Perspective::IS_CLIENT, session()->perspective()); 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 3771cdd..03bd1f7 100644 --- a/quic/core/http/quic_send_control_stream.h +++ b/quic/core/http/quic_send_control_stream.h
@@ -40,7 +40,7 @@ void MaybeSendSettingsFrame(); // Send a MAX_PUSH_ID frame on this stream, and a SETTINGS frame beforehand if - // one has not been already sent. + // one has not been already sent. Must only be called for a client. void SendMaxPushIdFrame(PushId max_push_id); // Send a PRIORITY_UPDATE frame on this stream, and a SETTINGS frame
diff --git a/quic/core/http/quic_spdy_client_session_test.cc b/quic/core/http/quic_spdy_client_session_test.cc index 15a9736..da05a15 100644 --- a/quic/core/http/quic_spdy_client_session_test.cc +++ b/quic/core/http/quic_spdy_client_session_test.cc
@@ -583,8 +583,10 @@ // Initialize crypto before the client session will create a stream. CompleteCryptoHandshake(); - session_->SetMaxAllowedPushId(GetNthServerInitiatedUnidirectionalStreamId( - connection_->transport_version(), 10)); + if (VersionHasIetfQuicFrames(connection_->transport_version())) { + session_->SetMaxPushId(GetNthServerInitiatedUnidirectionalStreamId( + connection_->transport_version(), 10)); + } MockQuicSpdyClientStream* stream = static_cast<MockQuicSpdyClientStream*>( session_->CreateOutgoingBidirectionalStream()); @@ -603,9 +605,9 @@ session_.get(), std::make_unique<QuicSpdyClientStream>( stream_id, session_.get(), BIDIRECTIONAL)); - session_->SetMaxAllowedPushId(GetNthServerInitiatedUnidirectionalStreamId( - connection_->transport_version(), 10)); if (VersionHasIetfQuicFrames(connection_->transport_version())) { + session_->SetMaxPushId(GetNthServerInitiatedUnidirectionalStreamId( + connection_->transport_version(), 10)); // TODO(b/136295430) Use PushId to represent Push IDs instead of // QuicStreamId. EXPECT_CALL( @@ -644,8 +646,10 @@ // Initialize crypto before the client session will create a stream. CompleteCryptoHandshake(); - session_->SetMaxAllowedPushId(GetNthServerInitiatedUnidirectionalStreamId( - connection_->transport_version(), 10)); + if (VersionHasIetfQuicFrames(connection_->transport_version())) { + session_->SetMaxPushId(GetNthServerInitiatedUnidirectionalStreamId( + connection_->transport_version(), 10)); + } MockQuicSpdyClientStream* stream = static_cast<MockQuicSpdyClientStream*>( session_->CreateOutgoingBidirectionalStream()); @@ -914,7 +918,9 @@ session_.get(), std::make_unique<QuicSpdyClientStream>( stream_id, session_.get(), BIDIRECTIONAL)); - session_->SetMaxAllowedPushId(kMaxQuicStreamId); + if (VersionHasIetfQuicFrames(connection_->transport_version())) { + session_->SetMaxPushId(kMaxQuicStreamId); + } EXPECT_CALL(*connection_, OnStreamReset(_, QUIC_REFUSED_STREAM));
diff --git a/quic/core/http/quic_spdy_session.cc b/quic/core/http/quic_spdy_session.cc index 2b1b457..52bd00e 100644 --- a/quic/core/http/quic_spdy_session.cc +++ b/quic/core/http/quic_spdy_session.cc
@@ -398,7 +398,7 @@ spdy_framer_visitor_(new SpdyFramerVisitor(this)), server_push_enabled_(true), ietf_server_push_enabled_(false), - max_allowed_push_id_(0), + max_push_id_(0), destruction_indicator_(123456789), debug_visitor_(nullptr), http3_goaway_received_(false), @@ -1211,33 +1211,43 @@ } } -void QuicSpdySession::SetMaxAllowedPushId(QuicStreamId max_allowed_push_id) { - if (!VersionUsesHttp3(transport_version())) { - return; - } +void QuicSpdySession::SetMaxPushId(QuicStreamId max_push_id) { + DCHECK(VersionUsesHttp3(transport_version())); + DCHECK_EQ(Perspective::IS_CLIENT, perspective()); + DCHECK_GE(max_push_id, max_push_id_); - QuicStreamId old_max_allowed_push_id = max_allowed_push_id_; - max_allowed_push_id_ = max_allowed_push_id; - QUIC_DVLOG(1) << ENDPOINT - << "Setting max_allowed_push_id to: " << max_allowed_push_id_ - << " from: " << old_max_allowed_push_id; + QuicStreamId old_max_push_id = max_push_id_; + max_push_id_ = max_push_id; + QUIC_DVLOG(1) << "Setting max_push_id to: " << max_push_id_ + << " from: " << old_max_push_id; - if (perspective() == Perspective::IS_SERVER) { - if (max_allowed_push_id_ > old_max_allowed_push_id) { - OnCanCreateNewOutgoingStream(true); - } - return; - } - - DCHECK(perspective() == Perspective::IS_CLIENT); if (OneRttKeysAvailable()) { SendMaxPushId(); } } +bool QuicSpdySession::OnMaxPushIdFrame(QuicStreamId max_push_id) { + DCHECK(VersionUsesHttp3(transport_version())); + DCHECK_EQ(Perspective::IS_SERVER, perspective()); + + QuicStreamId old_max_push_id = max_push_id_; + max_push_id_ = max_push_id; + QUIC_DVLOG(1) << "Setting max_push_id to: " << max_push_id_ + << " from: " << old_max_push_id; + + if (max_push_id_ > old_max_push_id) { + OnCanCreateNewOutgoingStream(true); + return true; + } + + // Equal value is not considered an error. + return max_push_id >= old_max_push_id; +} + void QuicSpdySession::SendMaxPushId() { DCHECK(VersionUsesHttp3(transport_version())); - send_control_stream_->SendMaxPushIdFrame(max_allowed_push_id_); + DCHECK_EQ(Perspective::IS_CLIENT, perspective()); + send_control_stream_->SendMaxPushIdFrame(max_push_id_); } void QuicSpdySession::EnableServerPush() {
diff --git a/quic/core/http/quic_spdy_session.h b/quic/core/http/quic_spdy_session.h index bd1438b..3fa2963 100644 --- a/quic/core/http/quic_spdy_session.h +++ b/quic/core/http/quic_spdy_session.h
@@ -295,16 +295,30 @@ // those streams are not initialized yet. void OnCanCreateNewOutgoingStream(bool unidirectional) override; - void SetMaxAllowedPushId(QuicStreamId max_allowed_push_id); + // Sets |max_push_id_| and sends a MAX_PUSH_ID frame. + // This method must only be called if protocol is IETF QUIC and perspective is + // client. |max_push_id| must be greater than or equal to current + // |max_push_id_|. + void SetMaxPushId(QuicStreamId max_push_id); - QuicStreamId max_allowed_push_id() { return max_allowed_push_id_; } + // Sets |max_push_id_|. + // This method must only be called if protocol is IETF QUIC and perspective is + // 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_|. + bool OnMaxPushIdFrame(QuicStreamId max_push_id); + + // TODO(b/151451061): Change this API to distinguish between having received + // no MAX_PUSH_ID frame and one MAX_PUSH_ID frame with push ID 0. + // TODO(b/136295430): Use sequential PUSH IDs instead of stream IDs. + QuicStreamId max_allowed_push_id() { return max_push_id_; } // Enables server push. // Must only be called when using IETF QUIC, for which server push is disabled // by default. Server push defaults to enabled and cannot be disabled for // Google QUIC. // Must only be called for a server. A client can effectively disable push by - // never calling SetMaxAllowedPushId(). + // never calling SetMaxPushId(). void EnableServerPush(); int32_t destruction_indicator() const { return destruction_indicator_; } @@ -506,7 +520,7 @@ // Defaults to false. bool ietf_server_push_enabled_; - QuicStreamId max_allowed_push_id_; + QuicStreamId max_push_id_; // An integer used for live check. The indicator is assigned a value in // constructor. As long as it is not the assigned value, that would indicate
diff --git a/quic/tools/quic_simple_server_session_test.cc b/quic/tools/quic_simple_server_session_test.cc index b127cd6..f9c28a7 100644 --- a/quic/tools/quic_simple_server_session_test.cc +++ b/quic/tools/quic_simple_server_session_test.cc
@@ -757,7 +757,7 @@ MaybeConsumeHeadersStreamData(); if (VersionUsesHttp3(transport_version())) { session_->EnableServerPush(); - session_->SetMaxAllowedPushId(kMaxQuicStreamId); + session_->OnMaxPushIdFrame(kMaxQuicStreamId); } size_t num_resources = kMaxStreamsForTest + 5; PromisePushResources(num_resources); @@ -771,7 +771,7 @@ MaybeConsumeHeadersStreamData(); if (VersionUsesHttp3(transport_version())) { session_->EnableServerPush(); - session_->SetMaxAllowedPushId(kMaxQuicStreamId); + session_->OnMaxPushIdFrame(kMaxQuicStreamId); } size_t num_resources = kMaxStreamsForTest + 1; QuicByteCount data_frame_header_length = PromisePushResources(num_resources); @@ -849,7 +849,7 @@ MaybeConsumeHeadersStreamData(); if (VersionUsesHttp3(transport_version())) { session_->EnableServerPush(); - session_->SetMaxAllowedPushId(kMaxQuicStreamId); + session_->OnMaxPushIdFrame(kMaxQuicStreamId); } // Having two extra resources to be send later. One of them will be reset, so @@ -937,7 +937,7 @@ MaybeConsumeHeadersStreamData(); if (VersionUsesHttp3(transport_version())) { session_->EnableServerPush(); - session_->SetMaxAllowedPushId(kMaxQuicStreamId); + session_->OnMaxPushIdFrame(kMaxQuicStreamId); } size_t num_resources = kMaxStreamsForTest + 1; if (VersionHasIetfQuicFrames(transport_version())) {
diff --git a/quic/tools/quic_spdy_client_base.cc b/quic/tools/quic_spdy_client_base.cc index 429b09d..b4d4382 100644 --- a/quic/tools/quic_spdy_client_base.cc +++ b/quic/tools/quic_spdy_client_base.cc
@@ -70,8 +70,9 @@ } client_session()->Initialize(); client_session()->CryptoConnect(); - if (max_allowed_push_id_ > 0) { - client_session()->SetMaxAllowedPushId(max_allowed_push_id_); + if (max_allowed_push_id_ > 0 && + VersionUsesHttp3(client_session()->transport_version())) { + client_session()->SetMaxPushId(max_allowed_push_id_); } }
diff --git a/quic/tools/quic_spdy_client_base.h b/quic/tools/quic_spdy_client_base.h index 4d1642e..c4381f7 100644 --- a/quic/tools/quic_spdy_client_base.h +++ b/quic/tools/quic_spdy_client_base.h
@@ -139,6 +139,7 @@ bool drop_response_body() const { return drop_response_body_; } // Set the max promise id for the client session. + // TODO(b/151641466): Rename this method. void SetMaxAllowedPushId(QuicStreamId max) { max_allowed_push_id_ = max; } // Disables the use of the QPACK dynamic table and of blocked streams.