Many assertion failures and crashes on SVG path animation cases when JS garbage collection happens quickly.

Based on upstream fix by said@apple.com at http://trac.webkit.org/changeset/197125

Since the whole document was leaking once an SVGAnimatedProperty was created
so there was no way to produce this bug. After fixing the leak, one crash
and one assert got uncovered. Both of them happen because of the fact:
"if an SVGAnimatedProperty is not referenced it will be deleted."

* svg/SVGPathElement.cpp:
(WebCore::SVGPathElement::lookupOrCreateDWrapper):
The code in this function was assuming that the wrapper will be created
only once which happens when SVGAnimatedProperty::lookupOrCreateWrapper()
is called. Before making this single call, lookupOrCreateDWrapper() was
building an initial SVGPathSegList from byte stream. But now
SVGAnimatedProperty::lookupWrapper() can return false even after creating
the SVGAnimatedProperty because it was deleted later. Calling
buildSVGPathSegListFromByteStream() more than once was causing
SVGAnimatedListPropertyTearOff::animationStarted() to fire the assertion
ASSERT(m_values.size() == m_wrappers.size()) because the path segments were
appended twice to m_values which is in fact SVGPathElement::m_pathSegList.value.
The fix is to build the initial SVGPathSegList only once which should happen
when m_pathSegList.value.isEmpty().

(WebCore::SVGPathElement::animatedPropertyWillBeDeleted):
* svg/SVGPathElement.h:
* svg/properties/SVGAnimatedPathSegListPropertyTearOff.h:
(WebCore::SVGAnimatedPathSegListPropertyTearOff::~SVGAnimatedPathSegListPropertyTearOff):
SVGPathElement is assuming the following equivalence relation:
m_pathSegList.shouldSynchronize ~ SVGAnimatedProperty_is_created_and_not_null.
SVGPathElement::animatedPathSegList() and animatedNormalizedPathSegList()
set m_pathSegList.shouldSynchronize to true when SVGAnimatedProperty is
created but nothing sets m_pathSegList.shouldSynchronize back to false.
This was not a problem when the SVGAnimatedProperty was leaking but after
ensuring it is deleted when it is not referenced this equivalence relation
becomes untrue sometimes. This caused SVGPathElement::svgAttributeChanged()
to crash when we check m_pathSegList.shouldSynchronize and if it is true we
assume that SVGAnimatedProperty::lookupWrapper() will return a non-null pointer
and therefore we deference this pointer and call SVGAnimatedProperty::isAnimating().
To fix this crash we need to set m_pathSegList.shouldSynchronize back to false
when the associated SVGAnimatedProperty is deleted.

Change-Id: I05be755635b02d0d76105fc2eb21c2f013298c4e
Reviewed-by: Michael Brüning <michael.bruning@theqtcompany.com>

3 files changed, 21 insertions, 3 deletions

diff --git a/Source/WebCore/svg/SVGPathElement.cpp b/Source/WebCore/svg/SVGPathElement.cpp
index b030c9a83..c34c476f4 100644
--- a/Source/WebCore/svg/SVGPathElement.cpp
+++ b/Source/WebCore/svg/SVGPathElement.cpp
@@ -329,13 +329,22 @@ PassRefPtr<SVGAnimatedProperty> SVGPathElement::lookupOrCreateDWrapper(SVGElemen
     if (RefPtr<SVGAnimatedProperty> property = SVGAnimatedProperty::lookupWrapper<SVGPathElement, SVGAnimatedPathSegListPropertyTearOff>(ownerType, dPropertyInfo()))
         return property;
 
-    // Build initial SVGPathSegList.
-    buildSVGPathSegListFromByteStream(ownerType->m_pathByteStream.get(), ownerType, ownerType->m_pathSegList.value, UnalteredParsing);
+    if (ownerType->m_pathSegList.value.isEmpty())
+        buildSVGPathSegListFromByteStream(ownerType->m_pathByteStream.get(), ownerType, ownerType->m_pathSegList.value, UnalteredParsing);
 
     return SVGAnimatedProperty::lookupOrCreateWrapper<SVGPathElement, SVGAnimatedPathSegListPropertyTearOff, SVGPathSegList>
            (ownerType, dPropertyInfo(), ownerType->m_pathSegList.value);
 }
 
+void SVGPathElement::animatedPropertyWillBeDeleted()
+{
+    // m_pathSegList.shouldSynchronize is set to true when the 'd' wrapper for m_pathSegList
+    // is created and cached. We need to reset it back to false when this wrapper is deleted
+    // so we can be sure if shouldSynchronize is true, SVGAnimatedProperty::lookupWrapper()
+    // will return a valid cached 'd' wrapper for the m_pathSegList.
+    m_pathSegList.shouldSynchronize = false;
+}
+
 void SVGPathElement::synchronizeD(SVGElement* contextElement)
 {
     ASSERT(contextElement);
diff --git a/Source/WebCore/svg/SVGPathElement.h b/Source/WebCore/svg/SVGPathElement.h
index 0ca66ac11..e9db8a56e 100644
--- a/Source/WebCore/svg/SVGPathElement.h
+++ b/Source/WebCore/svg/SVGPathElement.h
@@ -98,7 +98,9 @@ public:
 
     bool isAnimValObserved() const { return m_isAnimValObserved; }
 
-private:
+    void animatedPropertyWillBeDeleted();
+
+ private:
     SVGPathElement(const QualifiedName&, Document*);
 
     virtual bool isValid() const { return SVGTests::isValid(); }
diff --git a/Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.h b/Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.h
index 1cfe0230e..dd5396aff 100644
--- a/Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.h
+++ b/Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.h
@@ -113,6 +113,13 @@ private:
         : SVGAnimatedListPropertyTearOff<SVGPathSegList>(contextElement, attributeName, animatedPropertyType, values)
         , m_animatedPathByteStream(0)
     {
+        ASSERT(contextElement);
+        ASSERT(toSVGPathElement(contextElement));
+    }
+
+    virtual ~SVGAnimatedPathSegListPropertyTearOff()
+    {
+        static_cast<SVGPathElement*>(contextElement())->animatedPropertyWillBeDeleted();
     }
 
     SVGPathByteStream* m_animatedPathByteStream;