Remove Http2ReconstructObject. I do not believe it is worth it to keep Http2ReconstructObject() and Http2DefaultReconstructObject(): they are quite complicated and provide little value. I am not aware of any bugs caught by this device. gfe-relnote: n/a, test-only code. PiperOrigin-RevId: 289134799 Change-Id: Iedae75a482116753e4a975d8d55b758249757fac
diff --git a/http2/decoder/http2_frame_decoder_test.cc b/http2/decoder/http2_frame_decoder_test.cc index 20e39eb..410202f 100644 --- a/http2/decoder/http2_frame_decoder_test.cc +++ b/http2/decoder/http2_frame_decoder_test.cc
@@ -12,7 +12,6 @@ #include "testing/gtest/include/gtest/gtest.h" #include "net/third_party/quiche/src/http2/http2_constants.h" #include "net/third_party/quiche/src/http2/platform/api/http2_logging.h" -#include "net/third_party/quiche/src/http2/platform/api/http2_reconstruct_object.h" #include "net/third_party/quiche/src/http2/platform/api/http2_test_helpers.h" #include "net/third_party/quiche/src/http2/test_tools/frame_parts.h" #include "net/third_party/quiche/src/http2/test_tools/frame_parts_collector_listener.h" @@ -38,9 +37,9 @@ protected: void SetUp() override { // On any one run of this suite, we'll always choose the same value for - // use_default_reconstruct_ because the random seed is the same for each + // use_default_constructor_ because the random seed is the same for each // test case, but across runs the random seed changes. - use_default_reconstruct_ = Random().OneIn(2); + use_default_constructor_ = Random().OneIn(2); } DecodeStatus StartDecoding(DecodeBuffer* db) override { @@ -48,7 +47,7 @@ collector_.Reset(); PrepareDecoder(); - DecodeStatus status = decoder_.DecodeFrame(db); + DecodeStatus status = decoder_->DecodeFrame(db); if (status != DecodeStatus::kDecodeInProgress) { // Keep track of this so that a concrete test can verify that both fast // and slow decoding paths have been tested. @@ -62,7 +61,7 @@ DecodeStatus ResumeDecoding(DecodeBuffer* db) override { HTTP2_DVLOG(2) << "ResumeDecoding, db->Remaining=" << db->Remaining(); - DecodeStatus status = decoder_.DecodeFrame(db); + DecodeStatus status = decoder_->DecodeFrame(db); if (status != DecodeStatus::kDecodeInProgress) { // Keep track of this so that a concrete test can verify that both fast // and slow decoding paths have been tested. @@ -78,35 +77,31 @@ // stays there until the remaining bytes of the frame's payload have been // skipped over. There are no callbacks for this situation. void ConfirmDiscardsRemainingPayload() { - ASSERT_TRUE(decoder_.IsDiscardingPayload()); + ASSERT_TRUE(decoder_->IsDiscardingPayload()); size_t remaining = - Http2FrameDecoderPeer::remaining_total_payload(&decoder_); + Http2FrameDecoderPeer::remaining_total_payload(decoder_.get()); // The decoder will discard the remaining bytes, but not go beyond that, // which these conditions verify. size_t extra = 10; std::string junk(remaining + extra, '0'); DecodeBuffer tmp(junk); - EXPECT_EQ(DecodeStatus::kDecodeDone, decoder_.DecodeFrame(&tmp)); + EXPECT_EQ(DecodeStatus::kDecodeDone, decoder_->DecodeFrame(&tmp)); EXPECT_EQ(remaining, tmp.Offset()); EXPECT_EQ(extra, tmp.Remaining()); - EXPECT_FALSE(decoder_.IsDiscardingPayload()); + EXPECT_FALSE(decoder_->IsDiscardingPayload()); } void PrepareDecoder() { - // Save and restore the maximum_payload_size when reconstructing - // the decoder. - size_t maximum_payload_size = decoder_.maximum_payload_size(); - // Alternate which constructor is used. - if (use_default_reconstruct_) { - Http2DefaultReconstructObject(&decoder_, RandomPtr()); - decoder_.set_listener(&collector_); + if (use_default_constructor_) { + decoder_ = std::make_unique<Http2FrameDecoder>(); + decoder_->set_listener(&collector_); } else { - Http2ReconstructObject(&decoder_, RandomPtr(), &collector_); + decoder_ = std::make_unique<Http2FrameDecoder>(&collector_); } - decoder_.set_maximum_payload_size(maximum_payload_size); + decoder_->set_maximum_payload_size(maximum_payload_size_); - use_default_reconstruct_ = !use_default_reconstruct_; + use_default_constructor_ = !use_default_constructor_; } void ResetDecodeSpeedCounters() { @@ -216,9 +211,10 @@ // ResumeDecodingPayload. size_t slow_decode_count_ = 0; + uint32_t maximum_payload_size_ = Http2SettingsInfo::DefaultMaxFrameSize(); FramePartsCollectorListener collector_; - Http2FrameDecoder decoder_; - bool use_default_reconstruct_; + std::unique_ptr<Http2FrameDecoder> decoder_; + bool use_default_constructor_; }; //////////////////////////////////////////////////////////////////////////////// @@ -828,7 +824,7 @@ // The decoder calls the listener's OnFrameSizeError method if the frame's // payload is longer than the currently configured maximum payload size. TEST_F(Http2FrameDecoderTest, BeyondMaximum) { - decoder_.set_maximum_payload_size(2); + maximum_payload_size_ = 2; const char kFrameData[] = { '\x00', '\x00', '\x07', // Payload length: 7 '\x00', // DATA
diff --git a/http2/decoder/http2_structure_decoder_test.cc b/http2/decoder/http2_structure_decoder_test.cc index 09db43e..c522c20 100644 --- a/http2/decoder/http2_structure_decoder_test.cc +++ b/http2/decoder/http2_structure_decoder_test.cc
@@ -29,7 +29,6 @@ #include "net/third_party/quiche/src/http2/http2_constants.h" #include "net/third_party/quiche/src/http2/http2_structures_test_util.h" #include "net/third_party/quiche/src/http2/platform/api/http2_logging.h" -#include "net/third_party/quiche/src/http2/platform/api/http2_reconstruct_object.h" #include "net/third_party/quiche/src/http2/platform/api/http2_test_helpers.h" #include "net/third_party/quiche/src/http2/tools/http2_frame_builder.h" #include "net/third_party/quiche/src/http2/tools/random_decoder_test.h" @@ -56,12 +55,12 @@ } DecodeStatus StartDecoding(DecodeBuffer* b) override { - // Overwrite the current contents of |structure_|, in to which we'll + // Overwrite the current contents of |structure_|, into which we'll // decode the buffer, so that we can be confident that we really decoded // the structure every time. - Http2DefaultReconstructObject(&structure_, RandomPtr()); + structure_ = std::make_unique<S>(); uint32_t old_remaining = b->Remaining(); - if (structure_decoder_.Start(&structure_, b)) { + if (structure_decoder_.Start(structure_.get(), b)) { EXPECT_EQ(old_remaining - S::EncodedSize(), b->Remaining()); ++fast_decode_count_; return DecodeStatus::kDecodeDone; @@ -78,7 +77,7 @@ uint32_t old_offset = structure_decoder_.offset(); EXPECT_LT(old_offset, S::EncodedSize()); uint32_t avail = b->Remaining(); - if (structure_decoder_.Resume(&structure_, b)) { + if (structure_decoder_.Resume(structure_.get(), b)) { EXPECT_LE(S::EncodedSize(), old_offset + avail); EXPECT_EQ(b->Remaining(), avail - (S::EncodedSize() - old_offset)); ++slow_decode_count_; @@ -106,7 +105,7 @@ if (expected != nullptr) { validator = [expected, this](const DecodeBuffer& db, DecodeStatus status) -> AssertionResult { - VERIFY_EQ(*expected, structure_); + VERIFY_EQ(*expected, *structure_); return AssertionSuccess(); }; } @@ -138,8 +137,8 @@ } if (expected != nullptr) { HTTP2_DVLOG(1) << "DecodeLeadingStructure expected: " << *expected; - HTTP2_DVLOG(1) << "DecodeLeadingStructure actual: " << structure_; - VERIFY_EQ(*expected, structure_); + HTTP2_DVLOG(1) << "DecodeLeadingStructure actual: " << *structure_; + VERIFY_EQ(*expected, *structure_); } return AssertionSuccess(); } @@ -182,7 +181,7 @@ } uint32_t decode_offset_ = 0; - S structure_; + std::unique_ptr<S> structure_; Http2StructureDecoder structure_decoder_; size_t fast_decode_count_ = 0; size_t slow_decode_count_ = 0; @@ -207,10 +206,10 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ(5u, structure_.payload_length); - EXPECT_EQ(Http2FrameType::HEADERS, structure_.type); - EXPECT_EQ(Http2FrameFlag::PADDED, structure_.flags); - EXPECT_EQ(1u, structure_.stream_id); + EXPECT_EQ(5u, structure_->payload_length); + EXPECT_EQ(Http2FrameType::HEADERS, structure_->type); + EXPECT_EQ(Http2FrameFlag::PADDED, structure_->flags); + EXPECT_EQ(1u, structure_->stream_id); } { // Unlikely input. @@ -223,10 +222,10 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ((1u << 24) - 1u, structure_.payload_length); - EXPECT_EQ(static_cast<Http2FrameType>(255), structure_.type); - EXPECT_EQ(255, structure_.flags); - EXPECT_EQ(0x7FFFFFFFu, structure_.stream_id); + EXPECT_EQ((1u << 24) - 1u, structure_->payload_length); + EXPECT_EQ(static_cast<Http2FrameType>(255), structure_->type); + EXPECT_EQ(255, structure_->flags); + EXPECT_EQ(0x7FFFFFFFu, structure_->stream_id); } } @@ -248,9 +247,9 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ(5u, structure_.stream_dependency); - EXPECT_EQ(256u, structure_.weight); - EXPECT_EQ(true, structure_.is_exclusive); + EXPECT_EQ(5u, structure_->stream_dependency); + EXPECT_EQ(256u, structure_->weight); + EXPECT_EQ(true, structure_->is_exclusive); } { // clang-format off @@ -260,9 +259,9 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ(StreamIdMask(), structure_.stream_dependency); - EXPECT_EQ(1u, structure_.weight); - EXPECT_FALSE(structure_.is_exclusive); + EXPECT_EQ(StreamIdMask(), structure_->stream_dependency); + EXPECT_EQ(1u, structure_->weight); + EXPECT_FALSE(structure_->is_exclusive); } } @@ -283,8 +282,8 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_TRUE(structure_.IsSupportedErrorCode()); - EXPECT_EQ(Http2ErrorCode::PROTOCOL_ERROR, structure_.error_code); + EXPECT_TRUE(structure_->IsSupportedErrorCode()); + EXPECT_EQ(Http2ErrorCode::PROTOCOL_ERROR, structure_->error_code); } { // clang-format off @@ -293,8 +292,8 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_FALSE(structure_.IsSupportedErrorCode()); - EXPECT_EQ(static_cast<Http2ErrorCode>(0xffffffff), structure_.error_code); + EXPECT_FALSE(structure_->IsSupportedErrorCode()); + EXPECT_EQ(static_cast<Http2ErrorCode>(0xffffffff), structure_->error_code); } } @@ -316,9 +315,9 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_TRUE(structure_.IsSupportedParameter()); - EXPECT_EQ(Http2SettingsParameter::HEADER_TABLE_SIZE, structure_.parameter); - EXPECT_EQ(1u << 14, structure_.value); + EXPECT_TRUE(structure_->IsSupportedParameter()); + EXPECT_EQ(Http2SettingsParameter::HEADER_TABLE_SIZE, structure_->parameter); + EXPECT_EQ(1u << 14, structure_->value); } { // clang-format off @@ -328,8 +327,8 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_FALSE(structure_.IsSupportedParameter()); - EXPECT_EQ(static_cast<Http2SettingsParameter>(0), structure_.parameter); + EXPECT_FALSE(structure_->IsSupportedParameter()); + EXPECT_EQ(static_cast<Http2SettingsParameter>(0), structure_->parameter); } } @@ -350,7 +349,7 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ(101010u, structure_.promised_stream_id); + EXPECT_EQ(101010u, structure_->promised_stream_id); } { // Promised stream id has R-bit (reserved for future use) set, which @@ -362,7 +361,7 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ(StreamIdMask(), structure_.promised_stream_id); + EXPECT_EQ(StreamIdMask(), structure_->promised_stream_id); } } @@ -382,7 +381,7 @@ 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, }; ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ(ToStringPiece(kData), ToStringPiece(structure_.opaque_bytes)); + EXPECT_EQ(ToStringPiece(kData), ToStringPiece(structure_->opaque_bytes)); } { // All zeros, detect problems handling NULs. @@ -390,14 +389,14 @@ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, }; ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ(ToStringPiece(kData), ToStringPiece(structure_.opaque_bytes)); + EXPECT_EQ(ToStringPiece(kData), ToStringPiece(structure_->opaque_bytes)); } { const unsigned char kData[] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, }; ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ(ToStringPiece(kData), ToStringPiece(structure_.opaque_bytes)); + EXPECT_EQ(ToStringPiece(kData), ToStringPiece(structure_->opaque_bytes)); } } @@ -419,9 +418,9 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ(0u, structure_.last_stream_id); - EXPECT_TRUE(structure_.IsSupportedErrorCode()); - EXPECT_EQ(Http2ErrorCode::HTTP2_NO_ERROR, structure_.error_code); + EXPECT_EQ(0u, structure_->last_stream_id); + EXPECT_TRUE(structure_->IsSupportedErrorCode()); + EXPECT_EQ(Http2ErrorCode::HTTP2_NO_ERROR, structure_->error_code); } { // clang-format off @@ -431,9 +430,9 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ(1u, structure_.last_stream_id); - EXPECT_TRUE(structure_.IsSupportedErrorCode()); - EXPECT_EQ(Http2ErrorCode::HTTP_1_1_REQUIRED, structure_.error_code); + EXPECT_EQ(1u, structure_->last_stream_id); + EXPECT_TRUE(structure_->IsSupportedErrorCode()); + EXPECT_EQ(Http2ErrorCode::HTTP_1_1_REQUIRED, structure_->error_code); } { // clang-format off @@ -443,9 +442,9 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ(StreamIdMask(), structure_.last_stream_id); // No high-bit. - EXPECT_FALSE(structure_.IsSupportedErrorCode()); - EXPECT_EQ(static_cast<Http2ErrorCode>(0xffffffff), structure_.error_code); + EXPECT_EQ(StreamIdMask(), structure_->last_stream_id); // No high-bit. + EXPECT_FALSE(structure_->IsSupportedErrorCode()); + EXPECT_EQ(static_cast<Http2ErrorCode>(0xffffffff), structure_->error_code); } } @@ -466,7 +465,7 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ(1u << 16, structure_.window_size_increment); + EXPECT_EQ(1u << 16, structure_->window_size_increment); } { // Increment must be non-zero, but we need to be able to decode the invalid @@ -477,7 +476,7 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ(0u, structure_.window_size_increment); + EXPECT_EQ(0u, structure_->window_size_increment); } { // Increment has R-bit (reserved for future use) set, which @@ -489,7 +488,7 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ(StreamIdMask(), structure_.window_size_increment); + EXPECT_EQ(StreamIdMask(), structure_->window_size_increment); } } @@ -510,7 +509,7 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ(0, structure_.origin_length); + EXPECT_EQ(0, structure_->origin_length); } { // clang-format off @@ -519,7 +518,7 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ(20, structure_.origin_length); + EXPECT_EQ(20, structure_->origin_length); } { // clang-format off @@ -528,7 +527,7 @@ }; // clang-format on ASSERT_TRUE(DecodeLeadingStructure(kData)); - EXPECT_EQ(65535, structure_.origin_length); + EXPECT_EQ(65535, structure_->origin_length); } }
diff --git a/http2/decoder/payload_decoders/payload_decoder_base_test_util.cc b/http2/decoder/payload_decoders/payload_decoder_base_test_util.cc index 8e94f03..32969ef 100644 --- a/http2/decoder/payload_decoders/payload_decoder_base_test_util.cc +++ b/http2/decoder/payload_decoders/payload_decoder_base_test_util.cc
@@ -39,11 +39,11 @@ // Reconstruct the FrameDecoderState, prepare the listener, and add it to // the FrameDecoderState. - Http2DefaultReconstructObject(&frame_decoder_state_, RandomPtr()); - frame_decoder_state_.set_listener(PrepareListener()); + frame_decoder_state_ = std::make_unique<FrameDecoderState>(); + frame_decoder_state_->set_listener(PrepareListener()); // Make sure that a listener was provided. - if (frame_decoder_state_.listener() == nullptr) { + if (frame_decoder_state_->listener() == nullptr) { ADD_FAILURE() << "PrepareListener must return a listener."; return DecodeStatus::kDecodeError; } @@ -52,7 +52,8 @@ // Http2FrameHeader whose payload we're about to decode. That header is the // only state that a payload decoder should expect is valid when its Start // method is called. - FrameDecoderStatePeer::set_frame_header(frame_header_, &frame_decoder_state_); + FrameDecoderStatePeer::set_frame_header(frame_header_, + frame_decoder_state_.get()); DecodeStatus status = StartDecodingPayload(db); if (status != DecodeStatus::kDecodeInProgress) { // Keep track of this so that a concrete test can verify that both fast
diff --git a/http2/decoder/payload_decoders/payload_decoder_base_test_util.h b/http2/decoder/payload_decoders/payload_decoder_base_test_util.h index faa99fd..c5a97a5 100644 --- a/http2/decoder/payload_decoders/payload_decoder_base_test_util.h +++ b/http2/decoder/payload_decoders/payload_decoder_base_test_util.h
@@ -20,7 +20,6 @@ #include "net/third_party/quiche/src/http2/http2_constants_test_util.h" #include "net/third_party/quiche/src/http2/http2_structures.h" #include "net/third_party/quiche/src/http2/platform/api/http2_logging.h" -#include "net/third_party/quiche/src/http2/platform/api/http2_reconstruct_object.h" #include "net/third_party/quiche/src/http2/test_tools/frame_parts.h" #include "net/third_party/quiche/src/http2/tools/http2_frame_builder.h" #include "net/third_party/quiche/src/http2/tools/random_decoder_test.h" @@ -62,7 +61,7 @@ frame_header_is_set_ = true; } - FrameDecoderState* mutable_state() { return &frame_decoder_state_; } + FrameDecoderState* mutable_state() { return frame_decoder_state_.get(); } // Randomize the payload decoder, sets the payload decoder's frame_header_, // then start decoding the payload. Called by RandomDecoderTest. This method @@ -103,7 +102,7 @@ private: bool frame_header_is_set_ = false; Http2FrameHeader frame_header_; - FrameDecoderState frame_decoder_state_; + std::unique_ptr<FrameDecoderState> frame_decoder_state_; }; // Base class for payload decoders of type Decoder, with corresponding test @@ -156,7 +155,7 @@ } void PreparePayloadDecoder() override { - Http2DefaultReconstructObject(&payload_decoder_, RandomPtr()); + payload_decoder_ = std::make_unique<Decoder>(); } Http2FrameDecoderListener* PrepareListener() override { @@ -176,14 +175,14 @@ // Start decoding the payload. DecodeStatus StartDecodingPayload(DecodeBuffer* db) override { HTTP2_DVLOG(2) << "StartDecodingPayload, db->Remaining=" << db->Remaining(); - return payload_decoder_.StartDecodingPayload(mutable_state(), db); + return payload_decoder_->StartDecodingPayload(mutable_state(), db); } // Resume decoding the payload. DecodeStatus ResumeDecodingPayload(DecodeBuffer* db) override { HTTP2_DVLOG(2) << "ResumeDecodingPayload, db->Remaining=" << db->Remaining(); - return payload_decoder_.ResumeDecodingPayload(mutable_state(), db); + return payload_decoder_->ResumeDecodingPayload(mutable_state(), db); } // Decode one frame's payload and confirm that the listener recorded the @@ -318,13 +317,7 @@ } Listener listener_; - union { - // Confirm at compile time that Decoder can be in an anonymous union, - // i.e. complain loudly if Decoder has members that prevent this, as it - // becomes annoying and possibly difficult to deal with non-anonymous - // unions and such union members. - Decoder payload_decoder_; - }; + std::unique_ptr<Decoder> payload_decoder_; }; // A base class for tests parameterized by the total number of bytes of
diff --git a/http2/platform/api/http2_reconstruct_object.h b/http2/platform/api/http2_reconstruct_object.h deleted file mode 100644 index ddcb312..0000000 --- a/http2/platform/api/http2_reconstruct_object.h +++ /dev/null
@@ -1,34 +0,0 @@ -// Copyright 2017 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef QUICHE_HTTP2_PLATFORM_API_HTTP2_RECONSTRUCT_OBJECT_H_ -#define QUICHE_HTTP2_PLATFORM_API_HTTP2_RECONSTRUCT_OBJECT_H_ - -#include <utility> - -#include "net/http2/platform/impl/http2_reconstruct_object_impl.h" - -namespace http2 { -namespace test { - -class Http2Random; - -// Reconstruct an object so that it is initialized as when it was first -// constructed. Runs the destructor to handle objects that might own resources, -// and runs the constructor with the provided arguments, if any. -template <class T, class... Args> -void Http2ReconstructObject(T* ptr, Http2Random* rng, Args&&... args) { - Http2ReconstructObjectImpl(ptr, rng, std::forward<Args>(args)...); -} - -// This version applies default-initialization to the object. -template <class T> -void Http2DefaultReconstructObject(T* ptr, Http2Random* rng) { - Http2DefaultReconstructObjectImpl(ptr, rng); -} - -} // namespace test -} // namespace http2 - -#endif // QUICHE_HTTP2_PLATFORM_API_HTTP2_RECONSTRUCT_OBJECT_H_