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());
