ASSERT when SVGAnimatedProperty instance is assigned to multiple elements.

• Component: SVG
• Status: RESOLVED FIXED

[Justin Schuh]

Here's the repro:

<script>
  var oFEConvolveMatrix = document.createElementNS("http://www.w3.org/2000/svg", "feConvolveMatrix");
  document.createElementNS("http://www.w3.org/2000/svg", "altGlyph").x.baseVal.insertItemBefore(oFEConvolveMatrix.width.baseVal);
</script>



Inside SVGListPropertyTearOff::insertItemBefore we fail to remove the supplied item from its owning element before assigning it to a new element. The result is that the proprty ends up shared by two elements at once. The following ASSERT in SVGAnimatedProperty::removeItemFromList gets triggered when the property should be removed from the original owner:

    virtual int removeItemFromList(SVGProperty*, bool)
    {
        ASSERT_NOT_REACHED();
        return -1;
    }

Originally reported here: http://code.google.com/p/chromium/issues/detail?id=61064

[Nikolas Zimmermann]

Thanks for the report, very glad to have it!
Will look into this today, it's completly new code, and there may be a new bug..

[Nikolas Zimmermann]

Heh, I didn't think about calling insertItemBefore on a SVGLengthList with a SVGLength parameter that is not in a list, but just associated with a SVGAnimatedLength object.
The fix is easy, my tree is just not clean yet:


        int removedIndex = animatedPropertyOfItem->removeItemFromList(newItem, livesInOtherList);
        ASSERT(removedIndex != -1);

Replace that assertion by

        // If the item is not present in a list, we don't need to remove it.
        if (removedIndex == -1)
            return;

And remove the ASSERT_NOT_REACHED() in SVGAnimatedProperty.h.
We could add a "virtual bool isSVGAnimatedListProperty" method to SVGAnimatedProperty, to stop "abusing" removedIndex = -1 as indicator for "item is not member of a list". But I'll come to that in a later patch.

As soon as my tree is clean, I'll prepare a patch, or if you're faster just go ahead.

[Nikolas Zimmermann]

Created attachment 72333 [details]
Patch

Safe fix for the assertion, don't rely on the workaround I described before. Let's wait for EWS results from V8, as I can't test that at the moment (Snow Leopard laptop not with me).

[Nikolas Zimmermann]

EFL shows a build error:

In file included from /mnt/eflews/git/webkit/WebKitBuild/Release/DerivedSources/JSSVGLengthList.h:29,
                 from /mnt/eflews/git/webkit/WebKitBuild/Release/DerivedSources/JSSVGLengthList.cpp:25:
/mnt/eflews/git/webkit/WebCore/svg/properties/SVGListPropertyTearOff.h: In member function ‘void WebCore::SVGListPropertyTearOff<PropertyType>::removeItemFromListIfNeeded(WebCore::SVGPropertyTearOff<typename WebCore::SVGPropertyTraits<PropertyType>::ListItemType>*, unsigned int*) [with PropertyType = WebCore::SVGLengthList]’:
...

JSSVGLengthList.cpp:25 includes "JSSVGLengthList.h"
JSSVGLengthList.h:29 should include "SVGLengthList.h" and SVGAnimatedListPropertyTearOff.h should have been included already.

It builds fine here on my mac machine, I suspect it did not regenerate the bindings properly?

[Dirk Schulze]

Comment on attachment 72333 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72333&action=review

r=me

> WebCore/svg/properties/SVGAnimatedProperty.h:46
> +    virtual bool isAnimatedListTearOff() const { return false; }

Didn't we discussed about adding a boolean instead of the virtual removeItemFromList on the first patch that implemented it? ;-)

[Nikolas Zimmermann]

(In reply to comment #5)
> (From update of attachment 72333 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72333&action=review
> 
> r=me
> 
> > WebCore/svg/properties/SVGAnimatedProperty.h:46
> > +    virtual bool isAnimatedListTearOff() const { return false; }
> 
> Didn't we discussed about adding a boolean instead of the virtual removeItemFromList on the first patch that implemented it? ;-)

Yes, we did. I didn't want two virtual function calls, but forgot about the fact that I could just devirtualize removeItemFromList, and cast from SVGAnimatedPropertyTearOff to SVGAnimatedListPropertyTearOff, this way we still have just one virtual function call.

[Nikolas Zimmermann]

Landed in r70918.