QuicConnection methods call CloseConnection rather than TearDownLocalConnectionState Some of the QuicConnection methods call TearDownLocalConnectionState rather than CloseConnection. This changes then all to call CloseConnection, specifying ConnectionCloseBehavior::SILENT_CLOSE. Some tests modified to expect calls to CloseConnection (preventing test failure due to unexpected calls). Before this CL the connection closing happened via internal calls directly to TearDownLocalConnectionState; now these calls go through CloseConnection and trigger the expect. This is done in preparation for landing another CL adding IETF QUIC Connection Close functionality. QuicConnection::OnUnrecoverableError (which is an override of QuicPacketCreator::OnUnrecoverableError) called TearDownLocalConnectionState. It had to be modified to call CloseConnection. OnUnrecoverableError has a ConnectionCloseSource parameter, which CloseConnection does not take. Thus the parameter was removed from OnUnrecoverableError. This is OK since all of the calls to OnUnrecoverableError specified FROM_SELF as the source. CloseConnection uses FROM_SELF as the source (unless the error code is a stateless reject or public reset). gfe-relnote: No flag protection as it is only moving existing functionality around. PiperOrigin-RevId: 250341745 Change-Id: Iafe499eda682f3db678d0eddbf7e2e3e4367cebd
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc index d40ad73..942688a 100644 --- a/quic/core/http/quic_spdy_session_test.cc +++ b/quic/core/http/quic_spdy_session_test.cc
@@ -433,6 +433,7 @@ } TEST_P(QuicSpdySessionTestServer, IsCryptoHandshakeConfirmed) { + EXPECT_CALL(*connection_, CloseConnection(_, _, _)); EXPECT_FALSE(session_.IsCryptoHandshakeConfirmed()); CryptoHandshakeMessage message; session_.GetMutableCryptoStream()->OnHandshakeMessage(message); @@ -1020,6 +1021,7 @@ } TEST_P(QuicSpdySessionTestServer, IncreasedTimeoutAfterCryptoHandshake) { + EXPECT_CALL(*connection_, CloseConnection(_, _, _)); EXPECT_EQ(kInitialIdleTimeoutSecs + 3, QuicConnectionPeer::GetNetworkTimeout(connection_).ToSeconds()); CryptoHandshakeMessage msg;
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index db717b0..6afc62d 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -632,8 +632,8 @@ if (perspective_ == Perspective::IS_CLIENT) { const std::string error_details = "Protocol version mismatch."; QUIC_BUG << ENDPOINT << error_details; - TearDownLocalConnectionState(QUIC_INTERNAL_ERROR, error_details, - ConnectionCloseSource::FROM_SELF); + CloseConnection(QUIC_INTERNAL_ERROR, error_details, + ConnectionCloseBehavior::SILENT_CLOSE); return false; } if (no_version_negotiation_) { @@ -706,8 +706,8 @@ "Server received version negotiation packet."; QUIC_BUG << error_details; QUIC_CODE_COUNT(quic_tear_down_local_connection_on_version_negotiation); - TearDownLocalConnectionState(QUIC_INTERNAL_ERROR, error_details, - ConnectionCloseSource::FROM_SELF); + CloseConnection(QUIC_INTERNAL_ERROR, error_details, + ConnectionCloseBehavior::SILENT_CLOSE); return; } if (debug_visitor_ != nullptr) { @@ -724,9 +724,8 @@ "Server already supports client's version and should have accepted the " "connection."; QUIC_DLOG(WARNING) << error_details; - TearDownLocalConnectionState(QUIC_INVALID_VERSION_NEGOTIATION_PACKET, - error_details, - ConnectionCloseSource::FROM_SELF); + CloseConnection(QUIC_INVALID_VERSION_NEGOTIATION_PACKET, error_details, + ConnectionCloseBehavior::SILENT_CLOSE); return; } @@ -766,8 +765,8 @@ ParsedQuicVersionToString(original_version) + " and " + ParsedQuicVersionToString(version()) + " is currently unsupported."; QUIC_DLOG(WARNING) << error_details; - TearDownLocalConnectionState(QUIC_INVALID_VERSION, error_details, - ConnectionCloseSource::FROM_SELF); + CloseConnection(QUIC_INVALID_VERSION, error_details, + ConnectionCloseBehavior::SILENT_CLOSE); return; } @@ -2735,8 +2734,8 @@ QUIC_CODE_COUNT( quic_tear_down_local_connection_on_write_error_non_ietf); } - TearDownLocalConnectionState(QUIC_PACKET_WRITE_ERROR, error_details, - ConnectionCloseSource::FROM_SELF); + CloseConnection(QUIC_PACKET_WRITE_ERROR, error_details, + ConnectionCloseBehavior::SILENT_CLOSE); } } @@ -2747,7 +2746,7 @@ void QuicConnection::OnSerializedPacket(SerializedPacket* serialized_packet) { if (serialized_packet->encrypted_buffer == nullptr) { // We failed to serialize the packet, so close the connection. - // TearDownLocalConnectionState does not send close packet, so no infinite + // Specify that the close is silent, that no packet be sent, so no infinite // loop here. // TODO(ianswett): This is actually an internal error, not an // encryption failure. @@ -2758,10 +2757,9 @@ QUIC_CODE_COUNT( quic_tear_down_local_connection_on_serialized_packet_non_ietf); } - TearDownLocalConnectionState( - QUIC_ENCRYPTION_FAILURE, - "Serialized packet does not have an encrypted buffer.", - ConnectionCloseSource::FROM_SELF); + CloseConnection(QUIC_ENCRYPTION_FAILURE, + "Serialized packet does not have an encrypted buffer.", + ConnectionCloseBehavior::SILENT_CLOSE); return; } @@ -2777,8 +2775,7 @@ } void QuicConnection::OnUnrecoverableError(QuicErrorCode error, - const std::string& error_details, - ConnectionCloseSource source) { + const std::string& error_details) { // The packet creator or generator encountered an unrecoverable error: tear // down local connection state immediately. if (transport_version() > QUIC_VERSION_43) { @@ -2788,7 +2785,7 @@ QUIC_CODE_COUNT( quic_tear_down_local_connection_on_unrecoverable_error_non_ietf); } - TearDownLocalConnectionState(error, error_details, source); + CloseConnection(error, error_details, ConnectionCloseBehavior::SILENT_CLOSE); } void QuicConnection::OnCongestionChange() { @@ -3120,6 +3117,7 @@ // Regard stateless rejected connection as closed by server. source = ConnectionCloseSource::FROM_PEER; } + TearDownLocalConnectionState(error, error_details, source); }
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h index 3126e0b..9392f26 100644 --- a/quic/core/quic_connection.h +++ b/quic/core/quic_connection.h
@@ -535,8 +535,7 @@ char* GetPacketBuffer() override; void OnSerializedPacket(SerializedPacket* packet) override; void OnUnrecoverableError(QuicErrorCode error, - const std::string& error_details, - ConnectionCloseSource source) override; + const std::string& error_details) override; // QuicSentPacketManager::NetworkChangeVisitor void OnCongestionChange() override; @@ -974,7 +973,9 @@ typedef std::list<SerializedPacket> QueuedPacketList; // Notifies the visitor of the close and marks the connection as disconnected. - // Does not send a connection close frame to the peer. + // Does not send a connection close frame to the peer. It should only be + // called by CloseConnection or OnConnectionCloseFrame, OnPublicResetPacket, + // and OnAuthenticatedIetfStatelessResetPacket. void TearDownLocalConnectionState(QuicErrorCode error, const std::string& details, ConnectionCloseSource source);
diff --git a/quic/core/quic_dispatcher.cc b/quic/core/quic_dispatcher.cc index 0833a27..609c6b6 100644 --- a/quic/core/quic_dispatcher.cc +++ b/quic/core/quic_dispatcher.cc
@@ -73,8 +73,7 @@ } void OnUnrecoverableError(QuicErrorCode error, - const std::string& error_details, - ConnectionCloseSource source) override {} + const std::string& error_details) override {} void SaveStatelessRejectFrameData(QuicStringPiece reject) { struct iovec iovec;
diff --git a/quic/core/quic_packet_creator.cc b/quic/core/quic_packet_creator.cc index 9f9a68f..5a53ffd 100644 --- a/quic/core/quic_packet_creator.cc +++ b/quic/core/quic_packet_creator.cc
@@ -200,8 +200,7 @@ QUIC_BUG << error_details << " Constructed stream frame length: " << frame->stream_frame.data_length << " CHLO length: " << data_size; - delegate_->OnUnrecoverableError(QUIC_CRYPTO_CHLO_TOO_LARGE, error_details, - ConnectionCloseSource::FROM_SELF); + delegate_->OnUnrecoverableError(QUIC_CRYPTO_CHLO_TOO_LARGE, error_details); return false; } if (!AddFrame(*frame, /*save_retransmittable_frames=*/true, @@ -384,8 +383,7 @@ const std::string error_details = "Failed to SerializePacket."; QUIC_BUG << error_details; delegate_->OnUnrecoverableError(QUIC_FAILED_TO_SERIALIZE_PACKET, - error_details, - ConnectionCloseSource::FROM_SELF); + error_details); return; } @@ -897,8 +895,7 @@ QuicUtils::EncryptionLevelToString(packet_.encryption_level)); QUIC_BUG << error_details; delegate_->OnUnrecoverableError( - QUIC_ATTEMPT_TO_SEND_UNENCRYPTED_STREAM_DATA, error_details, - ConnectionCloseSource::FROM_SELF); + QUIC_ATTEMPT_TO_SEND_UNENCRYPTED_STREAM_DATA, error_details); return false; } size_t frame_len = framer_->GetSerializedFrameLength(
diff --git a/quic/core/quic_packet_creator.h b/quic/core/quic_packet_creator.h index 06acfbe..c9e31ac 100644 --- a/quic/core/quic_packet_creator.h +++ b/quic/core/quic_packet_creator.h
@@ -41,8 +41,7 @@ // Called when an unrecoverable error is encountered. virtual void OnUnrecoverableError(QuicErrorCode error, - const std::string& error_details, - ConnectionCloseSource source) = 0; + const std::string& error_details) = 0; }; // Interface which gets callbacks from the QuicPacketCreator at interesting
diff --git a/quic/core/quic_packet_creator_test.cc b/quic/core/quic_packet_creator_test.cc index 655d587..bf69563 100644 --- a/quic/core/quic_packet_creator_test.cc +++ b/quic/core/quic_packet_creator_test.cc
@@ -1402,7 +1402,7 @@ } creator_.set_encryption_level(ENCRYPTION_INITIAL); - EXPECT_CALL(delegate_, OnUnrecoverableError(_, _, _)); + EXPECT_CALL(delegate_, OnUnrecoverableError(_, _)); QuicStreamFrame stream_frame( QuicUtils::GetHeadersStreamId(client_framer_.transport_version()), /*fin=*/false, 0u, QuicStringPiece()); @@ -1418,7 +1418,7 @@ } creator_.set_encryption_level(ENCRYPTION_HANDSHAKE); - EXPECT_CALL(delegate_, OnUnrecoverableError(_, _, _)); + EXPECT_CALL(delegate_, OnUnrecoverableError(_, _)); QuicStreamFrame stream_frame( QuicUtils::GetHeadersStreamId(client_framer_.transport_version()), /*fin=*/false, 0u, QuicStringPiece()); @@ -1451,8 +1451,7 @@ MakeIOVector(QuicStringPiece(message_data->data(), message_data->length()), &iov); QuicFrame frame; - EXPECT_CALL(delegate_, - OnUnrecoverableError(QUIC_CRYPTO_CHLO_TOO_LARGE, _, _)); + EXPECT_CALL(delegate_, OnUnrecoverableError(QUIC_CRYPTO_CHLO_TOO_LARGE, _)); EXPECT_QUIC_BUG(creator_.ConsumeData(QuicUtils::GetCryptoStreamId( client_framer_.transport_version()), &iov, 1u, iov.iov_len, 0u, 0u, false,
diff --git a/quic/core/quic_packet_generator.cc b/quic/core/quic_packet_generator.cc index 6f578f5..d57aad4 100644 --- a/quic/core/quic_packet_generator.cc +++ b/quic/core/quic_packet_generator.cc
@@ -302,8 +302,7 @@ QUIC_LOG(INFO) << queued_control_frames_[0]; } delegate_->OnUnrecoverableError(QUIC_FAILED_TO_SERIALIZE_PACKET, - "Single frame cannot fit into a packet", - ConnectionCloseSource::FROM_SELF); + "Single frame cannot fit into a packet"); return; } }
diff --git a/quic/core/quic_packet_generator_test.cc b/quic/core/quic_packet_generator_test.cc index e1899ff..7212e22 100644 --- a/quic/core/quic_packet_generator_test.cc +++ b/quic/core/quic_packet_generator_test.cc
@@ -51,8 +51,7 @@ MOCK_METHOD1(PopulateStopWaitingFrame, void(QuicStopWaitingFrame*)); MOCK_METHOD0(GetPacketBuffer, char*()); MOCK_METHOD1(OnSerializedPacket, void(SerializedPacket* packet)); - MOCK_METHOD3(OnUnrecoverableError, - void(QuicErrorCode, const std::string&, ConnectionCloseSource)); + MOCK_METHOD2(OnUnrecoverableError, void(QuicErrorCode, const std::string&)); void SetCanWriteAnything() { EXPECT_CALL(*this, ShouldGeneratePacket(_, _)).WillRepeatedly(Return(true)); @@ -1452,8 +1451,7 @@ // This will not serialize any packets, because of the invalid frame. EXPECT_CALL(delegate_, - OnUnrecoverableError(QUIC_FAILED_TO_SERIALIZE_PACKET, _, - ConnectionCloseSource::FROM_SELF)); + OnUnrecoverableError(QUIC_FAILED_TO_SERIALIZE_PACKET, _)); EXPECT_QUIC_BUG(generator_.Flush(), "packet_number_length 1 is too small " "for least_unacked_delta: 1001");
diff --git a/quic/test_tools/quic_test_utils.h b/quic/test_tools/quic_test_utils.h index a889804..4f6a30a 100644 --- a/quic/test_tools/quic_test_utils.h +++ b/quic/test_tools/quic_test_utils.h
@@ -1046,10 +1046,7 @@ MOCK_METHOD0(GetPacketBuffer, char*()); MOCK_METHOD1(OnSerializedPacket, void(SerializedPacket* packet)); - MOCK_METHOD3(OnUnrecoverableError, - void(QuicErrorCode, - const std::string&, - ConnectionCloseSource source)); + MOCK_METHOD2(OnUnrecoverableError, void(QuicErrorCode, const std::string&)); }; class MockSessionNotifier : public SessionNotifierInterface {
diff --git a/quic/test_tools/simple_session_notifier_test.cc b/quic/test_tools/simple_session_notifier_test.cc index e5298eb..d2aad9f 100644 --- a/quic/test_tools/simple_session_notifier_test.cc +++ b/quic/test_tools/simple_session_notifier_test.cc
@@ -241,6 +241,7 @@ EXPECT_CALL(connection_, SendCryptoData(ENCRYPTION_INITIAL, 1024, 0)) .WillOnce(Invoke(&connection_, &MockQuicConnection::QuicConnection_SendCryptoData)); + EXPECT_CALL(connection_, CloseConnection(QUIC_PACKET_WRITE_ERROR, _, _)); producer.SaveCryptoData(ENCRYPTION_INITIAL, 0, std::string(1024, 'a')); producer.SaveCryptoData(ENCRYPTION_INITIAL, 500, std::string(524, 'a')); notifier_.WriteCryptoData(ENCRYPTION_INITIAL, 1024, 0);