[SVG] Defer recursive detachAnimatedProperty call to avoid touching invalidated iterator

Source/core/svg/properties/SVGAnimatedProperty.cpp

Index: Source/core/svg/properties/SVGAnimatedProperty.cpp
diff --git a/Source/core/svg/properties/SVGAnimatedProperty.cpp b/Source/core/svg/properties/SVGAnimatedProperty.cpp
index 2258ad93f57ec028c6defd8dd123d7983db95770..20a796c5182a1a28f0fe0ede9e51476870a1a81a 100644
--- a/Source/core/svg/properties/SVGAnimatedProperty.cpp
+++ b/Source/core/svg/properties/SVGAnimatedProperty.cpp
@@ -56,8 +56,14 @@ void SVGAnimatedProperty::detachAnimatedPropertiesForElement(SVGElement* element
         }
     }
 
-    for (Vector<SVGAnimatedPropertyDescription>::const_iterator it = keysToRemove.begin(); it != keysToRemove.end(); ++it)
+    // Workaround for http://crbug.com/333156 :

[pdr., 2014/01/14 04:33:25]

Please remove the workaround comment, as I think we're actually fixing the bug
even though it's local.

[kouhei (in TOK), 2014/01/14 04:51:27]

Done.

+    // There are unknown cases where detachAnimatedPropertiesForElement is called recursively from ~SVGAnimatedProperty.
+    // This vector makes this function safe by deferring the recursive call until we finish touching the HashMap.
+    Vector<RefPtr<SVGAnimatedProperty> > deferredPropertyDestruct;

[pdr., 2014/01/14 04:33:25]

I would also change this comment slightly to just say "There are cases where..."
because we certainly have such a case :)

[kouhei (in TOK), 2014/01/14 04:51:27]

Done.

+    for (Vector<SVGAnimatedPropertyDescription>::const_iterator it = keysToRemove.begin(); it != keysToRemove.end(); ++it) {
+        deferredPropertyDestruct.append(cache->get(*it));

[pdr., 2014/01/14 04:33:25]

I'm okay with a temporary workaround but I think we need to fully understand the
crash or we risk hiding a larger issue. For example, SVGPolyElement has a custom
points attribute that may affect the new property implementation.

Can you explain the chain of events that's leading to this bug?

I see that we have a call to detachAnimatedPropertiesForElement from an
SVGPolyElement dtor, then a SVGSVGElement dtor, and then another call to
detachAnimatedPropertiesForElement.

Is the bug that we are destructing 'this' SVGAnimatedProperty from the nested
detachAnimatedPropertiesForElement?

Would the following work or is the Vector required?
for (...) {
    RefPtr<SVGAniamtedProperty> protect = cache->get(*it);
    cache->remove(*it);
}

[kouhei (in TOK), 2014/01/14 04:45:07]

No. The problem is that SVGAnimatedProperty is somehow holding a reference to
SVGSVGElement. Vector<RefPtr<FrameView> > appearing in the stacktrace indicates
layout() may be related to this, but I'm not sure.
- detachAnimatedPropertiesForElement for SVGPolyElement destructs the property
holding the reference
  - Note that this is inside HashMap::remove().
- ~SVGSVGElement is removeLastRef-ed
- ~SVGSVGElement also calls detachAnimatedPropertiesForElement for itself.
- detachAnimatedPropertiesForElement calls HashMap::remove(), but this inside
another HashMap::remove() being called at detachAnimatedPropertisForElement for
SVGPolyElement. This invalidates the iterator used inside the first remove().
 
Yes. That is more clever.

[kouhei (in TOK), 2014/01/14 04:51:27]

Done.

         cache->remove(*it);
+    }
 }
 
 void SVGAnimatedProperty::commitChange()