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

Source/core/svg/properties/SVGAnimatedProperty.cpp

 /*
  * Copyright (C) Research In Motion Limited 2010. All rights reserved.
  * Copyright (C) 2013 Samsung Electronics. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
@@ skipping 38 matching lines @@
     Vector<SVGAnimatedPropertyDescription> keysToRemove;
 
     const Cache::const_iterator end = cache->end();
     for (Cache::const_iterator it = cache->begin(); it != end; ++it) {
         if (it->key.m_element == element) {
             it->value->resetContextElement();
             keysToRemove.append(it->key);
         }
     }
 
-    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()
 {
     ASSERT(m_contextElement);
     ASSERT_WITH_SECURITY_IMPLICATION(!m_contextElement->m_deletionHasBegun);
     m_contextElement->invalidateSVGAttributes();
     m_contextElement->svgAttributeChanged(m_attributeName);
 }
 
 SVGAnimatedProperty::Cache* SVGAnimatedProperty::animatedPropertyCache()
 {
     static Cache* s_cache = new Cache;
     return s_cache;
 }
 
 } // namespace WebCore