[Oilpan]: Make StylePropertySet fully garbage collected.

Source/core/css/resolver/MatchedPropertiesCache.h

Index: Source/core/css/resolver/MatchedPropertiesCache.h
diff --git a/Source/core/css/resolver/MatchedPropertiesCache.h b/Source/core/css/resolver/MatchedPropertiesCache.h
index d86bf01f856cfc35b29ce941a2c0e18c80581dd8..85e863165540ea1779b0b1f90709e2ea37bf8024 100644
--- a/Source/core/css/resolver/MatchedPropertiesCache.h
+++ b/Source/core/css/resolver/MatchedPropertiesCache.h
@@ -23,9 +23,10 @@
 #ifndef MatchedPropertiesCache_h
 #define MatchedPropertiesCache_h
 
+#include "core/css/StylePropertySet.h"
 #include "core/css/resolver/MatchResult.h"
-
 #include "platform/Timer.h"
+#include "platform/heap/Handle.h"
 #include "wtf/Forward.h"
 #include "wtf/HashMap.h"
 #include "wtf/Noncopyable.h"
@@ -35,17 +36,56 @@ namespace WebCore {
 class RenderStyle;
 class StyleResolverState;
 
-struct CachedMatchedProperties {
-    Vector<MatchedProperties> matchedProperties;
+class CachedMatchedProperties FINAL : public NoBaseWillBeGarbageCollectedFinalized<CachedMatchedProperties> {
+
+public:
+    WillBeHeapVector<MatchedProperties> matchedProperties;
     MatchRanges ranges;
     RefPtr<RenderStyle> renderStyle;
     RefPtr<RenderStyle> parentRenderStyle;
 
     void set(const RenderStyle*, const RenderStyle* parentStyle, const MatchResult&);
     void clear();
+    void trace(Visitor* visitor) { visitor->trace(matchedProperties); }
 };
 
+// Specialize the HashTraits for CachedMatchedProperties to check for dead
+// entries in the MatchedPropertiesCache.
+#if ENABLE(OILPAN)
+struct CachedMatchedPropertiesHashTraits : HashTraits<Member<CachedMatchedProperties> > {

[esprehn, 2014/06/26 09:48:53]

Does this really need to be inline? I'd rather it didn't add so much code to
this header.

+    static const WTF::WeakHandlingFlag weakHandlingFlag = WTF::WeakHandlingInCollections;
+    static bool traceInCollection(Visitor* visitor, Member<CachedMatchedProperties>& cachedProperties, WTF::ShouldWeakPointersBeMarkedStrongly strongify)
+    {
+        // Only honor the cache's weakness semantics if the collection
+        // is traced without the strongify flag set to WeakPointersActStrong.

[haraken, 2014/06/25 13:37:11]

... if the collection is traced with WeakPointersActWeak.

[wibling-chromium, 2014/06/26 09:31:00]

Done.

+        // Otherwise just trace the cachedProperties strongly, ie. call trace
+        // on it.
+        if (strongify != WTF::WeakPointersActStrong) {

[haraken, 2014/06/25 13:37:11]

if (strongify == WTF::WeakPointersActWeak)

[wibling-chromium, 2014/06/26 09:31:00]

Done.

+            // 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.
+            HeapVector<MatchedProperties>::iterator it = cachedProperties->matchedProperties.begin();
+            HeapVector<MatchedProperties>::iterator end = cachedProperties->matchedProperties.end();
+            for (; it != end; ++it) {
+                if (!visitor->isAlive(it->properties)) {
+                    // For now report the cache entry as dead. This might not
+                    // be the final result if in a subsequent call to this

[haraken, 2014/06/25 13:37:11]

in a subsequent call to this the properties... => in a subsequent call the
properties...

[wibling-chromium, 2014/06/26 09:30:59]

I have rephrased it a bit.

+                    // 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 normally.

[haraken, 2014/06/25 13:37:11]

normally => strongly

[wibling-chromium, 2014/06/26 09:30:59]

Done.

+        visitor->trace(cachedProperties);
+        return false;
+    }
+};
+#endif
+
 class MatchedPropertiesCache {
+    DISALLOW_ALLOCATION();
     WTF_MAKE_NONCOPYABLE(MatchedPropertiesCache);
 public:
     MatchedPropertiesCache();
@@ -58,7 +98,12 @@ public:
 
     static bool isCacheable(const Element*, const RenderStyle*, const RenderStyle* parentStyle);
 
+    void trace(Visitor*);
+
 private:
+#if ENABLE(OILPAN)
+    typedef HeapHashMap<unsigned, Member<CachedMatchedProperties>, DefaultHash<unsigned>::Hash, HashTraits<unsigned>, CachedMatchedPropertiesHashTraits > Cache;

[haraken, 2014/06/25 13:37:11]

I don't fully understand why this is not a WeakMember.

Without making it a WeakMember, the GC won't remove dead entries from the map
when the GC does the thread-specific weak processing for the map, will it? (The
ephemeron iteration just determines whether each entry should be alive or dead
and does not do any work to remove dead entries from the map.)

Probably I'm misunderstanding.

[wibling-chromium, 2014/06/26 09:31:00]

There is no requirement for a WeakMember to be in the HashMap where we want
weakness. The reason is that we can simulate the weakness by only tracing a
given member (in this case cachedProperties) if it is already alive or if we are
asked to trace it strongly due to an iterator pointing to the map.

In our case we cannot have the nested properties field be a WeakMember because
the same data structure is used in MatchResult and in that case it must be a
Member to ensure we keep the properties alive when doing a conservative GC and
scanning it from the stack.

[haraken, 2014/06/26 10:31:36]

Makes sense, thanks for the clarification!

+#else
     // Every N additions to the matched declaration cache trigger a sweep where entries holding
     // the last reference to a style declaration are garbage collected.
     void sweep(Timer<MatchedPropertiesCache>*);
@@ -66,9 +111,9 @@ private:
     unsigned m_additionsSinceLastSweep;
 
     typedef HashMap<unsigned, OwnPtr<CachedMatchedProperties> > Cache;
-    Cache m_cache;
-
     Timer<MatchedPropertiesCache> m_sweepTimer;
+#endif
+    Cache m_cache;
 };
 
 }