Make RepeatedField and RepeatedPtrField be destructor skippable for p…

…roto arenas.

This way, when e.g. `Arena::CreateMessage<RepeatedPtrField<Msg>>` is called with a non-null arena, we won't end up calling the destructor of the repeated field later.

Also add some missing includes in unittest.

PiperOrigin-RevId: 527382003

src/google/protobuf/BUILD.bazel

@@ -810,6 +810,7 @@ cc_library(
     deps = [
         ":cc_lite_test_protos",
         ":test_util2",
+        "@com_google_absl//absl/container:flat_hash_set",
         "@com_google_absl//absl/log:absl_check",
         "@com_google_googletest//:gtest",
     ],
@@ -1296,9 +1297,13 @@ cc_test(
     }),
     deps = [
         ":cc_test_protos",
+        ":lite_test_util",
         ":protobuf",
+        ":protobuf_lite",
+        "//src/google/protobuf/io",
         "//src/google/protobuf/stubs",
         "//src/google/protobuf/testing",
+        "@com_google_absl//absl/types:span",
         "@com_google_googletest//:gtest",
         "@com_google_googletest//:gtest_main",
     ],

src/google/protobuf/arena_test_util.h

@@ -31,6 +31,9 @@
 #ifndef GOOGLE_PROTOBUF_ARENA_TEST_UTIL_H__
 #define GOOGLE_PROTOBUF_ARENA_TEST_UTIL_H__
 
+#include <cstddef>
+
+#include "absl/container/flat_hash_set.h"
 #include "absl/log/absl_check.h"
 #include "google/protobuf/arena.h"
 #include "google/protobuf/io/coded_stream.h"
@@ -84,6 +87,25 @@ struct ArenaTestPeer {
   }
 };
 
+struct CleanupGrowthInfo {
+  size_t space_used;
+  absl::flat_hash_set<void*> cleanups;
+};
+
+template <typename Func>
+CleanupGrowthInfo CleanupGrowth(Arena& arena, Func f) {
+  auto old_space_used = arena.SpaceUsed();
+  auto old_cleanups = ArenaTestPeer::PeekCleanupListForTesting(&arena);
+  f();
+  auto new_space_used = arena.SpaceUsed();
+  auto new_cleanups = ArenaTestPeer::PeekCleanupListForTesting(&arena);
+  CleanupGrowthInfo res;
+  res.space_used = new_space_used - old_space_used;
+  res.cleanups.insert(new_cleanups.begin(), new_cleanups.end());
+  for (auto p : old_cleanups) res.cleanups.erase(p);
+  return res;
+}
+
 class NoHeapChecker {
  public:
   NoHeapChecker() { capture_alloc.Hook(); }

src/google/protobuf/map_test.cc

@@ -28,6 +28,7 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+#include <string>
 #include <type_traits>
 #include <utility>
 

src/google/protobuf/repeated_field.h

@@ -137,6 +137,16 @@ void memswap(char* a, char* b) {
 template <typename Element>
 class RepeatedIterator;
 
+// We can't skip the destructor for, e.g., arena allocated RepeatedField<Cord>.
+template <typename Element,
+          bool Trivial = Arena::is_destructor_skippable<Element>::value>
+struct RepeatedFieldDestructorSkippableBase {};
+
+template <typename Element>
+struct RepeatedFieldDestructorSkippableBase<Element, true> {
+  using DestructorSkippable_ = void;
+};
+
 }  // namespace internal
 
 // RepeatedField is used to represent repeated fields of a primitive type (in
@@ -148,7 +158,8 @@ class RepeatedIterator;
 // We have to specialize several methods in the Cord case to get the memory
 // management right; e.g. swapping when appropriate, etc.
 template <typename Element>
-class RepeatedField final {
+class RepeatedField final
+    : private internal::RepeatedFieldDestructorSkippableBase<Element> {
   static_assert(
       alignof(Arena) >= alignof(Element),
       "We only support types that have an alignment smaller than Arena");

src/google/protobuf/repeated_field_unittest.cc

@@ -38,21 +38,32 @@
 #include "google/protobuf/repeated_field.h"
 
 #include <algorithm>
+#include <cstddef>
+#include <cstdint>
 #include <cstdlib>
 #include <iterator>
 #include <limits>
 #include <list>
 #include <sstream>
 #include <string>
 #include <type_traits>
+#include <utility>
 #include <vector>
 
+#include "google/protobuf/arena.h"
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 #include "absl/log/absl_check.h"
 #include "absl/numeric/bits.h"
 #include "absl/strings/cord.h"
 #include "absl/strings/str_cat.h"
+#include "absl/types/span.h"
+#include "google/protobuf/arena_test_util.h"
+#include "google/protobuf/io/coded_stream.h"
+#include "google/protobuf/io/zero_copy_stream_impl_lite.h"
+#include "google/protobuf/parse_context.h"
+#include "google/protobuf/repeated_field.h"
+#include "google/protobuf/repeated_ptr_field.h"
 #include "google/protobuf/unittest.pb.h"
 
 
@@ -1211,6 +1222,19 @@ TEST(RepeatedField, PoisonsMemoryOnAssign) {
 
 #endif
 
+TEST(RepeatedField, Cleanups) {
+  Arena arena;
+  auto growth = internal::CleanupGrowth(
+      arena, [&] { Arena::CreateMessage<RepeatedField<int>>(&arena); });
+  EXPECT_THAT(growth.cleanups, testing::IsEmpty());
+
+  void* ptr;
+  growth = internal::CleanupGrowth(arena, [&] {
+    ptr = Arena::CreateMessage<RepeatedField<absl::Cord>>(&arena);
+  });
+  EXPECT_THAT(growth.cleanups, testing::UnorderedElementsAre(ptr));
+}
+
 // ===================================================================
 // RepeatedPtrField tests.  These pretty much just mirror the RepeatedField
 // tests above.
@@ -1992,6 +2016,19 @@ TEST(RepeatedPtrField, DeleteSubrange) {
   // DeleteSubrange is a trivial extension of ExtendSubrange.
 }
 
+TEST(RepeatedPtrField, Cleanups) {
+  Arena arena;
+  auto growth = internal::CleanupGrowth(arena, [&] {
+    Arena::CreateMessage<RepeatedPtrField<std::string>>(&arena);
+  });
+  EXPECT_THAT(growth.cleanups, testing::IsEmpty());
+
+  growth = internal::CleanupGrowth(arena, [&] {
+    Arena::CreateMessage<RepeatedPtrField<TestAllTypes>>(&arena);
+  });
+  EXPECT_THAT(growth.cleanups, testing::IsEmpty());
+}
+
 // ===================================================================
 
 // Iterator tests stolen from net/proto/proto-array_unittest.

src/google/protobuf/repeated_ptr_field.h

@@ -1224,7 +1224,8 @@ class RepeatedPtrField final : private internal::RepeatedPtrFieldBase {
   template <typename T>
   friend struct WeakRepeatedPtrField;
 
-  typedef void InternalArenaConstructable_;
+  using InternalArenaConstructable_ = void;
+  using DestructorSkippable_ = void;
 
 };