Update BalsaHeadersSequence and BalsaVisitorInterface to use std::unique_ptr for interim headers. This CL updates the related methods BalsaHeadersSequence::Append() and BalsaVisitorInterface::OnInterimHeaders() to use std::unique_ptr<BalsaHeaders>. The motivation for this change is to allow users to retain valid references an underlying BalsaHeaders even after transferring ownership to the BalsaHeadersSequence (e.g., http://screen/4GbAo5Z7jsZxBCK from the very-much-work-in-progress cl/536902857). If attempting to retain a reference to a bare BalsaHeaders that is then std::move'd, the reference will not be valid after the std::move (where the BalsaHeaders we want is now a move-constructed BalsaHeaders elsewhere). PiperOrigin-RevId: 540666570
diff --git a/quiche/balsa/balsa_frame.cc b/quiche/balsa/balsa_frame.cc index cd388846..908fc53 100644 --- a/quiche/balsa/balsa_frame.cc +++ b/quiche/balsa/balsa_frame.cc
@@ -861,7 +861,8 @@ // Deliver headers from this interim response but reset everything else to // prepare for the next set of headers. Skip 101 Switching Protocols // because these are considered final headers for the current protocol. - visitor_->OnInterimHeaders(std::move(*headers_)); + visitor_->OnInterimHeaders( + std::make_unique<BalsaHeaders>(std::move(*headers_))); Reset(); checkpoint = message_start = message_current; continue;
diff --git a/quiche/balsa/balsa_frame_test.cc b/quiche/balsa/balsa_frame_test.cc index 9b1db61..cb47cfa 100644 --- a/quiche/balsa/balsa_frame_test.cc +++ b/quiche/balsa/balsa_frame_test.cc
@@ -9,6 +9,7 @@ #include <cstdint> #include <limits> #include <map> +#include <memory> #include <random> #include <sstream> #include <string> @@ -40,6 +41,7 @@ using ::testing::IsEmpty; using ::testing::Mock; using ::testing::NiceMock; +using ::testing::Pointee; using ::testing::Property; using ::testing::Range; using ::testing::StrEq; @@ -556,7 +558,8 @@ MOCK_METHOD(void, OnChunkLength, (size_t length), (override)); MOCK_METHOD(void, OnChunkExtensionInput, (absl::string_view input), (override)); - MOCK_METHOD(void, OnInterimHeaders, (BalsaHeaders headers), (override)); + MOCK_METHOD(void, OnInterimHeaders, (std::unique_ptr<BalsaHeaders> headers), + (override)); MOCK_METHOD(void, ContinueHeaderDone, (), (override)); MOCK_METHOD(void, HeaderDone, (), (override)); MOCK_METHOD(void, MessageDone, (), (override)); @@ -3794,8 +3797,8 @@ balsa_frame_.set_use_interim_headers_callback(true); InSequence s; - EXPECT_CALL(visitor_mock_, OnInterimHeaders(Property( - &BalsaHeaders::parsed_response_code, 100))); + EXPECT_CALL(visitor_mock_, OnInterimHeaders(Pointee(Property( + &BalsaHeaders::parsed_response_code, 100)))); EXPECT_CALL(visitor_mock_, HeaderDone()).Times(0); EXPECT_CALL(visitor_mock_, MessageDone()).Times(0); @@ -3853,8 +3856,8 @@ balsa_frame_.set_use_interim_headers_callback(true); InSequence s; - EXPECT_CALL(visitor_mock_, OnInterimHeaders(Property( - &BalsaHeaders::parsed_response_code, 100))); + EXPECT_CALL(visitor_mock_, OnInterimHeaders(Pointee(Property( + &BalsaHeaders::parsed_response_code, 100)))); ASSERT_EQ( balsa_frame_.ProcessInput(initial_headers.data(), initial_headers.size()), initial_headers.size()); @@ -3892,8 +3895,8 @@ balsa_frame_.set_use_interim_headers_callback(true); InSequence s; - EXPECT_CALL(visitor_mock_, OnInterimHeaders(Property( - &BalsaHeaders::parsed_response_code, 100))); + EXPECT_CALL(visitor_mock_, OnInterimHeaders(Pointee(Property( + &BalsaHeaders::parsed_response_code, 100)))); EXPECT_CALL(visitor_mock_, ContinueHeaderDone).Times(0); ASSERT_EQ( balsa_frame_.ProcessInput(initial_headers.data(), initial_headers.size()), @@ -3960,8 +3963,8 @@ balsa_frame_.set_use_interim_headers_callback(true); InSequence s; - EXPECT_CALL(visitor_mock_, OnInterimHeaders(Property( - &BalsaHeaders::parsed_response_code, 100))); + EXPECT_CALL(visitor_mock_, OnInterimHeaders(Pointee(Property( + &BalsaHeaders::parsed_response_code, 100)))); ASSERT_EQ( balsa_frame_.ProcessInput(initial_headers.data(), initial_headers.size()), initial_headers.size()); @@ -4024,8 +4027,8 @@ balsa_frame_.set_use_interim_headers_callback(true); InSequence s; - EXPECT_CALL(visitor_mock_, OnInterimHeaders(Property( - &BalsaHeaders::parsed_response_code, 100))); + EXPECT_CALL(visitor_mock_, OnInterimHeaders(Pointee(Property( + &BalsaHeaders::parsed_response_code, 100)))); EXPECT_CALL(visitor_mock_, HeaderDone()); ASSERT_EQ(balsa_frame_.ProcessInput(both_headers.data(), both_headers.size()), @@ -4056,10 +4059,10 @@ balsa_frame_.set_use_interim_headers_callback(true); InSequence s; - EXPECT_CALL(visitor_mock_, OnInterimHeaders(Property( - &BalsaHeaders::parsed_response_code, 100))); - EXPECT_CALL(visitor_mock_, OnInterimHeaders(Property( - &BalsaHeaders::parsed_response_code, 103))); + EXPECT_CALL(visitor_mock_, OnInterimHeaders(Pointee(Property( + &BalsaHeaders::parsed_response_code, 100)))); + EXPECT_CALL(visitor_mock_, OnInterimHeaders(Pointee(Property( + &BalsaHeaders::parsed_response_code, 103)))); EXPECT_CALL(visitor_mock_, HeaderDone()); ASSERT_EQ(balsa_frame_.ProcessInput(all_headers.data(), all_headers.size()),
diff --git a/quiche/balsa/balsa_headers_sequence.cc b/quiche/balsa/balsa_headers_sequence.cc index 8e9fcff..137ae83 100644 --- a/quiche/balsa/balsa_headers_sequence.cc +++ b/quiche/balsa/balsa_headers_sequence.cc
@@ -1,10 +1,12 @@ #include "quiche/balsa/balsa_headers_sequence.h" +#include <memory> + #include "quiche/balsa/balsa_headers.h" namespace quiche { -void BalsaHeadersSequence::Append(BalsaHeaders headers) { +void BalsaHeadersSequence::Append(std::unique_ptr<BalsaHeaders> headers) { sequence_.push_back(std::move(headers)); } @@ -14,14 +16,14 @@ if (!HasNext()) { return nullptr; } - return &sequence_[next_]; + return sequence_[next_].get(); } BalsaHeaders* BalsaHeadersSequence::Next() { if (!HasNext()) { return nullptr; } - return &sequence_[next_++]; + return sequence_[next_++].get(); } void BalsaHeadersSequence::Clear() {
diff --git a/quiche/balsa/balsa_headers_sequence.h b/quiche/balsa/balsa_headers_sequence.h index ca521f6..3fe10fd 100644 --- a/quiche/balsa/balsa_headers_sequence.h +++ b/quiche/balsa/balsa_headers_sequence.h
@@ -2,6 +2,7 @@ #define QUICHE_BALSA_BALSA_HEADERS_SEQUENCE_H_ #include <cstddef> +#include <memory> #include "absl/container/inlined_vector.h" #include "quiche/balsa/balsa_headers.h" @@ -14,7 +15,7 @@ class QUICHE_EXPORT BalsaHeadersSequence { public: // Appends `headers` to the end of the sequence. - void Append(BalsaHeaders headers); + void Append(std::unique_ptr<BalsaHeaders> headers); // Returns true if there is a BalsaHeaders that has not yet been returned from // `Next()`. IFF true, `Next()` will return non-nullptr. @@ -34,7 +35,7 @@ private: // Typically at most two interim responses: an optional 100 Continue and an // optional 103 Early Hints. - absl::InlinedVector<BalsaHeaders, 2> sequence_; + absl::InlinedVector<std::unique_ptr<BalsaHeaders>, 2> sequence_; // The index of the next entry in the sequence. size_t next_ = 0;
diff --git a/quiche/balsa/balsa_headers_sequence_test.cc b/quiche/balsa/balsa_headers_sequence_test.cc index bbcebb0..ac62f5e 100644 --- a/quiche/balsa/balsa_headers_sequence_test.cc +++ b/quiche/balsa/balsa_headers_sequence_test.cc
@@ -1,5 +1,6 @@ #include "quiche/balsa/balsa_headers_sequence.h" +#include <memory> #include <utility> #include "quiche/balsa/balsa_headers.h" @@ -18,13 +19,13 @@ TEST(BalsaHeadersSequenceTest, Basic) { BalsaHeadersSequence sequence; - BalsaHeaders headers_one; - headers_one.AppendHeader("one", "fish"); + auto headers_one = std::make_unique<BalsaHeaders>(); + headers_one->AppendHeader("one", "fish"); sequence.Append(std::move(headers_one)); EXPECT_TRUE(sequence.HasNext()); - BalsaHeaders headers_two; - headers_two.AppendHeader("two", "fish"); + auto headers_two = std::make_unique<BalsaHeaders>(); + headers_two->AppendHeader("two", "fish"); sequence.Append(std::move(headers_two)); EXPECT_TRUE(sequence.HasNext()); @@ -44,13 +45,13 @@ TEST(BalsaHeadersSequenceTest, Clear) { BalsaHeadersSequence sequence; - BalsaHeaders headers_one; - headers_one.AppendHeader("one", "fish"); + auto headers_one = std::make_unique<BalsaHeaders>(); + headers_one->AppendHeader("one", "fish"); sequence.Append(std::move(headers_one)); EXPECT_TRUE(sequence.HasNext()); - BalsaHeaders headers_two; - headers_two.AppendHeader("two", "fish"); + auto headers_two = std::make_unique<BalsaHeaders>(); + headers_two->AppendHeader("two", "fish"); sequence.Append(std::move(headers_two)); EXPECT_TRUE(sequence.HasNext()); @@ -63,8 +64,8 @@ BalsaHeadersSequence sequence; EXPECT_EQ(sequence.PeekNext(), nullptr); - BalsaHeaders headers_one; - headers_one.AppendHeader("one", "fish"); + auto headers_one = std::make_unique<BalsaHeaders>(); + headers_one->AppendHeader("one", "fish"); sequence.Append(std::move(headers_one)); EXPECT_TRUE(sequence.HasNext()); @@ -77,8 +78,8 @@ EXPECT_EQ(sequence.PeekNext(), headers); // Adding more headers should not matter for peeking. - BalsaHeaders headers_two; - headers_two.AppendHeader("two", "fish"); + auto headers_two = std::make_unique<BalsaHeaders>(); + headers_two->AppendHeader("two", "fish"); sequence.Append(std::move(headers_two)); EXPECT_TRUE(sequence.HasNext()); EXPECT_EQ(sequence.PeekNext(), headers); @@ -101,6 +102,21 @@ EXPECT_EQ(sequence.PeekNext(), nullptr); } +TEST(BalsaHeadersSequenceTest, CanRetainValidReference) { + BalsaHeadersSequence sequence; + + auto headers = std::make_unique<BalsaHeaders>(); + headers->AppendHeader("one", "fish"); + + // This reference should still be valid, even after transferring ownership to + // the sequence. + BalsaHeaders* headers_ptr = headers.get(); + + sequence.Append(std::move(headers)); + ASSERT_TRUE(sequence.HasNext()); + EXPECT_EQ(sequence.Next(), headers_ptr); +} + } // namespace } // namespace test } // namespace quiche
diff --git a/quiche/balsa/balsa_visitor_interface.h b/quiche/balsa/balsa_visitor_interface.h index e2c4327..89355f3 100644 --- a/quiche/balsa/balsa_visitor_interface.h +++ b/quiche/balsa/balsa_visitor_interface.h
@@ -6,6 +6,7 @@ #define QUICHE_BALSA_BALSA_VISITOR_INTERFACE_H_ #include <cstddef> +#include <memory> #include "absl/strings/string_view.h" #include "quiche/balsa/balsa_enums.h" @@ -131,7 +132,7 @@ // Arguments: // headers - contains the parsed headers in the order in which they occurred // in the interim response. - virtual void OnInterimHeaders(BalsaHeaders headers) = 0; + virtual void OnInterimHeaders(std::unique_ptr<BalsaHeaders> headers) = 0; // Summary: // Called when the 100 Continue headers are framed and processed. This
diff --git a/quiche/balsa/noop_balsa_visitor.h b/quiche/balsa/noop_balsa_visitor.h index ce82d58..5645070 100644 --- a/quiche/balsa/noop_balsa_visitor.h +++ b/quiche/balsa/noop_balsa_visitor.h
@@ -6,6 +6,7 @@ #define QUICHE_BALSA_NOOP_BALSA_VISITOR_H_ #include <cstddef> +#include <memory> #include "absl/strings/string_view.h" #include "quiche/balsa/balsa_visitor_interface.h" @@ -46,7 +47,7 @@ absl::string_view /*reason_input*/) override {} void OnChunkLength(size_t /*chunk_length*/) override {} void OnChunkExtensionInput(absl::string_view /*input*/) override {} - void OnInterimHeaders(BalsaHeaders /*headers*/) override {} + void OnInterimHeaders(std::unique_ptr<BalsaHeaders> /*headers*/) override {} void ContinueHeaderDone() override {} void HeaderDone() override {} void MessageDone() override {}