Use GetReadableRegion() instead of PeekRegion() in QuicReceiveControlStream::OnDataAvailable(). This has been recommended at https://crbug.com/982648#c8. gfe-relnote: n/a, change in QUIC v99-only class. PiperOrigin-RevId: 258251582 Change-Id: If7343673d8ec17b668059bac25aa05859551ed96
diff --git a/quic/core/http/quic_receive_control_stream.cc b/quic/core/http/quic_receive_control_stream.cc index ee92720..c01040b 100644 --- a/quic/core/http/quic_receive_control_stream.cc +++ b/quic/core/http/quic_receive_control_stream.cc
@@ -133,11 +133,9 @@ QuicReceiveControlStream::QuicReceiveControlStream(PendingStream* pending) : QuicStream(pending, READ_UNIDIRECTIONAL, /*is_static=*/true), - current_priority_length_(0), - received_settings_length_(0), + settings_frame_received_(false), http_decoder_visitor_(QuicMakeUnique<HttpDecoderVisitor>(this)), - decoder_(http_decoder_visitor_.get()), - sequencer_offset_(sequencer()->NumBytesConsumed()) { + decoder_(http_decoder_visitor_.get()) { sequencer()->set_level_triggered(true); } @@ -154,31 +152,36 @@ void QuicReceiveControlStream::OnDataAvailable() { iovec iov; - while (session()->connection()->connected() && !reading_stopped() && - decoder_.error() == QUIC_NO_ERROR) { - DCHECK_GE(sequencer_offset_, sequencer()->NumBytesConsumed()); - if (!sequencer()->PeekRegion(sequencer_offset_, &iov)) { - break; - } - + while (!reading_stopped() && decoder_.error() == QUIC_NO_ERROR && + sequencer()->GetReadableRegion(&iov)) { DCHECK(!sequencer()->IsClosed()); + QuicByteCount processed_bytes = decoder_.ProcessInput( reinterpret_cast<const char*>(iov.iov_base), iov.iov_len); - sequencer_offset_ += processed_bytes; + sequencer()->MarkConsumed(processed_bytes); + + if (!session()->connection()->connected()) { + return; + } + + // The only reason QuicReceiveControlStream pauses HttpDecoder is an error, + // in which case the connection would have already been closed. + DCHECK_EQ(iov.iov_len, processed_bytes); } } bool QuicReceiveControlStream::OnSettingsFrameStart( - Http3FrameLengths frame_lengths) { - if (received_settings_length_ != 0) { + Http3FrameLengths /* frame_lengths */) { + if (settings_frame_received_) { // TODO(renjietang): Change error code to HTTP_UNEXPECTED_FRAME. session()->connection()->CloseConnection( QUIC_INVALID_STREAM_ID, "Settings frames are received twice.", ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET); return false; } - received_settings_length_ += - frame_lengths.header_length + frame_lengths.payload_length; + + settings_frame_received_ = true; + return true; } @@ -199,16 +202,12 @@ break; } } - sequencer()->MarkConsumed(received_settings_length_); return true; } bool QuicReceiveControlStream::OnPriorityFrameStart( - Http3FrameLengths frame_lengths) { + Http3FrameLengths /* frame_lengths */) { DCHECK_EQ(Perspective::IS_SERVER, session()->perspective()); - DCHECK_EQ(0u, current_priority_length_); - current_priority_length_ = - frame_lengths.header_length + frame_lengths.payload_length; return true; } @@ -222,8 +221,6 @@ if (stream) { stream->SetPriority(priority.weight); } - sequencer()->MarkConsumed(current_priority_length_); - current_priority_length_ = 0; return true; }
diff --git a/quic/core/http/quic_receive_control_stream.h b/quic/core/http/quic_receive_control_stream.h index 1bd13fe..f83b980 100644 --- a/quic/core/http/quic_receive_control_stream.h +++ b/quic/core/http/quic_receive_control_stream.h
@@ -41,21 +41,12 @@ // TODO(renjietang): Decode Priority in HTTP/3 style. bool OnPriorityFrame(const PriorityFrame& priority); - // Track the current priority frame length. - QuicByteCount current_priority_length_; - - // Track the number of settings bytes received. - size_t received_settings_length_; + // False until a SETTINGS frame is received. + bool settings_frame_received_; // HttpDecoder and its visitor. std::unique_ptr<HttpDecoderVisitor> http_decoder_visitor_; HttpDecoder decoder_; - - // Sequencer offset keeping track of how much data HttpDecoder has processed. - // Initial value is sequencer()->NumBytesConsumed() at time of - // QuicReceiveControlStream construction: that is the length of the - // unidirectional stream type at the beginning of the stream. - QuicStreamOffset sequencer_offset_; }; } // namespace quic
diff --git a/quic/core/http/quic_receive_control_stream_test.cc b/quic/core/http/quic_receive_control_stream_test.cc index 49c31d1..412ee0c 100644 --- a/quic/core/http/quic_receive_control_stream_test.cc +++ b/quic/core/http/quic_receive_control_stream_test.cc
@@ -135,20 +135,45 @@ EXPECT_EQ(5u, session_.max_outbound_header_list_size()); } +// Regression test for https://crbug.com/982648. +// QuicReceiveControlStream::OnDataAvailable() must stop processing input as +// soon as OnSettingsFrameStart() is called by HttpDecoder for the second frame. TEST_P(QuicReceiveControlStreamTest, ReceiveSettingsTwice) { SettingsFrame settings; - settings.values[3] = 2; - settings.values[kSettingsMaxHeaderListSize] = 5; - std::string data = EncodeSettings(settings); - QuicStreamFrame frame(receive_control_stream_->id(), false, 0, - QuicStringPiece(data)); - QuicStreamFrame frame2(receive_control_stream_->id(), false, data.length(), - QuicStringPiece(data)); - receive_control_stream_->OnStreamFrame(frame); + // Reserved identifiers, must be ignored. + settings.values[0x21] = 100; + settings.values[0x40] = 200; + + std::string settings_frame = EncodeSettings(settings); + + EXPECT_EQ(0u, NumBytesConsumed()); + + // Receive first SETTINGS frame. + receive_control_stream_->OnStreamFrame( + QuicStreamFrame(receive_control_stream_->id(), /* fin = */ false, + /* offset = */ 0, settings_frame)); + + // First SETTINGS frame is consumed. + EXPECT_EQ(settings_frame.size(), NumBytesConsumed()); + + // Second SETTINGS frame causes the connection to be closed. EXPECT_CALL(*connection_, CloseConnection(QUIC_INVALID_STREAM_ID, - "Settings frames are received twice.", _)); - receive_control_stream_->OnStreamFrame(frame2); + "Settings frames are received twice.", _)) + .WillOnce( + Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); + EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _)); + EXPECT_CALL(session_, OnConnectionClosed(_, _)); + + // Receive second SETTINGS frame. + receive_control_stream_->OnStreamFrame( + QuicStreamFrame(receive_control_stream_->id(), /* fin = */ false, + /* offset = */ settings_frame.size(), settings_frame)); + + // Frame header of second SETTINGS frame is consumed, but not frame payload. + QuicByteCount settings_frame_header_length = 2; + EXPECT_EQ(settings_frame.size() + settings_frame_header_length, + NumBytesConsumed()); } TEST_P(QuicReceiveControlStreamTest, ReceiveSettingsFragments) { @@ -217,43 +242,6 @@ receive_control_stream_->OnStreamFrame(frame); } -// Regression test for https://crbug.com/982648. -// QuicReceiveControlStream::OnDataAvailable() must stop processing input as -// soon as OnSettingsFrameStart() is called by HttpDecoder for the second frame. -TEST_P(QuicReceiveControlStreamTest, StopProcessingIfConnectionClosed) { - SettingsFrame settings; - // Reserved identifiers, must be ignored. - settings.values[0x21] = 100; - settings.values[0x40] = 200; - - std::string settings_frame = EncodeSettings(settings); - - EXPECT_EQ(0u, NumBytesConsumed()); - - // Receive first SETTINGS frame. - receive_control_stream_->OnStreamFrame( - QuicStreamFrame(receive_control_stream_->id(), /* fin = */ false, - /* offset = */ 0, settings_frame)); - - // First SETTINGS frame is consumed. - EXPECT_EQ(settings_frame.size(), NumBytesConsumed()); - - // Second SETTINGS frame causes the connection to be closed. - EXPECT_CALL(*connection_, CloseConnection(QUIC_INVALID_STREAM_ID, _, _)) - .WillOnce( - Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); - EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _)); - EXPECT_CALL(session_, OnConnectionClosed(_, _)); - - // Receive second SETTINGS frame. - receive_control_stream_->OnStreamFrame( - QuicStreamFrame(receive_control_stream_->id(), /* fin = */ false, - /* offset = */ settings_frame.size(), settings_frame)); - - // No new data is consumed. - EXPECT_EQ(settings_frame.size(), NumBytesConsumed()); -} - } // namespace } // namespace test } // namespace quic