Fix sending MAX_PUSH_ID frame during 0-RTT.
Assume SetMaxPushId() is called with some initial value. During 0-RTT,
SendInitialData() is called, it sends a MAX_PUSH_ID and sets
|http3_max_push_id_sent_|. If SetMaxPushId() is called with a different value
after this but before 1-RTT keys are available, it does not send the MAX_PUSH_ID
frame with the updated value because OneRttKeysAvailable() is false. Then
SendInitialData() is called when 1-RTT keys are available, but it does not send
the updated MAX_PUSH_ID frame either, because |http3_max_push_id_sent_| is true.
This CL fixes that by gating on IsEncryptionEstablished() instead of
OneRttKeysAvailable() in SetMaxPushId(). However, it introduces a new bug:
Assume there is no initial SetMaxPushId() call. During 0-RTT, SendInitialData()
is called, but it does not send a MAX_PUSH_ID frame because |max_push_id_| is
not set. Then SetMaxPushId() is called before 1-RTT keys are available, and it
now sends the MAX_PUSH_ID frame because IsEncryptionEstablished() is true. Then
SendInitialData() is called when 1-RTT keys are available. It should not send
another MAX_PUSH_ID frame with the same value. In order to achieve this,
|http3_max_push_id_sent_| must be set to true when the MAX_PUSH_ID frame is sent
by SetMaxPushId(). This is done by setting |http3_max_push_id_sent_| in
SendMaxPushId() instead of SendInitialData().
Client-only change.
PiperOrigin-RevId: 315734365
Change-Id: Ib4dc2f96624b1c8c9a27916aa4a6ab98fbb71494
diff --git a/quic/core/http/quic_spdy_client_session_test.cc b/quic/core/http/quic_spdy_client_session_test.cc
index ca9b1d5..fc6343e 100644
--- a/quic/core/http/quic_spdy_client_session_test.cc
+++ b/quic/core/http/quic_spdy_client_session_test.cc
@@ -43,6 +43,7 @@
using ::testing::AtLeast;
using ::testing::AtMost;
using ::testing::Invoke;
+using ::testing::StrictMock;
using ::testing::Truly;
namespace quic {
@@ -1234,6 +1235,119 @@
connection_, crypto_stream_, AlpnForVersion(connection_->version()));
}
+TEST_P(QuicSpdyClientSessionTest, SetMaxPushIdBeforeEncryptionEstablished) {
+ // 0-RTT is TLS-only, MAX_PUSH_ID frame is HTTP/3-only.
+ if (!session_->version().UsesTls() || !session_->version().UsesHttp3()) {
+ return;
+ }
+
+ CompleteFirstConnection();
+
+ CreateConnection();
+ StrictMock<MockHttp3DebugVisitor> debug_visitor;
+ session_->set_debug_visitor(&debug_visitor);
+
+ // No MAX_PUSH_ID frame is sent before encryption is established.
+ session_->SetMaxPushId(5);
+
+ EXPECT_FALSE(session_->IsEncryptionEstablished());
+ EXPECT_FALSE(session_->OneRttKeysAvailable());
+ EXPECT_EQ(ENCRYPTION_INITIAL, session_->connection()->encryption_level());
+
+ // MAX_PUSH_ID frame is sent upon encryption establishment with the value set
+ // by the earlier SetMaxPushId() call.
+ EXPECT_CALL(debug_visitor, OnSettingsFrameSent(_));
+ EXPECT_CALL(debug_visitor, OnMaxPushIdFrameSent(_))
+ .WillOnce(Invoke(
+ [](const MaxPushIdFrame& frame) { EXPECT_EQ(5u, frame.push_id); }));
+ session_->CryptoConnect();
+ testing::Mock::VerifyAndClearExpectations(&debug_visitor);
+
+ EXPECT_TRUE(session_->IsEncryptionEstablished());
+ EXPECT_FALSE(session_->OneRttKeysAvailable());
+ EXPECT_EQ(ENCRYPTION_ZERO_RTT, session_->connection()->encryption_level());
+
+ // Another SetMaxPushId() call with the same value does not trigger sending
+ // another MAX_PUSH_ID frame.
+ session_->SetMaxPushId(5);
+
+ // Calling SetMaxPushId() with a different value results in sending another
+ // MAX_PUSH_ID frame.
+ EXPECT_CALL(debug_visitor, OnMaxPushIdFrameSent(_))
+ .WillOnce(Invoke(
+ [](const MaxPushIdFrame& frame) { EXPECT_EQ(10u, frame.push_id); }));
+ session_->SetMaxPushId(10);
+ testing::Mock::VerifyAndClearExpectations(&debug_visitor);
+
+ QuicConfig config = DefaultQuicConfig();
+ crypto_test_utils::HandshakeWithFakeServer(
+ &config, server_crypto_config_.get(), &helper_, &alarm_factory_,
+ connection_, crypto_stream_, AlpnForVersion(connection_->version()));
+
+ EXPECT_TRUE(session_->IsEncryptionEstablished());
+ EXPECT_TRUE(session_->OneRttKeysAvailable());
+ EXPECT_EQ(ENCRYPTION_FORWARD_SECURE,
+ session_->connection()->encryption_level());
+ EXPECT_TRUE(session_->GetCryptoStream()->IsResumption());
+}
+
+TEST_P(QuicSpdyClientSessionTest, SetMaxPushIdAfterEncryptionEstablished) {
+ // 0-RTT is TLS-only, MAX_PUSH_ID frame is HTTP/3-only.
+ if (!session_->version().UsesTls() || !session_->version().UsesHttp3()) {
+ return;
+ }
+
+ CompleteFirstConnection();
+
+ CreateConnection();
+ StrictMock<MockHttp3DebugVisitor> debug_visitor;
+ session_->set_debug_visitor(&debug_visitor);
+
+ EXPECT_FALSE(session_->IsEncryptionEstablished());
+ EXPECT_FALSE(session_->OneRttKeysAvailable());
+ EXPECT_EQ(ENCRYPTION_INITIAL, session_->connection()->encryption_level());
+
+ // No MAX_PUSH_ID frame is sent if SetMaxPushId() has not been called.
+ EXPECT_CALL(debug_visitor, OnSettingsFrameSent(_));
+ session_->CryptoConnect();
+ testing::Mock::VerifyAndClearExpectations(&debug_visitor);
+
+ EXPECT_TRUE(session_->IsEncryptionEstablished());
+ EXPECT_FALSE(session_->OneRttKeysAvailable());
+ EXPECT_EQ(ENCRYPTION_ZERO_RTT, session_->connection()->encryption_level());
+
+ // Calling SetMaxPushId() for the first time after encryption is established
+ // results in sending a MAX_PUSH_ID frame.
+ EXPECT_CALL(debug_visitor, OnMaxPushIdFrameSent(_))
+ .WillOnce(Invoke(
+ [](const MaxPushIdFrame& frame) { EXPECT_EQ(5u, frame.push_id); }));
+ session_->SetMaxPushId(5);
+ testing::Mock::VerifyAndClearExpectations(&debug_visitor);
+
+ // Another SetMaxPushId() call with the same value does not trigger sending
+ // another MAX_PUSH_ID frame.
+ session_->SetMaxPushId(5);
+
+ // Calling SetMaxPushId() with a different value results in sending another
+ // MAX_PUSH_ID frame.
+ EXPECT_CALL(debug_visitor, OnMaxPushIdFrameSent(_))
+ .WillOnce(Invoke(
+ [](const MaxPushIdFrame& frame) { EXPECT_EQ(10u, frame.push_id); }));
+ session_->SetMaxPushId(10);
+ testing::Mock::VerifyAndClearExpectations(&debug_visitor);
+
+ QuicConfig config = DefaultQuicConfig();
+ crypto_test_utils::HandshakeWithFakeServer(
+ &config, server_crypto_config_.get(), &helper_, &alarm_factory_,
+ connection_, crypto_stream_, AlpnForVersion(connection_->version()));
+
+ EXPECT_TRUE(session_->IsEncryptionEstablished());
+ EXPECT_TRUE(session_->OneRttKeysAvailable());
+ EXPECT_EQ(ENCRYPTION_FORWARD_SECURE,
+ session_->connection()->encryption_level());
+ EXPECT_TRUE(session_->GetCryptoStream()->IsResumption());
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/core/http/quic_spdy_session.cc b/quic/core/http/quic_spdy_session.cc
index cb6a7f6..386fb8a 100644
--- a/quic/core/http/quic_spdy_session.cc
+++ b/quic/core/http/quic_spdy_session.cc
@@ -739,7 +739,6 @@
if (perspective() == Perspective::IS_CLIENT && max_push_id_.has_value() &&
!http3_max_push_id_sent_) {
SendMaxPushId();
- http3_max_push_id_sent_ = true;
}
}
@@ -1241,7 +1240,7 @@
}
max_push_id_ = max_push_id;
- if (OneRttKeysAvailable()) {
+ if (IsEncryptionEstablished()) {
SendMaxPushId();
}
}
@@ -1284,6 +1283,7 @@
DCHECK_EQ(Perspective::IS_CLIENT, perspective());
send_control_stream_->SendMaxPushIdFrame(max_push_id_.value());
+ http3_max_push_id_sent_ = true;
}
void QuicSpdySession::EnableServerPush() {
diff --git a/quic/core/http/quic_spdy_session.h b/quic/core/http/quic_spdy_session.h
index 069cceb..013bfc3 100644
--- a/quic/core/http/quic_spdy_session.h
+++ b/quic/core/http/quic_spdy_session.h
@@ -466,8 +466,11 @@
void CloseConnectionOnDuplicateHttp3UnidirectionalStreams(
quiche::QuicheStringPiece type);
- // Sends any data which should be sent at the start of a connection,
- // including the initial SETTINGS frame, etc.
+ // Sends any data which should be sent at the start of a connection, including
+ // the initial SETTINGS frame, and (when IETF QUIC is used) also a MAX_PUSH_ID
+ // frame if SetMaxPushId() had been called before encryption was established.
+ // When using 0-RTT, this method is called twice: once when encryption is
+ // established, and again when 1-RTT keys are available.
void SendInitialData();
// Send a MAX_PUSH_ID frame. Used in IETF QUIC only.
@@ -534,12 +537,17 @@
// Server push is enabled for a client by calling SetMaxPushId().
bool ietf_server_push_enabled_;
- // Used in IETF QUIC only. Unset until a MAX_PUSH_ID frame is received/sent.
- // For a server, the push ID in the most recently received MAX_PUSH_ID frame.
- // For a client before 1-RTT keys are available, the push ID to be sent in the
- // initial MAX_PUSH_ID frame.
- // For a client after 1-RTT keys are available, the push ID in the most
- // recently sent MAX_PUSH_ID frame.
+ // Used in IETF QUIC only.
+ // For a server:
+ // the push ID in the most recently received MAX_PUSH_ID frame,
+ // or unset if no MAX_PUSH_ID frame has been received.
+ // For a client:
+ // unset until SetMaxPushId() is called;
+ // before encryption is established, the push ID to be sent in the initial
+ // MAX_PUSH_ID frame;
+ // after encryption is established, the push ID in the most recently sent
+ // MAX_PUSH_ID frame.
+ // Once set, never goes back to unset.
quiche::QuicheOptional<PushId> max_push_id_;
// An integer used for live check. The indicator is assigned a value in
@@ -555,9 +563,9 @@
// If the endpoint has sent HTTP/3 GOAWAY frame.
bool http3_goaway_sent_;
- // If SendMaxPushId() has been called from SendInitialData(). Note that a
- // MAX_PUSH_ID frame is only sent if SetMaxPushId() had been called
- // beforehand.
+ // Only used by a client, only with IETF QUIC. True if a MAX_PUSH_ID frame
+ // has been sent, in which case |max_push_id_| has the value sent in the most
+ // recent MAX_PUSH_ID frame. Once true, never goes back to false.
bool http3_max_push_id_sent_;
// Priority values received in PRIORITY_UPDATE frames for streams that are not
