Cleanup the SVGTreeScopeResources interface

third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp

Index: third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp
diff --git a/third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp b/third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp
index 9617851877e31b78a83c69b68631c2ae233c8ad2..35acd7b803d2ea9bc1751a7f3bcc803fba3d0be8 100644
--- a/third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp
+++ b/third_party/WebKit/Source/core/svg/SVGTreeScopeResources.cpp
@@ -38,7 +38,7 @@ void SVGTreeScopeResources::updateResource(
   // Update cached resources of pending clients.
   for (Element* clientElement : *pendingElements) {
     DCHECK(clientElement->hasPendingResources());
-    clearHasPendingResourcesIfPossible(clientElement);
+    clearHasPendingResourcesIfPossible(*clientElement);
 
     LayoutObject* layoutObject = clientElement->layoutObject();
     if (!layoutObject)
@@ -68,89 +68,61 @@ LayoutSVGResourceContainer* SVGTreeScopeResources::resourceById(
 }
 
 void SVGTreeScopeResources::addPendingResource(const AtomicString& id,
-                                               Element* element) {
-  DCHECK(element);
-  DCHECK(element->isConnected());
+                                               Element& element) {
+  DCHECK(element.isConnected());
 
   if (id.isEmpty())
     return;
-
-  HeapHashMap<AtomicString, Member<SVGPendingElements>>::AddResult result =
-      m_pendingResources.insert(id, nullptr);
+  auto result = m_pendingResources.insert(id, nullptr);
   if (result.isNewEntry)
     result.storedValue->value = new SVGPendingElements;
-  result.storedValue->value->insert(element);
+  result.storedValue->value->insert(&element);
 
-  element->setHasPendingResources();
+  element.setHasPendingResources();
 }
 
-bool SVGTreeScopeResources::hasPendingResource(const AtomicString& id) const {
+bool SVGTreeScopeResources::isElementPendingResource(
+    Element& element,
+    const AtomicString& id) const {
   if (id.isEmpty())
     return false;
-  return m_pendingResources.contains(id);
+  const SVGPendingElements* pendingElements = m_pendingResources.get(id);
+  return pendingElements && pendingElements->contains(&element);
 }
 
-bool SVGTreeScopeResources::isElementPendingResources(Element* element) const {
+void SVGTreeScopeResources::clearHasPendingResourcesIfPossible(
+    Element& element) {
   // This algorithm takes time proportional to the number of pending resources
   // and need not.
   // If performance becomes an issue we can keep a counted set of elements and
   // answer the question efficiently.
-  DCHECK(element);
-
   for (const auto& entry : m_pendingResources) {
     SVGPendingElements* elements = entry.value.get();
     DCHECK(elements);
-    if (elements->contains(element))
-      return true;
+    if (elements->contains(&element))
+      return;
   }
-  return false;
-}
-
-bool SVGTreeScopeResources::isElementPendingResource(
-    Element* element,
-    const AtomicString& id) const {
-  DCHECK(element);
-  if (!hasPendingResource(id))
-    return false;
-  return m_pendingResources.get(id)->contains(element);
-}
-
-void SVGTreeScopeResources::clearHasPendingResourcesIfPossible(
-    Element* element) {
-  if (!isElementPendingResources(element))
-    element->clearHasPendingResources();
+  element.clearHasPendingResources();
 }
 
 void SVGTreeScopeResources::removeElementFromPendingResources(
-    Element* element) {
-  DCHECK(element);
-
+    Element& element) {
+  if (m_pendingResources.isEmpty() || !element.hasPendingResources())
+    return;
   // Remove the element from pending resources.
-  if (!m_pendingResources.isEmpty() && element->hasPendingResources()) {
-    Vector<AtomicString> toBeRemoved;
-    for (const auto& entry : m_pendingResources) {
-      SVGPendingElements* elements = entry.value.get();
-      DCHECK(elements);
-      DCHECK(!elements->isEmpty());
-
-      elements->erase(element);
-      if (elements->isEmpty())
-        toBeRemoved.push_back(entry.key);
-    }
-
-    clearHasPendingResourcesIfPossible(element);
-
-    // We use the removePendingResource function here because it deals with set
-    // lifetime correctly.
-    for (const AtomicString& id : toBeRemoved)
-      removePendingResource(id);
+  Vector<AtomicString> toBeRemoved;
+  for (const auto& entry : m_pendingResources) {
+    SVGPendingElements* elements = entry.value.get();
+    DCHECK(elements);
+    DCHECK(!elements->isEmpty());
+
+    elements->erase(&element);
+    if (elements->isEmpty())
+      toBeRemoved.push_back(entry.key);
   }
-}
+  m_pendingResources.removeAll(toBeRemoved);

[Stephen Chennney, 2017/02/23 17:00:50]

From memory this has to come after the clearHasPendingResourcesIfPossible for
use-after-free or some other bug, but maybe I'm wrong. Could you check whether
clearHasPendingResourcesIfPossible can possibly access one of the toBeRemoved
elements? And whether the possible destruction of objects due to
m_pendingResources.removeAll may require that element's pending resources have
been cleared first?

It's probably sufficient to look at change history around this code, going back
at least 4 years. But that might be harder than re-thinking from scratch.

[fs, 2017/02/23 17:10:45]

This structure is on the GC-heap, so if there're are references still [1]
they'll be kept alive. I can imagine this was quite a different story with
ref-counting though *brrr*.

[1] Theoretically there shouldn't be any, or this method (removeElementFrom...)
has a bug - because this method iterates m_pendingResources and removes
'element' from all the sets it's in, while clearHasPendingResourcesIfPossible
iterates m_pendingResources and clears the bit if 'element' is not in any of the
sets...

 
-SVGTreeScopeResources::SVGPendingElements*
-SVGTreeScopeResources::removePendingResource(const AtomicString& id) {
-  DCHECK(m_pendingResources.contains(id));
-  return m_pendingResources.take(id);
+  clearHasPendingResourcesIfPossible(element);
 }
 
 void SVGTreeScopeResources::notifyResourceAvailable(const AtomicString& id) {
@@ -174,7 +146,7 @@ void SVGTreeScopeResources::notifyResourceAvailable(const AtomicString& id) {
     else
       clientElement->buildPendingResource();
 
-    clearHasPendingResourcesIfPossible(clientElement);
+    clearHasPendingResourcesIfPossible(*clientElement);
   }
 }