Changeset 71236 in webkit


Ignore:
Timestamp:
Nov 3, 2010 7:12:54 AM (13 years ago)
Author:
Nikolas Zimmermann
Message:

2010-11-03 Nikolas Zimmermann <​nzimmermann@rim.com>

Reviewed by Dirk Schulze.

chrome.dll!WebCore::SVGListPropertyTearOff<...>::getItem ReadAV@NULL (578c0f7f21ca517ba29a4eafb7099c1b)
​https://bugs.webkit.org/show_bug.cgi?id=48829

Share SVGPropertyTearOff wrapper cache between SVGAnimatedListPropertyTearOff::baseVal/animVal.
When modifying the list through baseVal, and then grabbing the animVal list an assertion was fired,
as the wrapper cache was out of sync with the underlying SVG*List vector.

Test: svg/dom/baseVal-animVal-list-crash.html

  • svg/properties/SVGAnimatedListPropertyTearOff.h: (WebCore::SVGAnimatedListPropertyTearOff::baseVal): (WebCore::SVGAnimatedListPropertyTearOff::animVal): (WebCore::SVGAnimatedListPropertyTearOff::removeItemFromList): (WebCore::SVGAnimatedListPropertyTearOff::detachListWrappers): (WebCore::SVGAnimatedListPropertyTearOff::values): (WebCore::SVGAnimatedListPropertyTearOff::wrappers): (WebCore::SVGAnimatedListPropertyTearOff::create): (WebCore::SVGAnimatedListPropertyTearOff::SVGAnimatedListPropertyTearOff):
  • svg/properties/SVGListPropertyTearOff.h: (WebCore::SVGListPropertyTearOff::create): (WebCore::SVGListPropertyTearOff::removeItemFromList): (WebCore::SVGListPropertyTearOff::clear): (WebCore::SVGListPropertyTearOff::numberOfItems): (WebCore::SVGListPropertyTearOff::initialize): (WebCore::SVGListPropertyTearOff::getItem): (WebCore::SVGListPropertyTearOff::insertItemBefore): (WebCore::SVGListPropertyTearOff::replaceItem): (WebCore::SVGListPropertyTearOff::removeItem): (WebCore::SVGListPropertyTearOff::appendItem): (WebCore::SVGListPropertyTearOff::SVGListPropertyTearOff): (WebCore::SVGListPropertyTearOff::commitChange):

2010-11-03 Nikolas Zimmermann <​nzimmermann@rim.com>

Reviewed by Dirk Schulze.

chrome.dll!WebCore::SVGListPropertyTearOff<...>::getItem ReadAV@NULL (578c0f7f21ca517ba29a4eafb7099c1b)
​https://bugs.webkit.org/show_bug.cgi?id=48829

  • svg/dom/baseVal-animVal-list-crash-expected.txt: Added.
  • svg/dom/baseVal-animVal-list-crash.html: Added.
Location:
trunk
Files:
2 added
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r71233 r71236  
     12010-11-03  Nikolas Zimmermann  <nzimmermann@rim.com>
     2
     3        Reviewed by Dirk Schulze.
     4
     5        chrome.dll!WebCore::SVGListPropertyTearOff<...>::getItem ReadAV@NULL (578c0f7f21ca517ba29a4eafb7099c1b)
     6        https://bugs.webkit.org/show_bug.cgi?id=48829
     7
     8        * svg/dom/baseVal-animVal-list-crash-expected.txt: Added.
     9        * svg/dom/baseVal-animVal-list-crash.html: Added.
     10
    1112010-11-03  Adam Roben  <aroben@apple.com>
    212

  • trunk/WebCore/ChangeLog

    r71235 r71236  
     12010-11-03  Nikolas Zimmermann  <nzimmermann@rim.com>
     2
     3        Reviewed by Dirk Schulze.
     4
     5        chrome.dll!WebCore::SVGListPropertyTearOff<...>::getItem ReadAV@NULL (578c0f7f21ca517ba29a4eafb7099c1b)
     6        https://bugs.webkit.org/show_bug.cgi?id=48829
     7
     8        Share SVGPropertyTearOff wrapper cache between SVGAnimatedListPropertyTearOff::baseVal/animVal.
     9        When modifying the list through baseVal, and then grabbing the animVal list an assertion was fired,
     10        as the wrapper cache was out of sync with the underlying SVG*List vector.
     11
     12        Test: svg/dom/baseVal-animVal-list-crash.html
     13
     14        * svg/properties/SVGAnimatedListPropertyTearOff.h:
     15        (WebCore::SVGAnimatedListPropertyTearOff::baseVal):
     16        (WebCore::SVGAnimatedListPropertyTearOff::animVal):
     17        (WebCore::SVGAnimatedListPropertyTearOff::removeItemFromList):
     18        (WebCore::SVGAnimatedListPropertyTearOff::detachListWrappers):
     19        (WebCore::SVGAnimatedListPropertyTearOff::values):
     20        (WebCore::SVGAnimatedListPropertyTearOff::wrappers):
     21        (WebCore::SVGAnimatedListPropertyTearOff::create):
     22        (WebCore::SVGAnimatedListPropertyTearOff::SVGAnimatedListPropertyTearOff):
     23        * svg/properties/SVGListPropertyTearOff.h:
     24        (WebCore::SVGListPropertyTearOff::create):
     25        (WebCore::SVGListPropertyTearOff::removeItemFromList):
     26        (WebCore::SVGListPropertyTearOff::clear):
     27        (WebCore::SVGListPropertyTearOff::numberOfItems):
     28        (WebCore::SVGListPropertyTearOff::initialize):
     29        (WebCore::SVGListPropertyTearOff::getItem):
     30        (WebCore::SVGListPropertyTearOff::insertItemBefore):
     31        (WebCore::SVGListPropertyTearOff::replaceItem):
     32        (WebCore::SVGListPropertyTearOff::removeItem):
     33        (WebCore::SVGListPropertyTearOff::appendItem):
     34        (WebCore::SVGListPropertyTearOff::SVGListPropertyTearOff):
     35        (WebCore::SVGListPropertyTearOff::commitChange):
     36
    1372010-11-02  Ilya Tikhonovsky  <loislo@chromium.org>
    238

  • trunk/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h

    r70918 r71236  
    3333class SVGAnimatedListPropertyTearOff : public SVGAnimatedProperty {
    3434public:
     35    typedef typename SVGPropertyTraits<PropertyType>::ListItemType ListItemType;
     36    typedef SVGPropertyTearOff<ListItemType> ListItemTearOff;
     37    typedef Vector<RefPtr<ListItemTearOff> > ListWrapperCache;
     38
    3539    SVGProperty* baseVal()
    3640    {
    3741        if (!m_baseVal)
    38             m_baseVal = SVGListPropertyTearOff<PropertyType>::create(this, BaseValRole, m_property);
     42            m_baseVal = SVGListPropertyTearOff<PropertyType>::create(this, BaseValRole);
    3943        return m_baseVal.get();
    4044    }
    … …  
    4347    {
    4448        if (!m_animVal)
    45             m_animVal = SVGListPropertyTearOff<PropertyType>::create(this, AnimValRole, m_property);
     49            m_animVal = SVGListPropertyTearOff<PropertyType>::create(this, AnimValRole);
    4650        return m_animVal.get();
    4751    }
    … …  
    5155    int removeItemFromList(SVGProperty* property, bool shouldSynchronizeWrappers)
    5256    {
    53         // FIXME: No animVal support.
    54         if (!m_baseVal)
    55             return -1;
    56 
     57        // This should ever be called for our baseVal, as animVal can't modify the list.
    5758        typedef SVGPropertyTearOff<typename SVGPropertyTraits<PropertyType>::ListItemType> ListItemTearOff;
    5859        return static_pointer_cast<SVGListPropertyTearOff<PropertyType> >(m_baseVal)->removeItemFromList(static_cast<ListItemTearOff*>(property), shouldSynchronizeWrappers);
    … …  
    6162    void detachListWrappers(unsigned newListSize)
    6263    {
    63         if (m_baseVal)
    64             static_pointer_cast<SVGListPropertyTearOff<PropertyType> >(m_baseVal)->detachListWrappers(newListSize);
    65         if (m_animVal)
    66             static_pointer_cast<SVGListPropertyTearOff<PropertyType> >(m_animVal)->detachListWrappers(newListSize);
     64        // See SVGPropertyTearOff::detachWrapper() for an explaination what's happening here.
     65        unsigned size = m_wrappers.size();
     66        ASSERT(size == m_values.size());
     67        for (unsigned i = 0; i < size; ++i) {
     68            RefPtr<ListItemTearOff>& item = m_wrappers.at(i);
     69            if (!item)
     70                continue;
     71            item->detachWrapper();
     72        }
     73
     74        // Reinitialize the wrapper cache to be equal to the new values size, after the XML DOM changed the list.
     75        if (newListSize)
     76            m_wrappers.fill(0, newListSize);
     77        else
     78            m_wrappers.clear();
    6779    }
     80
     81    PropertyType& values() { return m_values; }
     82    ListWrapperCache& wrappers() { return m_wrappers; }
    6883
    6984private:
    7085    friend class SVGAnimatedProperty;
    7186
    72     static PassRefPtr<SVGAnimatedListPropertyTearOff<PropertyType> > create(SVGElement* contextElement, const QualifiedName& attributeName, PropertyType& property)
     87    static PassRefPtr<SVGAnimatedListPropertyTearOff<PropertyType> > create(SVGElement* contextElement, const QualifiedName& attributeName, PropertyType& values)
    7388    {
    7489        ASSERT(contextElement);
    75         return adoptRef(new SVGAnimatedListPropertyTearOff<PropertyType>(contextElement, attributeName, property));
     90        return adoptRef(new SVGAnimatedListPropertyTearOff<PropertyType>(contextElement, attributeName, values));
    7691    }
    7792
    78     SVGAnimatedListPropertyTearOff(SVGElement* contextElement, const QualifiedName& attributeName, PropertyType& property)
     93    SVGAnimatedListPropertyTearOff(SVGElement* contextElement, const QualifiedName& attributeName, PropertyType& values)
    7994        : SVGAnimatedProperty(contextElement, attributeName)
    80         , m_property(property)
     95        , m_values(values)
    8196    {
     97        if (!values.isEmpty())
     98            m_wrappers.fill(0, values.size());
    8299    }
    83100
    84101private:
    85     PropertyType& m_property;
     102    PropertyType& m_values;
     103
     104    // FIXME: The list wrapper cache is shared between baseVal/animVal. If we implement animVal,
     105    // we need two seperated wrapper caches if the attribute gets animated.
     106    ListWrapperCache m_wrappers;
    86107
    87108    RefPtr<SVGProperty> m_baseVal;

  • trunk/WebCore/svg/properties/SVGListPropertyTearOff.h

    r70918 r71236  
    4040    typedef SVGPropertyTearOff<ListItemType> ListItemTearOff;
    4141    typedef PassRefPtr<ListItemTearOff> PassListItemTearOff;
    42     typedef Vector<RefPtr<ListItemTearOff> > ListWrapperCache;
    43 
    44     // Used for [SVGAnimatedProperty] types (for example: SVGAnimatedLengthList::baseVal())
    45     static PassRefPtr<Self> create(SVGAnimatedProperty* animatedProperty, SVGPropertyRole role, PropertyType& values)
     42    typedef SVGAnimatedListPropertyTearOff<PropertyType> AnimatedListPropertyTearOff;
     43    typedef typename SVGAnimatedListPropertyTearOff<PropertyType>::ListWrapperCache ListWrapperCache;
     44
     45    static PassRefPtr<Self> create(AnimatedListPropertyTearOff* animatedProperty, SVGPropertyRole role)
    4646    {
    4747        ASSERT(animatedProperty);
    48         return adoptRef(new Self(animatedProperty, role, values));
    49     }
    50 
    51     // Used for non-animated POD types (for example: SVGStringList).
    52     static PassRefPtr<Self> create(const PropertyType& initialValue)
    53     {
    54         return adoptRef(new Self(initialValue));
     48        return adoptRef(new Self(animatedProperty, role));
    5549    }
    5650
    5751    int removeItemFromList(ListItemTearOff* removeItem, bool shouldSynchronizeWrappers)
    5852    {
     53        PropertyType& values = m_animatedProperty->values();
     54        ListWrapperCache& wrappers = m_animatedProperty->wrappers();
     55
    5956        // Lookup item in cache and remove its corresponding wrapper.
    60         unsigned size = m_wrappers.size();
    61         ASSERT(size == m_values->size());
     57        unsigned size = wrappers.size();
     58        ASSERT(size == values.size());
    6259        for (unsigned i = 0; i < size; ++i) {
    63             RefPtr<ListItemTearOff>& item = m_wrappers.at(i);
     60            RefPtr<ListItemTearOff>& item = wrappers.at(i);
    6461            if (item != removeItem)
    6562                continue;
    6663
    6764            item->detachWrapper();
    68             m_wrappers.remove(i);
    69             m_values->remove(i);
     65            wrappers.remove(i);
     66            values.remove(i);
    7067
    7168            if (shouldSynchronizeWrappers)
    … …  
    7875    }
    7976
    80     void detachListWrappers(unsigned newListSize)
    81     {
    82         // See SVGPropertyTearOff::detachWrapper() for an explaination what's happening here.
    83         unsigned size = m_wrappers.size();
    84         ASSERT(size == m_values->size());
    85         for (unsigned i = 0; i < size; ++i) {
    86             RefPtr<ListItemTearOff>& item = m_wrappers.at(i);
    87             if (!item)
    88                 continue;
    89             item->detachWrapper();
    90         }
    91 
    92         // Reinitialize the wrapper cache to be equal to the new values size, after the XML DOM changed the list.
    93         if (newListSize)
    94             m_wrappers.fill(0, newListSize);
    95         else
    96             m_wrappers.clear();
    97     }
    98 
    9977    // SVGList API
    10078    void clear(ExceptionCode& ec)
    … …  
    10583        }
    10684
    107         detachListWrappers(0);
    108         m_values->clear();
     85        m_animatedProperty->detachListWrappers(0);
     86        m_animatedProperty->values().clear();
    10987    }
    11088
    11189    unsigned numberOfItems() const
    11290    {
    113         return m_values->size();
     91        return m_animatedProperty->values().size();
    11492    }
    11593
    … …  
    127105        }
    128106
     107        PropertyType& values = m_animatedProperty->values();
     108        ListWrapperCache& wrappers = m_animatedProperty->wrappers();
     109
    129110        RefPtr<ListItemTearOff> newItem = passNewItem;
    130         ASSERT(m_values->size() == m_wrappers.size());
     111        ASSERT(values.size() == wrappers.size());
    131112
    132113        // Spec: If the inserted item is already in a list, it is removed from its previous list before it is inserted into this list.
    … …  
    134115
    135116        // Spec: Clears all existing current items from the list and re-initializes the list to hold the single item specified by the parameter.
    136         detachListWrappers(0);
    137         m_values->clear();
    138        
    139         m_values->append(newItem->propertyReference());
    140         m_wrappers.append(newItem);
     117        m_animatedProperty->detachListWrappers(0);
     118        values.clear();
     119
     120        values.append(newItem->propertyReference());
     121        wrappers.append(newItem);
    141122
    142123        commitChange();
    … …  
    146127    PassListItemTearOff getItem(unsigned index, ExceptionCode& ec)
    147128    {
    148         if (index >= m_values->size()) {
     129        PropertyType& values = m_animatedProperty->values();
     130        if (index >= values.size()) {
    149131            ec = INDEX_SIZE_ERR;
    150132            return 0;
    151133        }
     134
     135        ListWrapperCache& wrappers = m_animatedProperty->wrappers();
    152136
    153137        // Spec: Returns the specified item from the list. The returned item is the item itself and not a copy.
    154138        // Any changes made to the item are immediately reflected in the list.
    155         ASSERT(m_values->size() == m_wrappers.size());
    156         RefPtr<ListItemTearOff> wrapper = m_wrappers.at(index);
     139        ASSERT(values.size() == wrappers.size());
     140        RefPtr<ListItemTearOff> wrapper = wrappers.at(index);
    157141        if (!wrapper) {
    158142            // Create new wrapper, which is allowed to directly modify the item in the list, w/o copying and cache the wrapper in our map.
    159143            // It is also associated with our animated property, so it can notify the SVG Element which holds the SVGAnimated*List
    160144            // that it has been modified (and thus can call svgAttributeChanged(associatedAttributeName)).
    161             wrapper = ListItemTearOff::create(m_animatedProperty.get(), UndefinedRole, m_values->at(index));
    162             m_wrappers.at(index) = wrapper;
     145            wrapper = ListItemTearOff::create(m_animatedProperty.get(), UndefinedRole, values.at(index));
     146            wrappers.at(index) = wrapper;
    163147        }
    164148
    … …  
    179163        }
    180164
     165        PropertyType& values = m_animatedProperty->values();
     166        ListWrapperCache& wrappers = m_animatedProperty->wrappers();
     167
    181168        // Spec: If the index is greater than or equal to numberOfItems, then the new item is appended to the end of the list.
    182         if (index > m_values->size())
    183              index = m_values->size();
     169        if (index > values.size())
     170             index = values.size();
    184171
    185172        RefPtr<ListItemTearOff> newItem = passNewItem;
    186         ASSERT(m_values->size() == m_wrappers.size());
     173        ASSERT(values.size() == wrappers.size());
    187174
    188175        // Spec: If newItem is already in a list, it is removed from its previous list before it is inserted into this list.
    … …  
    191178        // Spec: Inserts a new item into the list at the specified position. The index of the item before which the new item is to be
    192179        // inserted. The first item is number 0. If the index is equal to 0, then the new item is inserted at the front of the list.
    193         m_values->insert(index, newItem->propertyReference());
     180        values.insert(index, newItem->propertyReference());
    194181
    195182        // Store new wrapper at position 'index', change its underlying value, so mutations of newItem, directly affect the item in the list.
    196         m_wrappers.insert(index, newItem);
     183        wrappers.insert(index, newItem);
    197184
    198185        commitChange();
    … …  
    207194        }
    208195
    209         if (index >= m_values->size()) {
     196        PropertyType& values = m_animatedProperty->values();
     197        if (index >= values.size()) {
    210198            ec = INDEX_SIZE_ERR;
    211199            return 0;
    … …  
    218206        }
    219207
     208        ListWrapperCache& wrappers = m_animatedProperty->wrappers();
     209        ASSERT(values.size() == wrappers.size());
    220210        RefPtr<ListItemTearOff> newItem = passNewItem;
    221         ASSERT(m_values->size() == m_wrappers.size());
    222211
    223212        // Spec: If newItem is already in a list, it is removed from its previous list before it is inserted into this list.
    … …  
    226215
    227216        // Detach the existing wrapper.
    228         RefPtr<ListItemTearOff>& oldItem = m_wrappers.at(index);
     217        RefPtr<ListItemTearOff>& oldItem = wrappers.at(index);
    229218        if (oldItem)
    230219            oldItem->detachWrapper();
    231220
    232221        // Update the value and the wrapper at the desired position 'index'.
    233         m_values->at(index) = newItem->propertyReference();
    234         m_wrappers.at(index) = newItem;
     222        values.at(index) = newItem->propertyReference();
     223        wrappers.at(index) = newItem;
    235224
    236225        commitChange();
    … …  
    245234        }
    246235
    247         if (index >= m_values->size()) {
     236        PropertyType& values = m_animatedProperty->values();
     237        if (index >= values.size()) {
    248238            ec = INDEX_SIZE_ERR;
    249239            return 0;
    250240        }
    251241
    252         ASSERT(m_values->size() == m_wrappers.size());
     242        ListWrapperCache& wrappers = m_animatedProperty->wrappers();
     243        ASSERT(values.size() == wrappers.size());
    253244
    254245        // Detach the existing wrapper.
    255         RefPtr<ListItemTearOff>& oldItem = m_wrappers.at(index);
     246        RefPtr<ListItemTearOff>& oldItem = wrappers.at(index);
    256247        if (oldItem) {
    257248            oldItem->detachWrapper();
    258             m_wrappers.remove(index);
    259         }
    260 
    261         m_values->remove(index);
     249            wrappers.remove(index);
     250        }
     251
     252        values.remove(index);
    262253
    263254        commitChange();
    … …  
    278269        }
    279270
     271        PropertyType& values = m_animatedProperty->values();
     272        ListWrapperCache& wrappers = m_animatedProperty->wrappers();
     273
    280274        RefPtr<ListItemTearOff> newItem = passNewItem;
    281         ASSERT(m_values->size() == m_wrappers.size());
     275        ASSERT(values.size() == wrappers.size());
    282276
    283277        // Spec: If newItem is already in a list, it is removed from its previous list before it is inserted into this list.
    … …  
    285279
    286280        // Append the value and wrapper at the end of the list.
    287         m_values->append(newItem->propertyReference());
    288         m_wrappers.append(newItem);
     281        values.append(newItem->propertyReference());
     282        wrappers.append(newItem);
    289283
    290284        commitChange();
    … …  
    293287
    294288private:
    295     SVGListPropertyTearOff(SVGAnimatedProperty* animatedProperty, SVGPropertyRole role, PropertyType& values)
     289    SVGListPropertyTearOff(AnimatedListPropertyTearOff* animatedProperty, SVGPropertyRole role)
    296290        : m_animatedProperty(animatedProperty)
    297291        , m_role(role)
    298         , m_values(&values)
    299         , m_valuesIsCopy(false)
    300     {
    301         // Using operator & is completly fine, as SVGAnimatedProperty owns this reference,
    302         // and we're guaranteed to live as long as SVGAnimatedProperty does.
    303         if (!values.isEmpty())
    304             m_wrappers.fill(0, values.size());
    305     }
    306 
    307     SVGListPropertyTearOff(const PropertyType& initialValue)
    308         : m_animatedProperty(0)
    309         , m_role(UndefinedRole)
    310         , m_values(new PropertyType(initialValue))
    311         , m_valuesIsCopy(true)
    312     {
    313     }
    314 
    315     virtual ~SVGListPropertyTearOff()
    316     {
    317         if (m_valuesIsCopy)
    318             delete m_values;
     292    {
     293        ASSERT(animatedProperty);
     294        ASSERT(role != UndefinedRole);
    319295    }
    320296
    321297    void commitChange()
    322298    {
    323         // Update existing wrappers, as the index in the m_values list has changed.
    324         unsigned size = m_wrappers.size();
    325         ASSERT(size == m_values->size());
     299        PropertyType& values = m_animatedProperty->values();
     300        ListWrapperCache& wrappers = m_animatedProperty->wrappers();
     301
     302        // Update existing wrappers, as the index in the values list has changed.
     303        unsigned size = wrappers.size();
     304        ASSERT(size == values.size());
    326305        for (unsigned i = 0; i < size; ++i) {
    327             RefPtr<ListItemTearOff>& item = m_wrappers.at(i);
     306            RefPtr<ListItemTearOff>& item = wrappers.at(i);
    328307            if (!item)
    329308                continue;
    330309            item->setAnimatedProperty(m_animatedProperty.get());
    331             item->setValue(m_values->at(i));
    332         }
    333 
    334         ASSERT(!m_valuesIsCopy);
    335         ASSERT(m_animatedProperty);
     310            item->setValue(values.at(i));
     311        }
     312
    336313        m_animatedProperty->commitChange();
    337314    }
    … …  
    364341    // Back pointer to the animated property that created us
    365342    // For example (text.x.baseVal): m_animatedProperty points to the 'x' SVGAnimatedLengthList object
    366     RefPtr<SVGAnimatedProperty> m_animatedProperty;
     343    RefPtr<AnimatedListPropertyTearOff> m_animatedProperty;
    367344
    368345    // The role of this property (baseVal or animVal)
    369346    SVGPropertyRole m_role;
    370 
    371     // For the example above (text.x.baseVal): A reference to the SVGLengthList& stored in the SVGTextElement, which we can directly modify
    372     PropertyType* m_values;
    373     bool m_valuesIsCopy : 1;
    374 
    375     // A list of wrappers, which is always in sync between m_values.
    376     ListWrapperCache m_wrappers;
    377347};
    378348
Note: See TracChangeset for help on using the changeset viewer.