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
