Disallow sending of MAX_STREAMS and STREAM_BLOCKED frame in QuicStreamIdManager before the session is configured. gfe-relnote: protected by gfe2_reloadable_flag_quic_enable_version_draft_25_v3 PiperOrigin-RevId: 300366362 Change-Id: I66770a7047169452f423f5b13a94083d07c6e662
diff --git a/quic/core/quic_session_test.cc b/quic/core/quic_session_test.cc index 0d7d80e..2daef86 100644 --- a/quic/core/quic_session_test.cc +++ b/quic/core/quic_session_test.cc
@@ -2847,98 +2847,6 @@ ::testing::ValuesIn(AllSupportedVersions()), ::testing::PrintToStringParamName()); -TEST_P(QuicSessionTestClientUnconfigured, HoldMaxStreamsFrame) { - if (!VersionHasIetfQuicFrames(transport_version())) { - return; - } - QuicStreamIdManager* stream_id_manager = - QuicSessionPeer::v99_unidirectional_stream_id_manager(&session_); - - EXPECT_CALL(*connection_, SendControlFrame(_)).Times(0); - QuicStreamsBlockedFrame frame(1u, 0u, /*unidirectional=*/true); - session_.OnStreamsBlockedFrame(frame); - EXPECT_CALL(*connection_, SendControlFrame(_)) - .Times(1) - .WillRepeatedly(Invoke(&session_, &TestSession::SaveFrame)); - session_.OnConfigNegotiated(); - EXPECT_EQ(MAX_STREAMS_FRAME, session_.save_frame().type); - EXPECT_EQ(stream_id_manager->incoming_actual_max_streams(), - session_.save_frame().max_streams_frame.stream_count); - EXPECT_EQ(1u, session_.save_frame().max_streams_frame.control_frame_id); -} - -TEST_P(QuicSessionTestClientUnconfigured, HoldStreamsBlockedFrameXmit) { - if (!VersionHasIetfQuicFrames(transport_version())) { - // Applicable only to IETF QUIC - return; - } - QuicStreamIdManager* stream_id_manager = - QuicSessionPeer::v99_unidirectional_stream_id_manager(&session_); - - // Set the stream limit to 0 which will cause - // CanOpenNextOutgoingUnidirectionalStream() - // to generated a STREAMS_BLOCKED frame. - QuicStreamIdManagerPeer::set_outgoing_max_streams(stream_id_manager, 0); - - // Since the stream limit is 0 and no sreams can be created this should return - // false and have forced a streams-blocked to be queued up, with the - // blocked stream id == 0. - EXPECT_CALL(*connection_, SendControlFrame(_)).Times(0); - EXPECT_FALSE(session_.CanOpenNextOutgoingUnidirectionalStream()); - - // We will expect two calls to SendControlFrame: The first is because - // OnConfigNegotiated does not increase the limit, so the app still can not - // create new streams (and therefore needs the STREAMS-BLOCKED to go out). The - // second is because the ensuing CanOpenNext.. call will fail (this test not - // actually increasing the limit) and that will send another STREAMS-BLOCKED. - EXPECT_CALL(*connection_, SendControlFrame(_)) - .Times(2) - .WillRepeatedly(Invoke(&session_, &TestSession::SaveFrame)); - // Set configuration data so that when the config happens, the stream limit is - // not increased and another STREAMS-BLOCKED will be needed.. - QuicConfigPeer::SetReceivedMaxUnidirectionalStreams(session_.config(), 0); - - session_.OnConfigNegotiated(); - - EXPECT_EQ(STREAMS_BLOCKED_FRAME, session_.save_frame().type); - EXPECT_EQ(0u, session_.save_frame().streams_blocked_frame.stream_count); - EXPECT_EQ(1u, session_.save_frame().streams_blocked_frame.control_frame_id); - - EXPECT_FALSE(session_.CanOpenNextOutgoingUnidirectionalStream()); - EXPECT_EQ(STREAMS_BLOCKED_FRAME, session_.save_frame().type); - EXPECT_EQ(0u, session_.save_frame().streams_blocked_frame.stream_count); - EXPECT_EQ(2u, session_.save_frame().streams_blocked_frame.control_frame_id); -} - -TEST_P(QuicSessionTestClientUnconfigured, HoldStreamsBlockedFrameNoXmit) { - if (!VersionHasIetfQuicFrames(transport_version())) { - return; - } - QuicStreamIdManager* stream_id_manager = - QuicSessionPeer::v99_unidirectional_stream_id_manager(&session_); - - // Set the stream limit to 0 which will cause - // CanOpenNextOutgoingUnidirectionalStream() - // to generated a STREAMS_BLOCKED frame. - QuicStreamIdManagerPeer::set_outgoing_max_streams(stream_id_manager, 0); - - // Since the stream limit is 0 and no streams can be created this should - // return false and have forced a streams-blocked to be queued up, with the - // blocked stream id == 0. - EXPECT_CALL(*connection_, SendControlFrame(_)).Times(0); - EXPECT_FALSE(session_.CanOpenNextOutgoingUnidirectionalStream()); - - // Set configuration data so that when the config happens, the stream limit is - // increased. - QuicConfigPeer::SetReceivedMaxUnidirectionalStreams(session_.config(), 10); - - // STREAMS_BLOCKED frame should not be sent because streams can now be - // created. - EXPECT_CALL(*connection_, SendControlFrame(_)).Times(0); - session_.OnConfigNegotiated(); - EXPECT_TRUE(session_.CanOpenNextOutgoingUnidirectionalStream()); -} - TEST_P(QuicSessionTestClientUnconfigured, StreamInitiallyBlockedThenUnblocked) { if (!connection_->version().AllowsLowFlowControlLimits()) { return;
diff --git a/quic/core/quic_stream_id_manager.cc b/quic/core/quic_stream_id_manager.cc index 8f1b413..a2d1bad 100644 --- a/quic/core/quic_stream_id_manager.cc +++ b/quic/core/quic_stream_id_manager.cc
@@ -3,6 +3,7 @@ // found in the LICENSE file. #include "net/third_party/quiche/src/quic/core/quic_stream_id_manager.h" +#include <cstdint> #include <string> #include "net/third_party/quiche/src/quic/core/quic_connection.h" @@ -42,10 +43,7 @@ incoming_stream_count_(0), largest_peer_created_stream_id_( QuicUtils::GetInvalidStreamId(transport_version)), - max_streams_window_(0), - pending_max_streams_(false), - pending_streams_blocked_( - QuicUtils::GetInvalidStreamId(transport_version)) { + max_streams_window_(0) { CalculateIncomingMaxStreamsWindow(); } @@ -142,7 +140,7 @@ // MAX STREAMS frame yet. Record that we would have sent one and then // return. A new frame will be generated once the configuration is // received. - pending_max_streams_ = true; + QUIC_BUG << "Attempt to send Max Streams Frame before config is negotiated"; return; } incoming_advertised_max_streams_ = incoming_actual_max_streams_; @@ -188,15 +186,12 @@ if (outgoing_stream_count_ < outgoing_max_streams_) { return true; } - // Next stream ID would exceed the limit, need to inform the peer. - if (!is_config_negotiated_) { - // The config is not negotiated, so we can not send the STREAMS_BLOCKED - // frame yet. Record that we would have sent one, and what the limit was - // when we were blocked, and return. - pending_streams_blocked_ = outgoing_max_streams_; + QUIC_BUG + << "Creating streams before Quic session is configured is prohibitied"; return false; } + // Next stream ID would exceed the limit, need to inform the peer. delegate_->SendStreamsBlocked(outgoing_max_streams_, unidirectional_); QUIC_CODE_COUNT(quic_reached_outgoing_stream_id_limit); return false; @@ -326,23 +321,6 @@ void QuicStreamIdManager::OnConfigNegotiated() { is_config_negotiated_ = true; - // If a STREAMS_BLOCKED or MAX_STREAMS is pending, send it and clear - // the pending state. - if (pending_streams_blocked_ != - QuicUtils::GetInvalidStreamId(transport_version_)) { - if (pending_streams_blocked_ >= outgoing_max_streams_) { - // There is a pending STREAMS_BLOCKED frame and the current limit does not - // let new streams be formed. Regenerate and send the frame. - delegate_->SendStreamsBlocked(outgoing_max_streams_, unidirectional_); - } - pending_streams_blocked_ = - QuicUtils::GetInvalidStreamId(transport_version_); - } - if (pending_max_streams_) { - // Generate a MAX_STREAMS using the current stream limits. - SendMaxStreamsFrame(); - pending_max_streams_ = false; - } } } // namespace quic
diff --git a/quic/core/quic_stream_id_manager.h b/quic/core/quic_stream_id_manager.h index 9fa26c6..15b19d3 100644 --- a/quic/core/quic_stream_id_manager.h +++ b/quic/core/quic_stream_id_manager.h
@@ -254,13 +254,6 @@ // max_streams_window_ is set to 1/2 of the initial number of incoming streams // that are allowed (as set in the constructor). QuicStreamId max_streams_window_; - - // MAX_STREAMS and STREAMS_BLOCKED frames are not sent before the session has - // been configured. Instead, the relevant information is stored in - // |pending_max_streams_| and |pending_streams_blocked_| and sent when - // OnConfigNegotiated() is invoked. - bool pending_max_streams_; - QuicStreamId pending_streams_blocked_; }; } // namespace quic
diff --git a/quic/core/quic_stream_id_manager_test.cc b/quic/core/quic_stream_id_manager_test.cc index a114295..12368e9 100644 --- a/quic/core/quic_stream_id_manager_test.cc +++ b/quic/core/quic_stream_id_manager_test.cc
@@ -483,50 +483,24 @@ stream_id_manager_.OnStreamsBlockedFrame(frame); } -TEST_P(QuicStreamIdManagerTest, HoldMaxStreamsFrame) { - // The config has not been negotiated so the MAX_STREAMS frame will not be - // sent. +TEST_P(QuicStreamIdManagerTest, NoMaxStreamsFrameBeforeConfigured) { + // The config has not been negotiated so QUIC_BUG will be triggered. EXPECT_CALL(delegate_, SendMaxStreams(_, _)).Times(0); QuicStreamsBlockedFrame frame(1u, 0u, IsUnidirectional()); - // Should cause change in pending_max_streams. - stream_id_manager_.OnStreamsBlockedFrame(frame); - - EXPECT_CALL(delegate_, SendMaxStreams(_, IsUnidirectional())); - - // MAX_STREAMS will be sent now that the config has been negotiated. - stream_id_manager_.OnConfigNegotiated(); + // Trigger sending of MAX_STREAMS. + EXPECT_QUIC_BUG( + stream_id_manager_.OnStreamsBlockedFrame(frame), + "Attempt to send Max Streams Frame before config is negotiated"); } -TEST_P(QuicStreamIdManagerTest, HoldStreamsBlockedFrameXmit) { +TEST_P(QuicStreamIdManagerTest, NoStreamCreationBeforeConfigured) { // We should not see a STREAMS_BLOCKED frame because we're not configured.. EXPECT_CALL(delegate_, SendStreamsBlocked(_, _)).Times(0); - // Since the stream limit is 0 and no sreams can be created this should return - // false and have forced a STREAMS_BLOCKED to be queued up, with the - // blocked stream id == 0. - EXPECT_FALSE(stream_id_manager_.CanOpenNextOutgoingStream()); - - // Since the steam limit has not been increased when the config was negotiated - // a STREAMS_BLOCKED frame should be sent. - EXPECT_CALL(delegate_, SendStreamsBlocked(_, IsUnidirectional())); - stream_id_manager_.OnConfigNegotiated(); -} - -TEST_P(QuicStreamIdManagerTest, HoldStreamsBlockedFrameNoXmit) { - // We should not see a STREAMS_BLOCKED frame because we're not configured.. - EXPECT_CALL(delegate_, SendStreamsBlocked(_, IsUnidirectional())).Times(0); - - // Since the stream limit is 0 and no sreams can be created this should return - // false and have forced a STREAMS_BLOCKED to be queued up, with the - // blocked stream id == 0. - EXPECT_FALSE(stream_id_manager_.CanOpenNextOutgoingStream()); - - EXPECT_CALL(delegate_, OnCanCreateNewOutgoingStream(IsUnidirectional())); - stream_id_manager_.SetMaxOpenOutgoingStreams(10); - // Since the stream limit has been increase which allows streams to be created - // no STREAMS_BLOCKED should be send. - stream_id_manager_.OnConfigNegotiated(); + EXPECT_QUIC_BUG( + stream_id_manager_.CanOpenNextOutgoingStream(), + "Creating streams before Quic session is configured is prohibitied"); } TEST_P(QuicStreamIdManagerTest, CheckMaxAllowedOutgoingInitialization) {
diff --git a/quic/core/uber_quic_stream_id_manager_test.cc b/quic/core/uber_quic_stream_id_manager_test.cc index b7acc68..d7e9cc1 100644 --- a/quic/core/uber_quic_stream_id_manager_test.cc +++ b/quic/core/uber_quic_stream_id_manager_test.cc
@@ -145,6 +145,7 @@ } TEST_P(UberQuicStreamIdManagerTest, SetMaxOpenOutgoingStreams) { + manager_.OnConfigNegotiated(); const size_t kNumMaxOutgoingStream = 123; // Set the uni- and bi- directional limits to different values to ensure // that they are managed separately. @@ -171,6 +172,7 @@ manager_.GetNextOutgoingUnidirectionalStreamId(); // Both should be exhausted... + EXPECT_CALL(delegate_, SendStreamsBlocked(_, _)).Times(2); EXPECT_FALSE(manager_.CanOpenNextOutgoingUnidirectionalStream()); EXPECT_FALSE(manager_.CanOpenNextOutgoingBidirectionalStream()); } @@ -351,6 +353,7 @@ } TEST_P(UberQuicStreamIdManagerTest, SetMaxOpenOutgoingStreamsPlusFrame) { + manager_.OnConfigNegotiated(); const size_t kNumMaxOutgoingStream = 123; // Set the uni- and bi- directional limits to different values to ensure // that they are managed separately. @@ -377,6 +380,7 @@ manager_.GetNextOutgoingUnidirectionalStreamId(); // Both should be exhausted... + EXPECT_CALL(delegate_, SendStreamsBlocked(_, _)).Times(3); EXPECT_FALSE(manager_.CanOpenNextOutgoingUnidirectionalStream()); EXPECT_FALSE(manager_.CanOpenNextOutgoingBidirectionalStream());