Changeset 196268 in webkit


Ignore:
Timestamp:
Feb 8, 2016 12:54:05 PM (8 years ago)
Author:
commit-queue@webkit.org
Message:

REGRESSION(r181345): SVG polyline and polygon leak page
​https://bugs.webkit.org/show_bug.cgi?id=152759

Patch by Said Abou-Hallawa <​sabouhallawa@apple.com> on 2016-02-08
Reviewed by Darin Adler.

Source/WebCore:

The leak happens because of cyclic reference between SVGListPropertyTearOff
and SVGAnimatedListPropertyTearOff which is derived from SVGAnimatedProperty.
There is also cyclic reference between SVGAnimatedProperty and SVGElement
and this causes the whole document to be leaked. So if the JS requests, for
example, an instance of SVGPolylineElement.points, the whole document will be
leaked.

The fix depends on having the cyclic reference as is since the owning and the
owned classes have to live together if any of them is referenced. But the owning
class caches a raw 'ref-counted' pointer of the owned class. If it is requested
for an instance of the owned class it returned a RefPtr<> of it. Once the owned
class is not used, it can delete itself. The only thing needed here is to notify
the owner class of the deletion so it cleans its caches and be able to create a
new pointer if it is requested for an instance of the owned class later.

Revert the change of r181345 in SVGAnimatedProperty::lookupOrCreateWrapper()
to break the cyclic reference between SVGElement and SVGAnimatedProperty.

Also apply the same approach in SVGAnimatedListPropertyTearOff::baseVal() and
animVal() to break cyclic reference between SVGListPropertyTearOff and
SVGAnimatedListPropertyTearOff.

Test: svg/animations/smil-leak-list-property-instances.svg

  • bindings/scripts/CodeGeneratorJS.pm:

(NativeToJSValue): The SVG non-string list tear-off properties became of
type RefPtr<>. So we need to use get() with the casting expressions.

  • svg/SVGMarkerElement.cpp:

(WebCore::SVGMarkerElement::orientType):
Use 'auto' type for the return of SVGAnimatedProperty::lookupWrapper().

  • svg/SVGPathElement.cpp:

(WebCore::SVGPathElement::pathByteStream):
(WebCore::SVGPathElement::lookupOrCreateDWrapper):
Since SVGAnimatedProperty::lookupWrappe() returns a RefPtr<> we need to
use get() for the casting expressions.

(WebCore::SVGPathElement::pathSegList):
(WebCore::SVGPathElement::normalizedPathSegList):
(WebCore::SVGPathElement::animatedPathSegList):
(WebCore::SVGPathElement::animatedNormalizedPathSegList):

  • svg/SVGPathElement.h:

Change the return value from raw pointer to RefPtr<>.

  • svg/SVGPathSegWithContext.h:

(WebCore::SVGPathSegWithContext::animatedProperty):
Change the return type to be RefPtr<> to preserve the value from being deleted.

  • svg/SVGPolyElement.cpp:

(WebCore::SVGPolyElement::parseAttribute):
Since SVGAnimatedProperty::lookupWrapper() returns a RefPtr<> we need to
use get() for the casting expressions.

(WebCore::SVGPolyElement::points):
(WebCore::SVGPolyElement::animatedPoints):

  • svg/SVGPolyElement.h:

Change the return value from raw pointer to RefPtr<>.

  • svg/SVGViewSpec.cpp:

(WebCore::SVGViewSpec::setTransformString):
Since SVGAnimatedProperty::lookupWrapper() returns a RefPtr<> we need to
use get() for the casting expressions.

(WebCore::SVGViewSpec::transform):

  • svg/SVGViewSpec.h:

Change the return value from raw pointer to RefPtr<>.

  • svg/properties/SVGAnimatedListPropertyTearOff.h:

(WebCore::SVGAnimatedListPropertyTearOff::baseVal):
(WebCore::SVGAnimatedListPropertyTearOff::animVal):
Change the return value from raw pointer to RefPtr<> and change the cached
value from RefPtr<> to raw pointer. If the property is null, it will be
created, its raw pointer will be cached and the only ref-counted RefPtr<>
will be returned. This will guarantee, the RefPtr<> will be deleted once
it is not used anymore.

(WebCore::SVGAnimatedListPropertyTearOff::propertyWillBeDeleted):
Clean the raw pointer caches m_baseVal and m_animVal upon deleting the
actual pointer. This function will be called from the destructor of
SVGListPropertyTearOff.

(WebCore::SVGAnimatedListPropertyTearOff::findItem):
(WebCore::SVGAnimatedListPropertyTearOff::removeItemFromList):
We have to ensure the baseVal() is created before using it.

(WebCore::SVGAnimatedListPropertyTearOff::detachListWrappers):
(WebCore::SVGAnimatedListPropertyTearOff::currentAnimatedValue):
(WebCore::SVGAnimatedListPropertyTearOff::animationStarted):
(WebCore::SVGAnimatedListPropertyTearOff::animationEnded):
(WebCore::SVGAnimatedListPropertyTearOff::synchronizeWrappersIfNeeded):
(WebCore::SVGAnimatedListPropertyTearOff::animValWillChange):
(WebCore::SVGAnimatedListPropertyTearOff::animValDidChange):
For animation, a separate RefPtr<> 'm_animatingAnimVal' will be assigned
to the animVal(). This will prevent deleting m_animVal while animation.

  • svg/properties/SVGAnimatedPathSegListPropertyTearOff.h:

(WebCore::SVGAnimatedPathSegListPropertyTearOff::baseVal):
(WebCore::SVGAnimatedPathSegListPropertyTearOff::animVal):
Same as what is done in SVGAnimatedListPropertyTearOff.

(WebCore::SVGAnimatedPathSegListPropertyTearOff::findItem):
(WebCore::SVGAnimatedPathSegListPropertyTearOff::removeItemFromList):
Same as what is done in SVGAnimatedListPropertyTearOff.

  • svg/properties/SVGAnimatedProperty.h:

(WebCore::SVGAnimatedProperty::lookupOrCreateWrapper):
Change the return value from raw reference to Ref<> and change the
cached value from Ref<> to raw pointer. This reverts the change of
r181345 in this function.

(WebCore::SVGAnimatedProperty::lookupWrapper):
Change the return value from raw pointer to RefPtr<>.

  • svg/properties/SVGAnimatedPropertyMacros.h:

Use 'auto' type for the return of SVGAnimatedProperty::lookupWrapper().

  • svg/properties/SVGAnimatedTransformListPropertyTearOff.h:

(WebCore::SVGAnimatedTransformListPropertyTearOff::baseVal):
(WebCore::SVGAnimatedTransformListPropertyTearOff::animVal):
Same as what is done in SVGAnimatedListPropertyTearOff.

  • svg/properties/SVGListPropertyTearOff.h:

(WebCore::SVGListPropertyTearOff::~SVGListPropertyTearOff):
Call the SVGAnimatedListPropertyTearOff::propertyWillBeDeleted() to clean
its raw pointers when the RefPtr<> deletes itself.

LayoutTests:

  • svg/animations/smil-leak-list-property-instances-expected.txt: Added.
  • svg/animations/smil-leak-list-property-instances.svg: Added.

Ensure if SVGPolylineElement.points is requested from JS, the document will
not leak.

Location:
trunk
Files:
2 added
19 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r196262 r196268  
     12016-02-08  Said Abou-Hallawa  <sabouhallawa@apple.com>
     2
     3        REGRESSION(r181345): SVG polyline and polygon leak page
     4        https://bugs.webkit.org/show_bug.cgi?id=152759
     5
     6        Reviewed by Darin Adler.
     7
     8        * TestExpectations: Remove flaky tests from test expectation.
     9       
     10        * svg/animations/smil-leak-list-property-instances-expected.txt: Added.
     11        * svg/animations/smil-leak-list-property-instances.svg: Added.
     12        Ensure if SVGPolylineElement.points is requested from JS, the document will
     13        not leak.
     14
    1152016-02-08  Brady Eidson  <beidson@apple.com>
    216

  • trunk/LayoutTests/TestExpectations

    r196222 r196268  
    638638http/tests/contentextensions [ Skip ]
    639639
    640 # These tests were flaky on Mac only but they became flaky on all ports after r181345
    641 webkit.org/b/114280 svg/animations/smil-leak-dynamically-added-element-instances.svg [ Pass Failure ]
    642 webkit.org/b/114280 svg/animations/smil-leak-element-instances-noBaseValRef.svg [ Pass Failure ]
    643 webkit.org/b/114280 svg/animations/smil-leak-element-instances.svg [ Pass Failure ]
    644 webkit.org/b/114280 svg/animations/smil-leak-elements.svg [ Pass Failure ]
    645 
    646640webkit.org/b/149072 svg/animations/svgboolean-animation-1.html [ Pass Failure ]
    647641

  • trunk/Source/WebCore/ChangeLog

    r196265 r196268  
     12016-02-08  Said Abou-Hallawa  <sabouhallawa@apple.com>
     2
     3        REGRESSION(r181345): SVG polyline and polygon leak page
     4        https://bugs.webkit.org/show_bug.cgi?id=152759
     5
     6        Reviewed by Darin Adler.
     7
     8        The leak happens because of cyclic reference between SVGListPropertyTearOff
     9        and SVGAnimatedListPropertyTearOff which is derived from SVGAnimatedProperty.
     10        There is also cyclic reference between SVGAnimatedProperty and SVGElement
     11        and this causes the whole document to be leaked. So if the JS requests, for
     12        example, an instance of SVGPolylineElement.points, the whole document will be
     13        leaked.
     14
     15        The fix depends on having the cyclic reference as is since the owning and the
     16        owned classes have to live together if any of them is referenced. But the owning
     17        class caches a raw 'ref-counted' pointer of the owned class. If it is requested
     18        for an instance of the owned class it returned a RefPtr<> of it. Once the owned
     19        class is not used, it can delete itself. The only thing needed here is to notify
     20        the owner class of the deletion so it cleans its caches and be able to create a
     21        new pointer if it is requested for an instance of the owned class later.
     22
     23        Revert the change of r181345 in SVGAnimatedProperty::lookupOrCreateWrapper()
     24        to break the cyclic reference between SVGElement and SVGAnimatedProperty.
     25       
     26        Also apply the same approach in SVGAnimatedListPropertyTearOff::baseVal() and
     27        animVal() to break cyclic reference between SVGListPropertyTearOff and
     28        SVGAnimatedListPropertyTearOff.
     29
     30        Test: svg/animations/smil-leak-list-property-instances.svg
     31
     32        * bindings/scripts/CodeGeneratorJS.pm:
     33        (NativeToJSValue): The SVG non-string list tear-off properties became of
     34        type RefPtr<>. So we need to use get() with the casting expressions.
     35       
     36        * svg/SVGMarkerElement.cpp:
     37        (WebCore::SVGMarkerElement::orientType):
     38        Use 'auto' type for the return of SVGAnimatedProperty::lookupWrapper().
     39
     40        * svg/SVGPathElement.cpp:
     41        (WebCore::SVGPathElement::pathByteStream):
     42        (WebCore::SVGPathElement::lookupOrCreateDWrapper):
     43        Since SVGAnimatedProperty::lookupWrappe() returns a RefPtr<> we need to
     44        use get() for the casting expressions.
     45       
     46        (WebCore::SVGPathElement::pathSegList):
     47        (WebCore::SVGPathElement::normalizedPathSegList):
     48        (WebCore::SVGPathElement::animatedPathSegList):
     49        (WebCore::SVGPathElement::animatedNormalizedPathSegList):
     50        * svg/SVGPathElement.h:
     51        Change the return value from raw pointer to RefPtr<>.
     52
     53        * svg/SVGPathSegWithContext.h:
     54        (WebCore::SVGPathSegWithContext::animatedProperty):
     55        Change the return type to be RefPtr<> to preserve the value from being deleted.
     56       
     57        * svg/SVGPolyElement.cpp:
     58        (WebCore::SVGPolyElement::parseAttribute):
     59        Since SVGAnimatedProperty::lookupWrapper() returns a RefPtr<> we need to
     60        use get() for the casting expressions.
     61       
     62        (WebCore::SVGPolyElement::points):
     63        (WebCore::SVGPolyElement::animatedPoints):
     64        * svg/SVGPolyElement.h:
     65        Change the return value from raw pointer to RefPtr<>.
     66       
     67        * svg/SVGViewSpec.cpp:
     68        (WebCore::SVGViewSpec::setTransformString):
     69        Since SVGAnimatedProperty::lookupWrapper() returns a RefPtr<> we need to
     70        use get() for the casting expressions.
     71
     72        (WebCore::SVGViewSpec::transform):
     73        * svg/SVGViewSpec.h:
     74        Change the return value from raw pointer to RefPtr<>.
     75       
     76        * svg/properties/SVGAnimatedListPropertyTearOff.h:
     77        (WebCore::SVGAnimatedListPropertyTearOff::baseVal):
     78        (WebCore::SVGAnimatedListPropertyTearOff::animVal):
     79        Change the return value from raw pointer to RefPtr<> and change the cached
     80        value from RefPtr<> to raw pointer. If the property is null, it will be
     81        created, its raw pointer will be cached and the only ref-counted RefPtr<>
     82        will be returned. This will guarantee, the RefPtr<> will be deleted once
     83        it is not used anymore.
     84       
     85        (WebCore::SVGAnimatedListPropertyTearOff::propertyWillBeDeleted):
     86        Clean the raw pointer caches m_baseVal and m_animVal upon deleting the
     87        actual pointer. This function will be called from the destructor of
     88        SVGListPropertyTearOff.
     89       
     90        (WebCore::SVGAnimatedListPropertyTearOff::findItem):
     91        (WebCore::SVGAnimatedListPropertyTearOff::removeItemFromList):
     92        We have to ensure the baseVal() is created before using it.
     93       
     94        (WebCore::SVGAnimatedListPropertyTearOff::detachListWrappers):
     95        (WebCore::SVGAnimatedListPropertyTearOff::currentAnimatedValue):
     96        (WebCore::SVGAnimatedListPropertyTearOff::animationStarted):
     97        (WebCore::SVGAnimatedListPropertyTearOff::animationEnded):
     98        (WebCore::SVGAnimatedListPropertyTearOff::synchronizeWrappersIfNeeded):
     99        (WebCore::SVGAnimatedListPropertyTearOff::animValWillChange):
     100        (WebCore::SVGAnimatedListPropertyTearOff::animValDidChange):
     101        For animation, a separate RefPtr<> 'm_animatingAnimVal' will be assigned
     102        to the animVal(). This will prevent deleting m_animVal while animation.
     103       
     104        * svg/properties/SVGAnimatedPathSegListPropertyTearOff.h:
     105        (WebCore::SVGAnimatedPathSegListPropertyTearOff::baseVal):
     106        (WebCore::SVGAnimatedPathSegListPropertyTearOff::animVal):
     107        Same as what is done in SVGAnimatedListPropertyTearOff.
     108       
     109        (WebCore::SVGAnimatedPathSegListPropertyTearOff::findItem):
     110        (WebCore::SVGAnimatedPathSegListPropertyTearOff::removeItemFromList):
     111        Same as what is done in SVGAnimatedListPropertyTearOff.
     112       
     113        * svg/properties/SVGAnimatedProperty.h:
     114        (WebCore::SVGAnimatedProperty::lookupOrCreateWrapper):
     115        Change the return value from raw reference to Ref<> and change the
     116        cached value from Ref<> to raw pointer. This reverts the change of
     117        r181345 in this function.
     118       
     119        (WebCore::SVGAnimatedProperty::lookupWrapper):
     120        Change the return value from raw pointer to RefPtr<>.
     121       
     122        * svg/properties/SVGAnimatedPropertyMacros.h:
     123        Use 'auto' type for the return of SVGAnimatedProperty::lookupWrapper().
     124       
     125        * svg/properties/SVGAnimatedTransformListPropertyTearOff.h:
     126        (WebCore::SVGAnimatedTransformListPropertyTearOff::baseVal):
     127        (WebCore::SVGAnimatedTransformListPropertyTearOff::animVal):
     128        Same as what is done in SVGAnimatedListPropertyTearOff.
     129
     130        * svg/properties/SVGListPropertyTearOff.h:
     131        (WebCore::SVGListPropertyTearOff::~SVGListPropertyTearOff):
     132        Call the SVGAnimatedListPropertyTearOff::propertyWillBeDeleted() to clean
     133        its raw pointers when the RefPtr<> deletes itself.
     134
    11352016-02-08  Carlos Garcia Campos  <cgarcia@igalia.com>
    2136

  • trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm

    r196200 r196268  
    42534253    }
    42544254
    4255     if ($codeGenerator->IsSVGAnimatedType($interfaceName) or $interfaceName eq "SVGViewSpec") {
     4255    # $type has to be used here because SVGViewSpec.transform is of type SVGTransformList.
     4256    if ($codeGenerator->IsSVGTypeNeedingTearOff($type) and $type =~ /(?<!String)List$/) {
     4257        # Convert from abstract RefPtr<ListProperty> to real type, so the right toJS() method can be invoked.
     4258        $value = "static_cast<" . GetNativeType($type) . ">($value" . ".get())";
     4259    } elsif ($codeGenerator->IsSVGAnimatedType($interfaceName) or $interfaceName eq "SVGViewSpec") {
    42564260        # Convert from abstract SVGProperty to real type, so the right toJS() method can be invoked.
    42574261        $value = "static_cast<" . GetNativeType($type) . ">($value)";

  • trunk/Source/WebCore/svg/SVGMarkerElement.cpp

    r194819 r196268  
    252252SVGMarkerOrientType& SVGMarkerElement::orientType() const
    253253{
    254     if (SVGAnimatedEnumeration* wrapper = SVGAnimatedProperty::lookupWrapper<UseOwnerType, SVGAnimatedEnumeration>(this, orientTypePropertyInfo())) {
     254    if (auto wrapper = SVGAnimatedProperty::lookupWrapper<UseOwnerType, SVGAnimatedEnumeration>(this, orientTypePropertyInfo())) {
    255255        if (wrapper->isAnimating()) {
    256256            ASSERT(wrapper->currentAnimatedValue() < SVGMarkerOrientMax);

  • trunk/Source/WebCore/svg/SVGPathElement.cpp

    r194964 r196268  
    295295const SVGPathByteStream& SVGPathElement::pathByteStream() const
    296296{
    297     SVGAnimatedProperty* property = SVGAnimatedProperty::lookupWrapper<SVGPathElement, SVGAnimatedPathSegListPropertyTearOff>(this, dPropertyInfo());
     297    auto property = SVGAnimatedProperty::lookupWrapper<SVGPathElement, SVGAnimatedPathSegListPropertyTearOff>(this, dPropertyInfo());
    298298    if (!property || !property->isAnimating())
    299299        return m_pathByteStream;
    300300   
    301     SVGPathByteStream* animatedPathByteStream = static_cast<SVGAnimatedPathSegListPropertyTearOff*>(property)->animatedPathByteStream();
     301    SVGPathByteStream* animatedPathByteStream = static_cast<SVGAnimatedPathSegListPropertyTearOff*>(property.get())->animatedPathByteStream();
    302302    if (!animatedPathByteStream)
    303303        return m_pathByteStream;
    … …  
    311311    SVGPathElement& ownerType = downcast<SVGPathElement>(*contextElement);
    312312
    313     if (SVGAnimatedProperty* property = SVGAnimatedProperty::lookupWrapper<SVGPathElement, SVGAnimatedPathSegListPropertyTearOff>(&ownerType, dPropertyInfo()))
     313    if (auto property = SVGAnimatedProperty::lookupWrapper<SVGPathElement, SVGAnimatedPathSegListPropertyTearOff>(&ownerType, dPropertyInfo()))
    314314        return *property;
    315315
    … …  
    330330}
    331331
    332 SVGPathSegListPropertyTearOff* SVGPathElement::pathSegList()
     332RefPtr<SVGPathSegListPropertyTearOff> SVGPathElement::pathSegList()
    333333{
    334334    m_pathSegList.shouldSynchronize = true;
    335     return static_cast<SVGPathSegListPropertyTearOff*>(static_reference_cast<SVGAnimatedPathSegListPropertyTearOff>(lookupOrCreateDWrapper(this))->baseVal());
    336 }
    337 
    338 SVGPathSegListPropertyTearOff* SVGPathElement::normalizedPathSegList()
     335    return static_cast<SVGPathSegListPropertyTearOff*>(static_reference_cast<SVGAnimatedPathSegListPropertyTearOff>(lookupOrCreateDWrapper(this))->baseVal().get());
     336}
     337
     338RefPtr<SVGPathSegListPropertyTearOff> SVGPathElement::normalizedPathSegList()
    339339{
    340340    // FIXME: https://bugs.webkit.org/show_bug.cgi?id=15412 - Implement normalized path segment lists!
    341     return 0;
    342 }
    343 
    344 SVGPathSegListPropertyTearOff* SVGPathElement::animatedPathSegList()
     341    return nullptr;
     342}
     343
     344RefPtr<SVGPathSegListPropertyTearOff> SVGPathElement::animatedPathSegList()
    345345{
    346346    m_pathSegList.shouldSynchronize = true;
    347347    m_isAnimValObserved = true;
    348     return static_cast<SVGPathSegListPropertyTearOff*>(static_reference_cast<SVGAnimatedPathSegListPropertyTearOff>(lookupOrCreateDWrapper(this))->animVal());
    349 }
    350 
    351 SVGPathSegListPropertyTearOff* SVGPathElement::animatedNormalizedPathSegList()
     348    return static_cast<SVGPathSegListPropertyTearOff*>(static_reference_cast<SVGAnimatedPathSegListPropertyTearOff>(lookupOrCreateDWrapper(this))->animVal().get());
     349}
     350
     351RefPtr<SVGPathSegListPropertyTearOff> SVGPathElement::animatedNormalizedPathSegList()
    352352{
    353353    // FIXME: https://bugs.webkit.org/show_bug.cgi?id=15412 - Implement normalized path segment lists!
    354     return 0;
     354    return nullptr;
    355355}
    356356

  • trunk/Source/WebCore/svg/SVGPathElement.h

    r194964 r196268  
    8383
    8484    // Used in the bindings only.
    85     SVGPathSegListPropertyTearOff* pathSegList();
    86     SVGPathSegListPropertyTearOff* animatedPathSegList();
    87     SVGPathSegListPropertyTearOff* normalizedPathSegList();
    88     SVGPathSegListPropertyTearOff* animatedNormalizedPathSegList();
     85    RefPtr<SVGPathSegListPropertyTearOff> pathSegList();
     86    RefPtr<SVGPathSegListPropertyTearOff> animatedPathSegList();
     87    RefPtr<SVGPathSegListPropertyTearOff> normalizedPathSegList();
     88    RefPtr<SVGPathSegListPropertyTearOff> animatedNormalizedPathSegList();
    8989
    9090    const SVGPathByteStream& pathByteStream() const;

  • trunk/Source/WebCore/svg/SVGPathSegWithContext.h

    r194964 r196268  
    3333    }
    3434
    35     SVGAnimatedProperty* animatedProperty() const
     35    RefPtr<SVGAnimatedProperty> animatedProperty() const
    3636    {
    3737        if (!m_element)

  • trunk/Source/WebCore/svg/SVGPolyElement.cpp

    r184852 r196268  
    6969            document().accessSVGExtensions().reportError("Problem parsing points=\"" + value + "\"");
    7070
    71         if (SVGAnimatedProperty* wrapper = SVGAnimatedProperty::lookupWrapper<SVGPolyElement, SVGAnimatedPointList>(this, pointsPropertyInfo()))
    72             static_cast<SVGAnimatedPointList*>(wrapper)->detachListWrappers(newList.size());
     71        if (auto wrapper = SVGAnimatedProperty::lookupWrapper<SVGPolyElement, SVGAnimatedPointList>(this, pointsPropertyInfo()))
     72            static_pointer_cast<SVGAnimatedPointList>(wrapper)->detachListWrappers(newList.size());
    7373
    7474        m_points.value = newList;
    … …  
    119119}
    120120
    121 SVGListPropertyTearOff<SVGPointList>* SVGPolyElement::points()
     121RefPtr<SVGListPropertyTearOff<SVGPointList>> SVGPolyElement::points()
    122122{
    123123    m_points.shouldSynchronize = true;
    124     return static_cast<SVGListPropertyTearOff<SVGPointList>*>(static_reference_cast<SVGAnimatedPointList>(lookupOrCreatePointsWrapper(this))->baseVal());
     124    return static_cast<SVGListPropertyTearOff<SVGPointList>*>(static_reference_cast<SVGAnimatedPointList>(lookupOrCreatePointsWrapper(this))->baseVal().get());
    125125}
    126126
    127 SVGListPropertyTearOff<SVGPointList>* SVGPolyElement::animatedPoints()
     127RefPtr<SVGListPropertyTearOff<SVGPointList>> SVGPolyElement::animatedPoints()
    128128{
    129129    m_points.shouldSynchronize = true;
    130     return static_cast<SVGListPropertyTearOff<SVGPointList>*>(static_reference_cast<SVGAnimatedPointList>(lookupOrCreatePointsWrapper(this))->animVal());
     130    return static_cast<SVGListPropertyTearOff<SVGPointList>*>(static_reference_cast<SVGAnimatedPointList>(lookupOrCreatePointsWrapper(this))->animVal().get());
    131131}
    132132

  • trunk/Source/WebCore/svg/SVGPolyElement.h

    r185503 r196268  
    3232class SVGPolyElement : public SVGGraphicsElement, public SVGExternalResourcesRequired {
    3333public:
    34     SVGListPropertyTearOff<SVGPointList>* points();
    35     SVGListPropertyTearOff<SVGPointList>* animatedPoints();
     34    RefPtr<SVGListPropertyTearOff<SVGPointList>> points();
     35    RefPtr<SVGListPropertyTearOff<SVGPointList>> animatedPoints();
    3636
    3737    SVGPointList& pointList() const { return m_points.value; }

  • trunk/Source/WebCore/svg/SVGViewSpec.cpp

    r194819 r196268  
    115115    newList.parse(transform);
    116116
    117     if (SVGAnimatedProperty* wrapper = SVGAnimatedProperty::lookupWrapper<SVGElement, SVGAnimatedTransformList>(m_contextElement, transformPropertyInfo()))
    118         static_cast<SVGAnimatedTransformList*>(wrapper)->detachListWrappers(newList.size());
     117    if (auto wrapper = SVGAnimatedProperty::lookupWrapper<SVGElement, SVGAnimatedTransformList>(m_contextElement, transformPropertyInfo()))
     118        static_pointer_cast<SVGAnimatedTransformList>(wrapper)->detachListWrappers(newList.size());
    119119
    120120    m_transform = newList;
    … …  
    146146}
    147147
    148 SVGTransformListPropertyTearOff* SVGViewSpec::transform()
     148RefPtr<SVGTransformListPropertyTearOff> SVGViewSpec::transform()
    149149{
    150150    if (!m_contextElement)
    151151        return nullptr;
    152152    // Return the animVal here, as its readonly by default - which is exactly what we want here.
    153     return static_cast<SVGTransformListPropertyTearOff*>(static_reference_cast<SVGAnimatedTransformList>(lookupOrCreateTransformWrapper(this))->animVal());
     153    return static_pointer_cast<SVGTransformListPropertyTearOff>(static_reference_cast<SVGAnimatedTransformList>(lookupOrCreateTransformWrapper(this))->animVal());
    154154}
    155155

  • trunk/Source/WebCore/svg/SVGViewSpec.h

    r184852 r196268  
    6969
    7070    // Custom non-animated 'transform' property.
    71     SVGTransformListPropertyTearOff* transform();
     71    RefPtr<SVGTransformListPropertyTearOff> transform();
    7272    SVGTransformList transformBaseValue() const { return m_transform; }
    7373

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

    r181345 r196268  
    4040    typedef PropertyType ContentType;
    4141
    42     virtual ListProperty* baseVal()
    43     {
    44         if (!m_baseVal)
    45             m_baseVal = ListPropertyTearOff::create(this, BaseValRole, m_values, m_wrappers);
    46         return static_cast<ListProperty*>(m_baseVal.get());
    47     }
    48 
    49     virtual ListProperty* animVal()
    50     {
    51         if (!m_animVal)
    52             m_animVal = ListPropertyTearOff::create(this, AnimValRole, m_values, m_wrappers);
    53         return static_cast<ListProperty*>(m_animVal.get());
     42    virtual RefPtr<ListProperty> baseVal()
     43    {
     44        if (m_baseVal)
     45            return m_baseVal;
     46
     47        auto property = ListPropertyTearOff::create(this, BaseValRole, m_values, m_wrappers);
     48        m_baseVal = property.ptr();
     49        return WTFMove(property);
     50    }
     51
     52    virtual RefPtr<ListProperty> animVal()
     53    {
     54        if (m_animVal)
     55            return m_animVal;
     56
     57        auto property = ListPropertyTearOff::create(this, AnimValRole, m_values, m_wrappers);
     58        m_animVal = property.ptr();
     59        return WTFMove(property);
     60    }
     61
     62    void propertyWillBeDeleted(const ListProperty& property)
     63    {
     64        if (&property == m_baseVal)
     65            m_baseVal = nullptr;
     66        else if (&property == m_animVal)
     67            m_animVal = nullptr;
    5468    }
    5569
    5670    virtual bool isAnimatedListTearOff() const override { return true; }
    5771
    58     int findItem(SVGProperty* property) const
     72    int findItem(SVGProperty* property)
    5973    {
    6074        // This should ever be called for our baseVal, as animVal can't modify the list.
    6175        // It's safe to cast to ListPropertyTearOff here as all classes inheriting from us supply their own removeItemFromList() method.
    6276        typedef SVGPropertyTearOff<typename SVGPropertyTraits<PropertyType>::ListItemType> ListItemTearOff;
    63         return static_cast<ListPropertyTearOff*>(m_baseVal.get())->findItem(static_cast<ListItemTearOff*>(property));
     77        return static_pointer_cast<ListPropertyTearOff>(baseVal())->findItem(static_cast<ListItemTearOff*>(property));
    6478    }
    6579
    … …  
    6882        // This should ever be called for our baseVal, as animVal can't modify the list.
    6983        // It's safe to cast to ListPropertyTearOff here as all classes inheriting from us supply their own removeItemFromList() method.
    70         static_cast<ListPropertyTearOff*>(m_baseVal.get())->removeItemFromList(itemIndex, shouldSynchronizeWrappers);
     84        static_pointer_cast<ListPropertyTearOff>(baseVal())->removeItemFromList(itemIndex, shouldSynchronizeWrappers);
    7185    }
    7286
    … …  
    7993    {
    8094        ASSERT(m_isAnimating);
    81         ASSERT(m_animVal);
    82         return static_cast<ListProperty*>(m_animVal.get())->values();
     95        ASSERT(m_animatingAnimVal);
     96        return static_pointer_cast<ListProperty>(m_animatingAnimVal)->values();
    8397    }
    8498
    … …  
    91105    {
    92106        ASSERT(!m_isAnimating);
     107        ASSERT(!m_animatingAnimVal);
    93108        ASSERT(newAnimVal);
    94109        ASSERT(m_values.size() == m_wrappers.size());
    … …  
    99114            m_animatedWrappers.fill(0, newAnimVal->size());
    100115
    101         ListProperty* animVal = static_cast<ListProperty*>(this->animVal());
    102         animVal->setValuesAndWrappers(newAnimVal, &m_animatedWrappers, shouldOwnValues);
    103         ASSERT(animVal->values().size() == animVal->wrappers().size());
    104         ASSERT(animVal->wrappers().size() == m_animatedWrappers.size());
     116        m_animatingAnimVal = animVal();
     117        m_animatingAnimVal->setValuesAndWrappers(newAnimVal, &m_animatedWrappers, shouldOwnValues);
     118        ASSERT(m_animatingAnimVal->values().size() == m_animatingAnimVal->wrappers().size());
     119        ASSERT(m_animatingAnimVal->wrappers().size() == m_animatedWrappers.size());
    105120        m_isAnimating = true;
    106121    }
    … …  
    109124    {
    110125        ASSERT(m_isAnimating);
    111         ASSERT(m_animVal);
    112         ASSERT(m_values.size() == m_wrappers.size());
    113 
    114         ListProperty* animVal = static_cast<ListProperty*>(m_animVal.get());
    115         ASSERT(animVal->values().size() == animVal->wrappers().size());
    116         ASSERT(animVal->wrappers().size() == m_animatedWrappers.size());
    117 
    118         animVal->setValuesAndWrappers(&m_values, &m_wrappers, false);
    119         ASSERT(animVal->values().size() == animVal->wrappers().size());
    120         ASSERT(animVal->wrappers().size() == m_wrappers.size());
     126        ASSERT(m_animatingAnimVal);
     127        ASSERT(m_values.size() == m_wrappers.size());
     128
     129        ASSERT(m_animatingAnimVal->values().size() == m_animatingAnimVal->wrappers().size());
     130        ASSERT(m_animatingAnimVal->wrappers().size() == m_animatedWrappers.size());
     131
     132        m_animatingAnimVal->setValuesAndWrappers(&m_values, &m_wrappers, false);
     133        ASSERT(m_animatingAnimVal->values().size() == m_animatingAnimVal->wrappers().size());
     134        ASSERT(m_animatingAnimVal->wrappers().size() == m_wrappers.size());
    121135
    122136        m_animatedWrappers.clear();
     137        m_animatingAnimVal = nullptr;
    123138        m_isAnimating = false;
    124139    }
    … …  
    126141    void synchronizeWrappersIfNeeded()
    127142    {
     143        ASSERT(m_isAnimating);
     144        ASSERT(m_animatingAnimVal);
     145
    128146        // Eventually the wrapper list needs synchronization because any SVGAnimateLengthList::calculateAnimatedValue() call may
    129147        // mutate the length of our values() list, and thus the wrapper() cache needs synchronization, to have the same size.
    … …  
    131149        // of them is created, so existing animVal variables in JS are kept-alive). If we'd detach them later the underlying
    132150        // SVGLengthList was already mutated, and our list item wrapper tear offs would point nowhere. Assertions would fire.
    133         ListProperty* animVal = static_cast<ListProperty*>(m_animVal.get());
    134         animVal->detachListWrappers(animVal->values().size());
    135 
    136         ASSERT(animVal->values().size() == animVal->wrappers().size());
    137         ASSERT(animVal->wrappers().size() == m_animatedWrappers.size());
     151        m_animatingAnimVal->detachListWrappers(m_animatingAnimVal->values().size());
     152
     153        ASSERT(m_animatingAnimVal->values().size() == m_animatingAnimVal->wrappers().size());
     154        ASSERT(m_animatingAnimVal->wrappers().size() == m_animatedWrappers.size());
    138155    }
    139156
    140157    void animValWillChange()
    141158    {
    142         ASSERT(m_isAnimating);
    143         ASSERT(m_animVal);
    144159        ASSERT(m_values.size() == m_wrappers.size());
    145160        synchronizeWrappersIfNeeded();
    … …  
    148163    void animValDidChange()
    149164    {
    150         ASSERT(m_isAnimating);
    151         ASSERT(m_animVal);
    152165        ASSERT(m_values.size() == m_wrappers.size());
    153166        synchronizeWrappersIfNeeded();
    … …  
    174187    ListWrapperCache m_animatedWrappers;
    175188
    176     RefPtr<SVGProperty> m_baseVal;
    177     RefPtr<SVGProperty> m_animVal;
     189    // Cache the raw pointer but return a RefPtr<>. This will break the cyclic reference
     190    // between SVGListPropertyTearOff and SVGAnimatedListPropertyTearOff once the property
     191    // pointer is not needed.
     192    ListProperty* m_baseVal { nullptr };
     193    ListProperty* m_animVal { nullptr };
     194
     195    RefPtr<ListProperty> m_animatingAnimVal;
    178196};
    179197

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

    r190844 r196268  
    3232class SVGAnimatedPathSegListPropertyTearOff : public SVGAnimatedListPropertyTearOff<SVGPathSegList> {
    3333public:
    34     virtual SVGListProperty<SVGPathSegList>* baseVal() override
     34    virtual RefPtr<ListProperty> baseVal() override
    3535    {
    36         if (!m_baseVal)
    37             m_baseVal = SVGPathSegListPropertyTearOff::create(this, BaseValRole, PathSegUnalteredRole, m_values, m_wrappers);
    38         return static_cast<SVGListProperty<SVGPathSegList>*>(m_baseVal.get());
     36        if (m_baseVal)
     37            return m_baseVal;
     38
     39        auto property = SVGPathSegListPropertyTearOff::create(this, BaseValRole, PathSegUnalteredRole, m_values, m_wrappers);
     40        m_baseVal = property.ptr();
     41        return WTFMove(property);
    3942    }
    4043
    41     virtual SVGListProperty<SVGPathSegList>* animVal() override
     44    virtual RefPtr<ListProperty> animVal() override
    4245    {
    43         if (!m_animVal)
    44             m_animVal = SVGPathSegListPropertyTearOff::create(this, AnimValRole, PathSegUnalteredRole, m_values, m_wrappers);
    45         return static_cast<SVGListProperty<SVGPathSegList>*>(m_animVal.get());
     46        if (m_animVal)
     47            return m_animVal;
     48
     49        auto property = SVGPathSegListPropertyTearOff::create(this, AnimValRole, PathSegUnalteredRole, m_values, m_wrappers);
     50        m_animVal = property.ptr();
     51        return WTFMove(property);
    4652    }
    4753
    48     int findItem(const RefPtr<SVGPathSeg>& segment) const
     54    int findItem(const RefPtr<SVGPathSeg>& segment)
    4955    {
    5056        // This should ever be called for our baseVal, as animVal can't modify the list.
    51         ASSERT(m_baseVal);
    52         return static_cast<SVGPathSegListPropertyTearOff*>(m_baseVal.get())->findItem(segment);
     57        return static_cast<SVGPathSegListPropertyTearOff*>(baseVal().get())->findItem(segment);
    5358    }
    5459
    … …  
    5661    {
    5762        // This should ever be called for our baseVal, as animVal can't modify the list.
    58         ASSERT(m_baseVal);
    59         static_cast<SVGPathSegListPropertyTearOff*>(m_baseVal.get())->removeItemFromList(itemIndex, shouldSynchronizeWrappers);
     63        static_cast<SVGPathSegListPropertyTearOff*>(baseVal().get())->removeItemFromList(itemIndex, shouldSynchronizeWrappers);
    6064    }
    6165

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

    r181345 r196268  
    4949
    5050    template<typename OwnerType, typename TearOffType, typename PropertyType>
    51     static TearOffType& lookupOrCreateWrapper(OwnerType* element, const SVGPropertyInfo* info, PropertyType& property)
     51    static Ref<TearOffType> lookupOrCreateWrapper(OwnerType* element, const SVGPropertyInfo* info, PropertyType& property)
    5252    {
    5353        ASSERT(info);
    5454        SVGAnimatedPropertyDescription key(element, info->propertyIdentifier);
    55         auto& slot = animatedPropertyCache()->add(key, nullptr).iterator->value;
    56         if (!slot) {
    57             Ref<SVGAnimatedProperty> wrapper = TearOffType::create(element, info->attributeName, info->animatedPropertyType, property);
    58             if (info->animatedPropertyState == PropertyIsReadOnly)
    59                 wrapper->setIsReadOnly();
    60             slot = &wrapper.leakRef();
    61         }
    62         return static_cast<TearOffType&>(*slot);
     55
     56        auto result = animatedPropertyCache()->add(key, nullptr);
     57        if (!result.isNewEntry)
     58            return static_cast<TearOffType&>(*result.iterator->value);
     59
     60        Ref<SVGAnimatedProperty> wrapper = TearOffType::create(element, info->attributeName, info->animatedPropertyType, property);
     61        if (info->animatedPropertyState == PropertyIsReadOnly)
     62            wrapper->setIsReadOnly();
     63
     64        // Cache the raw pointer but return a Ref<>. This will break the cyclic reference
     65        // between SVGAnimatedProperty and SVGElement once the property pointer is not needed.
     66        result.iterator->value = wrapper.ptr();
     67        return static_reference_cast<TearOffType>(wrapper);
    6368    }
    6469
    6570    template<typename OwnerType, typename TearOffType>
    66     static TearOffType* lookupWrapper(OwnerType* element, const SVGPropertyInfo* info)
     71    static RefPtr<TearOffType> lookupWrapper(OwnerType* element, const SVGPropertyInfo* info)
    6772    {
    6873        ASSERT(info);
    … …  
    7277
    7378    template<typename OwnerType, typename TearOffType>
    74     static TearOffType* lookupWrapper(const OwnerType* element, const SVGPropertyInfo* info)
     79    static RefPtr<TearOffType> lookupWrapper(const OwnerType* element, const SVGPropertyInfo* info)
    7580    {
    7681        return lookupWrapper<OwnerType, TearOffType>(const_cast<OwnerType*>(element), info);

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

    r194987 r196268  
    124124    PropertyType& LowerProperty() const \
    125125    { \
    126         if (TearOffType* wrapper = SVGAnimatedProperty::lookupWrapper<UseOwnerType, TearOffType>(this, LowerProperty##PropertyInfo())) { \
     126        if (auto wrapper = SVGAnimatedProperty::lookupWrapper<UseOwnerType, TearOffType>(this, LowerProperty##PropertyInfo())) { \
    127127            if (wrapper->isAnimating()) \
    128128                return wrapper->currentAnimatedValue(); \
    … …  
    185185void detachAnimated##UpperProperty##ListWrappers(unsigned newListSize) \
    186186{ \
    187     if (TearOffType* wrapper = SVGAnimatedProperty::lookupWrapper<UseOwnerType, TearOffType>(this, LowerProperty##PropertyInfo())) \
     187    if (auto wrapper = SVGAnimatedProperty::lookupWrapper<UseOwnerType, TearOffType>(this, LowerProperty##PropertyInfo())) \
    188188        wrapper->detachListWrappers(newListSize); \
    189189}

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

    r181345 r196268  
    2929class SVGAnimatedTransformListPropertyTearOff : public SVGAnimatedListPropertyTearOff<SVGTransformList> {
    3030public:
    31     virtual SVGListPropertyTearOff<SVGTransformList>* baseVal() override
     31    virtual RefPtr<ListProperty> baseVal() override
    3232    {
    33         if (!m_baseVal)
    34             m_baseVal = SVGTransformListPropertyTearOff::create(this, BaseValRole, m_values, m_wrappers);
    35         return static_cast<SVGListPropertyTearOff<SVGTransformList>*>(m_baseVal.get());
     33        if (m_baseVal)
     34            return m_baseVal;
     35
     36        auto property = SVGTransformListPropertyTearOff::create(this, BaseValRole, m_values, m_wrappers);
     37        m_baseVal = property.ptr();
     38        return WTFMove(property);
    3639    }
    3740
    38     virtual SVGListPropertyTearOff<SVGTransformList>* animVal() override
     41    virtual RefPtr<ListProperty> animVal() override
    3942    {
    40         if (!m_animVal)
    41             m_animVal = SVGTransformListPropertyTearOff::create(this, AnimValRole, m_values, m_wrappers);
    42         return static_cast<SVGListPropertyTearOff<SVGTransformList>*>(m_animVal.get());
     43        if (m_animVal)
     44            return m_animVal;
     45
     46        auto property = SVGTransformListPropertyTearOff::create(this, AnimValRole, m_values, m_wrappers);
     47        m_animVal = property.ptr();
     48        return WTFMove(property);
    4349    }
    4450

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

    r181345 r196268  
    121121    }
    122122
     123    virtual ~SVGListPropertyTearOff()
     124    {
     125        if (m_animatedProperty)
     126            m_animatedProperty->propertyWillBeDeleted(*this);
     127    }
     128
    123129    virtual bool isReadOnly() const override
    124130    {

  • trunk/Source/WebCore/svg/properties/SVGPathSegListPropertyTearOff.cpp

    r194964 r196268  
    9191{
    9292    SVGPathSegWithContext* newItemWithContext = static_cast<SVGPathSegWithContext*>(newItem.get());
    93     SVGAnimatedProperty* animatedPropertyOfItem = newItemWithContext->animatedProperty();
     93    RefPtr<SVGAnimatedProperty> animatedPropertyOfItem = newItemWithContext->animatedProperty();
    9494
    9595    // Alter the role, after calling animatedProperty(), as that may influence the returned animated property.
    … …  
    107107    // 'newItem' is already living in another list. If it's not our list, synchronize the other lists wrappers after the removal.
    108108    bool livesInOtherList = animatedPropertyOfItem != m_animatedProperty;
    109     SVGAnimatedPathSegListPropertyTearOff* propertyTearOff = static_cast<SVGAnimatedPathSegListPropertyTearOff*>(animatedPropertyOfItem);
     109    RefPtr<SVGAnimatedPathSegListPropertyTearOff> propertyTearOff = static_pointer_cast<SVGAnimatedPathSegListPropertyTearOff>(animatedPropertyOfItem);
    110110    int indexToRemove = propertyTearOff->findItem(newItem.get());
    111111    ASSERT(indexToRemove != -1);
Note: See TracChangeset for help on using the changeset viewer.