heap: HeapVector: Use standard GC types

In addition, avoid using base::Optional<> within fields of
GarbageCollected objects. This does not work well in general, as
emplacing a new value constructs an object in place which does not
trigger a write barrier. Eventually, this will lead to heap
corruptions. The check is implemented in the GC plugin but compilation
now also fails because of missing trace traits.

base::Optional<> used from stack or parameters is supported and stay
unaffected of this change.

Bug: 1056170
Change-Id: I197c0bf0975823a77c873a7dbfd2fdd9f2668ed5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2621892
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842180}

third_party/blink/renderer/core/paint/paint_timing_detector.cc

@@ -440,7 +440,6 @@ void PaintTimingDetector::Trace(Visitor* visitor) const {
   visitor->Trace(frame_view_);
   visitor->Trace(largest_contentful_paint_calculator_);
   visitor->Trace(callback_manager_);
-  visitor->Trace(visualizer_);
 }
 
 void PaintTimingCallbackManagerImpl::

third_party/blink/renderer/core/paint/text_paint_timing_detector.cc

@@ -257,7 +257,7 @@ void TextRecordsManager::RemoveVisibleRecord(const LayoutObject& object) {
 }
 
 void TextRecordsManager::CleanUpLargestTextPaint() {
-  ltp_manager_.reset();
+  ltp_manager_.Clear();
 }
 
 void TextRecordsManager::RemoveInvisibleRecord(const LayoutObject& object) {
@@ -370,9 +370,10 @@ base::WeakPtr<TextRecord> LargestTextPaintManager::FindLargestPaintCandidate() {
 
 TextRecordsManager::TextRecordsManager(
     LocalFrameView* frame_view,
-    PaintTimingDetector* paint_timing_detector) {
-  ltp_manager_.emplace(frame_view, paint_timing_detector);
-}
+    PaintTimingDetector* paint_timing_detector)
+    : ltp_manager_(MakeGarbageCollected<LargestTextPaintManager>(
+          frame_view,
+          paint_timing_detector)) {}
 
 void TextRecordsManager::Trace(Visitor* visitor) const {
   visitor->Trace(text_element_timing_);

third_party/blink/renderer/core/paint/text_paint_timing_detector.h

@@ -47,8 +47,8 @@ class TextRecord : public base::SupportsWeakPtr<TextRecord> {
   base::TimeTicks paint_time = base::TimeTicks();
 };
 
-class CORE_EXPORT LargestTextPaintManager {
-  DISALLOW_NEW();
+class CORE_EXPORT LargestTextPaintManager final
+    : public GarbageCollected<LargestTextPaintManager> {
   using TextRecordSetComparator = bool (*)(const base::WeakPtr<TextRecord>&,
                                            const base::WeakPtr<TextRecord>&);
   using TextRecordSet =
@@ -173,9 +173,7 @@ class CORE_EXPORT TextRecordsManager {
   // candidate.
   void ReportLargestIgnoredText();
 
-  inline bool IsRecordingLargestTextPaint() const {
-    return ltp_manager_.has_value();
-  }
+  inline bool IsRecordingLargestTextPaint() const { return ltp_manager_; }
 
   void Trace(Visitor*) const;
 
@@ -198,7 +196,7 @@ class CORE_EXPORT TextRecordsManager {
   // LCP computations, even if the size of the text itself is not 0. They are
   // considered invisible objects by Largest Contentful Paint.
   Deque<std::unique_ptr<TextRecord>> size_zero_texts_queued_for_paint_time_;
-  base::Optional<LargestTextPaintManager> ltp_manager_;
+  Member<LargestTextPaintManager> ltp_manager_;
   Member<TextElementTiming> text_element_timing_;
 };
 

third_party/blink/renderer/core/paint/text_paint_timing_detector_test.cc

@@ -73,7 +73,7 @@ class TextPaintTimingDetectorTest : public testing::Test {
         .GetTextPaintTimingDetector();
   }
 
-  base::Optional<LargestTextPaintManager>& GetLargestTextPaintManager() {
+  LargestTextPaintManager* GetLargestTextPaintManager() {
     return GetTextPaintTimingDetector()->records_manager_.ltp_manager_;
   }
 

third_party/blink/renderer/core/testing/dictionary_test.cc

@@ -66,8 +66,10 @@ void DictionaryTest::set(const InternalDictionary* testing_dictionary) {
     enum_or_null_member_ = testing_dictionary->enumOrNullMember();
   if (testing_dictionary->hasElementMember())
     element_member_ = testing_dictionary->elementMember();
-  if (testing_dictionary->hasElementOrNullMember())
+  if (testing_dictionary->hasElementOrNullMember()) {
     element_or_null_member_ = testing_dictionary->elementOrNullMember();
+    has_element_or_null_member_ = true;
+  }
   if (testing_dictionary->hasObjectMember())
     object_member_ = testing_dictionary->objectMember();
   object_or_null_member_with_default_ =
@@ -75,7 +77,9 @@ void DictionaryTest::set(const InternalDictionary* testing_dictionary) {
   if (testing_dictionary->hasDoubleOrStringMember())
     double_or_string_member_ = testing_dictionary->doubleOrStringMember();
   if (testing_dictionary->hasDoubleOrStringSequenceMember()) {
-    double_or_string_sequence_member_ =
+    double_or_string_sequence_or_null_member_ =
+        MakeGarbageCollected<HeapVector<DoubleOrString>>();
+    *double_or_string_sequence_or_null_member_ =
         testing_dictionary->doubleOrStringSequenceMember();
   }
   // eventTargetOrNullMember has a default null value.
@@ -149,10 +153,12 @@ void DictionaryTest::Reset() {
   enum_member_with_default_ = String();
   enum_or_null_member_ = base::nullopt;
   element_member_ = nullptr;
-  element_or_null_member_.reset();
+  element_or_null_member_.Clear();
+  has_element_or_null_member_ = false;
   object_member_ = ScriptValue();
   object_or_null_member_with_default_ = ScriptValue();
   double_or_string_member_ = DoubleOrString();
+  double_or_string_sequence_or_null_member_ = nullptr;
   event_target_or_null_member_ = nullptr;
   derived_string_member_ = base::nullopt;
   derived_string_member_with_default_ = String();
@@ -211,15 +217,15 @@ void DictionaryTest::GetInternals(InternalDictionary* dict) {
     dict->setEnumOrNullMember(enum_or_null_member_.value());
   if (element_member_)
     dict->setElementMember(element_member_);
-  if (element_or_null_member_.has_value())
-    dict->setElementOrNullMember(element_or_null_member_.value());
+  if (has_element_or_null_member_)
+    dict->setElementOrNullMember(element_or_null_member_);
   dict->setObjectMember(object_member_);
   dict->setObjectOrNullMemberWithDefault(object_or_null_member_with_default_);
   if (!double_or_string_member_.IsNull())
     dict->setDoubleOrStringMember(double_or_string_member_);
-  if (double_or_string_sequence_member_) {
+  if (double_or_string_sequence_or_null_member_) {
     dict->setDoubleOrStringSequenceMember(
-        double_or_string_sequence_member_.value());
+        *double_or_string_sequence_or_null_member_);
   }
   dict->setEventTargetOrNullMember(event_target_or_null_member_);
   dict->setInternalEnumOrInternalEnumSequenceMember(
@@ -251,7 +257,7 @@ void DictionaryTest::Trace(Visitor* visitor) const {
   visitor->Trace(element_or_null_member_);
   visitor->Trace(object_member_);
   visitor->Trace(object_or_null_member_with_default_);
-  visitor->Trace(double_or_string_sequence_member_);
+  visitor->Trace(double_or_string_sequence_or_null_member_);
   visitor->Trace(event_target_or_null_member_);
   visitor->Trace(any_member_);
   visitor->Trace(callback_function_member_);

third_party/blink/renderer/core/testing/dictionary_test.h

@@ -56,6 +56,8 @@ class DictionaryTest : public ScriptWrappable {
   // Some members are not wrapped with Optional because:
   //  - |longMemberWithDefault| has a non-null default value
   //  - String and PtrTypes can express whether they are null
+  //  - base::Optional does not work with GarbageCollected types when used on
+  //  heap.
   base::Optional<int> long_member_;
   base::Optional<int> long_member_with_clamp_;
   base::Optional<int> long_member_with_enforce_range_;
@@ -82,11 +84,12 @@ class DictionaryTest : public ScriptWrappable {
   base::Optional<String> enum_or_null_member_;
 #endif
   Member<Element> element_member_;
-  base::Optional<Member<Element>> element_or_null_member_;
+  Member<Element> element_or_null_member_;
+  bool has_element_or_null_member_ = false;
   ScriptValue object_member_;
   ScriptValue object_or_null_member_with_default_;
   DoubleOrString double_or_string_member_;
-  base::Optional<HeapVector<DoubleOrString>> double_or_string_sequence_member_;
+  Member<HeapVector<DoubleOrString>> double_or_string_sequence_or_null_member_;
   Member<EventTarget> event_target_or_null_member_;
   base::Optional<String> derived_string_member_;
   String derived_string_member_with_default_;

third_party/blink/renderer/modules/mediasource/track_default_list.cc

@@ -59,8 +59,6 @@ TrackDefault* TrackDefaultList::item(unsigned index) const {
   return track_defaults_[index].Get();
 }
 
-TrackDefaultList::TrackDefaultList() = default;
-
 TrackDefaultList::TrackDefaultList(
     const HeapVector<Member<TrackDefault>>& track_defaults)
     : track_defaults_(track_defaults) {}

third_party/blink/renderer/modules/mediasource/track_default_list.h

@@ -21,7 +21,7 @@ class TrackDefaultList final : public ScriptWrappable {
   static TrackDefaultList* Create(const HeapVector<Member<TrackDefault>>&,
                                   ExceptionState&);
 
-  TrackDefaultList();
+  TrackDefaultList() = default;
   explicit TrackDefaultList(const HeapVector<Member<TrackDefault>>&);
 
   unsigned length() const { return track_defaults_.size(); }
@@ -30,7 +30,7 @@ class TrackDefaultList final : public ScriptWrappable {
   void Trace(Visitor*) const override;
 
  private:
-  const HeapVector<Member<TrackDefault>> track_defaults_;
+  const HeapVector<Member<TrackDefault>> track_defaults_{};
 };
 
 }  // namespace blink

third_party/blink/renderer/modules/xr/xr_render_state.cc

@@ -63,7 +63,6 @@ base::Optional<double> XRRenderState::inlineVerticalFieldOfView() const {
 
 void XRRenderState::Trace(Visitor* visitor) const {
   visitor->Trace(base_layer_);
-  visitor->Trace(inline_vertical_fov_);
   ScriptWrappable::Trace(visitor);
 }
 

third_party/blink/renderer/platform/heap/collection_support/heap_vector.h

@@ -5,6 +5,7 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_COLLECTION_SUPPORT_HEAP_VECTOR_H_
 #define THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_COLLECTION_SUPPORT_HEAP_VECTOR_H_
 
+#include "third_party/blink/renderer/platform/heap/heap.h"
 #include "third_party/blink/renderer/platform/heap/heap_allocator_impl.h"
 #include "third_party/blink/renderer/platform/wtf/type_traits.h"
 #include "third_party/blink/renderer/platform/wtf/vector.h"
@@ -12,56 +13,25 @@
 namespace blink {
 
 template <typename T, wtf_size_t inlineCapacity = 0>
-class HeapVector final : public Vector<T, inlineCapacity, HeapAllocator> {
-  IS_GARBAGE_COLLECTED_CONTAINER_TYPE();
+class HeapVector final : public GarbageCollected<HeapVector<T, inlineCapacity>>,
+                         public Vector<T, inlineCapacity, HeapAllocator> {
   DISALLOW_NEW();
 
-  static void CheckType() {
-    static_assert(
-        std::is_trivially_destructible<HeapVector>::value || inlineCapacity,
-        "HeapVector must be trivially destructible.");
-    static_assert(WTF::IsTraceable<T>::value,
-                  "For vectors without traceable elements, use Vector<> "
-                  "instead of HeapVector<>.");
-    static_assert(!WTF::IsWeak<T>::value,
-                  "Weak types are not allowed in HeapVector.");
-    static_assert(WTF::IsTraceableInCollectionTrait<VectorTraits<T>>::value,
-                  "Type must be traceable in collection");
-  }
-
  public:
-  template <typename>
-  static void* AllocateObject(size_t size) {
-    // On-heap HeapVectors generally should not have inline capacity, but it is
-    // hard to avoid when using a type alias. Hence we only disallow the
-    // VectorTraits<T>::kNeedsDestruction case for now.
-    static_assert(inlineCapacity == 0 || !VectorTraits<T>::kNeedsDestruction,
-                  "on-heap HeapVector<> should not have an inline capacity");
-    return ThreadHeap::Allocate<HeapVector<T, inlineCapacity>>(size);
-  }
-
-  HeapVector() { CheckType(); }
+  HeapVector() = default;
 
   explicit HeapVector(wtf_size_t size)
-      : Vector<T, inlineCapacity, HeapAllocator>(size) {
-    CheckType();
-  }
+      : Vector<T, inlineCapacity, HeapAllocator>(size) {}
 
   HeapVector(wtf_size_t size, const T& val)
-      : Vector<T, inlineCapacity, HeapAllocator>(size, val) {
-    CheckType();
-  }
+      : Vector<T, inlineCapacity, HeapAllocator>(size, val) {}
 
   template <wtf_size_t otherCapacity>
   HeapVector(const HeapVector<T, otherCapacity>& other)  // NOLINT
-      : Vector<T, inlineCapacity, HeapAllocator>(other) {
-    CheckType();
-  }
+      : Vector<T, inlineCapacity, HeapAllocator>(other) {}
 
   HeapVector(const HeapVector& other)
-      : Vector<T, inlineCapacity, HeapAllocator>(other) {
-    CheckType();
-  }
+      : Vector<T, inlineCapacity, HeapAllocator>(other) {}
 
   HeapVector& operator=(const HeapVector& other) {
     Vector<T, inlineCapacity, HeapAllocator>::operator=(other);
@@ -69,9 +39,7 @@ class HeapVector final : public Vector<T, inlineCapacity, HeapAllocator> {
   }
 
   HeapVector(HeapVector&& other) noexcept
-      : Vector<T, inlineCapacity, HeapAllocator>(std::move(other)) {
-    CheckType();
-  }
+      : Vector<T, inlineCapacity, HeapAllocator>(std::move(other)) {}
 
   HeapVector& operator=(HeapVector&& other) noexcept {
     Vector<T, inlineCapacity, HeapAllocator>::operator=(std::move(other));
@@ -79,8 +47,25 @@ class HeapVector final : public Vector<T, inlineCapacity, HeapAllocator> {
   }
 
   HeapVector(std::initializer_list<T> elements)
-      : Vector<T, inlineCapacity, HeapAllocator>(elements) {
+      : Vector<T, inlineCapacity, HeapAllocator>(elements) {}
+
+  void Trace(Visitor* visitor) const {
     CheckType();
+    Vector<T, inlineCapacity, HeapAllocator>::Trace(visitor);
+  }
+
+ private:
+  static constexpr void CheckType() {
+    static_assert(
+        std::is_trivially_destructible<HeapVector>::value || inlineCapacity,
+        "HeapVector must be trivially destructible.");
+    static_assert(WTF::IsTraceable<T>::value,
+                  "For vectors without traceable elements, use Vector<> "
+                  "instead of HeapVector<>.");
+    static_assert(!WTF::IsWeak<T>::value,
+                  "Weak types are not allowed in HeapVector.");
+    static_assert(WTF::IsTraceableInCollectionTrait<VectorTraits<T>>::value,
+                  "Type must be traceable in collection");
   }
 };
 

third_party/blink/renderer/platform/heap/impl/trace_traits.h

@@ -186,22 +186,6 @@ struct TraceTrait<std::pair<T, U>> {
   }
 };
 
-// While using base::Optional<T> with garbage-collected types is generally
-// disallowed by the OptionalGarbageCollected check in blink_gc_plugin,
-// garbage-collected containers such as HeapVector are allowed and need to be
-// traced.
-template <typename T>
-struct TraceTrait<base::Optional<T>> {
-  STATIC_ONLY(TraceTrait);
-
- public:
-  static void Trace(Visitor* visitor, const base::Optional<T>* optional) {
-    if (*optional != base::nullopt) {
-      TraceIfNeeded<T>::Trace(visitor, optional->value());
-    }
-  }
-};
-
 // Helper for processing ephemerons represented as KeyValuePair. Reorders
 // parameters if needed so that KeyType is always weak.
 template <typename _KeyType,

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

@@ -65,15 +65,6 @@ class __thisIsHereToForceASemicolonAfterThisMacro;
  private:                                   \
   friend class ::WTF::internal::__thisIsHereToForceASemicolonAfterThisMacro
 
-#define IS_GARBAGE_COLLECTED_CONTAINER_TYPE()         \
-  IS_GARBAGE_COLLECTED_TYPE();                        \
-                                                      \
- public:                                              \
-  using IsGarbageCollectedCollectionTypeMarker = int; \
-                                                      \
- private:                                             \
-  friend class ::WTF::internal::__thisIsHereToForceASemicolonAfterThisMacro
-
 #if defined(__clang__)
 #define ANNOTATE_STACK_ALLOCATED \
   __attribute__((annotate("blink_stack_allocated")))