Changeset 85413 in webkit


Ignore:
Timestamp:
May 1, 2011 7:38:44 AM (13 years ago)
Author:
Nikolas Zimmermann
Message:

2011-05-01 Nikolas Zimmermann <​nzimmermann@rim.com>

Reviewed by Dirk Schulze.

LEAK: SVGElement leaks when detaching it in a pending resource state
​https://bugs.webkit.org/show_bug.cgi?id=59072

Add testcase that used to leak, the leaks bot will assure they won't in future.

  • svg/custom/pending-resource-leak-2-expected.txt: Added.
  • svg/custom/pending-resource-leak-2.svg: Added.
  • svg/custom/pending-resource-leak-3-expected.txt: Added.
  • svg/custom/pending-resource-leak-3.svg: Added.
  • svg/custom/pending-resource-leak-expected.txt: Added.
  • svg/custom/pending-resource-leak.svg: Added.

2011-05-01 Nikolas Zimmermann <​nzimmermann@rim.com>

Reviewed by Dirk Schulze.

LEAK: SVGElement leaks when detaching it in a pending resource state
​https://bugs.webkit.org/show_bug.cgi?id=59072

Make the pending resources set non-refcounted again. We made it refcounted a while ago
to fix a security bug, as we had dangling pointers in the set in SVGDocumentExtensions.
Fix the underlying problem, by removing all pending resources referencing to a particular
SVGElement, upon its destruction or upon removing it from the document.

Example: <rect fill="url(#foo)" id="rect">
When we try to render the rect, the foo paint server can't be found and thus "foo" will be
added to the pending resource set, with "rect" as client. When "foo" appears, it would remove
itself from the pending resource set, and a ref count to the "rect" would be released.
If "foo" never appears, SVGDocumentExtensions still holds a ref to the <rect>, thus keeping
it and the associated document alive.

Tests: svg/custom/pending-resource-leak-2.svg

svg/custom/pending-resource-leak-3.svg
svg/custom/pending-resource-leak.svg

These tests cover several scenarios where we used to leak. Should fix several SVG*Element leaks on the bots.
I manually tested reloading above testcases dozens of times, before the leak count was incremented by 2 nodes on every reload, that's gone now.

  • rendering/svg/RenderSVGResourceContainer.cpp: (WebCore::RenderSVGResourceContainer::registerResource):
  • rendering/svg/RenderSVGShadowTreeRootContainer.cpp: (WebCore::RenderSVGShadowTreeRootContainer::updateFromElement):
  • rendering/svg/SVGResources.cpp: (WebCore::registerPendingResource):
  • svg/SVGDocumentExtensions.cpp: (WebCore::SVGDocumentExtensions::addPendingResource): (WebCore::SVGDocumentExtensions::hasPendingResources): (WebCore::SVGDocumentExtensions::removeElementFromPendingResources): (WebCore::SVGDocumentExtensions::removePendingResource):
  • svg/SVGDocumentExtensions.h:
  • svg/SVGElement.cpp:
  • svg/SVGElement.h:
  • svg/SVGElementRareData.h: (WebCore::SVGElementRareData::SVGElementRareData): (WebCore::SVGElementRareData::hasPendingResources): (WebCore::SVGElementRareData::setHasPendingResources):
  • svg/SVGStyledElement.cpp: (WebCore::SVGStyledElement::~SVGStyledElement): (WebCore::SVGStyledElement::insertedIntoDocument): (WebCore::SVGStyledElement::removedFromDocument): (WebCore::SVGStyledElement::hasPendingResources): (WebCore::SVGStyledElement::setHasPendingResources):
  • svg/SVGStyledElement.h: (WebCore::SVGStyledElement::needsPendingResourceHandling): (WebCore::SVGStyledElement::buildPendingResource):
  • svg/SVGUseElement.cpp: (WebCore::SVGUseElement::SVGUseElement): (WebCore::SVGUseElement::insertedIntoDocument): (WebCore::SVGUseElement::svgAttributeChanged): (WebCore::SVGUseElement::buildPendingResource):
  • svg/SVGUseElement.h:
Location:
trunk
Files:
6 added
14 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r85411 r85413  
     12011-05-01  Nikolas Zimmermann  <nzimmermann@rim.com>
     2
     3        Reviewed by Dirk Schulze.
     4
     5        LEAK: SVGElement leaks when detaching it in a pending resource state
     6        https://bugs.webkit.org/show_bug.cgi?id=59072
     7
     8        Add testcase that used to leak, the leaks bot will assure they won't in future.
     9
     10        * svg/custom/pending-resource-leak-2-expected.txt: Added.
     11        * svg/custom/pending-resource-leak-2.svg: Added.
     12        * svg/custom/pending-resource-leak-3-expected.txt: Added.
     13        * svg/custom/pending-resource-leak-3.svg: Added.
     14        * svg/custom/pending-resource-leak-expected.txt: Added.
     15        * svg/custom/pending-resource-leak.svg: Added.
     16
    1172011-05-01  Dan Bernstein  <mitz@apple.com>
    218

  • trunk/Source/WebCore/ChangeLog

    r85412 r85413  
     12011-05-01  Nikolas Zimmermann  <nzimmermann@rim.com>
     2
     3        Reviewed by Dirk Schulze.
     4
     5        LEAK: SVGElement leaks when detaching it in a pending resource state
     6        https://bugs.webkit.org/show_bug.cgi?id=59072
     7
     8        Make the pending resources set non-refcounted again. We made it refcounted a while ago
     9        to fix a security bug, as we had dangling pointers in the set in SVGDocumentExtensions.
     10        Fix the underlying problem, by removing all pending resources referencing to a particular
     11        SVGElement, upon its destruction or upon removing it from the document.
     12
     13        Example: <rect fill="url(#foo)" id="rect">
     14        When we try to render the rect, the foo paint server can't be found and thus "foo" will be
     15        added to the pending resource set, with "rect" as client. When "foo" appears, it would remove
     16        itself from the pending resource set, and a ref count to the "rect" would be released.
     17        If "foo" never appears, SVGDocumentExtensions still holds a ref to the <rect>, thus keeping
     18        it and the associated document alive.
     19
     20        Tests: svg/custom/pending-resource-leak-2.svg
     21               svg/custom/pending-resource-leak-3.svg
     22               svg/custom/pending-resource-leak.svg
     23
     24        These tests cover several scenarios where we used to leak. Should fix several SVG*Element leaks on the bots.
     25        I manually tested reloading above testcases dozens of times, before the leak count was incremented by 2 nodes on every reload, that's gone now.
     26
     27        * rendering/svg/RenderSVGResourceContainer.cpp:
     28        (WebCore::RenderSVGResourceContainer::registerResource):
     29        * rendering/svg/RenderSVGShadowTreeRootContainer.cpp:
     30        (WebCore::RenderSVGShadowTreeRootContainer::updateFromElement):
     31        * rendering/svg/SVGResources.cpp:
     32        (WebCore::registerPendingResource):
     33        * svg/SVGDocumentExtensions.cpp:
     34        (WebCore::SVGDocumentExtensions::addPendingResource):
     35        (WebCore::SVGDocumentExtensions::hasPendingResources):
     36        (WebCore::SVGDocumentExtensions::removeElementFromPendingResources):
     37        (WebCore::SVGDocumentExtensions::removePendingResource):
     38        * svg/SVGDocumentExtensions.h:
     39        * svg/SVGElement.cpp:
     40        * svg/SVGElement.h:
     41        * svg/SVGElementRareData.h:
     42        (WebCore::SVGElementRareData::SVGElementRareData):
     43        (WebCore::SVGElementRareData::hasPendingResources):
     44        (WebCore::SVGElementRareData::setHasPendingResources):
     45        * svg/SVGStyledElement.cpp:
     46        (WebCore::SVGStyledElement::~SVGStyledElement):
     47        (WebCore::SVGStyledElement::insertedIntoDocument):
     48        (WebCore::SVGStyledElement::removedFromDocument):
     49        (WebCore::SVGStyledElement::hasPendingResources):
     50        (WebCore::SVGStyledElement::setHasPendingResources):
     51        * svg/SVGStyledElement.h:
     52        (WebCore::SVGStyledElement::needsPendingResourceHandling):
     53        (WebCore::SVGStyledElement::buildPendingResource):
     54        * svg/SVGUseElement.cpp:
     55        (WebCore::SVGUseElement::SVGUseElement):
     56        (WebCore::SVGUseElement::insertedIntoDocument):
     57        (WebCore::SVGUseElement::svgAttributeChanged):
     58        (WebCore::SVGUseElement::buildPendingResource):
     59        * svg/SVGUseElement.h:
     60
    1612011-05-01  Rafael Brandao  <rafael.lobo@openbossa.org>
    262

  • trunk/Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp

    r76146 r85413  
    155155{
    156156    SVGDocumentExtensions* extensions = svgExtensionsFromNode(node());
    157     if (!extensions->isPendingResource(m_id)) {
     157    if (!extensions->hasPendingResources(m_id)) {
    158158        extensions->addResource(m_id, this);
    159159        return;
    … …  
    168168    const SVGDocumentExtensions::SVGPendingElements::const_iterator end = clients->end();
    169169    for (SVGDocumentExtensions::SVGPendingElements::const_iterator it = clients->begin(); it != end; ++it) {
     170        ASSERT((*it)->hasPendingResources());
     171        (*it)->setHasPendingResources(false);
    170172        RenderObject* renderer = (*it)->renderer();
    171173        if (!renderer)

  • trunk/Source/WebCore/rendering/svg/RenderSVGShadowTreeRootContainer.cpp

    r84350 r85413  
    7474    // If the use element is a pending resource, and a) or b) is true, do nothing, and wait for the use
    7575    // element to be asked to buildPendingResource(), this will call us again, with m_recreateTrue=true.
    76     if ((shouldRecreateTree || !hadExistingTree) && !useElement->isPendingResource()) {
     76    if ((shouldRecreateTree || !hadExistingTree) && !useElement->hasPendingResources()) {
    7777        useElement->buildShadowAndInstanceTree(m_shadowRoot.get());
    7878

  • trunk/Source/WebCore/rendering/svg/SVGResources.cpp

    r84815 r85413  
    172172{
    173173    ASSERT(element);
    174     if (!element->isStyled())
    175         return;
    176 
     174    ASSERT(element->isStyled());
    177175    extensions->addPendingResource(id, static_cast<SVGStyledElement*>(element));
    178176}

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

    r84891 r85413  
    212212}
    213213
    214 void SVGDocumentExtensions::addPendingResource(const AtomicString& id, PassRefPtr<SVGStyledElement> obj)
    215 {
    216     ASSERT(obj);
     214void SVGDocumentExtensions::addPendingResource(const AtomicString& id, SVGStyledElement* element)
     215{
     216    ASSERT(element);
    217217
    218218    if (id.isEmpty())
    … …  
    220220
    221221    if (m_pendingResources.contains(id))
    222         m_pendingResources.get(id)->add(obj);
     222        m_pendingResources.get(id)->add(element);
    223223    else {
    224224        SVGPendingElements* set = new SVGPendingElements;
    225         set->add(obj);
     225        set->add(element);
    226226
    227227        m_pendingResources.add(id, set);
    228228    }
    229 }
    230 
    231 bool SVGDocumentExtensions::isPendingResource(const AtomicString& id) const
     229
     230    element->setHasPendingResources(true);
     231}
     232
     233bool SVGDocumentExtensions::hasPendingResources(const AtomicString& id) const
    232234{
    233235    if (id.isEmpty())
    … …  
    237239}
    238240
    239 PassOwnPtr<HashSet<RefPtr<SVGStyledElement> > > SVGDocumentExtensions::removePendingResource(const AtomicString& id)
     241void SVGDocumentExtensions::removeElementFromPendingResources(SVGStyledElement* element)
     242{
     243    ASSERT(element);
     244
     245    if (m_pendingResources.isEmpty() || !element->hasPendingResources())
     246        return;
     247
     248    element->setHasPendingResources(false);
     249
     250    Vector<AtomicString> toBeRemoved;
     251    HashMap<AtomicString, SVGPendingElements*>::iterator end = m_pendingResources.end();
     252    for (HashMap<AtomicString, SVGPendingElements*>::iterator it = m_pendingResources.begin(); it != end; ++it) {
     253        SVGPendingElements* elements = it->second;
     254        ASSERT(elements);
     255        ASSERT(!elements->isEmpty());
     256
     257        elements->remove(element);
     258        if (elements->isEmpty())
     259            toBeRemoved.append(it->first);
     260    }
     261
     262    if (toBeRemoved.isEmpty())
     263        return;
     264
     265    Vector<AtomicString>::iterator endVector = toBeRemoved.end();
     266    for (Vector<AtomicString>::iterator it = toBeRemoved.begin(); it != endVector; ++it)
     267        m_pendingResources.remove(*it);
     268}
     269
     270PassOwnPtr<SVGDocumentExtensions::SVGPendingElements> SVGDocumentExtensions::removePendingResource(const AtomicString& id)
    240271{
    241272    ASSERT(m_pendingResources.contains(id));

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

    r79569 r85413  
    4343    WTF_MAKE_NONCOPYABLE(SVGDocumentExtensions); WTF_MAKE_FAST_ALLOCATED;
    4444public:
    45     typedef HashSet<RefPtr<SVGStyledElement> > SVGPendingElements;
     45    typedef HashSet<SVGStyledElement*> SVGPendingElements;
    4646    SVGDocumentExtensions(Document*);
    4747    ~SVGDocumentExtensions();
    … …  
    8080    // which are referenced by any object in the SVG document, but do NOT exist yet.
    8181    // For instance, dynamically build gradients / patterns / clippers...
    82     void addPendingResource(const AtomicString& id, PassRefPtr<SVGStyledElement>);
    83     bool isPendingResource(const AtomicString& id) const;
     82    void addPendingResource(const AtomicString& id, SVGStyledElement*);
     83    bool hasPendingResources(const AtomicString& id) const;
     84    void removeElementFromPendingResources(SVGStyledElement*);
    8485    PassOwnPtr<SVGPendingElements> removePendingResource(const AtomicString& id);
    8586};

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

    r81932 r85413  
    339339}
    340340
    341 void SVGElement::insertedIntoDocument()
    342 {
    343     StyledElement::insertedIntoDocument();
    344 
    345     if (!needsPendingResourceHandling())
    346         return;
    347 
    348     SVGDocumentExtensions* extensions = document()->accessSVGExtensions();
    349     String resourceId = getIdAttribute();
    350     if (!extensions->isPendingResource(resourceId))
    351         return;
    352    
    353     OwnPtr<SVGDocumentExtensions::SVGPendingElements> clients(extensions->removePendingResource(resourceId));
    354     if (clients->isEmpty())
    355         return;
    356 
    357     const SVGDocumentExtensions::SVGPendingElements::const_iterator end = clients->end();
    358     for (SVGDocumentExtensions::SVGPendingElements::const_iterator it = clients->begin(); it != end; ++it)
    359         (*it)->buildPendingResource();
    360 }
    361 
    362341void SVGElement::attributeChanged(Attribute* attr, bool preserveDecls)
    363342{

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

    r79569 r85413  
    116116
    117117    virtual void finishParsingChildren();
    118     virtual void insertedIntoDocument();
    119118    virtual void attributeChanged(Attribute*, bool preserveDecls = false);
    120119    virtual bool childShouldCreateRenderer(Node*) const;
    … …  
    132131    virtual bool isSupported(StringImpl* feature, StringImpl* version) const;
    133132
    134     virtual bool needsPendingResourceHandling() const { return true; }
    135     virtual void buildPendingResource() { }
    136 
    137133    void mapInstanceToElement(SVGElementInstance*);
    138134    void removeInstanceMapping(SVGElementInstance*);

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

    r76248 r85413  
    3939        , m_cursorImageValue(0)
    4040        , m_instancesUpdatesBlocked(false)
     41        , m_hasPendingResources(false)
    4142    {
    4243    }
    … …  
    6162    void setInstanceUpdatesBlocked(bool value) { m_instancesUpdatesBlocked = value; }
    6263
     64    bool hasPendingResources() const { return m_hasPendingResources; }
     65    void setHasPendingResources(bool value) { m_hasPendingResources = value; }
     66
    6367    SVGCursorElement* cursorElement() const { return m_cursorElement; }
    6468    void setCursorElement(SVGCursorElement* cursorElement) { m_cursorElement = cursorElement; }
    … …  
    7276    CSSCursorImageValue* m_cursorImageValue;
    7377    bool m_instancesUpdatesBlocked : 1;
     78    bool m_hasPendingResources : 1;
    7479};
    7580

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

    r84225 r85413  
    6767SVGStyledElement::~SVGStyledElement()
    6868{
     69    if (needsPendingResourceHandling() && hasPendingResources() && document())
     70        document()->accessSVGExtensions()->removeElementFromPendingResources(this);
     71
     72    ASSERT(!hasPendingResources());
    6973}
    7074
    … …  
    360364    SVGElement::insertedIntoDocument();
    361365    updateRelativeLengthsInformation();
     366
     367    Document* document = this->document();
     368    if (!needsPendingResourceHandling() || !document)
     369        return;
     370
     371    SVGDocumentExtensions* extensions = document->accessSVGExtensions();
     372    String resourceId = getIdAttribute();
     373    if (!extensions->hasPendingResources(resourceId))
     374        return;
     375
     376    OwnPtr<SVGDocumentExtensions::SVGPendingElements> clients(extensions->removePendingResource(resourceId));
     377    ASSERT(!clients->isEmpty());
     378
     379    const SVGDocumentExtensions::SVGPendingElements::const_iterator end = clients->end();
     380    for (SVGDocumentExtensions::SVGPendingElements::const_iterator it = clients->begin(); it != end; ++it) {
     381        ASSERT((*it)->hasPendingResources());
     382        (*it)->buildPendingResource();
     383        (*it)->setHasPendingResources(false);
     384    }
    362385}
    363386
    … …  
    366389    updateRelativeLengthsInformation(false, this);
    367390    SVGElement::removedFromDocument();
     391
     392    Document* document = this->document();
     393    if (!needsPendingResourceHandling() || !document)
     394        return;
     395
     396    document->accessSVGExtensions()->removeElementFromPendingResources(this);
    368397}
    369398
    … …  
    420449}
    421450
     451bool SVGStyledElement::hasPendingResources() const
     452{
     453    return hasRareSVGData() && rareSVGData()->hasPendingResources();
     454}
     455
     456void SVGStyledElement::setHasPendingResources(bool value)
     457{
     458    ensureRareSVGData()->setHasPendingResources(value);
     459}
     460
    422461AffineTransform SVGStyledElement::localCoordinateSpaceTransform(SVGLocatable::CTMScope) const
    423462{

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

    r78543 r85413  
    5353    void setInstanceUpdatesBlocked(bool);
    5454
     55    bool hasPendingResources() const;
     56    void setHasPendingResources(bool);
     57
    5558    AnimatedAttributeType animatedPropertyTypeForCSSProperty(const QualifiedName&);
    5659    static bool isAnimatableCSSProperty(const QualifiedName&);
    … …  
    5962
    6063    virtual CSSStyleDeclaration* style() { return StyledElement::style(); }
     64    virtual bool needsPendingResourceHandling() const { return true; }
    6165
    6266protected:
    … …  
    8286
    8387    virtual bool selfHasRelativeLengths() const { return false; }
     88    virtual void buildPendingResource() { }
    8489
    8590private:

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

    r84899 r85413  
    7373    , m_height(LengthModeHeight)
    7474    , m_updatesBlocked(false)
    75     , m_isPendingResource(false)
    7675    , m_needsShadowTreeRecreation(false)
    7776{
    … …  
    140139    SVGStyledTransformableElement::insertedIntoDocument();
    141140    ASSERT(!m_targetElementInstance || !isWellFormedDocument(document()));
    142     ASSERT(!m_isPendingResource || !isWellFormedDocument(document()));
     141    ASSERT(!hasPendingResources() || !isWellFormedDocument(document()));
    143142}
    144143
    … …  
    167166
    168167    if (SVGURIReference::isKnownAttribute(attrName)) {
    169         if (m_isPendingResource) {
    170             document()->accessSVGExtensions()->removePendingResource(m_resourceId);
     168        if (hasPendingResources()) {
     169            OwnPtr<SVGDocumentExtensions::SVGPendingElements> clients(document()->accessSVGExtensions()->removePendingResource(m_resourceId));
     170            ASSERT(!clients->isEmpty());
     171
     172            const SVGDocumentExtensions::SVGPendingElements::const_iterator end = clients->end();
     173            for (SVGDocumentExtensions::SVGPendingElements::const_iterator it = clients->begin(); it != end; ++it) {
     174                ASSERT((*it)->hasPendingResources());
     175                (*it)->setHasPendingResources(false);
     176            }
     177
    171178            m_resourceId = String();
    172             m_isPendingResource = false;
     179            setHasPendingResources(false);
    173180        }
    174181
    … …  
    471478
    472479    if (!targetElement) {
    473         if (m_isPendingResource || id.isEmpty())
     480        if (hasPendingResources() || id.isEmpty())
    474481            return;
    475482
    476         m_isPendingResource = true;
    477483        m_resourceId = id;
     484        ASSERT(!hasPendingResources());
    478485        document()->accessSVGExtensions()->addPendingResource(id, this);
    479         return;
    480     }
    481 
    482     if (m_isPendingResource) {
     486        ASSERT(hasPendingResources());
     487        return;
     488    }
     489
     490
     491    if (hasPendingResources()) {
    483492        ASSERT(!m_targetElementInstance);
    484         m_isPendingResource = false;   
    485493        m_resourceId = String();
    486494        invalidateShadowTree();

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

    r81950 r85413  
    7878
    7979    friend class RenderSVGShadowTreeRootContainer;
    80     bool isPendingResource() const { return m_isPendingResource; }
    8180    void buildShadowAndInstanceTree(SVGShadowTreeRootElement*);
    8281    void detachInstance();
    … …  
    119118
    120119    bool m_updatesBlocked;
    121     bool m_isPendingResource;
    122120    bool m_needsShadowTreeRecreation;
    123121    String m_resourceId;
Note: See TracChangeset for help on using the changeset viewer.