Changeset 221292 in webkit


Ignore:
Timestamp:
Aug 29, 2017 2:32:34 AM (7 years ago)
Author:
svillar@igalia.com
Message:

[SVG] Leak in SVGAnimatedListPropertyTearOff
​https://bugs.webkit.org/show_bug.cgi?id=172545

Reviewed by Darin Adler.

Source/WebCore:

SVGAnimatedListPropertyTearOff maintains a vector m_wrappers with references to
SVGPropertyTraits<PropertyType>::ListItemTearOff. Apart from that SVGPropertyTearOff has a
reference to SVGAnimatedProperty.

When SVGListProperty::getItemValuesAndWrappers() is called, it creates a
SVGPropertyTraits<PropertyType>::ListItemTearOff pointing to the same SVGAnimatedProperty (a
SVGAnimatedListPropertyTearOff) which stores the m_wrappers vector where the ListItemTearOff
is going to be added to. This effectively creates a reference cycle between the
SVGAnimatedListPropertyTearOff and all the ListItemTearOff it stores in m_wrappers.

In order to effectively break the cycle without freeing too many wrappers we should take two
measures:
1) Break the reference cycle by storing raw pointers in the m_wrappers Vector
2) Remove the ListItemTearOff which is being deleted (it notifies the animated property by
calling propertyWillBeDeleted) from the m_wrappers Vector.

This is a re-land of r219334 which caused early releases of custom data attribute objects
added to SVG elements (wkb.ug/175023).

Tests: svg/animations/animation-leak-list-property-instances.html

svg/dom/SVGAnimatedListPropertyTearOff-crash-2.html
svg/dom/SVGAnimatedListPropertyTearOff-crash.html
svg/dom/SVGAnimatedListPropertyTearOff-leak.html

  • svg/properties/SVGAnimatedListPropertyTearOff.h:
  • svg/properties/SVGListProperty.h:

(WebCore::SVGListProperty::getItemValuesAndWrappers):

  • svg/properties/SVGListPropertyTearOff.h:

(WebCore::SVGListPropertyTearOff::removeItemFromList):

LayoutTests:

The list of new added tests includes the one for the original bug, a new test for the
regression and a couple of tests imported from Blink which verify that
SVGAnimatedListPropertyTearOff does not crash after the context element goes out of scope.

  • svg/animations/animation-leak-list-property-instances-expected.txt: Added.
  • svg/animations/animation-leak-list-property-instances.html: Added.
  • svg/dom/SVGAnimatedListPropertyTearOff-crash-2-expected.txt: Added. Imported from Blink.
  • svg/dom/SVGAnimatedListPropertyTearOff-crash-2.html: Added. Imported from Blink.
  • svg/dom/SVGAnimatedListPropertyTearOff-crash-expected.txt: Added. Imported from Blink.
  • svg/dom/SVGAnimatedListPropertyTearOff-crash.html: Added. Imported from Blink.
  • svg/dom/SVGAnimatedListPropertyTearOff-leak-expected.txt: Added.
  • svg/dom/SVGAnimatedListPropertyTearOff-leak.html: Added.
Location:
trunk
Files:
8 added
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r221287 r221292  
     12017-08-19  Sergio Villar Senin  <svillar@igalia.com>
     2
     3        [SVG] Leak in SVGAnimatedListPropertyTearOff
     4        https://bugs.webkit.org/show_bug.cgi?id=172545
     5
     6        Reviewed by Darin Adler.
     7
     8        The list of new added tests includes the one for the original bug, a new test for the
     9        regression and a couple of tests imported from Blink which verify that
     10        SVGAnimatedListPropertyTearOff does not crash after the context element goes out of scope.
     11
     12        * svg/animations/animation-leak-list-property-instances-expected.txt: Added.
     13        * svg/animations/animation-leak-list-property-instances.html: Added.
     14        * svg/dom/SVGAnimatedListPropertyTearOff-crash-2-expected.txt: Added. Imported from Blink.
     15        * svg/dom/SVGAnimatedListPropertyTearOff-crash-2.html: Added. Imported from Blink.
     16        * svg/dom/SVGAnimatedListPropertyTearOff-crash-expected.txt: Added. Imported from Blink.
     17        * svg/dom/SVGAnimatedListPropertyTearOff-crash.html: Added. Imported from Blink.
     18        * svg/dom/SVGAnimatedListPropertyTearOff-leak-expected.txt: Added.
     19        * svg/dom/SVGAnimatedListPropertyTearOff-leak.html: Added.
     20
    1212017-08-28  Per Arne Vollan  <pvollan@apple.com>
    222

  • trunk/Source/WebCore/ChangeLog

    r221291 r221292  
     12017-08-19  Sergio Villar Senin  <svillar@igalia.com>
     2
     3        [SVG] Leak in SVGAnimatedListPropertyTearOff
     4        https://bugs.webkit.org/show_bug.cgi?id=172545
     5
     6        Reviewed by Darin Adler.
     7
     8        SVGAnimatedListPropertyTearOff maintains a vector m_wrappers with references to
     9        SVGPropertyTraits<PropertyType>::ListItemTearOff. Apart from that SVGPropertyTearOff has a
     10        reference to SVGAnimatedProperty.
     11
     12        When SVGListProperty::getItemValuesAndWrappers() is called, it creates a
     13        SVGPropertyTraits<PropertyType>::ListItemTearOff pointing to the same SVGAnimatedProperty (a
     14        SVGAnimatedListPropertyTearOff) which stores the m_wrappers vector where the ListItemTearOff
     15        is going to be added to. This effectively creates a reference cycle between the
     16        SVGAnimatedListPropertyTearOff and all the ListItemTearOff it stores in m_wrappers.
     17
     18        In order to effectively break the cycle without freeing too many wrappers we should take two
     19        measures:
     20        1) Break the reference cycle by storing raw pointers in the m_wrappers Vector
     21        2) Remove the ListItemTearOff which is being deleted (it notifies the animated property by
     22        calling propertyWillBeDeleted) from the m_wrappers Vector.
     23
     24        This is a re-land of r219334 which caused early releases of custom data attribute objects
     25        added to SVG elements (wkb.ug/175023).
     26
     27        Tests: svg/animations/animation-leak-list-property-instances.html
     28               svg/dom/SVGAnimatedListPropertyTearOff-crash-2.html
     29               svg/dom/SVGAnimatedListPropertyTearOff-crash.html
     30               svg/dom/SVGAnimatedListPropertyTearOff-leak.html
     31
     32        * svg/properties/SVGAnimatedListPropertyTearOff.h:
     33        * svg/properties/SVGListProperty.h:
     34        (WebCore::SVGListProperty::getItemValuesAndWrappers):
     35        * svg/properties/SVGListPropertyTearOff.h:
     36        (WebCore::SVGListPropertyTearOff::removeItemFromList):
     37
    1382017-08-29  Andy Estes  <aestes@apple.com>
    239

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

    r220484 r221292  
    3535    using ListItemType = typename SVGPropertyTraits<PropertyType>::ListItemType;
    3636    using ListItemTearOff = typename SVGPropertyTraits<PropertyType>::ListItemTearOff;
    37     using ListWrapperCache = Vector<RefPtr<ListItemTearOff>>;
     37    using ListWrapperCache = Vector<ListItemTearOff*>;
    3838    using ListProperty = SVGListProperty<PropertyType>;
    3939    using ListPropertyTearOff = typename SVGPropertyTraits<PropertyType>::ListPropertyTearOff;
    … …  
    7474        else if (&property == m_animVal)
    7575            m_animVal = nullptr;
     76        else {
     77            size_t i = m_wrappers.find(&property);
     78            if (i != notFound)
     79                m_wrappers[i] = nullptr;
     80        }
    7681    }
    7782

  • trunk/Source/WebCore/svg/properties/SVGListProperty.h

    r219856 r221292  
    207207            // that it has been modified (and thus can call svgAttributeChanged(associatedAttributeName)).
    208208            wrapper = ListItemTearOff::create(animatedList, UndefinedRole, m_values->at(index));
    209             m_wrappers->at(index) = wrapper;
     209            m_wrappers->at(index) = wrapper.get();
    210210        }
    211211

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

    r208863 r221292  
    6767        ASSERT_WITH_SECURITY_IMPLICATION(itemIndex < m_wrappers->size());
    6868
    69         RefPtr<ListItemTearOff>& item = m_wrappers->at(itemIndex);
     69        RefPtr<ListItemTearOff> item = m_wrappers->at(itemIndex);
    7070        item->detachWrapper();
    7171        m_wrappers->remove(itemIndex);
    … …  
    142142        ASSERT(size == m_values->size());
    143143        for (unsigned i = 0; i < size; ++i) {
    144             ListItemTearOff* item = m_wrappers->at(i).get();
     144            ListItemTearOff* item = m_wrappers->at(i);
    145145            if (!item)
    146146                continue;
Note: See TracChangeset for help on using the changeset viewer.