Bug 48555

Summary: ASSERT when SVGAnimatedProperty instance is assigned to multiple elements.
Product: WebKit Reporter: Justin Schuh <jschuh>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: krit, mdelaney7, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch krit: review+

Description Justin Schuh 2010-10-28 13:23:03 PDT
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
Comment 1 Nikolas Zimmermann 2010-10-28 23:31:08 PDT
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..
Comment 2 Nikolas Zimmermann 2010-10-29 06:47:24 PDT
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.
Comment 3 Nikolas Zimmermann 2010-10-29 07:29:24 PDT
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).
Comment 4 Nikolas Zimmermann 2010-10-29 08:00:24 PDT
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?
Comment 5 Dirk Schulze 2010-10-29 12:26:27 PDT
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? ;-)
Comment 6 Nikolas Zimmermann 2010-10-29 13:18:27 PDT
(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.
Comment 7 Nikolas Zimmermann 2010-10-29 13:33:42 PDT
Landed in r70918.