Merge changes Ie1e628a3,Icb06e692 * changes: IETF CONNECTION CLOSE code is varint encoded gfe-relnote: Deprecate gfe2_reloadable_flag_quic_check_connected_before_flush.
diff --git a/quic/core/congestion_control/bandwidth_sampler.cc b/quic/core/congestion_control/bandwidth_sampler.cc index 88343b4..11a625c 100644 --- a/quic/core/congestion_control/bandwidth_sampler.cc +++ b/quic/core/congestion_control/bandwidth_sampler.cc
@@ -20,7 +20,8 @@ last_acked_packet_sent_time_(QuicTime::Zero()), last_acked_packet_ack_time_(QuicTime::Zero()), is_app_limited_(false), - connection_state_map_() {} + connection_state_map_(), + max_tracked_packets_(GetQuicFlag(FLAGS_quic_max_tracked_packet_count)) {} BandwidthSampler::~BandwidthSampler() {} @@ -55,7 +56,7 @@ if (!connection_state_map_.IsEmpty() && packet_number > - connection_state_map_.last_packet() + kMaxTrackedPackets) { + connection_state_map_.last_packet() + max_tracked_packets_) { QUIC_BUG << "BandwidthSampler in-flight packet map has exceeded maximum " "number " "of tracked packets.";
diff --git a/quic/core/congestion_control/bandwidth_sampler.h b/quic/core/congestion_control/bandwidth_sampler.h index 4de05b0..9a58cf6 100644 --- a/quic/core/congestion_control/bandwidth_sampler.h +++ b/quic/core/congestion_control/bandwidth_sampler.h
@@ -324,6 +324,9 @@ // sent, indexed by the packet number. PacketNumberIndexedQueue<ConnectionStateOnSentPacket> connection_state_map_; + // Maximum number of tracked packets. + const QuicPacketCount max_tracked_packets_; + // Handles the actual bandwidth calculations, whereas the outer method handles // retrieving and removing |sent_packet|. BandwidthSample OnPacketAcknowledgedInner(
diff --git a/quic/core/frames/quic_rst_stream_frame.h b/quic/core/frames/quic_rst_stream_frame.h index 0957614..9a9ed56 100644 --- a/quic/core/frames/quic_rst_stream_frame.h +++ b/quic/core/frames/quic_rst_stream_frame.h
@@ -39,6 +39,7 @@ QuicRstStreamErrorCode error_code; // In IETF QUIC the code is up to the app on top of quic, so is // more general than QuicRstStreamErrorCode allows. + // TODO(fkastenholz): Upgrade to uint64_t uint16_t ietf_error_code; };
diff --git a/quic/core/http/http_decoder.cc b/quic/core/http/http_decoder.cc index 1c4c083..3388b2c 100644 --- a/quic/core/http/http_decoder.cc +++ b/quic/core/http/http_decoder.cc
@@ -449,27 +449,42 @@ bool HttpDecoder::ParsePriorityFrame(QuicDataReader* reader, PriorityFrame* frame) { uint8_t flags; - bool success = reader->ReadUInt8(&flags); - DCHECK(success); + if (!reader->ReadUInt8(&flags)) { + // TODO(b/124216424): Use HTTP_MALFORMED_FRAME. + RaiseError(QUIC_INVALID_FRAME_DATA, "Unable to read PRIORITY frame flags."); + return false; + } frame->prioritized_type = static_cast<PriorityElementType>(ExtractBits(flags, 2, 6)); frame->dependency_type = static_cast<PriorityElementType>(ExtractBits(flags, 2, 4)); + // TODO(b/137662729): Update bitmask for exclusive flag. frame->exclusive = flags % 2 == 1; - // TODO(bnc): Handle partial delivery. + // TODO(b/137359636): Handle partial delivery. if (frame->prioritized_type != ROOT_OF_TREE && !reader->ReadVarInt62(&frame->prioritized_element_id)) { - RaiseError(QUIC_INTERNAL_ERROR, "Unable to read prioritized_element_id"); + // TODO(b/124216424): Use HTTP_MALFORMED_FRAME. + RaiseError(QUIC_INVALID_FRAME_DATA, + "Unable to read prioritized_element_id."); return false; } if (frame->dependency_type != ROOT_OF_TREE && !reader->ReadVarInt62(&frame->element_dependency_id)) { - RaiseError(QUIC_INTERNAL_ERROR, "Unable to read element_dependency_id"); + // TODO(b/124216424): Use HTTP_MALFORMED_FRAME. + RaiseError(QUIC_INVALID_FRAME_DATA, + "Unable to read element_dependency_id."); return false; } if (!reader->ReadUInt8(&frame->weight)) { - RaiseError(QUIC_INTERNAL_ERROR, "Unable to read priority frame weight"); + // TODO(b/124216424): Use HTTP_MALFORMED_FRAME. + RaiseError(QUIC_INVALID_FRAME_DATA, + "Unable to read priority frame weight."); + return false; + } + if (!reader->IsDoneReading()) { + // TODO(b/124216424): Use HTTP_MALFORMED_FRAME. + RaiseError(QUIC_INVALID_FRAME_DATA, "Superfluous data in priority frame."); return false; } return true;
diff --git a/quic/core/http/http_decoder_test.cc b/quic/core/http/http_decoder_test.cc index 9229db0..735f322 100644 --- a/quic/core/http/http_decoder_test.cc +++ b/quic/core/http/http_decoder_test.cc
@@ -411,6 +411,52 @@ EXPECT_EQ("", decoder_.error_detail()); } +// Regression test for https://crbug.com/981291 and https://crbug.com/981646. +TEST_F(HttpDecoderTest, CorruptPriorityFrame) { + const char* const payload1 = + "\x01" // request stream, request stream, exclusive + "\x03" // prioritized_element_id + "\x04" // element_dependency_id + "\xFF" // weight + "\xFF"; // superfluous data + const char* const payload2 = + "\xf1" // root of tree, root of tree, exclusive + "\xFF" // weight + "\xFF"; // superfluous data + struct { + const char* const payload; + size_t payload_length; + const char* const error_message; + } kTestData[] = { + {payload1, 0, "Unable to read PRIORITY frame flags."}, + {payload1, 1, "Unable to read prioritized_element_id."}, + {payload1, 2, "Unable to read element_dependency_id."}, + {payload1, 3, "Unable to read priority frame weight."}, + {payload1, 5, "Superfluous data in priority frame."}, + {payload2, 0, "Unable to read PRIORITY frame flags."}, + {payload2, 1, "Unable to read priority frame weight."}, + {payload2, 3, "Superfluous data in priority frame."}, + }; + + for (const auto& test_data : kTestData) { + std::string input; + input.push_back(2u); // type PRIORITY + input.push_back(test_data.payload_length); + size_t header_length = input.size(); + input.append(test_data.payload, test_data.payload_length); + + HttpDecoder decoder(&visitor_); + EXPECT_CALL(visitor_, OnPriorityFrameStart(Http3FrameLengths( + header_length, test_data.payload_length))); + + QuicByteCount processed_bytes = + decoder.ProcessInput(input.data(), input.size()); + EXPECT_EQ(input.size(), processed_bytes); + EXPECT_EQ(QUIC_INVALID_FRAME_DATA, decoder.error()); + EXPECT_EQ(test_data.error_message, decoder.error_detail()); + } +} + TEST_F(HttpDecoderTest, SettingsFrame) { InSequence s; std::string input(
diff --git a/quic/core/http/quic_server_session_base_test.cc b/quic/core/http/quic_server_session_base_test.cc index b521046..0041c85 100644 --- a/quic/core/http/quic_server_session_base_test.cc +++ b/quic/core/http/quic_server_session_base_test.cc
@@ -361,8 +361,8 @@ QuicStreamId stream_id = GetNthClientInitiatedBidirectionalId(0); // Open the max configured number of streams, should be no problem. for (size_t i = 0; i < kMaxStreamsForTest; ++i) { - EXPECT_TRUE(QuicServerSessionBasePeer::GetOrCreateDynamicStream( - session_.get(), stream_id)); + EXPECT_TRUE(QuicServerSessionBasePeer::GetOrCreateStream(session_.get(), + stream_id)); stream_id += QuicUtils::StreamIdDelta(connection_->transport_version()); } @@ -370,8 +370,8 @@ // Open more streams: server should accept slightly more than the limit. // Excess streams are for non-version-99 only. for (size_t i = 0; i < kMaxStreamsMinimumIncrement; ++i) { - EXPECT_TRUE(QuicServerSessionBasePeer::GetOrCreateDynamicStream( - session_.get(), stream_id)); + EXPECT_TRUE(QuicServerSessionBasePeer::GetOrCreateStream(session_.get(), + stream_id)); stream_id += QuicUtils::StreamIdDelta(connection_->transport_version()); } } @@ -390,8 +390,8 @@ EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(1); } // Even if the connection remains open, the stream creation should fail. - EXPECT_FALSE(QuicServerSessionBasePeer::GetOrCreateDynamicStream( - session_.get(), stream_id)); + EXPECT_FALSE( + QuicServerSessionBasePeer::GetOrCreateStream(session_.get(), stream_id)); } TEST_P(QuicServerSessionBaseTest, MaxAvailableBidirectionalStreams) { @@ -404,7 +404,7 @@ session_->MaxAvailableBidirectionalStreams(); EXPECT_EQ(0u, session_->GetNumOpenIncomingStreams()); - EXPECT_TRUE(QuicServerSessionBasePeer::GetOrCreateDynamicStream( + EXPECT_TRUE(QuicServerSessionBasePeer::GetOrCreateStream( session_.get(), GetNthClientInitiatedBidirectionalId(0))); // Establish available streams up to the server's limit. @@ -416,7 +416,7 @@ // This exceeds the stream limit. In versions other than 99 // this is allowed. Version 99 hews to the IETF spec and does // not allow it. - EXPECT_TRUE(QuicServerSessionBasePeer::GetOrCreateDynamicStream( + EXPECT_TRUE(QuicServerSessionBasePeer::GetOrCreateStream( session_.get(), kLimitingStreamId)); // A further available stream will result in connection close. EXPECT_CALL(*connection_, @@ -428,14 +428,14 @@ // This forces stream kLimitingStreamId + 2 to become available, which // violates the quota. - EXPECT_FALSE(QuicServerSessionBasePeer::GetOrCreateDynamicStream( + EXPECT_FALSE(QuicServerSessionBasePeer::GetOrCreateStream( session_.get(), kLimitingStreamId + 2 * next_id)); } TEST_P(QuicServerSessionBaseTest, GetEvenIncomingError) { // Incoming streams on the server session must be odd. EXPECT_CALL(*connection_, CloseConnection(QUIC_INVALID_STREAM_ID, _, _)); - EXPECT_EQ(nullptr, QuicServerSessionBasePeer::GetOrCreateDynamicStream( + EXPECT_EQ(nullptr, QuicServerSessionBasePeer::GetOrCreateStream( session_.get(), session_->next_outgoing_unidirectional_stream_id())); } @@ -448,7 +448,7 @@ // Don't create new streams if the connection is disconnected. QuicConnectionPeer::TearDownLocalConnectionState(connection_); - EXPECT_QUIC_BUG(QuicServerSessionBasePeer::GetOrCreateDynamicStream( + EXPECT_QUIC_BUG(QuicServerSessionBasePeer::GetOrCreateStream( session_.get(), GetNthClientInitiatedBidirectionalId(0)), "ShouldCreateIncomingStream called when disconnected"); }
diff --git a/quic/core/http/quic_spdy_client_session_base.cc b/quic/core/http/quic_spdy_client_session_base.cc index ba5a9cc..f27411a 100644 --- a/quic/core/http/quic_spdy_client_session_base.cc +++ b/quic/core/http/quic_spdy_client_session_base.cc
@@ -24,7 +24,8 @@ : QuicSpdySession(connection, nullptr, config, supported_versions), push_promise_index_(push_promise_index), largest_promised_stream_id_( - QuicUtils::GetInvalidStreamId(connection->transport_version())) {} + QuicUtils::GetInvalidStreamId(connection->transport_version())), + max_allowed_push_id_(0) {} QuicSpdyClientSessionBase::~QuicSpdyClientSessionBase() { // all promised streams for this session @@ -88,6 +89,14 @@ ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET); return; } + + if (VersionHasIetfQuicFrames(connection()->transport_version()) && + promised_stream_id > max_allowed_push_id()) { + connection()->CloseConnection( + QUIC_INVALID_STREAM_ID, + "Received push stream id higher than MAX_PUSH_ID.", + ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET); + } largest_promised_stream_id_ = promised_stream_id; QuicSpdyStream* stream = GetSpdyDataStream(stream_id); @@ -209,4 +218,9 @@ return !HasActiveRequestStreams() && promised_by_id_.empty(); } +void QuicSpdyClientSessionBase::set_max_allowed_push_id( + QuicStreamId max_allowed_push_id) { + max_allowed_push_id_ = max_allowed_push_id; +} + } // namespace quic
diff --git a/quic/core/http/quic_spdy_client_session_base.h b/quic/core/http/quic_spdy_client_session_base.h index aec5e75..98b8589 100644 --- a/quic/core/http/quic_spdy_client_session_base.h +++ b/quic/core/http/quic_spdy_client_session_base.h
@@ -111,6 +111,10 @@ // Returns true if there are no active requests and no promised streams. bool ShouldReleaseHeadersStreamSequencerBuffer() override; + void set_max_allowed_push_id(QuicStreamId max_allowed_push_id); + + QuicStreamId max_allowed_push_id() { return max_allowed_push_id_; } + size_t get_max_promises() const { return max_open_incoming_unidirectional_streams() * kMaxPromisedStreamsMultiplier; @@ -134,6 +138,7 @@ QuicClientPushPromiseIndex* push_promise_index_; QuicPromisedByIdMap promised_by_id_; QuicStreamId largest_promised_stream_id_; + QuicStreamId max_allowed_push_id_; }; } // namespace quic
diff --git a/quic/core/http/quic_spdy_client_session_test.cc b/quic/core/http/quic_spdy_client_session_test.cc index 716ae42..dfa3031 100644 --- a/quic/core/http/quic_spdy_client_session_test.cc +++ b/quic/core/http/quic_spdy_client_session_test.cc
@@ -610,6 +610,38 @@ QuicHeaderList()); } +TEST_P(QuicSpdyClientSessionTest, PushPromiseStreamIdTooHigh) { + // Initialize crypto before the client session will create a stream. + CompleteCryptoHandshake(); + QuicStreamId stream_id = + QuicSessionPeer::GetNextOutgoingBidirectionalStreamId(session_.get()); + QuicSessionPeer::ActivateStream( + session_.get(), QuicMakeUnique<QuicSpdyClientStream>( + stream_id, session_.get(), BIDIRECTIONAL)); + + session_->set_max_allowed_push_id(GetNthServerInitiatedUnidirectionalStreamId( + connection_->transport_version(), 10)); + if (VersionHasIetfQuicFrames(connection_->transport_version())) { + // TODO(b/136295430) Use PushId to represent Push IDs instead of + // QuicStreamId. + EXPECT_CALL( + *connection_, + CloseConnection(QUIC_INVALID_STREAM_ID, + "Received push stream id higher than MAX_PUSH_ID.", _)); + } + auto promise_id = GetNthServerInitiatedUnidirectionalStreamId( + connection_->transport_version(), 11); + auto headers = QuicHeaderList(); + headers.OnHeaderBlockStart(); + headers.OnHeader(":path", "/bar"); + headers.OnHeader(":authority", "www.google.com"); + headers.OnHeader(":version", "HTTP/1.1"); + headers.OnHeader(":method", "GET"); + headers.OnHeader(":scheme", "https"); + headers.OnHeaderBlockEnd(0, 0); + session_->OnPromiseHeaderList(stream_id, promise_id, 0, headers); +} + TEST_P(QuicSpdyClientSessionTest, PushPromiseOnPromiseHeadersAlreadyClosed) { // Initialize crypto before the client session will create a stream. CompleteCryptoHandshake();
diff --git a/quic/core/http/quic_spdy_session.cc b/quic/core/http/quic_spdy_session.cc index 3fe1f5c..4a8dad7 100644 --- a/quic/core/http/quic_spdy_session.cc +++ b/quic/core/http/quic_spdy_session.cc
@@ -375,12 +375,12 @@ kDefaultQpackMaxDynamicTableCapacity); } - headers_stream_ = QuicMakeUnique<QuicHeadersStream>((this)); + auto headers_stream = QuicMakeUnique<QuicHeadersStream>((this)); DCHECK_EQ(QuicUtils::GetHeadersStreamId(connection()->transport_version()), - headers_stream_->id()); + headers_stream->id()); - unowned_headers_stream_ = headers_stream_.get(); - RegisterStaticStream(std::move(headers_stream_), + headers_stream_ = headers_stream.get(); + RegisterStaticStream(std::move(headers_stream), /*stream_already_counted = */ false); if (VersionHasStreamType(connection()->transport_version())) { @@ -559,7 +559,14 @@ QuicSpdyStream* QuicSpdySession::GetSpdyDataStream( const QuicStreamId stream_id) { - QuicStream* stream = GetOrCreateDynamicStream(stream_id); + QuicStream* stream = nullptr; + if (GetQuicReloadableFlag(quic_inline_getorcreatedynamicstream) && + GetQuicReloadableFlag(quic_handle_staticness_for_spdy_stream)) { + QUIC_RELOADABLE_FLAG_COUNT(quic_inline_getorcreatedynamicstream); + stream = GetOrCreateStream(stream_id); + } else { + stream = GetOrCreateDynamicStream(stream_id); + } if (GetQuicReloadableFlag(quic_handle_staticness_for_spdy_stream) && stream && stream->is_static()) { QUIC_RELOADABLE_FLAG_COUNT(quic_handle_staticness_for_spdy_stream);
diff --git a/quic/core/http/quic_spdy_session.h b/quic/core/http/quic_spdy_session.h index 0a009bf..1acdbd5 100644 --- a/quic/core/http/quic_spdy_session.h +++ b/quic/core/http/quic_spdy_session.h
@@ -147,11 +147,9 @@ QpackEncoder* qpack_encoder(); QpackDecoder* qpack_decoder(); - QuicHeadersStream* headers_stream() { return unowned_headers_stream_; } + QuicHeadersStream* headers_stream() { return headers_stream_; } - const QuicHeadersStream* headers_stream() const { - return unowned_headers_stream_; - } + const QuicHeadersStream* headers_stream() const { return headers_stream_; } bool server_push_enabled() const { return server_push_enabled_; } @@ -285,15 +283,8 @@ std::unique_ptr<QpackEncoder> qpack_encoder_; std::unique_ptr<QpackDecoder> qpack_decoder_; - // TODO(123528590): Remove this member. - std::unique_ptr<QuicHeadersStream> headers_stream_; - - // Unowned headers stream pointer that points to the stream - // in dynamic_stream_map. - // TODO(renjietang): Merge this with headers_stream_ and clean up other - // static_stream_map logic when flag eliminate_static_stream_map - // is deprecated. - QuicHeadersStream* unowned_headers_stream_; + // Pointer to the header stream in stream_map_. + QuicHeadersStream* headers_stream_; // HTTP/3 control streams. They are owned by QuicSession inside dynamic // stream map, and can be accessed by those unowned pointers below.
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc index 4b450e9..36fd20c 100644 --- a/quic/core/http/quic_spdy_session_test.cc +++ b/quic/core/http/quic_spdy_session_test.cc
@@ -212,8 +212,8 @@ return QuicSession::IsClosedStream(id); } - QuicStream* GetOrCreateDynamicStream(QuicStreamId stream_id) { - return QuicSpdySession::GetOrCreateDynamicStream(stream_id); + QuicStream* GetOrCreateStream(QuicStreamId stream_id) { + return QuicSpdySession::GetOrCreateStream(stream_id); } QuicConsumedData WritevData(QuicStream* stream, @@ -426,16 +426,16 @@ } TEST_P(QuicSpdySessionTestServer, AvailableStreams) { - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthClientInitiatedBidirectionalId(2)) != nullptr); // Both client initiated streams with smaller stream IDs are available. EXPECT_TRUE(QuicSessionPeer::IsStreamAvailable( &session_, GetNthClientInitiatedBidirectionalId(0))); EXPECT_TRUE(QuicSessionPeer::IsStreamAvailable( &session_, GetNthClientInitiatedBidirectionalId(1))); - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthClientInitiatedBidirectionalId(1)) != nullptr); - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthClientInitiatedBidirectionalId(0)) != nullptr); } @@ -455,15 +455,15 @@ TEST_P(QuicSpdySessionTestServer, IsClosedStreamPeerCreated) { QuicStreamId stream_id1 = GetNthClientInitiatedBidirectionalId(0); QuicStreamId stream_id2 = GetNthClientInitiatedBidirectionalId(1); - session_.GetOrCreateDynamicStream(stream_id1); - session_.GetOrCreateDynamicStream(stream_id2); + session_.GetOrCreateStream(stream_id1); + session_.GetOrCreateStream(stream_id2); CheckClosedStreams(); CloseStream(stream_id1); CheckClosedStreams(); CloseStream(stream_id2); // Create a stream, and make another available. - QuicStream* stream3 = session_.GetOrCreateDynamicStream(stream_id2 + 4); + QuicStream* stream3 = session_.GetOrCreateStream(stream_id2 + 4); CheckClosedStreams(); // Close one, but make sure the other is still not closed CloseStream(stream3->id()); @@ -484,13 +484,13 @@ headers_stream_offset, Perspective::IS_CLIENT, // Client initates stream, allocs stream id. /*bidirectional=*/true); - EXPECT_NE(nullptr, session_.GetOrCreateDynamicStream(stream_id)); + EXPECT_NE(nullptr, session_.GetOrCreateStream(stream_id)); stream_id = StreamCountToId( QuicSessionPeer::v99_streamid_manager(&session_) ->actual_max_allowed_incoming_unidirectional_streams(), Perspective::IS_CLIENT, /*bidirectional=*/false); - EXPECT_NE(nullptr, session_.GetOrCreateDynamicStream(stream_id)); + EXPECT_NE(nullptr, session_.GetOrCreateStream(stream_id)); EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(2); // Get the (max allowed stream ID)++. These should all fail. stream_id = StreamCountToId( @@ -499,7 +499,7 @@ 1 - headers_stream_offset, Perspective::IS_CLIENT, /*bidirectional=*/true); - EXPECT_EQ(nullptr, session_.GetOrCreateDynamicStream(stream_id)); + EXPECT_EQ(nullptr, session_.GetOrCreateStream(stream_id)); stream_id = StreamCountToId( QuicSessionPeer::v99_streamid_manager(&session_) @@ -507,14 +507,14 @@ 1, Perspective::IS_CLIENT, /*bidirectional=*/false); - EXPECT_EQ(nullptr, session_.GetOrCreateDynamicStream(stream_id)); + EXPECT_EQ(nullptr, session_.GetOrCreateStream(stream_id)); } else { QuicStreamId stream_id = GetNthClientInitiatedBidirectionalId(0); - session_.GetOrCreateDynamicStream(stream_id); + session_.GetOrCreateStream(stream_id); EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0); EXPECT_NE( nullptr, - session_.GetOrCreateDynamicStream( + session_.GetOrCreateStream( stream_id + IdDelta() * (session_.max_open_incoming_bidirectional_streams() - 1))); @@ -524,7 +524,7 @@ TEST_P(QuicSpdySessionTestServer, TooManyAvailableStreams) { QuicStreamId stream_id1 = GetNthClientInitiatedBidirectionalId(0); QuicStreamId stream_id2; - EXPECT_NE(nullptr, session_.GetOrCreateDynamicStream(stream_id1)); + EXPECT_NE(nullptr, session_.GetOrCreateStream(stream_id1)); // A stream ID which is too large to create. stream_id2 = GetNthClientInitiatedBidirectionalId( 2 * session_.MaxAvailableBidirectionalStreams() + 4); @@ -534,7 +534,7 @@ EXPECT_CALL(*connection_, CloseConnection(QUIC_TOO_MANY_AVAILABLE_STREAMS, _, _)); } - EXPECT_EQ(nullptr, session_.GetOrCreateDynamicStream(stream_id2)); + EXPECT_EQ(nullptr, session_.GetOrCreateStream(stream_id2)); } TEST_P(QuicSpdySessionTestServer, ManyAvailableStreams) { @@ -547,12 +547,12 @@ } QuicStreamId stream_id = GetNthClientInitiatedBidirectionalId(0); // Create one stream. - session_.GetOrCreateDynamicStream(stream_id); + session_.GetOrCreateStream(stream_id); EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0); // Stream count is 200, GetNth... starts counting at 0, so the 200'th stream // is 199. BUT actually we need to do 198 because the crypto stream (Stream // ID 0) has not been registered, but GetNth... assumes that it has. - EXPECT_NE(nullptr, session_.GetOrCreateDynamicStream( + EXPECT_NE(nullptr, session_.GetOrCreateStream( GetNthClientInitiatedBidirectionalId(198))); } @@ -928,9 +928,9 @@ } TestHeadersStream* headers_stream; - QuicSpdySessionPeer::SetUnownedHeadersStream(&session_, nullptr); + QuicSpdySessionPeer::SetHeadersStream(&session_, nullptr); headers_stream = new TestHeadersStream(&session_); - QuicSpdySessionPeer::SetUnownedHeadersStream(&session_, headers_stream); + QuicSpdySessionPeer::SetHeadersStream(&session_, headers_stream); session_.MarkConnectionLevelWriteBlocked( QuicUtils::GetHeadersStreamId(connection_->transport_version())); EXPECT_CALL(*headers_stream, OnCanWrite()); @@ -964,7 +964,7 @@ EXPECT_CALL(*connection_, OnStreamReset(kTestStreamId, QUIC_STREAM_PEER_GOING_AWAY)) .Times(0); - EXPECT_TRUE(session_.GetOrCreateDynamicStream(kTestStreamId)); + EXPECT_TRUE(session_.GetOrCreateStream(kTestStreamId)); } TEST_P(QuicSpdySessionTestServer, DoNotSendGoAwayTwice) { @@ -1662,16 +1662,16 @@ } TEST_P(QuicSpdySessionTestClient, AvailableStreamsClient) { - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthServerInitiatedBidirectionalId(2)) != nullptr); // Both server initiated streams with smaller stream IDs should be available. EXPECT_TRUE(QuicSessionPeer::IsStreamAvailable( &session_, GetNthServerInitiatedBidirectionalId(0))); EXPECT_TRUE(QuicSessionPeer::IsStreamAvailable( &session_, GetNthServerInitiatedBidirectionalId(1))); - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthServerInitiatedBidirectionalId(0)) != nullptr); - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthServerInitiatedBidirectionalId(1)) != nullptr); // And client initiated stream ID should be not available. EXPECT_FALSE(QuicSessionPeer::IsStreamAvailable( @@ -1741,9 +1741,9 @@ TEST_P(QuicSpdySessionTestClient, WritePriority) { TestHeadersStream* headers_stream; - QuicSpdySessionPeer::SetUnownedHeadersStream(&session_, nullptr); + QuicSpdySessionPeer::SetHeadersStream(&session_, nullptr); headers_stream = new TestHeadersStream(&session_); - QuicSpdySessionPeer::SetUnownedHeadersStream(&session_, headers_stream); + QuicSpdySessionPeer::SetHeadersStream(&session_, headers_stream); // Make packet writer blocked so |headers_stream| will buffer its write data. MockPacketWriter* writer = static_cast<MockPacketWriter*>( @@ -1789,7 +1789,7 @@ QuicStreamFrame data1(stream_id1, false, 0, QuicStringPiece(data)); session_.OnStreamFrame(data1); EXPECT_EQ(1u, session_.GetNumOpenIncomingStreams()); - QuicStream* stream = session_.GetOrCreateDynamicStream(stream_id1); + QuicStream* stream = session_.GetOrCreateStream(stream_id1); EXPECT_EQ(1u, stream->flow_controller()->bytes_consumed()); EXPECT_EQ(1u, session_.flow_controller()->bytes_consumed()); @@ -1800,7 +1800,7 @@ QuicStreamFrame data2(stream_id2, false, 0, QuicStringPiece(data)); session_.OnStreamFrame(data2); EXPECT_EQ(2u, session_.GetNumOpenIncomingStreams()); - stream = session_.GetOrCreateDynamicStream(stream_id2); + stream = session_.GetOrCreateStream(stream_id2); EXPECT_EQ(4u, stream->flow_controller()->bytes_consumed()); EXPECT_EQ(5u, session_.flow_controller()->bytes_consumed()); }
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 1ad2ddb..ecab751 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -244,7 +244,7 @@ last_decrypted_packet_level_(ENCRYPTION_INITIAL), should_last_packet_instigate_acks_(false), max_undecryptable_packets_(0), - max_tracked_packets_(kMaxTrackedPackets), + max_tracked_packets_(GetQuicFlag(FLAGS_quic_max_tracked_packet_count)), pending_version_negotiation_packet_(false), send_ietf_version_negotiation_packet_(false), idle_timeout_connection_close_behavior_( @@ -963,7 +963,8 @@ return true; } const AckResult ack_result = sent_packet_manager_.OnAckFrameEnd( - time_of_last_received_packet_, last_decrypted_packet_level_); + time_of_last_received_packet_, last_header_.packet_number, + last_decrypted_packet_level_); if (ack_result != PACKETS_NEWLY_ACKED && ack_result != NO_PACKETS_NEWLY_ACKED) { // Error occurred (e.g., this ACK tries to ack packets in wrong packet @@ -2422,6 +2423,13 @@ void QuicConnection::OnRetransmissionTimeout() { DCHECK(!sent_packet_manager_.unacked_packets().empty()); + const QuicPacketNumber previous_created_packet_number = + packet_generator_.packet_number(); + const size_t previous_crypto_retransmit_count = + stats_.crypto_retransmit_count; + const size_t previous_loss_timeout_count = stats_.loss_timeout_count; + const size_t previous_tlp_count = stats_.tlp_count; + const size_t pervious_rto_count = stats_.rto_count; if (close_connection_after_five_rtos_ && sent_packet_manager_.GetConsecutiveRtoCount() >= 4) { // Close on the 5th consecutive RTO, so after 4 previous RTOs have occurred. @@ -2446,6 +2454,29 @@ WriteIfNotBlocked(); } + if (sent_packet_manager_.fix_rto_retransmission()) { + // Making sure at least one packet is created when retransmission timer + // fires in TLP, RTO or HANDSHAKE mode. It is possible that loss algorithm + // invokes timer based loss but the packet does not need to be + // retransmitted. + QUIC_BUG_IF(stats_.loss_timeout_count == previous_loss_timeout_count && + packet_generator_.packet_number() == + previous_created_packet_number) + << "previous_crypto_retransmit_count: " + << previous_crypto_retransmit_count + << ", crypto_retransmit_count: " << stats_.crypto_retransmit_count + << ", previous_loss_timeout_count: " << previous_loss_timeout_count + << ", loss_timeout_count: " << stats_.loss_timeout_count + << ", previous_tlp_count: " << previous_tlp_count + << ", tlp_count: " << stats_.tlp_count + << ", pervious_rto_count: " << pervious_rto_count + << ", rto_count: " << stats_.rto_count + << ", previous_created_packet_number: " + << previous_created_packet_number + << ", packet_number: " << packet_generator_.packet_number() + << ", session has data to write: " << visitor_->WillingAndAbleToWrite(); + } + // Ensure the retransmission alarm is always set if there are unacked packets // and nothing waiting to be sent. // This happens if the loss algorithm invokes a timer based loss, but the
diff --git a/quic/core/quic_connection_id.cc b/quic/core/quic_connection_id.cc index a74e3b1..7b4f41b 100644 --- a/quic/core/quic_connection_id.cc +++ b/quic/core/quic_connection_id.cc
@@ -4,11 +4,14 @@ #include "net/third_party/quiche/src/quic/core/quic_connection_id.h" +#include <cstddef> #include <cstdint> #include <cstring> #include <iomanip> #include <string> +#include "third_party/boringssl/src/include/openssl/siphash.h" +#include "net/third_party/quiche/src/quic/core/crypto/quic_random.h" #include "net/third_party/quiche/src/quic/core/quic_types.h" #include "net/third_party/quiche/src/quic/platform/api/quic_bug_tracker.h" #include "net/third_party/quiche/src/quic/platform/api/quic_endian.h" @@ -19,6 +22,34 @@ namespace quic { +namespace { + +// QuicConnectionIdHasher can be used to generate a stable connection ID hash +// function that will return the same value for two equal connection IDs for +// the duration of process lifetime. It is meant to be used as input to data +// structures that do not outlast process lifetime. A new key is generated once +// per process to prevent attackers from crafting connection IDs in such a way +// that they always land in the same hash bucket. +class QuicConnectionIdHasher { + public: + inline QuicConnectionIdHasher() + : QuicConnectionIdHasher(QuicRandom::GetInstance()) {} + + inline QuicConnectionIdHasher(QuicRandom* random) { + random->RandBytes(&sip_hash_key_, sizeof(sip_hash_key_)); + } + + inline size_t Hash(const char* input, size_t input_len) const { + return static_cast<size_t>(SIPHASH_24( + sip_hash_key_, reinterpret_cast<const uint8_t*>(input), input_len)); + } + + private: + uint64_t sip_hash_key_[2]; +}; + +} // namespace + QuicConnectionId::QuicConnectionId() : QuicConnectionId(nullptr, 0) {} QuicConnectionId::QuicConnectionId(const char* data, uint8_t length) { @@ -127,14 +158,20 @@ } size_t QuicConnectionId::Hash() const { - uint64_t data_bytes[3] = {0, 0, 0}; - static_assert(sizeof(data_bytes) >= kQuicMaxConnectionIdLength, - "kQuicMaxConnectionIdLength changed"); - memcpy(data_bytes, data(), length_); - // This Hash function is designed to return the same value as the host byte - // order representation when the connection ID length is 64 bits. - return QuicEndian::NetToHost64(kQuicDefaultConnectionIdLength ^ length_ ^ - data_bytes[0] ^ data_bytes[1] ^ data_bytes[2]); + if (!GetQuicRestartFlag(quic_connection_id_use_siphash)) { + uint64_t data_bytes[3] = {0, 0, 0}; + static_assert(sizeof(data_bytes) >= kQuicMaxConnectionIdLength, + "kQuicMaxConnectionIdLength changed"); + memcpy(data_bytes, data(), length_); + // This Hash function is designed to return the same value as the host byte + // order representation when the connection ID length is 64 bits. + return QuicEndian::NetToHost64(kQuicDefaultConnectionIdLength ^ length_ ^ + data_bytes[0] ^ data_bytes[1] ^ + data_bytes[2]); + } + QUIC_RESTART_FLAG_COUNT(quic_connection_id_use_siphash); + static const QuicConnectionIdHasher hasher = QuicConnectionIdHasher(); + return hasher.Hash(data(), length_); } std::string QuicConnectionId::ToString() const {
diff --git a/quic/core/quic_connection_id.h b/quic/core/quic_connection_id.h index 4b76f31..6b1b0bc 100644 --- a/quic/core/quic_connection_id.h +++ b/quic/core/quic_connection_id.h
@@ -72,6 +72,11 @@ bool IsEmpty() const; // Hash() is required to use connection IDs as keys in hash tables. + // During the lifetime of a process, the output of Hash() is guaranteed to be + // the same for connection IDs that are equal to one another. Note however + // that this property is not guaranteed across process lifetimes. This makes + // Hash() suitable for data structures such as hash tables but not for sending + // a hash over the network. size_t Hash() const; // Generates an ASCII string that represents @@ -112,6 +117,11 @@ QUIC_EXPORT_PRIVATE QuicConnectionId EmptyQuicConnectionId(); // QuicConnectionIdHash can be passed as hash argument to hash tables. +// During the lifetime of a process, the output of QuicConnectionIdHash is +// guaranteed to be the same for connection IDs that are equal to one another. +// Note however that this property is not guaranteed across process lifetimes. +// This makes QuicConnectionIdHash suitable for data structures such as hash +// tables but not for sending a hash over the network. class QuicConnectionIdHash { public: size_t operator()(QuicConnectionId const& connection_id) const noexcept {
diff --git a/quic/core/quic_connection_id_test.cc b/quic/core/quic_connection_id_test.cc index 1d290fc..64afaf8 100644 --- a/quic/core/quic_connection_id_test.cc +++ b/quic/core/quic_connection_id_test.cc
@@ -89,6 +89,17 @@ EXPECT_NE(connection_id64_1.Hash(), connection_id64_2.Hash()); EXPECT_NE(connection_id64_1.Hash(), connection_id64_3.Hash()); EXPECT_NE(connection_id64_2.Hash(), connection_id64_3.Hash()); + + // Verify that any two all-zero connection IDs of different lengths never + // have the same hash. + const char connection_id_bytes[kQuicMaxConnectionIdLength] = {}; + for (uint8_t i = 0; i < kQuicMaxConnectionIdLength - 1; ++i) { + QuicConnectionId connection_id_i(connection_id_bytes, i); + for (uint8_t j = i + 1; j < kQuicMaxConnectionIdLength; ++j) { + QuicConnectionId connection_id_j(connection_id_bytes, j); + EXPECT_NE(connection_id_i.Hash(), connection_id_j.Hash()); + } + } } TEST_F(QuicConnectionIdTest, AssignAndCopy) {
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index f04d142..6b5b943 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -4065,6 +4065,54 @@ EXPECT_EQ(QuicPacketNumber(1u), stop_waiting()->least_unacked); } +// Regression test of b/133771183. +TEST_P(QuicConnectionTest, RtoWithNoDataToRetransmit) { + if (!connection_.session_decides_what_to_write()) { + return; + } + connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); + EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_)); + connection_.SetMaxTailLossProbes(0); + + SendStreamDataToPeer(3, "foo", 0, NO_FIN, nullptr); + // Connection is cwnd limited. + CongestionBlockWrites(); + // Stream gets reset. + SendRstStream(3, QUIC_ERROR_PROCESSING_STREAM, 3); + // Simulate the retransmission alarm firing. + clock_.AdvanceTime(DefaultRetransmissionTime()); + // RTO fires, but there is no packet to be RTOed. + if (GetQuicReloadableFlag(quic_fix_rto_retransmission)) { + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1); + } else { + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(0); + } + connection_.GetRetransmissionAlarm()->Fire(); + if (GetQuicReloadableFlag(quic_fix_rto_retransmission)) { + EXPECT_EQ(1u, writer_->rst_stream_frames().size()); + } + + EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(40); + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(20); + if (GetQuicReloadableFlag(quic_fix_rto_retransmission)) { + EXPECT_CALL(visitor_, WillingAndAbleToWrite()) + .WillRepeatedly(Return(false)); + } else { + EXPECT_CALL(visitor_, WillingAndAbleToWrite()).WillRepeatedly(Return(true)); + } + if (GetQuicReloadableFlag(quic_fix_rto_retransmission)) { + EXPECT_CALL(visitor_, OnAckNeedsRetransmittableFrame()).Times(1); + } else { + // Since there is a buffered RST_STREAM, no retransmittable frame is bundled + // with ACKs. + EXPECT_CALL(visitor_, OnAckNeedsRetransmittableFrame()).Times(0); + } + // Receives packets 1 - 40. + for (size_t i = 1; i <= 40; ++i) { + ProcessDataPacket(i); + } +} + TEST_P(QuicConnectionTest, RetransmitWithSameEncryptionLevel) { use_tagging_decrypter();
diff --git a/quic/core/quic_constants.h b/quic/core/quic_constants.h index e718602..442fb09 100644 --- a/quic/core/quic_constants.h +++ b/quic/core/quic_constants.h
@@ -87,9 +87,6 @@ // Minimum size of the CWND, in packets, when doing bandwidth resumption. const QuicPacketCount kMinCongestionWindowForBandwidthResumption = 10; -// Maximum number of tracked packets. -const QuicPacketCount kMaxTrackedPackets = 10000; - // Default size of the socket receive buffer in bytes. const QuicByteCount kDefaultSocketReceiveBuffer = 1024 * 1024;
diff --git a/quic/core/quic_flow_controller.cc b/quic/core/quic_flow_controller.cc index 3fb5d75..dbc5100 100644 --- a/quic/core/quic_flow_controller.cc +++ b/quic/core/quic_flow_controller.cc
@@ -21,6 +21,13 @@ #define ENDPOINT \ (perspective_ == Perspective::IS_SERVER ? "Server: " : "Client: ") +std::string QuicFlowController::LogLabel() { + if (is_connection_flow_controller_) { + return "connection"; + } + return QuicStrCat("stream ", id_); +} + QuicFlowController::QuicFlowController( QuicSession* session, QuicStreamId id, @@ -51,7 +58,7 @@ QuicUtils::GetInvalidStreamId( session_->connection()->transport_version()) == id_); - QUIC_DVLOG(1) << ENDPOINT << "Created flow controller for stream " << id_ + QUIC_DVLOG(1) << ENDPOINT << "Created flow controller for " << LogLabel() << ", setting initial receive window offset to: " << receive_window_offset_ << ", max receive window to: " << receive_window_size_ @@ -62,8 +69,8 @@ void QuicFlowController::AddBytesConsumed(QuicByteCount bytes_consumed) { bytes_consumed_ += bytes_consumed; - QUIC_DVLOG(1) << ENDPOINT << "Stream " << id_ << " consumed " - << bytes_consumed_ << " bytes."; + QUIC_DVLOG(1) << ENDPOINT << LogLabel() << " consumed " << bytes_consumed_ + << " bytes."; MaybeSendWindowUpdate(); } @@ -75,7 +82,7 @@ return false; } - QUIC_DVLOG(1) << ENDPOINT << "Stream " << id_ + QUIC_DVLOG(1) << ENDPOINT << LogLabel() << " highest byte offset increased from " << highest_received_byte_offset_ << " to " << new_offset; highest_received_byte_offset_ = new_offset; @@ -84,7 +91,7 @@ void QuicFlowController::AddBytesSent(QuicByteCount bytes_sent) { if (bytes_sent_ + bytes_sent > send_window_offset_) { - QUIC_BUG << ENDPOINT << "Stream " << id_ << " Trying to send an extra " + QUIC_BUG << ENDPOINT << LogLabel() << " Trying to send an extra " << bytes_sent << " bytes, when bytes_sent = " << bytes_sent_ << ", and send_window_offset_ = " << send_window_offset_; bytes_sent_ = send_window_offset_; @@ -99,13 +106,13 @@ } bytes_sent_ += bytes_sent; - QUIC_DVLOG(1) << ENDPOINT << "Stream " << id_ << " sent " << bytes_sent_ + QUIC_DVLOG(1) << ENDPOINT << LogLabel() << " sent " << bytes_sent_ << " bytes."; } bool QuicFlowController::FlowControlViolation() { if (highest_received_byte_offset_ > receive_window_offset_) { - QUIC_DLOG(INFO) << ENDPOINT << "Flow control violation on stream " << id_ + QUIC_DLOG(INFO) << ENDPOINT << "Flow control violation on " << LogLabel() << ", receive window offset: " << receive_window_offset_ << ", highest received byte offset: " << highest_received_byte_offset_; @@ -128,7 +135,7 @@ QuicTime prev = prev_window_update_time_; prev_window_update_time_ = now; if (!prev.IsInitialized()) { - QUIC_DVLOG(1) << ENDPOINT << "first window update for stream " << id_; + QUIC_DVLOG(1) << ENDPOINT << "first window update for " << LogLabel(); return; } @@ -140,7 +147,7 @@ QuicTime::Delta rtt = connection_->sent_packet_manager().GetRttStats()->smoothed_rtt(); if (rtt.IsZero()) { - QUIC_DVLOG(1) << ENDPOINT << "rtt zero for stream " << id_; + QUIC_DVLOG(1) << ENDPOINT << "rtt zero for " << LogLabel(); return; } @@ -157,7 +164,7 @@ IncreaseWindowSize(); if (receive_window_size_ > old_window) { - QUIC_DVLOG(1) << ENDPOINT << "New max window increase for stream " << id_ + QUIC_DVLOG(1) << ENDPOINT << "New max window increase for " << LogLabel() << " after " << since_last.ToMicroseconds() << " us, and RTT is " << rtt.ToMicroseconds() << "us. max wndw: " << receive_window_size_; @@ -168,7 +175,7 @@ } else { // TODO(ckrasic) - add a varz to track this (?). QUIC_LOG_FIRST_N(INFO, 1) - << ENDPOINT << "Max window at limit for stream " << id_ << " after " + << ENDPOINT << "Max window at limit for " << LogLabel() << " after " << since_last.ToMicroseconds() << " us, and RTT is " << rtt.ToMicroseconds() << "us. Limit size: " << receive_window_size_; } @@ -199,7 +206,7 @@ } if (available_window >= threshold) { - QUIC_DVLOG(1) << ENDPOINT << "Not sending WindowUpdate for stream " << id_ + QUIC_DVLOG(1) << ENDPOINT << "Not sending WindowUpdate for " << LogLabel() << ", available window: " << available_window << " >= threshold: " << threshold; return; @@ -214,7 +221,7 @@ // Update our receive window. receive_window_offset_ += (receive_window_size_ - available_window); - QUIC_DVLOG(1) << ENDPOINT << "Sending WindowUpdate frame for stream " << id_ + QUIC_DVLOG(1) << ENDPOINT << "Sending WindowUpdate frame for " << LogLabel() << ", consumed bytes: " << bytes_consumed_ << ", available window: " << available_window << ", and threshold: " << WindowUpdateThreshold() @@ -229,8 +236,7 @@ last_blocked_send_window_offset_ >= send_window_offset_) { return false; } - QUIC_DLOG(INFO) << ENDPOINT << "Stream " << id_ - << " is flow control blocked. " + QUIC_DLOG(INFO) << ENDPOINT << LogLabel() << " is flow control blocked. " << "Send window: " << SendWindowSize() << ", bytes sent: " << bytes_sent_ << ", send limit: " << send_window_offset_; @@ -250,7 +256,7 @@ return false; } - QUIC_DVLOG(1) << ENDPOINT << "UpdateSendWindowOffset for stream " << id_ + QUIC_DVLOG(1) << ENDPOINT << "UpdateSendWindowOffset for " << LogLabel() << " with new offset " << new_send_window_offset << " current offset: " << send_window_offset_ << " bytes_sent: " << bytes_sent_; @@ -286,7 +292,7 @@ void QuicFlowController::UpdateReceiveWindowSize(QuicStreamOffset size) { DCHECK_LE(size, receive_window_size_limit_); - QUIC_DVLOG(1) << ENDPOINT << "UpdateReceiveWindowSize for stream " << id_ + QUIC_DVLOG(1) << ENDPOINT << "UpdateReceiveWindowSize for " << LogLabel() << ": " << size; if (receive_window_size_ != receive_window_offset_) { QUIC_BUG << "receive_window_size_:" << receive_window_size_
diff --git a/quic/core/quic_flow_controller.h b/quic/core/quic_flow_controller.h index 4de292c..e627c8c 100644 --- a/quic/core/quic_flow_controller.h +++ b/quic/core/quic_flow_controller.h
@@ -121,6 +121,10 @@ // Double the window size as long as we haven't hit the max window size. void IncreaseWindowSize(); + // Returns "stream $ID" (where $ID is set to |id_|) or "connection" based on + // |is_connection_flow_controller_|. + std::string LogLabel(); + // The parent session/connection, used to send connection close on flow // control violation, and WINDOW_UPDATE and BLOCKED frames when appropriate. // Not owned.
diff --git a/quic/core/quic_framer.cc b/quic/core/quic_framer.cc index 5324206..1280f2e 100644 --- a/quic/core/quic_framer.cc +++ b/quic/core/quic_framer.cc
@@ -556,7 +556,8 @@ if (VersionHasIetfQuicFrames(version)) { return QuicDataWriter::GetVarInt62Len(frame.stream_id) + QuicDataWriter::GetVarInt62Len(frame.byte_offset) + - kQuicFrameTypeSize + kQuicIetfQuicErrorCodeSize; + kQuicFrameTypeSize + + QuicDataWriter::GetVarInt62Len(frame.ietf_error_code); } return kQuicFrameTypeSize + kQuicMaxStreamIdSize + kQuicMaxStreamOffsetSize + kQuicErrorCodeSize; @@ -665,7 +666,7 @@ // static size_t QuicFramer::GetStopSendingFrameSize(const QuicStopSendingFrame& frame) { return kQuicFrameTypeSize + QuicDataWriter::GetVarInt62Len(frame.stream_id) + - sizeof(QuicApplicationErrorCode); + QuicDataWriter::GetVarInt62Len(frame.application_error_code); } // static @@ -5812,7 +5813,7 @@ set_detailed_error("Writing reset-stream stream id failed."); return false; } - if (!writer->WriteUInt16(frame.ietf_error_code)) { + if (!writer->WriteVarInt62(static_cast<uint64_t>(frame.ietf_error_code))) { set_detailed_error("Writing reset-stream error code failed."); return false; } @@ -5833,10 +5834,18 @@ return false; } - if (!reader->ReadUInt16(&frame->ietf_error_code)) { + uint64_t error_code; + if (!reader->ReadVarInt62(&error_code)) { set_detailed_error("Unable to read rst stream error code."); return false; } + if (error_code > 0xffff) { + frame->ietf_error_code = 0xffff; + QUIC_DLOG(ERROR) << "Reset stream error code (" << error_code + << ") > 0xffff"; + } else { + frame->ietf_error_code = static_cast<uint16_t>(error_code); + } if (!reader->ReadVarInt62(&frame->byte_offset)) { set_detailed_error("Unable to read rst stream sent byte offset."); @@ -5853,10 +5862,20 @@ return false; } - if (!reader->ReadUInt16(&stop_sending_frame->application_error_code)) { + uint64_t error_code; + if (!reader->ReadVarInt62(&error_code)) { set_detailed_error("Unable to read stop sending application error code."); return false; } + // TODO(fkastenholz): when error codes go to uint64_t, remove this. + if (error_code > 0xffff) { + stop_sending_frame->application_error_code = 0xffff; + QUIC_DLOG(ERROR) << "Stop sending error code (" << error_code + << ") > 0xffff"; + } else { + stop_sending_frame->application_error_code = + static_cast<uint16_t>(error_code); + } return true; } @@ -5867,7 +5886,8 @@ set_detailed_error("Can not write stop sending stream id"); return false; } - if (!writer->WriteUInt16(stop_sending_frame.application_error_code)) { + if (!writer->WriteVarInt62( + static_cast<uint64_t>(stop_sending_frame.application_error_code))) { set_detailed_error("Can not write application error code"); return false; }
diff --git a/quic/core/quic_framer_test.cc b/quic/core/quic_framer_test.cc index 29ae36f..690e911 100644 --- a/quic/core/quic_framer_test.cc +++ b/quic/core/quic_framer_test.cc
@@ -4544,7 +4544,7 @@ {kVarInt62FourBytes + 0x01, 0x02, 0x03, 0x04}}, // application error code {"Unable to read rst stream error code.", - {0x00, 0x01}}, // Not varint62 encoded + {kVarInt62OneByte + 0x01}}, // Final Offset {"Unable to read rst stream sent byte offset.", {kVarInt62EightBytes + 0x3a, 0x98, 0xFE, 0xDC, 0x32, 0x10, 0x76, 0x54}} @@ -7801,8 +7801,8 @@ 0x04, // stream id kVarInt62FourBytes + 0x01, 0x02, 0x03, 0x04, - // error code (not VarInt32 encoded) - 0x00, 0x01, + // error code + kVarInt62OneByte + 0x01, // sent byte offset kVarInt62EightBytes + 0x08, 0x07, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01 }; @@ -11650,7 +11650,7 @@ {"Unable to read stop sending stream id.", {kVarInt62FourBytes + 0x01, 0x02, 0x03, 0x04}}, {"Unable to read stop sending application error code.", - {0x76, 0x54}}, + {kVarInt62FourBytes + 0x00, 0x00, 0x76, 0x54}}, }; // clang-format on @@ -11701,7 +11701,7 @@ // Stream ID kVarInt62FourBytes + 0x01, 0x02, 0x03, 0x04, // Application error code - 0xff, 0xff + kVarInt62FourBytes + 0x00, 0x00, 0xff, 0xff }; // clang-format on
diff --git a/quic/core/quic_ietf_framer_test.cc b/quic/core/quic_ietf_framer_test.cc index 9d07e49..dbbacdd 100644 --- a/quic/core/quic_ietf_framer_test.cc +++ b/quic/core/quic_ietf_framer_test.cc
@@ -457,9 +457,10 @@ // Write the frame to the packet buffer. EXPECT_TRUE(QuicFramerPeer::AppendIetfResetStreamFrame( &framer_, transmit_frame, &writer)); - // Check that the size of the serialzed frame is in the allowed range. - EXPECT_LT(3u, writer.length()); - EXPECT_GT(19u, writer.length()); + // Check that the size of the serialzed frame is in the allowed range (3 to + // 24 bytes, inclusive). + EXPECT_LT(2u, writer.length()); + EXPECT_GT(25u, writer.length()); // Now set up a reader to read in the thing in. QuicDataReader reader(packet_buffer, writer.length(), NETWORK_BYTE_ORDER);
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc index a4aa08e..e7cd1a5 100644 --- a/quic/core/quic_sent_packet_manager.cc +++ b/quic/core/quic_sent_packet_manager.cc
@@ -120,10 +120,15 @@ loss_removes_from_inflight_( GetQuicReloadableFlag(quic_loss_removes_from_inflight)), ignore_tlpr_if_no_pending_stream_data_( - GetQuicReloadableFlag(quic_ignore_tlpr_if_no_pending_stream_data)) { + GetQuicReloadableFlag(quic_ignore_tlpr_if_no_pending_stream_data)), + fix_rto_retransmission_( + GetQuicReloadableFlag(quic_fix_rto_retransmission)) { if (loss_removes_from_inflight_) { QUIC_RELOADABLE_FLAG_COUNT(quic_loss_removes_from_inflight); } + if (fix_rto_retransmission_) { + QUIC_RELOADABLE_FLAG_COUNT(quic_fix_rto_retransmission); + } SetSendAlgorithm(congestion_control_type); } @@ -276,6 +281,7 @@ } void QuicSentPacketManager::PostProcessNewlyAckedPackets( + QuicPacketNumber ack_packet_number, const QuicAckFrame& ack_frame, QuicTime ack_receive_time, bool rtt_updated, @@ -321,9 +327,9 @@ } if (debug_delegate_ != nullptr) { - debug_delegate_->OnIncomingAck(ack_frame, ack_receive_time, - LargestAcked(ack_frame), rtt_updated, - GetLeastUnacked()); + debug_delegate_->OnIncomingAck(ack_packet_number, ack_frame, + ack_receive_time, LargestAcked(ack_frame), + rtt_updated, GetLeastUnacked()); } // Remove packets below least unacked from all_packets_acked_ and // last_ack_frame_. @@ -803,7 +809,7 @@ if (session_decides_what_to_write()) { has_retransmissions = it->state != OUTSTANDING; } - if (it->in_flight && !has_retransmissions && + if (!fix_rto_retransmission_ && it->in_flight && !has_retransmissions && !unacked_packets_.HasRetransmittableFrames(*it)) { // Log only for non-retransmittable data. // Retransmittable data is marked as lost during loss detection, and will @@ -825,6 +831,13 @@ for (QuicPacketNumber retransmission : retransmissions) { MarkForRetransmission(retransmission, RTO_RETRANSMISSION); } + if (fix_rto_retransmission_ && retransmissions.empty()) { + QUIC_BUG_IF(pending_timer_transmission_count_ != 0); + // No packets to be RTO retransmitted, raise up a credit to allow + // connection to send. + QUIC_CODE_COUNT(no_packets_to_be_rto_retransmitted); + pending_timer_transmission_count_ = 1; + } } } @@ -1158,6 +1171,7 @@ AckResult QuicSentPacketManager::OnAckFrameEnd( QuicTime ack_receive_time, + QuicPacketNumber ack_packet_number, EncryptionLevel ack_decrypted_level) { QuicByteCount prior_bytes_in_flight = unacked_packets_.bytes_in_flight(); // Reverse packets_acked_ so that it is in ascending order. @@ -1218,7 +1232,8 @@ acked_packet.receive_timestamp); } const bool acked_new_packet = !packets_acked_.empty(); - PostProcessNewlyAckedPackets(last_ack_frame_, ack_receive_time, rtt_updated_, + PostProcessNewlyAckedPackets(ack_packet_number, last_ack_frame_, + ack_receive_time, rtt_updated_, prior_bytes_in_flight); return acked_new_packet ? PACKETS_NEWLY_ACKED : NO_PACKETS_NEWLY_ACKED;
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h index b90d977..b08704d 100644 --- a/quic/core/quic_sent_packet_manager.h +++ b/quic/core/quic_sent_packet_manager.h
@@ -57,7 +57,8 @@ TransmissionType /*transmission_type*/, QuicByteCount /*byte_size*/) {} - virtual void OnIncomingAck(const QuicAckFrame& /*ack_frame*/, + virtual void OnIncomingAck(QuicPacketNumber /*ack_packet_number*/, + const QuicAckFrame& /*ack_frame*/, QuicTime /*ack_receive_time*/, QuicPacketNumber /*largest_observed*/, bool /*rtt_updated*/, @@ -278,6 +279,7 @@ // Called when an ack frame is parsed completely. AckResult OnAckFrameEnd(QuicTime ack_receive_time, + QuicPacketNumber ack_packet_number, EncryptionLevel ack_decrypted_level); // Called to enable/disable letting session decide what to write. @@ -389,6 +391,8 @@ return ignore_tlpr_if_no_pending_stream_data_; } + bool fix_rto_retransmission() const { return fix_rto_retransmission_; } + private: friend class test::QuicConnectionPeer; friend class test::QuicSentPacketManagerPeer; @@ -493,7 +497,8 @@ QuicTransmissionInfo* transmission_info); // Called after packets have been marked handled with last received ack frame. - void PostProcessNewlyAckedPackets(const QuicAckFrame& ack_frame, + void PostProcessNewlyAckedPackets(QuicPacketNumber ack_packet_number, + const QuicAckFrame& ack_frame, QuicTime ack_receive_time, bool rtt_updated, QuicByteCount prior_bytes_in_flight); @@ -634,6 +639,9 @@ // Latched value of quic_ignore_tlpr_if_no_pending_stream_data. const bool ignore_tlpr_if_no_pending_stream_data_; + + // Latched value of quic_fix_rto_retransmission. + const bool fix_rto_retransmission_; }; } // namespace quic
diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc index 075690e..4fce40f 100644 --- a/quic/core/quic_sent_packet_manager_test.cc +++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -404,7 +404,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(2), QuicPacketNumber(3)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); if (manager_.session_decides_what_to_write()) { EXPECT_CALL(notifier_, IsFrameOutstanding(_)).WillRepeatedly(Return(false)); } @@ -436,7 +437,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); // There should no longer be a pending retransmission. EXPECT_FALSE(manager_.HasPendingRetransmissions()); @@ -491,7 +493,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); if (manager_.session_decides_what_to_write()) { EXPECT_CALL(notifier_, IsFrameOutstanding(_)).WillRepeatedly(Return(false)); } @@ -508,7 +511,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(3)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(2), + ENCRYPTION_INITIAL)); } EXPECT_EQ(1u, stats_.packets_spuriously_retransmitted); @@ -526,7 +530,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); SendDataPacket(3); SendDataPacket(4); @@ -540,7 +545,8 @@ manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(4)); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(2), + ENCRYPTION_INITIAL)); ExpectAck(4); manager_.OnAckFrameStart(QuicPacketNumber(4), QuicTime::Delta::Infinite(), @@ -548,7 +554,8 @@ manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(5)); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(3), + ENCRYPTION_INITIAL)); ExpectAckAndLoss(true, 5, 2); if (manager_.session_decides_what_to_write()) { @@ -563,7 +570,8 @@ manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(6)); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(4), + ENCRYPTION_INITIAL)); if (manager_.session_decides_what_to_write()) { uint64_t unacked[] = {2}; @@ -600,7 +608,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); // Since 2 was marked for retransmit, when 1 is acked, 2 is kept for RTT. uint64_t unacked[] = {2}; @@ -638,7 +647,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); if (manager_.session_decides_what_to_write()) { // Frames in packets 2 and 3 are acked. EXPECT_CALL(notifier_, IsFrameOutstanding(_)) @@ -667,7 +677,8 @@ manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(5)); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(2), + ENCRYPTION_INITIAL)); uint64_t unacked2[] = {2}; VerifyUnackedPackets(unacked2, QUIC_ARRAYSIZE(unacked2)); @@ -689,7 +700,8 @@ manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(6)); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(3), + ENCRYPTION_INITIAL)); if (manager_.session_decides_what_to_write()) { uint64_t unacked[] = {2}; @@ -723,7 +735,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); } SendDataPacket(3); @@ -737,7 +750,8 @@ manager_.OnAckRange(QuicPacketNumber(4), QuicPacketNumber(5)); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(2), + ENCRYPTION_INITIAL)); RetransmitAndSendPacket(3, 5, LOSS_RETRANSMISSION); } @@ -753,7 +767,8 @@ manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(5)); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(3), + ENCRYPTION_INITIAL)); if (manager_.session_decides_what_to_write()) { // Ack 3 will not cause 5 be considered as a spurious retransmission. Ack // 5 will cause 5 be considered as a spurious retransmission as no new @@ -766,7 +781,8 @@ manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(6)); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(4), + ENCRYPTION_INITIAL)); } } } @@ -792,7 +808,8 @@ QuicTime::Delta::FromMilliseconds(5), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(3)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); EXPECT_EQ(QuicPacketNumber(1), manager_.largest_packet_peer_knows_is_acked()); SendAckPacket(3, 3); @@ -804,7 +821,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(4)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(2), + ENCRYPTION_INITIAL)); EXPECT_EQ(QuicPacketNumber(3u), manager_.largest_packet_peer_knows_is_acked()); } @@ -819,7 +837,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); EXPECT_EQ(expected_rtt, manager_.GetRttStats()->latest_rtt()); } @@ -836,7 +855,8 @@ QuicTime::Delta::FromMilliseconds(11), clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); EXPECT_EQ(expected_rtt, manager_.GetRttStats()->latest_rtt()); } @@ -852,7 +872,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); EXPECT_EQ(expected_rtt, manager_.GetRttStats()->latest_rtt()); } @@ -868,7 +889,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); EXPECT_EQ(expected_rtt, manager_.GetRttStats()->latest_rtt()); } @@ -919,7 +941,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(4)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); EXPECT_TRUE(manager_.HasInFlightPackets()); @@ -941,7 +964,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(6)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(2), + ENCRYPTION_INITIAL)); EXPECT_FALSE(manager_.HasPendingRetransmissions()); EXPECT_FALSE(manager_.HasInFlightPackets()); @@ -1048,7 +1072,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(103), QuicPacketNumber(104)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); // All packets before 103 should be lost. if (manager_.session_decides_what_to_write()) { // Packet 104 is still in flight. @@ -1134,7 +1159,8 @@ manager_.OnAckRange(QuicPacketNumber(8), QuicPacketNumber(10)); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(6)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); EXPECT_FALSE(manager_.HasUnackedCryptoPackets()); } @@ -1206,7 +1232,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(8), QuicPacketNumber(10)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); if (manager_.session_decides_what_to_write()) { EXPECT_CALL(notifier_, HasUnackedCryptoData()) .WillRepeatedly(Return(false)); @@ -1252,7 +1279,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(2), QuicPacketNumber(3)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); EXPECT_FALSE(manager_.HasUnackedCryptoPackets()); if (GetQuicReloadableFlag(quic_loss_removes_from_inflight)) { @@ -1378,7 +1406,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(4)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); VerifyUnackedPackets(nullptr, 0); VerifyRetransmittablePackets(nullptr, 0); } @@ -1441,7 +1470,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(102), QuicPacketNumber(103)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); } TEST_P(QuicSentPacketManagerTest, RetransmissionTimeoutOnePacket) { @@ -1550,7 +1580,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(102), QuicPacketNumber(103)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); } TEST_P(QuicSentPacketManagerTest, TwoRetransmissionTimeoutsAckSecond) { @@ -1595,7 +1626,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(2), QuicPacketNumber(3)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); // The original packet and newest should be outstanding. EXPECT_EQ(2 * kDefaultLength, manager_.GetBytesInFlight()); @@ -1643,7 +1675,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(4)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); // The first two packets should still be outstanding. EXPECT_EQ(2 * kDefaultLength, manager_.GetBytesInFlight()); @@ -1955,7 +1988,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(2), QuicPacketNumber(3)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); EXPECT_FALSE(manager_.HasPendingRetransmissions()); EXPECT_EQ(5 * kDefaultLength, manager_.GetBytesInFlight()); @@ -2090,7 +2124,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(2), QuicPacketNumber(3)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); QuicTime timeout(clock_.Now() + QuicTime::Delta::FromMilliseconds(10)); EXPECT_CALL(*loss_algorithm, GetLossTimeout()) @@ -2575,7 +2610,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); } TEST_P(QuicSentPacketManagerTest, OnAckRangeSlowPath) { @@ -2597,7 +2633,8 @@ // Make sure empty range does not harm. manager_.OnAckRange(QuicPacketNumber(4), QuicPacketNumber(4)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); // Ack [4, 8), [9, 13), [14, 21). uint64_t acked2[] = {4, 7, 9, 12, 14, 17, 18, 19, 20}; @@ -2608,7 +2645,8 @@ manager_.OnAckRange(QuicPacketNumber(9), QuicPacketNumber(13)); manager_.OnAckRange(QuicPacketNumber(4), QuicPacketNumber(8)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(2), + ENCRYPTION_INITIAL)); } TEST_P(QuicSentPacketManagerTest, TolerateReneging) { @@ -2628,7 +2666,8 @@ manager_.OnAckRange(QuicPacketNumber(10), QuicPacketNumber(12)); manager_.OnAckRange(QuicPacketNumber(5), QuicPacketNumber(7)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); // Making sure reneged ACK does not harm. Ack [4, 8), [9, 13). uint64_t acked2[] = {4, 7, 9, 12}; @@ -2638,7 +2677,8 @@ manager_.OnAckRange(QuicPacketNumber(9), QuicPacketNumber(13)); manager_.OnAckRange(QuicPacketNumber(4), QuicPacketNumber(8)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(2), + ENCRYPTION_INITIAL)); EXPECT_EQ(QuicPacketNumber(16), manager_.GetLargestObserved()); } @@ -2660,7 +2700,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); EXPECT_EQ(QuicPacketNumber(1), manager_.GetLargestAckedPacket(ENCRYPTION_INITIAL)); EXPECT_FALSE( @@ -2680,7 +2721,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(2), QuicPacketNumber(3)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_HANDSHAKE)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(2), + ENCRYPTION_HANDSHAKE)); EXPECT_EQ(QuicPacketNumber(2), manager_.GetLargestAckedPacket(ENCRYPTION_HANDSHAKE)); EXPECT_FALSE( @@ -2691,7 +2733,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(2), QuicPacketNumber(4)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_HANDSHAKE)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(3), + ENCRYPTION_HANDSHAKE)); EXPECT_EQ(QuicPacketNumber(3), manager_.GetLargestAckedPacket(ENCRYPTION_HANDSHAKE)); EXPECT_FALSE( @@ -2713,7 +2756,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(5), QuicPacketNumber(6)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_FORWARD_SECURE)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(4), + ENCRYPTION_FORWARD_SECURE)); EXPECT_EQ(QuicPacketNumber(3), manager_.GetLargestAckedPacket(ENCRYPTION_HANDSHAKE)); EXPECT_EQ(QuicPacketNumber(5), @@ -2740,7 +2784,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(4), QuicPacketNumber(9)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_FORWARD_SECURE)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(5), + ENCRYPTION_FORWARD_SECURE)); EXPECT_EQ(QuicPacketNumber(3), manager_.GetLargestAckedPacket(ENCRYPTION_HANDSHAKE)); EXPECT_EQ(QuicPacketNumber(8), @@ -2762,7 +2807,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(4)); EXPECT_EQ(PACKETS_ACKED_IN_WRONG_PACKET_NUMBER_SPACE, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); } TEST_P(QuicSentPacketManagerTest, PacketsGetAckedInWrongPacketNumberSpace2) { @@ -2778,7 +2824,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(4)); EXPECT_EQ(PACKETS_ACKED_IN_WRONG_PACKET_NUMBER_SPACE, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_HANDSHAKE)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_HANDSHAKE)); } TEST_P(QuicSentPacketManagerTest, @@ -2792,7 +2839,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); // Send packets 2 and 3. SendDataPacket(2, ENCRYPTION_HANDSHAKE); @@ -2806,7 +2854,8 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(4)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_HANDSHAKE)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(2), + ENCRYPTION_HANDSHAKE)); } // Regression test for b/133771183. @@ -2848,7 +2897,8 @@ manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(5)); manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), + ENCRYPTION_INITIAL)); uint64_t acked2[] = {5, 6}; uint64_t loss[] = {2}; @@ -2865,7 +2915,39 @@ clock_.Now()); manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(7)); EXPECT_EQ(PACKETS_NEWLY_ACKED, - manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(2), + ENCRYPTION_INITIAL)); +} + +TEST_P(QuicSentPacketManagerTest, RtoFiresNoPacketToRetransmit) { + if (!manager_.session_decides_what_to_write()) { + return; + } + // Send 10 packets. + for (size_t i = 1; i <= 10; ++i) { + SendDataPacket(i); + } + EXPECT_CALL(notifier_, RetransmitFrames(_, _)) + .Times(2) + .WillOnce(WithArgs<1>(Invoke( + [this](TransmissionType type) { RetransmitDataPacket(11, type); }))) + .WillOnce(WithArgs<1>(Invoke( + [this](TransmissionType type) { RetransmitDataPacket(12, type); }))); + manager_.OnRetransmissionTimeout(); + EXPECT_EQ(1u, stats_.rto_count); + EXPECT_EQ(0u, manager_.pending_timer_transmission_count()); + + // RTO fires again, but there is no packet to be RTO retransmitted. + EXPECT_CALL(notifier_, IsFrameOutstanding(_)).WillRepeatedly(Return(false)); + EXPECT_CALL(notifier_, RetransmitFrames(_, _)).Times(0); + manager_.OnRetransmissionTimeout(); + EXPECT_EQ(2u, stats_.rto_count); + if (GetQuicReloadableFlag(quic_fix_rto_retransmission)) { + // Verify a credit is raised up. + EXPECT_EQ(1u, manager_.pending_timer_transmission_count()); + } else { + EXPECT_EQ(0u, manager_.pending_timer_transmission_count()); + } } } // namespace
diff --git a/quic/core/quic_session.cc b/quic/core/quic_session.cc index 750f256..b2d0bce 100644 --- a/quic/core/quic_session.cc +++ b/quic/core/quic_session.cc
@@ -1071,7 +1071,7 @@ void QuicSession::HandleRstOnValidNonexistentStream( const QuicRstStreamFrame& frame) { // If the stream is neither originally in active streams nor created in - // GetOrCreateDynamicStream(), it could be a closed stream in which case its + // GetOrCreateStream(), it could be a closed stream in which case its // final received byte offset need to be updated. if (IsClosedStream(frame.stream_id)) { // The RST frame contains the final byte offset for the stream: we can now @@ -1224,7 +1224,42 @@ stream_id)) { return GetMutableCryptoStream(); } - return GetOrCreateDynamicStream(stream_id); + + StreamMap::iterator it = stream_map_.find(stream_id); + if (it != stream_map_.end()) { + return it->second.get(); + } + + if (IsClosedStream(stream_id)) { + return nullptr; + } + + if (!IsIncomingStream(stream_id)) { + HandleFrameOnNonexistentOutgoingStream(stream_id); + return nullptr; + } + + // TODO(fkastenholz): If we are creating a new stream and we have + // sent a goaway, we should ignore the stream creation. Need to + // add code to A) test if goaway was sent ("if (goaway_sent_)") and + // B) reject stream creation ("return nullptr") + + if (!MaybeIncreaseLargestPeerStreamId(stream_id)) { + return nullptr; + } + + if (!VersionHasIetfQuicFrames(connection_->transport_version())) { + // TODO(fayang): Let LegacyQuicStreamIdManager count open streams and make + // CanOpenIncomingStream interface consistent with that of v99. + if (!stream_id_manager_.CanOpenIncomingStream( + GetNumOpenIncomingStreams())) { + // Refuse to open the stream. + SendRstStream(stream_id, QUIC_REFUSED_STREAM, 0); + return nullptr; + } + } + + return CreateIncomingStream(stream_id); } void QuicSession::StreamDraining(QuicStreamId stream_id) { @@ -1278,6 +1313,7 @@ QuicStream* QuicSession::GetOrCreateDynamicStream( const QuicStreamId stream_id) { + DCHECK(!GetQuicReloadableFlag(quic_inline_getorcreatedynamicstream)); StreamMap::iterator it = stream_map_.find(stream_id); if (it != stream_map_.end()) { return it->second.get();
diff --git a/quic/core/quic_session.h b/quic/core/quic_session.h index 5ef497d..24060e8 100644 --- a/quic/core/quic_session.h +++ b/quic/core/quic_session.h
@@ -370,6 +370,7 @@ // such stream exists, and |stream_id| is a peer-created stream id, // then a new stream is created and returned. In all other cases, nullptr is // returned. + // Caller does not own the returned stream. QuicStream* GetOrCreateStream(const QuicStreamId stream_id); // Mark a stream as draining. @@ -475,6 +476,8 @@ // returned. However if |stream_id| is a locally-created id and no such stream // exists, the connection is closed. // Caller does not own the returned stream. + // TODO(renjietang): Remove this method after + // quic_inline_getorcreatedynamicstream is deprecated. QuicStream* GetOrCreateDynamicStream(QuicStreamId stream_id); // Performs the work required to close |stream_id|. If |locally_reset|
diff --git a/quic/core/quic_session_test.cc b/quic/core/quic_session_test.cc index 0f8420d..49316dd 100644 --- a/quic/core/quic_session_test.cc +++ b/quic/core/quic_session_test.cc
@@ -187,7 +187,7 @@ max_open_incoming_bidirectional_streams() && !VersionHasIetfQuicFrames(connection()->transport_version())) { // No need to do this test for version 99; it's done by - // QuicSession::GetOrCreateDynamicStream. + // QuicSession::GetOrCreateStream. connection()->CloseConnection( QUIC_TOO_MANY_OPEN_STREAMS, "Too many streams!", ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET); @@ -232,8 +232,8 @@ return QuicSession::IsClosedStream(id); } - QuicStream* GetOrCreateDynamicStream(QuicStreamId stream_id) { - return QuicSession::GetOrCreateDynamicStream(stream_id); + QuicStream* GetOrCreateStream(QuicStreamId stream_id) { + return QuicSession::GetOrCreateStream(stream_id); } bool ShouldKeepConnectionAlive() const override { @@ -531,30 +531,30 @@ } TEST_P(QuicSessionTestServer, AvailableBidirectionalStreams) { - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthClientInitiatedBidirectionalId(3)) != nullptr); // Smaller bidirectional streams should be available. EXPECT_TRUE(QuicSessionPeer::IsStreamAvailable( &session_, GetNthClientInitiatedBidirectionalId(1))); EXPECT_TRUE(QuicSessionPeer::IsStreamAvailable( &session_, GetNthClientInitiatedBidirectionalId(2))); - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthClientInitiatedBidirectionalId(2)) != nullptr); - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthClientInitiatedBidirectionalId(1)) != nullptr); } TEST_P(QuicSessionTestServer, AvailableUnidirectionalStreams) { - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthClientInitiatedUnidirectionalId(3)) != nullptr); // Smaller unidirectional streams should be available. EXPECT_TRUE(QuicSessionPeer::IsStreamAvailable( &session_, GetNthClientInitiatedUnidirectionalId(1))); EXPECT_TRUE(QuicSessionPeer::IsStreamAvailable( &session_, GetNthClientInitiatedUnidirectionalId(2))); - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthClientInitiatedUnidirectionalId(2)) != nullptr); - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthClientInitiatedUnidirectionalId(1)) != nullptr); } @@ -613,15 +613,15 @@ TEST_P(QuicSessionTestServer, IsClosedBidirectionalStreamPeerCreated) { QuicStreamId stream_id1 = GetNthClientInitiatedBidirectionalId(0); QuicStreamId stream_id2 = GetNthClientInitiatedBidirectionalId(1); - session_.GetOrCreateDynamicStream(stream_id1); - session_.GetOrCreateDynamicStream(stream_id2); + session_.GetOrCreateStream(stream_id1); + session_.GetOrCreateStream(stream_id2); CheckClosedStreams(); CloseStream(stream_id1); CheckClosedStreams(); CloseStream(stream_id2); // Create a stream, and make another available. - QuicStream* stream3 = session_.GetOrCreateDynamicStream( + QuicStream* stream3 = session_.GetOrCreateStream( stream_id2 + 2 * QuicUtils::StreamIdDelta(connection_->transport_version())); CheckClosedStreams(); @@ -633,15 +633,15 @@ TEST_P(QuicSessionTestServer, IsClosedUnidirectionalStreamPeerCreated) { QuicStreamId stream_id1 = GetNthClientInitiatedUnidirectionalId(0); QuicStreamId stream_id2 = GetNthClientInitiatedUnidirectionalId(1); - session_.GetOrCreateDynamicStream(stream_id1); - session_.GetOrCreateDynamicStream(stream_id2); + session_.GetOrCreateStream(stream_id1); + session_.GetOrCreateStream(stream_id2); CheckClosedStreams(); CloseStream(stream_id1); CheckClosedStreams(); CloseStream(stream_id2); // Create a stream, and make another available. - QuicStream* stream3 = session_.GetOrCreateDynamicStream( + QuicStream* stream3 = session_.GetOrCreateStream( stream_id2 + 2 * QuicUtils::StreamIdDelta(connection_->transport_version())); CheckClosedStreams(); @@ -652,28 +652,26 @@ TEST_P(QuicSessionTestServer, MaximumAvailableOpenedBidirectionalStreams) { QuicStreamId stream_id = GetNthClientInitiatedBidirectionalId(0); - session_.GetOrCreateDynamicStream(stream_id); + session_.GetOrCreateStream(stream_id); EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0); - EXPECT_NE( - nullptr, - session_.GetOrCreateDynamicStream(GetNthClientInitiatedBidirectionalId( - session_.max_open_incoming_bidirectional_streams() - 1))); + EXPECT_NE(nullptr, + session_.GetOrCreateStream(GetNthClientInitiatedBidirectionalId( + session_.max_open_incoming_bidirectional_streams() - 1))); } TEST_P(QuicSessionTestServer, MaximumAvailableOpenedUnidirectionalStreams) { QuicStreamId stream_id = GetNthClientInitiatedUnidirectionalId(0); - session_.GetOrCreateDynamicStream(stream_id); + session_.GetOrCreateStream(stream_id); EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0); - EXPECT_NE( - nullptr, - session_.GetOrCreateDynamicStream(GetNthClientInitiatedUnidirectionalId( - session_.max_open_incoming_unidirectional_streams() - 1))); + EXPECT_NE(nullptr, + session_.GetOrCreateStream(GetNthClientInitiatedUnidirectionalId( + session_.max_open_incoming_unidirectional_streams() - 1))); } TEST_P(QuicSessionTestServer, TooManyAvailableBidirectionalStreams) { QuicStreamId stream_id1 = GetNthClientInitiatedBidirectionalId(0); QuicStreamId stream_id2; - EXPECT_NE(nullptr, session_.GetOrCreateDynamicStream(stream_id1)); + EXPECT_NE(nullptr, session_.GetOrCreateStream(stream_id1)); // A stream ID which is too large to create. stream_id2 = GetNthClientInitiatedBidirectionalId( session_.MaxAvailableBidirectionalStreams() + 2); @@ -686,13 +684,13 @@ EXPECT_CALL(*connection_, CloseConnection(QUIC_TOO_MANY_AVAILABLE_STREAMS, _, _)); } - EXPECT_EQ(nullptr, session_.GetOrCreateDynamicStream(stream_id2)); + EXPECT_EQ(nullptr, session_.GetOrCreateStream(stream_id2)); } TEST_P(QuicSessionTestServer, TooManyAvailableUnidirectionalStreams) { QuicStreamId stream_id1 = GetNthClientInitiatedUnidirectionalId(0); QuicStreamId stream_id2; - EXPECT_NE(nullptr, session_.GetOrCreateDynamicStream(stream_id1)); + EXPECT_NE(nullptr, session_.GetOrCreateStream(stream_id1)); // A stream ID which is too large to create. stream_id2 = GetNthClientInitiatedUnidirectionalId( session_.MaxAvailableUnidirectionalStreams() + 2); @@ -705,7 +703,7 @@ EXPECT_CALL(*connection_, CloseConnection(QUIC_TOO_MANY_AVAILABLE_STREAMS, _, _)); } - EXPECT_EQ(nullptr, session_.GetOrCreateDynamicStream(stream_id2)); + EXPECT_EQ(nullptr, session_.GetOrCreateStream(stream_id2)); } TEST_P(QuicSessionTestServer, ManyAvailableBidirectionalStreams) { @@ -720,21 +718,21 @@ } // Create a stream at the start of the range. QuicStreamId stream_id = GetNthClientInitiatedBidirectionalId(0); - EXPECT_NE(nullptr, session_.GetOrCreateDynamicStream(stream_id)); + EXPECT_NE(nullptr, session_.GetOrCreateStream(stream_id)); // Create the largest stream ID of a threatened total of 200 streams. // GetNth... starts at 0, so for 200 streams, get the 199th. EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0); - EXPECT_NE(nullptr, session_.GetOrCreateDynamicStream( + EXPECT_NE(nullptr, session_.GetOrCreateStream( GetNthClientInitiatedBidirectionalId(199))); if (VersionHasIetfQuicFrames(transport_version())) { // If IETF QUIC, check to make sure that creating bidirectional // streams does not mess up the unidirectional streams. stream_id = GetNthClientInitiatedUnidirectionalId(0); - EXPECT_NE(nullptr, session_.GetOrCreateDynamicStream(stream_id)); + EXPECT_NE(nullptr, session_.GetOrCreateStream(stream_id)); // Now try to get the last possible unidirectional stream. - EXPECT_NE(nullptr, session_.GetOrCreateDynamicStream( + EXPECT_NE(nullptr, session_.GetOrCreateStream( GetNthClientInitiatedUnidirectionalId(49))); // and this should fail because it exceeds the unidirectional limit // (but not the bi-) @@ -746,7 +744,7 @@ )) .Times(1); - EXPECT_EQ(nullptr, session_.GetOrCreateDynamicStream( + EXPECT_EQ(nullptr, session_.GetOrCreateStream( GetNthClientInitiatedUnidirectionalId(199))); } } @@ -763,20 +761,20 @@ } // Create one stream. QuicStreamId stream_id = GetNthClientInitiatedUnidirectionalId(0); - EXPECT_NE(nullptr, session_.GetOrCreateDynamicStream(stream_id)); + EXPECT_NE(nullptr, session_.GetOrCreateStream(stream_id)); // Create the largest stream ID of a threatened total of 200 streams. // GetNth... starts at 0, so for 200 streams, get the 199th. EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0); - EXPECT_NE(nullptr, session_.GetOrCreateDynamicStream( + EXPECT_NE(nullptr, session_.GetOrCreateStream( GetNthClientInitiatedUnidirectionalId(199))); if (VersionHasIetfQuicFrames(transport_version())) { // If IETF QUIC, check to make sure that creating unidirectional // streams does not mess up the bidirectional streams. stream_id = GetNthClientInitiatedBidirectionalId(0); - EXPECT_NE(nullptr, session_.GetOrCreateDynamicStream(stream_id)); + EXPECT_NE(nullptr, session_.GetOrCreateStream(stream_id)); // Now try to get the last possible bidirectional stream. - EXPECT_NE(nullptr, session_.GetOrCreateDynamicStream( + EXPECT_NE(nullptr, session_.GetOrCreateStream( GetNthClientInitiatedBidirectionalId(49))); // and this should fail because it exceeds the bnidirectional limit // (but not the uni-) @@ -791,7 +789,7 @@ CloseConnection(QUIC_INVALID_STREAM_ID, error_detail, ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET)) .Times(1); - EXPECT_EQ(nullptr, session_.GetOrCreateDynamicStream( + EXPECT_EQ(nullptr, session_.GetOrCreateStream( GetNthClientInitiatedBidirectionalId(199))); } } @@ -1186,7 +1184,7 @@ EXPECT_CALL(*connection_, OnStreamReset(kTestStreamId, QUIC_STREAM_PEER_GOING_AWAY)) .Times(0); - EXPECT_TRUE(session_.GetOrCreateDynamicStream(kTestStreamId)); + EXPECT_TRUE(session_.GetOrCreateStream(kTestStreamId)); } TEST_P(QuicSessionTestServer, DoNotSendGoAwayTwice) { @@ -1821,16 +1819,16 @@ ::testing::ValuesIn(AllSupportedVersions())); TEST_P(QuicSessionTestClient, AvailableBidirectionalStreamsClient) { - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthServerInitiatedBidirectionalId(2)) != nullptr); // Smaller bidirectional streams should be available. EXPECT_TRUE(QuicSessionPeer::IsStreamAvailable( &session_, GetNthServerInitiatedBidirectionalId(0))); EXPECT_TRUE(QuicSessionPeer::IsStreamAvailable( &session_, GetNthServerInitiatedBidirectionalId(1))); - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthServerInitiatedBidirectionalId(0)) != nullptr); - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthServerInitiatedBidirectionalId(1)) != nullptr); // And 5 should be not available. EXPECT_FALSE(QuicSessionPeer::IsStreamAvailable( @@ -1838,16 +1836,16 @@ } TEST_P(QuicSessionTestClient, AvailableUnidirectionalStreamsClient) { - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthServerInitiatedUnidirectionalId(2)) != nullptr); // Smaller unidirectional streams should be available. EXPECT_TRUE(QuicSessionPeer::IsStreamAvailable( &session_, GetNthServerInitiatedUnidirectionalId(0))); EXPECT_TRUE(QuicSessionPeer::IsStreamAvailable( &session_, GetNthServerInitiatedUnidirectionalId(1))); - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthServerInitiatedUnidirectionalId(0)) != nullptr); - ASSERT_TRUE(session_.GetOrCreateDynamicStream( + ASSERT_TRUE(session_.GetOrCreateStream( GetNthServerInitiatedUnidirectionalId(1)) != nullptr); // And 5 should be not available. EXPECT_FALSE(QuicSessionPeer::IsStreamAvailable(
diff --git a/quic/core/quic_trace_visitor.cc b/quic/core/quic_trace_visitor.cc index f15e149..42ee003 100644 --- a/quic/core/quic_trace_visitor.cc +++ b/quic/core/quic_trace_visitor.cc
@@ -225,6 +225,7 @@ } void QuicTraceVisitor::OnIncomingAck( + QuicPacketNumber /*ack_packet_number*/, const QuicAckFrame& ack_frame, QuicTime ack_receive_time, QuicPacketNumber /*largest_observed*/,
diff --git a/quic/core/quic_trace_visitor.h b/quic/core/quic_trace_visitor.h index 8021865..0494d87 100644 --- a/quic/core/quic_trace_visitor.h +++ b/quic/core/quic_trace_visitor.h
@@ -23,7 +23,8 @@ TransmissionType transmission_type, QuicTime sent_time) override; - void OnIncomingAck(const QuicAckFrame& ack_frame, + void OnIncomingAck(QuicPacketNumber ack_packet_number, + const QuicAckFrame& ack_frame, QuicTime ack_receive_time, QuicPacketNumber largest_observed, bool rtt_updated,
diff --git a/quic/test_tools/quic_server_session_base_peer.h b/quic/test_tools/quic_server_session_base_peer.h index b29fe46..ab5b1f0 100644 --- a/quic/test_tools/quic_server_session_base_peer.h +++ b/quic/test_tools/quic_server_session_base_peer.h
@@ -14,9 +14,9 @@ class QuicServerSessionBasePeer { public: - static QuicStream* GetOrCreateDynamicStream(QuicServerSessionBase* s, - QuicStreamId id) { - return s->GetOrCreateDynamicStream(id); + static QuicStream* GetOrCreateStream(QuicServerSessionBase* s, + QuicStreamId id) { + return s->GetOrCreateStream(id); } static void SetCryptoStream(QuicServerSessionBase* s, QuicCryptoServerStream* crypto_stream) {
diff --git a/quic/test_tools/quic_session_peer.cc b/quic/test_tools/quic_session_peer.cc index 0dfba43..e78b059 100644 --- a/quic/test_tools/quic_session_peer.cc +++ b/quic/test_tools/quic_session_peer.cc
@@ -118,9 +118,9 @@ } // static -QuicStream* QuicSessionPeer::GetOrCreateDynamicStream(QuicSession* session, - QuicStreamId stream_id) { - return session->GetOrCreateDynamicStream(stream_id); +QuicStream* QuicSessionPeer::GetOrCreateStream(QuicSession* session, + QuicStreamId stream_id) { + return session->GetOrCreateStream(stream_id); } // static
diff --git a/quic/test_tools/quic_session_peer.h b/quic/test_tools/quic_session_peer.h index 9ca8530..f027eb5 100644 --- a/quic/test_tools/quic_session_peer.h +++ b/quic/test_tools/quic_session_peer.h
@@ -52,8 +52,8 @@ static QuicCryptoStream* GetMutableCryptoStream(QuicSession* session); static QuicWriteBlockedList* GetWriteBlockedStreams(QuicSession* session); - static QuicStream* GetOrCreateDynamicStream(QuicSession* session, - QuicStreamId stream_id); + static QuicStream* GetOrCreateStream(QuicSession* session, + QuicStreamId stream_id); static std::map<QuicStreamId, QuicStreamOffset>& GetLocallyClosedStreamsHighestOffset(QuicSession* session); static QuicSession::StreamMap& stream_map(QuicSession* session);
diff --git a/quic/test_tools/quic_spdy_session_peer.cc b/quic/test_tools/quic_spdy_session_peer.cc index be0849b..68552a8 100644 --- a/quic/test_tools/quic_spdy_session_peer.cc +++ b/quic/test_tools/quic_spdy_session_peer.cc
@@ -16,15 +16,13 @@ return session->headers_stream(); } -void QuicSpdySessionPeer::SetUnownedHeadersStream( - QuicSpdySession* session, - QuicHeadersStream* headers_stream) { +void QuicSpdySessionPeer::SetHeadersStream(QuicSpdySession* session, + QuicHeadersStream* headers_stream) { for (auto& it : session->stream_map()) { if (it.first == QuicUtils::GetHeadersStreamId( session->connection()->transport_version())) { it.second.reset(headers_stream); - session->unowned_headers_stream_ = - static_cast<QuicHeadersStream*>(it.second.get()); + session->headers_stream_ = static_cast<QuicHeadersStream*>(it.second.get()); break; } }
diff --git a/quic/test_tools/quic_spdy_session_peer.h b/quic/test_tools/quic_spdy_session_peer.h index f97b264..aacf712 100644 --- a/quic/test_tools/quic_spdy_session_peer.h +++ b/quic/test_tools/quic_spdy_session_peer.h
@@ -24,8 +24,8 @@ QuicSpdySessionPeer() = delete; static QuicHeadersStream* GetHeadersStream(QuicSpdySession* session); - static void SetUnownedHeadersStream(QuicSpdySession* session, - QuicHeadersStream* headers_stream); + static void SetHeadersStream(QuicSpdySession* session, + QuicHeadersStream* headers_stream); static const spdy::SpdyFramer& GetSpdyFramer(QuicSpdySession* session); static void SetHpackEncoderDebugVisitor( QuicSpdySession* session,
diff --git a/quic/tools/quic_client_base.cc b/quic/tools/quic_client_base.cc index b03f7cc..e53911d 100644 --- a/quic/tools/quic_client_base.cc +++ b/quic/tools/quic_client_base.cc
@@ -85,6 +85,10 @@ break; } } + if (max_allowed_push_id_ > 0 && + dynamic_cast<QuicSpdyClientSession*>(session())) + static_cast<QuicSpdyClientSession*>(session())->set_max_allowed_push_id( + max_allowed_push_id_); return session()->connection()->connected(); }
diff --git a/quic/tools/quic_client_base.h b/quic/tools/quic_client_base.h index fb15b86..a54c621 100644 --- a/quic/tools/quic_client_base.h +++ b/quic/tools/quic_client_base.h
@@ -211,6 +211,9 @@ crypto_config_.set_pre_shared_key(key); } + // Set the max promise id for the client session. + void set_max_allowed_push_id(QuicStreamId max) { max_allowed_push_id_ = max; } + protected: // TODO(rch): Move GetNumSentClientHellosFromSession and // GetNumReceivedServerConfigUpdatesFromSession into a new/better @@ -329,6 +332,9 @@ // The network helper used to create sockets and manage the event loop. // Not owned by this class. std::unique_ptr<NetworkHelper> network_helper_; + + // The max promise id to set on the client session when created. + QuicStreamId max_allowed_push_id_; }; } // namespace quic
diff --git a/quic/tools/quic_simple_server_session_test.cc b/quic/tools/quic_simple_server_session_test.cc index eeb9ce3..9bc3573 100644 --- a/quic/tools/quic_simple_server_session_test.cc +++ b/quic/tools/quic_simple_server_session_test.cc
@@ -522,12 +522,12 @@ } TEST_P(QuicSimpleServerSessionTest, GetEvenIncomingError) { - // Tests that calling GetOrCreateDynamicStream() on an outgoing stream not + // Tests that calling GetOrCreateStream() on an outgoing stream not // promised yet should result close connection. EXPECT_CALL(*connection_, CloseConnection(QUIC_INVALID_STREAM_ID, "Data for nonexistent stream", _)); EXPECT_EQ(nullptr, - QuicSessionPeer::GetOrCreateDynamicStream( + QuicSessionPeer::GetOrCreateStream( session_.get(), GetNthServerInitiatedUnidirectionalId(1))); }