Revert "css/resolver: Refactor weaknesss in MatchedPropertiesCache"

This reverts commit b936d5f1.

Reason for revert: Flaky crashes on webgl bot; see bug.

Original change's description:
> css/resolver: Refactor weaknesss in MatchedPropertiesCache
> 
> MatchedPropertiesCache was the single user of custom weakness using
> HashTraits. Supporting custom weakness in hash tables is responsible for
> a lot of complications in the tracing protocol.
> 
> MatchedPropertiesCache was using weakness to hold alive a cached entry as long
> as all of its matched properties are alive. Once a single matched property dies
> the entry was removed.
> 
> This can be modeled using a custom weakness callback that performs the same
> logic. Cached entry don't have any outgoing edges except for their cached
> matched properties, so ephemeron semantic is not required. Using a custom weak
> callback is also faster because the callback is only executed once and not part
> of the ephemeron fixed point protocol. The cached entry is unlinked using the weak callback and collected on next GC cylce.
> 
> After this conversion, custom weakness can be removed from hash traits.
> Weakness should always go through already supported types (e.g.
> WeakMember).
> 
> Bug: 1019191
> Change-Id: I52b7b24dcba8a3aaca896f86a351da665667fcf9
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1886611
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Reviewed-by: Omer Katz <omerkatz@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#710693}

TBR=haraken@chromium.org,mlippautz@chromium.org,omerkatz@chromium.org

Change-Id: I30c098eb560e7fa62c6f5bd354188fc2a3eb2ab2
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1019191
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1891190Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710885}

third_party/blink/renderer/core/css/resolver/match_result.h

@@ -46,7 +46,7 @@ struct CORE_EXPORT MatchedProperties {
 
   Member<CSSPropertyValueSet> properties;
 
-  struct Data {
+  struct {
     unsigned link_match_type : 2;
     unsigned valid_property_filter : 2;
     // This is approximately equivalent to the 'shadow-including tree order'.
@@ -61,8 +61,7 @@ struct CORE_EXPORT MatchedProperties {
     //
     // https://drafts.csswg.org/css-scoping/#shadow-cascading
     uint16_t tree_order;
-  };
-  Data types_;
+  } types_;
 };
 
 }  // namespace blink

third_party/blink/renderer/core/css/resolver/matched_properties_cache.cc

@@ -39,10 +39,7 @@ namespace blink {
 void CachedMatchedProperties::Set(const ComputedStyle& style,
                                   const ComputedStyle& parent_style,
                                   const MatchedPropertiesVector& properties) {
-  for (const auto& new_matched_properties : properties) {
-    matched_properties.push_back(new_matched_properties.properties);
-    matched_properties_types.push_back(new_matched_properties.types_);
-  }
+  matched_properties.AppendVector(properties);
 
   // Note that we don't cache the original ComputedStyle instance. It may be
   // further modified.  The ComputedStyle in the cache is really just a holder
@@ -71,37 +68,18 @@ const CachedMatchedProperties* MatchedPropertiesCache::Find(
   CachedMatchedProperties* cache_item = it->value.Get();
   if (!cache_item)
     return nullptr;
-  if (*cache_item != properties)
+
+  wtf_size_t size = properties.size();
+  if (size != cache_item->matched_properties.size())
     return nullptr;
   if (cache_item->computed_style->InsideLink() !=
       style_resolver_state.Style()->InsideLink())
     return nullptr;
-  return cache_item;
-}
-
-bool CachedMatchedProperties::operator==(
-    const MatchedPropertiesVector& properties) {
-  if (properties.size() != matched_properties.size())
-    return false;
-  for (wtf_size_t i = 0; i < properties.size(); ++i) {
-    if (properties[i].properties != matched_properties[i])
-      return false;
-    if (properties[i].types_.link_match_type !=
-        matched_properties_types[i].link_match_type)
-      return false;
-    if (properties[i].types_.tree_order !=
-        matched_properties_types[i].tree_order)
-      return false;
-    if (properties[i].types_.valid_property_filter !=
-        matched_properties_types[i].valid_property_filter)
-      return false;
+  for (wtf_size_t i = 0; i < size; ++i) {
+    if (properties[i] != cache_item->matched_properties[i])
+      return nullptr;
   }
-  return true;
-}
-
-bool CachedMatchedProperties::operator!=(
-    const MatchedPropertiesVector& properties) {
-  return !(*this == properties);
+  return cache_item;
 }
 
 void MatchedPropertiesCache::Add(const ComputedStyle& style,
@@ -180,28 +158,6 @@ bool MatchedPropertiesCache::IsCacheable(const StyleResolverState& state) {
 
 void MatchedPropertiesCache::Trace(blink::Visitor* visitor) {
   visitor->Trace(cache_);
-  visitor->RegisterWeakMembers<
-      MatchedPropertiesCache,
-      &MatchedPropertiesCache::RemoveCachedMatchedPropertiesWithDeadEntries>(
-      this);
-}
-
-void MatchedPropertiesCache::RemoveCachedMatchedPropertiesWithDeadEntries(
-    Visitor* visitor) {
-  Vector<unsigned> to_remove;
-  for (const auto& entry_pair : cache_) {
-    for (const auto& matched_properties :
-         entry_pair.value->matched_properties) {
-      if (!ThreadHeap::IsHeapObjectAlive(matched_properties)) {
-        to_remove.push_back(entry_pair.key);
-        break;
-      }
-    }
-  }
-  // Allocation is forbidden during executing weak callbacks, so the data
-  // structure will not be rehashed here. The next insertion/deletion from
-  // regular code will take care of shrinking accordingly.
-  cache_.RemoveAll(to_remove);
 }
 
 }  // namespace blink

third_party/blink/renderer/core/css/resolver/matched_properties_cache.h

@@ -39,11 +39,7 @@ class StyleResolverState;
 class CachedMatchedProperties final
     : public GarbageCollected<CachedMatchedProperties> {
  public:
-  // Caches data of MachedProperties. See MatchedPropertiesCache::Cache for
-  // semantics.
-  Vector<UntracedMember<CSSPropertyValueSet>> matched_properties;
-  Vector<MatchedProperties::Data> matched_properties_types;
-
+  HeapVector<MatchedProperties> matched_properties;
   scoped_refptr<ComputedStyle> computed_style;
   scoped_refptr<ComputedStyle> parent_computed_style;
 
@@ -51,11 +47,55 @@ class CachedMatchedProperties final
            const ComputedStyle& parent_style,
            const MatchedPropertiesVector&);
   void Clear();
+  void Trace(blink::Visitor* visitor) { visitor->Trace(matched_properties); }
+};
 
-  void Trace(blink::Visitor*) {}
-
-  bool operator==(const MatchedPropertiesVector& properties);
-  bool operator!=(const MatchedPropertiesVector& properties);
+// Specialize the HashTraits for CachedMatchedProperties to check for dead
+// entries in the MatchedPropertiesCache.
+struct CachedMatchedPropertiesHashTraits
+    : HashTraits<Member<CachedMatchedProperties>> {
+  static const WTF::WeakHandlingFlag kWeakHandlingFlag = WTF::kWeakHandling;
+
+  static bool IsAlive(Member<CachedMatchedProperties>& cached_properties) {
+    // Semantics see |CachedMatchedPropertiesHashTraits::TraceInCollection|.
+    if (cached_properties) {
+      for (const auto& matched_properties :
+           cached_properties->matched_properties) {
+        if (!ThreadHeap::IsHeapObjectAlive(matched_properties.properties)) {
+          return false;
+        }
+      }
+    }
+    return true;
+  }
+
+  template <typename VisitorDispatcher>
+  static bool TraceInCollection(
+      VisitorDispatcher visitor,
+      Member<CachedMatchedProperties>& cached_properties,
+      WTF::WeakHandlingFlag weakness) {
+    // Only honor the cache's weakness semantics if the collection is traced
+    // with |kWeakPointersActWeak|. Otherwise just trace the cachedProperties
+    // strongly, i.e., call trace on it.
+    if (cached_properties && weakness == WTF::kWeakHandling) {
+      // A given cache entry is only kept alive if none of the MatchedProperties
+      // in the CachedMatchedProperties value contain a dead "properties" field.
+      // If there is a dead field the entire cache entry is removed.
+      for (const auto& matched_properties :
+           cached_properties->matched_properties) {
+        if (!ThreadHeap::IsHeapObjectAlive(matched_properties.properties)) {
+          // For now report the cache entry as dead. This might not
+          // be the final result if in a subsequent call for this entry,
+          // the "properties" field has been marked via another path.
+          return true;
+        }
+      }
+    }
+    // At this point none of the entries in the matchedProperties vector
+    // had a dead "properties" field so trace CachedMatchedProperties strongly.
+    visitor->Trace(cached_properties);
+    return false;
+  }
 };
 
 class CORE_EXPORT MatchedPropertiesCache {
@@ -82,17 +122,11 @@ class CORE_EXPORT MatchedPropertiesCache {
   void Trace(blink::Visitor*);
 
  private:
-  // The cache is mapping a hash to a cached entry where the entry is kept as
-  // long as *all* properties referred to by the entry are alive. This requires
-  // custom weakness which is managed through
-  // |RemoveCachedMatchedPropertiesWithDeadEntries|.
   using Cache = HeapHashMap<unsigned,
                             Member<CachedMatchedProperties>,
                             DefaultHash<unsigned>::Hash,
-                            HashTraits<unsigned>>;
-
-  void RemoveCachedMatchedPropertiesWithDeadEntries(Visitor*);
-
+                            HashTraits<unsigned>,
+                            CachedMatchedPropertiesHashTraits>;
   Cache cache_;
   DISALLOW_COPY_AND_ASSIGN(MatchedPropertiesCache);
 };