heap: HeapHashSet: Use standard GC types

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

third_party/blink/renderer/modules/geolocation/geolocation.cc

@@ -122,7 +122,9 @@ Geolocation::Geolocation(Navigator& navigator)
     : Supplement<Navigator>(navigator),
       ExecutionContextLifecycleObserver(navigator.DomWindow()),
       PageVisibilityObserver(navigator.DomWindow()->GetFrame()->GetPage()),
+      one_shots_(MakeGarbageCollected<GeoNotifierSet>()),
       watchers_(MakeGarbageCollected<GeolocationWatchers>()),
+      one_shots_being_invoked_(MakeGarbageCollected<GeoNotifierSet>()),
       geolocation_(navigator.DomWindow()),
       geolocation_service_(navigator.DomWindow()) {}
 
@@ -148,7 +150,7 @@ LocalFrame* Geolocation::GetFrame() const {
 
 void Geolocation::ContextDestroyed() {
   StopTimers();
-  one_shots_.clear();
+  one_shots_->clear();
   watchers_->Clear();
 
   StopUpdating();
@@ -198,7 +200,7 @@ void Geolocation::getCurrentPosition(V8PositionCallback* success_callback,
   auto* notifier = MakeGarbageCollected<GeoNotifier>(this, success_callback,
                                                      error_callback, options);
 
-  one_shots_.insert(notifier);
+  one_shots_->insert(notifier);
 
   StartRequest(notifier);
 }
@@ -260,7 +262,7 @@ void Geolocation::FatalErrorOccurred(GeoNotifier* notifier) {
   DCHECK(!notifier->IsTimerActive());
 
   // This request has failed fatally. Remove it from our lists.
-  one_shots_.erase(notifier);
+  one_shots_->erase(notifier);
   watchers_->Remove(notifier);
 
   if (!HasListeners())
@@ -274,8 +276,8 @@ void Geolocation::RequestUsesCachedPosition(GeoNotifier* notifier) {
 
   // If this is a one-shot request, stop it. Otherwise, if the watch still
   // exists, start the service to get updates.
-  if (one_shots_.Contains(notifier)) {
-    one_shots_.erase(notifier);
+  if (one_shots_->Contains(notifier)) {
+    one_shots_->erase(notifier);
   } else if (watchers_->Contains(notifier)) {
     StartUpdating(notifier);
   }
@@ -288,15 +290,15 @@ void Geolocation::RequestTimedOut(GeoNotifier* notifier) {
   DCHECK(!notifier->IsTimerActive());
 
   // If this is a one-shot request, stop it.
-  one_shots_.erase(notifier);
+  one_shots_->erase(notifier);
 
   if (!HasListeners())
     StopUpdating();
 }
 
 bool Geolocation::DoesOwnNotifier(GeoNotifier* notifier) const {
-  return one_shots_.Contains(notifier) ||
-         one_shots_being_invoked_.Contains(notifier) ||
+  return one_shots_->Contains(notifier) ||
+         one_shots_being_invoked_->Contains(notifier) ||
          watchers_->Contains(notifier) ||
          watchers_being_invoked_.Contains(notifier);
 }
@@ -328,7 +330,7 @@ void Geolocation::clearWatch(int watch_id) {
 }
 
 void Geolocation::StopTimers() {
-  for (const auto& notifier : one_shots_) {
+  for (const auto& notifier : *one_shots_) {
     notifier->StopTimer();
   }
 
@@ -340,7 +342,7 @@ void Geolocation::StopTimers() {
 void Geolocation::HandleError(GeolocationPositionError* error) {
   DCHECK(error);
 
-  DCHECK(one_shots_being_invoked_.IsEmpty());
+  DCHECK(one_shots_being_invoked_->IsEmpty());
   DCHECK(watchers_being_invoked_.IsEmpty());
 
   if (error->IsFatal()) {
@@ -370,7 +372,7 @@ void Geolocation::HandleError(GeolocationPositionError* error) {
   // already scheduled must be immediately cancelled according to the spec. But
   // the current implementation doesn't support such case.
   // TODO(mattreynolds): Support watcher cancellation inside notifier callbacks.
-  for (auto& notifier : one_shots_being_invoked_) {
+  for (auto& notifier : *one_shots_being_invoked_) {
     if (error->IsFatal() || !notifier->UseCachedPosition())
       notifier->RunErrorCallback(error);
   }
@@ -387,23 +389,23 @@ void Geolocation::HandleError(GeolocationPositionError* error) {
 
   if (!error->IsFatal()) {
     // Keep the notifiers that are okay with a cached position in |one_shots_|.
-    for (const auto& notifier : one_shots_being_invoked_) {
+    for (const auto& notifier : *one_shots_being_invoked_) {
       if (notifier->UseCachedPosition())
-        one_shots_.InsertWithoutTimerCheck(notifier.Get());
+        one_shots_->InsertWithoutTimerCheck(notifier.Get());
       else
         notifier->StopTimer();
     }
-    one_shots_being_invoked_.ClearWithoutTimerCheck();
+    one_shots_being_invoked_->ClearWithoutTimerCheck();
   }
 
-  one_shots_being_invoked_.clear();
+  one_shots_being_invoked_->clear();
   watchers_being_invoked_.clear();
 }
 
 void Geolocation::MakeSuccessCallbacks() {
   DCHECK(last_position_);
 
-  DCHECK(one_shots_being_invoked_.IsEmpty());
+  DCHECK(one_shots_being_invoked_->IsEmpty());
   DCHECK(watchers_being_invoked_.IsEmpty());
 
   // Set |one_shots_being_invoked_| and |watchers_being_invoked_| to the
@@ -420,7 +422,7 @@ void Geolocation::MakeSuccessCallbacks() {
   // already scheduled must be immediately cancelled according to the spec. But
   // the current implementation doesn't support such case.
   // TODO(mattreynolds): Support watcher cancellation inside notifier callbacks.
-  for (auto& notifier : one_shots_being_invoked_)
+  for (auto& notifier : *one_shots_being_invoked_)
     notifier->RunSuccessCallback(last_position_);
   for (auto& notifier : watchers_being_invoked_)
     notifier->RunSuccessCallback(last_position_);
@@ -428,7 +430,7 @@ void Geolocation::MakeSuccessCallbacks() {
   if (!HasListeners())
     StopUpdating();
 
-  one_shots_being_invoked_.clear();
+  one_shots_being_invoked_->clear();
   watchers_being_invoked_.clear();
 }
 
@@ -511,7 +513,7 @@ void Geolocation::PageVisibilityChanged() {
 }
 
 bool Geolocation::HasPendingActivity() const {
-  return !one_shots_.IsEmpty() || !one_shots_being_invoked_.IsEmpty() ||
+  return !one_shots_->IsEmpty() || !one_shots_being_invoked_->IsEmpty() ||
          !watchers_->IsEmpty() || !watchers_being_invoked_.IsEmpty();
 }
 

third_party/blink/renderer/modules/geolocation/geolocation.h

@@ -120,49 +120,47 @@ class MODULES_EXPORT Geolocation final
  private:
   // Customized HeapHashSet class that checks notifiers' timers. Notifier's
   // timer may be active only when the notifier is owned by the Geolocation.
-  class GeoNotifierSet : private HeapHashSet<Member<GeoNotifier>> {
-    using BaseClass = HeapHashSet<Member<GeoNotifier>>;
-
+  class GeoNotifierSet final : public GarbageCollected<GeoNotifierSet> {
    public:
-    using BaseClass::Trace;
-
-    using BaseClass::const_iterator;
-    using BaseClass::iterator;
+    void Trace(Visitor* visitor) const { visitor->Trace(set_); }
 
-    using BaseClass::begin;
-    using BaseClass::end;
-    using BaseClass::size;
+    auto begin() const { return set_.begin(); }
+    auto end() const { return set_.end(); }
+    auto size() const { return set_.size(); }
 
     auto insert(GeoNotifier* value) {
       DCHECK(!value->IsTimerActive());
-      return BaseClass::insert(value);
+      return set_.insert(value);
     }
 
     void erase(GeoNotifier* value) {
       DCHECK(!value->IsTimerActive());
-      return BaseClass::erase(value);
+      return set_.erase(value);
     }
 
     void clear() {
 #if DCHECK_IS_ON()
-      for (const auto& notifier : *this) {
+      for (const auto& notifier : set_) {
         DCHECK(!notifier->IsTimerActive());
       }
 #endif
-      BaseClass::clear();
+      set_.clear();
     }
 
-    using BaseClass::Contains;
-    using BaseClass::IsEmpty;
+    auto Contains(GeoNotifier* value) const { return set_.Contains(value); }
+    auto IsEmpty() const { return set_.IsEmpty(); }
 
     auto InsertWithoutTimerCheck(GeoNotifier* value) {
-      return BaseClass::insert(value);
+      return set_.insert(value);
     }
-    void ClearWithoutTimerCheck() { BaseClass::clear(); }
+    void ClearWithoutTimerCheck() { set_.clear(); }
+
+   private:
+    HeapHashSet<Member<GeoNotifier>> set_;
   };
 
   bool HasListeners() const {
-    return !one_shots_.IsEmpty() || !watchers_->IsEmpty();
+    return !one_shots_->IsEmpty() || !watchers_->IsEmpty();
   }
 
   void StopTimers();
@@ -204,7 +202,7 @@ class MODULES_EXPORT Geolocation final
   void OnGeolocationPermissionStatusUpdated(GeoNotifier*,
                                             mojom::PermissionStatus);
 
-  GeoNotifierSet one_shots_;
+  Member<GeoNotifierSet> one_shots_;
   Member<GeolocationWatchers> watchers_;
   // GeoNotifiers that are in the middle of invocation.
   //
@@ -217,7 +215,7 @@ class MODULES_EXPORT Geolocation final
   // wrapper-tracing.
   // TODO(https://crbug.com/796145): Remove this hack once on-stack objects
   // get supported by either of wrapper-tracing or unified GC.
-  GeoNotifierSet one_shots_being_invoked_;
+  Member<GeoNotifierSet> one_shots_being_invoked_;
   HeapVector<Member<GeoNotifier>> watchers_being_invoked_;
   Member<Geoposition> last_position_;
 

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

@@ -5,7 +5,9 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_COLLECTION_SUPPORT_HEAP_HASH_SET_H_
 #define THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_COLLECTION_SUPPORT_HEAP_HASH_SET_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/heap/visitor.h"
 #include "third_party/blink/renderer/platform/wtf/hash_set.h"
 
 namespace blink {
@@ -13,12 +15,21 @@ namespace blink {
 template <typename ValueArg,
           typename HashArg = typename DefaultHash<ValueArg>::Hash,
           typename TraitsArg = HashTraits<ValueArg>>
-class HeapHashSet
-    : public HashSet<ValueArg, HashArg, TraitsArg, HeapAllocator> {
-  IS_GARBAGE_COLLECTED_CONTAINER_TYPE();
+class HeapHashSet final
+    : public GarbageCollected<HeapHashSet<ValueArg, HashArg, TraitsArg>>,
+      public HashSet<ValueArg, HashArg, TraitsArg, HeapAllocator> {
   DISALLOW_NEW();
 
-  static void CheckType() {
+ public:
+  HeapHashSet() = default;
+
+  void Trace(Visitor* visitor) const {
+    CheckType();
+    HashSet<ValueArg, HashArg, TraitsArg, HeapAllocator>::Trace(visitor);
+  }
+
+ private:
+  static constexpr void CheckType() {
     static_assert(WTF::IsMemberOrWeakMemberType<ValueArg>::value,
                   "HeapHashSet supports only Member and WeakMember.");
     static_assert(std::is_trivially_destructible<HeapHashSet>::value,
@@ -27,21 +38,8 @@ class HeapHashSet
                   "For hash sets without traceable elements, use HashSet<> "
                   "instead of HeapHashSet<>.");
   }
-
- public:
-  template <typename>
-  static void* AllocateObject(size_t size) {
-    return ThreadHeap::Allocate<HeapHashSet<ValueArg, HashArg, TraitsArg>>(
-        size);
-  }
-
-  HeapHashSet() { CheckType(); }
 };
 
-template <typename T, typename U, typename V>
-struct GCInfoTrait<HeapHashSet<T, U, V>>
-    : public GCInfoTrait<HashSet<T, U, V, HeapAllocator>> {};
-
 }  // namespace blink
 
 #endif  // THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_COLLECTION_SUPPORT_HEAP_HASH_SET_H_

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

@@ -14,6 +14,15 @@
 #include "third_party/blink/renderer/platform/heap/impl/member.h"
 #endif  // !USE_V8_OILPAN
 
+namespace blink {
+
+template <typename T>
+inline void swap(Member<T>& a, Member<T>& b) {
+  a.Swap(b);
+}
+
+}  // namespace blink
+
 namespace WTF {
 
 template <typename T>