[Oilpan]: Make StylePropertySet fully garbage collected.

Source/core/css/resolver/MatchedPropertiesCache.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 2004-2005 Allan Sandfeld Jensen (kde@carewolf.com)
  * Copyright (C) 2006, 2007 Nicholas Shanks (webkit@nickshanks.com)
  * Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012, 2013 Apple Inc. All rights reserved.
  * Copyright (C) 2007 Alexey Proskuryakov <ap@webkit.org>
  * Copyright (C) 2007, 2008 Eric Seidel <eric@webkit.org>
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  * Copyright (c) 2011, Code Aurora Forum. All rights reserved.
  * Copyright (C) Research In Motion Limited 2011. All rights reserved.
@@ skipping 38 matching lines @@
 void CachedMatchedProperties::clear()
 {
     matchedProperties.clear();
     renderStyle = nullptr;
     parentRenderStyle = nullptr;
 }
 
 MatchedPropertiesCache::MatchedPropertiesCache()
     : m_additionsSinceLastSweep(0)
     , m_sweepTimer(this, &MatchedPropertiesCache::sweep)
+#if ENABLE(OILPAN)
+    , m_cacheNeedsCleaning(false)
+#endif
 {
 }
 
 const CachedMatchedProperties* MatchedPropertiesCache::find(unsigned hash, const StyleResolverState& styleResolverState, const MatchResult& matchResult)
 {
     ASSERT(hash);
 
+#if ENABLE(OILPAN)
+    // If we had a GC since the last find we could have entries that are now invalid.
+    // Sweep the cache to ensure it is in a valid state before doing the find.
+    if (m_cacheNeedsCleaning)
+        sweep(0);

[haraken, 2014/05/08 11:43:45]

I understand what you're doing here, but this significantly increases the
likelihood of sweeping the cache. So we want to invent a better approach to
handle this.
Can we just disallow resize() during a GC? (I think that previously we were
disallowing resize() during a GC because we were not allowed to allocate any
on-heap object during a GC.)

[wibling-chromium, 2014/05/08 12:38:55]

It does increase the likelihood, but it will only happen once per GC or when the
timer fires (and that is only in oilpan). I have talked to Erik about if we can
get a way to process collections during weak callbacks and ensure there is no
reallocations which would be ideal.
See above.

+#endif
+
     Cache::iterator it = m_cache.find(hash);
     if (it == m_cache.end())
         return 0;
     CachedMatchedProperties* cacheItem = it->value.get();
     ASSERT(cacheItem);
 
     size_t size = matchResult.matchedProperties.size();
     if (size != cacheItem->matchedProperties.size())
         return 0;
     if (cacheItem->renderStyle->insideLink() != styleResolverState.style()->insideLink())
@@ skipping 12 matching lines @@
     static const unsigned maxAdditionsBetweenSweeps = 100;
     if (++m_additionsSinceLastSweep >= maxAdditionsBetweenSweeps
         && !m_sweepTimer.isActive()) {
         static const unsigned sweepTimeInSeconds = 60;
         m_sweepTimer.startOneShot(sweepTimeInSeconds, FROM_HERE);
     }
 
     ASSERT(hash);
     Cache::AddResult addResult = m_cache.add(hash, nullptr);
     if (addResult.isNewEntry)
-        addResult.storedValue->value = adoptPtr(new CachedMatchedProperties);
+        addResult.storedValue->value = adoptPtrWillBeNoop(new CachedMatchedProperties);
 
     CachedMatchedProperties* cacheItem = addResult.storedValue->value.get();
     if (!addResult.isNewEntry)
         cacheItem->clear();
 
     cacheItem->set(style, parentStyle, matchResult);
 }
 
 void MatchedPropertiesCache::clear()
 {
     m_cache.clear();
 }
 
 void MatchedPropertiesCache::clearViewportDependent()
 {
     Vector<unsigned, 16> toRemove;
     for (Cache::iterator it = m_cache.begin(); it != m_cache.end(); ++it) {
         CachedMatchedProperties* cacheItem = it->value.get();
         if (cacheItem->renderStyle->hasViewportUnits())
             toRemove.append(it->key);
     }
     m_cache.removeAll(toRemove);
 }
 
 void MatchedPropertiesCache::sweep(Timer<MatchedPropertiesCache>*)
 {
+#if ENABLE(OILPAN)
+    m_cacheNeedsCleaning = false;
+#endif
     // Look for cache entries containing a style declaration with a single ref and remove them.
     // This may happen when an element attribute mutation causes it to generate a new inlineStyle()
     // or presentationAttributeStyle(), potentially leaving this cache with the last ref on the old one.
     Vector<unsigned, 16> toRemove;
     Cache::iterator it = m_cache.begin();
     Cache::iterator end = m_cache.end();
     for (; it != end; ++it) {
         CachedMatchedProperties* cacheItem = it->value.get();
-        Vector<MatchedProperties>& matchedProperties = cacheItem->matchedProperties;
+        WillBeHeapVector<MatchedProperties>& matchedProperties = cacheItem->matchedProperties;
         for (size_t i = 0; i < matchedProperties.size(); ++i) {
-            if (matchedProperties[i].properties->hasOneRef()) {
+#if ENABLE(OILPAN)
+            if (!matchedProperties[i].properties)
+#else
+            if (matchedProperties[i].properties->hasOneRef())

[haraken, 2014/05/08 11:43:45]

Even if we make m_cache a hash map of weak members, I'm afraid it won't solve
the problem. Given that StyleRule holds a strong reference to StylePropertySets
and that StyleRule is basically alive while a Document is alive (because
StyleElement has the same lifetime as the Document in common cases), the weak
members won't be cleared until the Document dies. Thus, even if we make m_cache
a hash map of weak members, we will end up bloating the cache until the Document
dies.

[wibling-chromium, 2014/05/08 12:38:55]

I don't quite follow. If we make m_cache a hash map of weak members what will
keep CachedMatchedProperties alive? It was previously an OwnPtr so if we make it
weak it will just die in the next GC round. Or are you suggesting something
else?

[haraken, 2014/05/08 12:53:38]

(Probably I'm misunderstanding.)

- First of all, WillBeHeapHashMap<unsigned,
OwnPtrWillBeWeakMember<CachedMatchedProperties> > won't solve the issue. The
target of the weak processing should be
CachedMatchedProperties::matchedProperties[i].properties, not
CachedMatchedProperties.

- Even if we implement the weak processing for
CachedMatchedProperties::matchedProperties[i].properties, it won't solve the
issue either. This is because the StyleRule retains a strong member to
CachedMatchedProperties::matchedProperties[i].properties, and thus the weak
member is not cleared until the Document that contains the StyleRule dies. If
so, we will end up bloating the cache until the Document dies. (This is why
we're currently removing the cache entry when matchedProperties[i].properties
has one ref. The one ref is a reference from the StyleRule.)

tasak@ would know better about this. tasak@, would you correct me if I'm wrong?

+#endif
+            {
                 toRemove.append(it->key);
                 break;
             }
         }
     }
     m_cache.removeAll(toRemove);
     m_additionsSinceLastSweep = 0;
 }
 
 bool MatchedPropertiesCache::isCacheable(const Element* element, const RenderStyle* style, const RenderStyle* parentStyle)
@@ skipping 13 matching lines @@
         return false;
     // CSSPropertyInternalCallback sets the rule's selector name into the RenderStyle, and that's not recalculated if the RenderStyle is loaded from the cache, so don't cache it.
     if (!style->callbackSelectors().isEmpty())
         return false;
     // The cache assumes static knowledge about which properties are inherited.
     if (parentStyle->hasExplicitlyInheritedProperties())
         return false;
     return true;
 }
 
+void MatchedPropertiesCache::trace(Visitor* visitor)
+{
+    visitor->trace(m_cache);
+    visitor->registerWeakMembers<MatchedPropertiesCache, &MatchedPropertiesCache::processWeakMembers>(this);
 }
+
+void MatchedPropertiesCache::processWeakMembers(Visitor*)
+{
+#if ENABLE(OILPAN)
+    m_cacheNeedsCleaning = true;
+#endif
+}
+
+}