Oilpan: Add Destructor to Promptly Free Stack Allocated HeapHashCollections

This CL adds the destructor in order to promptly freed stack-allocated
HeapHashCollections, and also added tests to check it work exactly.

heap_allocator: We needs to check whether the backing collection is
sweep-forbidden when the destructor is called. So, we added this check flag.


Bug: 854480
Change-Id: If9fc8324b839714ae978665b6f7540d7f5ea15e4
Reviewed-on: https://chromium-review.googlesource.com/1123969
Commit-Queue: Haruka Matsumura <harukamt@google.com>
Reviewed-by: Hayato Ito <hayato@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575570}

third_party/blink/renderer/core/dom/node_rare_data.cc

@@ -47,6 +47,10 @@ struct SameSizeAsNodeRareData {
   unsigned bitfields_;
 };
 
+NodeMutationObserverData* NodeMutationObserverData::Create() {
+  return new NodeMutationObserverData;
+}
+
 static_assert(sizeof(NodeRareData) == sizeof(SameSizeAsNodeRareData),
               "NodeRareData should stay small");
 

third_party/blink/renderer/core/dom/node_rare_data.h

@@ -39,9 +39,7 @@ class NodeListsNodeData;
 class NodeMutationObserverData final
     : public GarbageCollected<NodeMutationObserverData> {
  public:
-  static NodeMutationObserverData* Create() {
-    return new NodeMutationObserverData;
-  }
+  static NodeMutationObserverData* Create();
 
   const HeapVector<TraceWrapperMember<MutationObserverRegistration>>&
   Registry() {

third_party/blink/renderer/platform/heap/heap_allocator.h

@@ -168,6 +168,10 @@ class PLATFORM_EXPORT HeapAllocator {
     return ThreadState::Current()->IsObjectResurrectionForbidden();
   }
 
+  static bool IsSweepForbidden() {
+    return ThreadState::Current()->SweepForbidden();
+  }
+
   template <typename T>
   static bool IsHeapObjectAlive(T* object) {
     return ThreadHeap::IsHeapObjectAlive(object);

third_party/blink/renderer/platform/heap/heap_test.cc

@@ -6882,4 +6882,49 @@ TEST(HeapTest, PersistentHeapVectorCopyAssignment) {
   PreciselyCollectGarbage();
 }
 
+TEST(HeapTest, PromptlyFreeStackAllocatedHeapHashSet) {
+  NormalPageArena* normal_arena = static_cast<NormalPageArena*>(
+      ThreadState::Current()->Heap().Arena(BlinkGC::kHashTableArenaIndex));
+  CHECK(normal_arena);
+  Address before;
+  {
+    HeapHashSet<Member<IntWrapper>> hash_set;
+    hash_set.insert(new IntWrapper(0));
+    before = normal_arena->CurrentAllocationPoint();
+  }
+  Address after = normal_arena->CurrentAllocationPoint();
+  // We check the allocation point to see if promptly freed
+  EXPECT_NE(after, before);
+}
+
+TEST(HeapTest, PromptlyFreeStackAllocatedHeapListHashSet) {
+  NormalPageArena* normal_arena = static_cast<NormalPageArena*>(
+      ThreadState::Current()->Heap().Arena(BlinkGC::kHashTableArenaIndex));
+  CHECK(normal_arena);
+  Address before;
+  {
+    HeapListHashSet<Member<IntWrapper>> list_hash_set;
+    list_hash_set.insert(new IntWrapper(0));
+    before = normal_arena->CurrentAllocationPoint();
+  }
+  Address after = normal_arena->CurrentAllocationPoint();
+  // We check the allocation point to see if promptly freed
+  EXPECT_NE(after, before);
+}
+
+TEST(HeapTest, PromptlyFreeStackAllocatedHeapLinkedHashSet) {
+  NormalPageArena* normal_arena = static_cast<NormalPageArena*>(
+      ThreadState::Current()->Heap().Arena(BlinkGC::kHashTableArenaIndex));
+  CHECK(normal_arena);
+  Address before;
+  {
+    HeapLinkedHashSet<Member<IntWrapper>> linked_hash_set;
+    linked_hash_set.insert(new IntWrapper(0));
+    before = normal_arena->CurrentAllocationPoint();
+  }
+  Address after = normal_arena->CurrentAllocationPoint();
+  // We check the allocation point to see if promptly freed
+  EXPECT_NE(after, before);
+}
+
 }  // namespace blink

third_party/blink/renderer/platform/heap/incremental_marking_test.cc

@@ -844,10 +844,11 @@ template <typename Container>
 void Move() {
   Object* obj = Object::Create();
   Container container1;
+  Container container2;
   container1.insert(obj);
   {
     ExpectWriteBarrierFires scope(ThreadState::Current(), {obj});
-    Container container2(std::move(container1));
+    container2 = std::move(container1);
   }
 }
 

third_party/blink/renderer/platform/wtf/allocator/partition_allocator.h

@@ -105,6 +105,7 @@ class WTF_EXPORT PartitionAllocator {
 
   static bool IsAllocationAllowed() { return true; }
   static bool IsObjectResurrectionForbidden() { return false; }
+  static bool IsSweepForbidden() { return false; }
 
   static void EnterGCForbiddenScope() {}
   static void LeaveGCForbiddenScope() {}

third_party/blink/renderer/platform/wtf/hash_table.h

@@ -29,7 +29,6 @@
 #include "third_party/blink/renderer/platform/wtf/allocator.h"
 #include "third_party/blink/renderer/platform/wtf/allocator/partition_allocator.h"
 #include "third_party/blink/renderer/platform/wtf/assertions.h"
-#include "third_party/blink/renderer/platform/wtf/conditional_destructor.h"
 #include "third_party/blink/renderer/platform/wtf/construct_traits.h"
 #include "third_party/blink/renderer/platform/wtf/hash_traits.h"
 
@@ -691,15 +690,7 @@ template <typename Key,
           typename Traits,
           typename KeyTraits,
           typename Allocator>
-class HashTable final
-    : public ConditionalDestructor<HashTable<Key,
-                                             Value,
-                                             Extractor,
-                                             HashFunctions,
-                                             Traits,
-                                             KeyTraits,
-                                             Allocator>,
-                                   Allocator::kIsGarbageCollected> {
+class HashTable final {
   DISALLOW_NEW();
 
  public:
@@ -730,10 +721,16 @@ class HashTable final
   typedef HashTableAddResult<HashTable, ValueType> AddResult;
 
   HashTable();
-  void Finalize() {
-    DCHECK(!Allocator::kIsGarbageCollected);
+
+  // For design of the destructor, please refer to
+  // [here](https://docs.google.com/document/d/1AoGTvb3tNLx2tD1hNqAfLRLmyM59GM0O-7rCHTT_7_U/)
+  ~HashTable() {
     if (LIKELY(!table_))
       return;
+    // If this is called during sweeping, it must not touch other heap objects
+    // such as the backing.
+    if (Allocator::IsSweepForbidden())
+      return;
     EnterAccessForbiddenScope();
     DeleteAllBucketsAndDeallocate(table_, table_size_);
     LeaveAccessForbiddenScope();

third_party/blink/renderer/platform/wtf/list_hash_set.h

@@ -76,10 +76,7 @@ template <typename ValueArg,
           typename HashArg = typename DefaultHash<ValueArg>::Hash,
           typename AllocatorArg =
               ListHashSetAllocator<ValueArg, inlineCapacity>>
-class ListHashSet
-    : public ConditionalDestructor<
-          ListHashSet<ValueArg, inlineCapacity, HashArg, AllocatorArg>,
-          AllocatorArg::kIsGarbageCollected> {
+class ListHashSet {
   typedef AllocatorArg Allocator;
   USE_ALLOCATOR(ListHashSet, Allocator);
 
@@ -149,7 +146,7 @@ class ListHashSet
   ListHashSet(ListHashSet&&);
   ListHashSet& operator=(const ListHashSet&);
   ListHashSet& operator=(ListHashSet&&);
-  void Finalize();
+  ~ListHashSet();
 
   void Swap(ListHashSet&);
 
@@ -798,10 +795,14 @@ inline void ListHashSet<T, inlineCapacity, U, V>::Swap(ListHashSet& other) {
   allocator_provider_.Swap(other.allocator_provider_);
 }
 
+// For design of the destructor, please refer to
+// [here](https://docs.google.com/document/d/1AoGTvb3tNLx2tD1hNqAfLRLmyM59GM0O-7rCHTT_7_U/)
 template <typename T, size_t inlineCapacity, typename U, typename V>
-inline void ListHashSet<T, inlineCapacity, U, V>::Finalize() {
-  static_assert(!Allocator::kIsGarbageCollected,
-                "heap allocated ListHashSet should never call finalize()");
+inline ListHashSet<T, inlineCapacity, U, V>::~ListHashSet() {
+  // If this is called during GC sweeping, it must not touch other heap objects
+  // such as the ListHashSetNodes that is touching in DeleteAllNodes().
+  if (Allocator::IsSweepForbidden())
+    return;
   DeleteAllNodes();
   allocator_provider_.ReleaseAllocator();
 }