Cleanup the SVGTreeScopeResources interface

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

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/svg/SVGTreeScopeResources.h"
 
 #include "core/dom/Element.h"
 #include "core/dom/TreeScope.h"
 #include "core/layout/svg/LayoutSVGResourceContainer.h"
 #include "core/layout/svg/SVGResourcesCache.h"
@@ skipping 20 matching lines @@
     return;
   // Replaces resource if already present, to handle potential id changes.
   m_resources.set(id, resource);
 
   SVGPendingElements* pendingElements = m_pendingResources.take(id);
   if (!pendingElements)
     return;
   // Update cached resources of pending clients.
   for (Element* clientElement : *pendingElements) {
     DCHECK(clientElement->hasPendingResources());
-    clearHasPendingResourcesIfPossible(clientElement);
+    clearHasPendingResourcesIfPossible(*clientElement);
 
     LayoutObject* layoutObject = clientElement->layoutObject();
     if (!layoutObject)
       continue;
     DCHECK(layoutObject->isSVG());
 
     StyleDifference diff;
     diff.setNeedsFullLayout();
     SVGResourcesCache::clientStyleChanged(layoutObject, diff,
                                           layoutObject->styleRef());
     layoutObject->setNeedsLayoutAndFullPaintInvalidation(
         LayoutInvalidationReason::SvgResourceInvalidated);
   }
 }
 
 void SVGTreeScopeResources::removeResource(const AtomicString& id) {
   if (id.isEmpty())
     return;
   m_resources.erase(id);
 }
 
 LayoutSVGResourceContainer* SVGTreeScopeResources::resourceById(
     const AtomicString& id) const {
   if (id.isEmpty())
     return nullptr;
   return m_resources.get(id);
 }
 
 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.
+  Vector<AtomicString> toBeRemoved;
+  for (const auto& entry : m_pendingResources) {
+    SVGPendingElements* elements = entry.value.get();
+    DCHECK(elements);
+    DCHECK(!elements->isEmpty());
 
-  // 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);
+  }
+  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...

 
-      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);
-  }
-}
-
-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) {
   if (id.isEmpty())
     return;
   // Get pending elements for this id.
   SVGPendingElements* pendingElements = m_pendingResources.take(id);
   if (!pendingElements)
     return;
   // Rebuild pending resources for each client of a pending resource that is
   // being removed.
   for (Element* clientElement : *pendingElements) {
     DCHECK(clientElement->hasPendingResources());
     if (!clientElement->hasPendingResources())
       continue;
     // TODO(fs): Ideally we'd always resolve pending resources async instead of
     // inside insertedInto and svgAttributeChanged. For now we only do it for
     // <use> since that would stamp out DOM.
     if (isSVGUseElement(clientElement))
       toSVGUseElement(clientElement)->invalidateShadowTree();
     else
       clientElement->buildPendingResource();
 
-    clearHasPendingResourcesIfPossible(clientElement);
+    clearHasPendingResourcesIfPossible(*clientElement);
   }
 }
 
 DEFINE_TRACE(SVGTreeScopeResources) {
   visitor->trace(m_pendingResources);
 }
 }