Null dereference in SVGDocumentExtensions::removePendingResource when updating <use...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 21 Jan 2012 10:00:36 +0000 (10:00 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 21 Jan 2012 10:00:36 +0000 (10:00 +0000)
https://bugs.webkit.org/show_bug.cgi?id=69284

Patch by Florin Malita <fmalita@google.com> on 2012-01-21
Reviewed by Nikolas Zimmermann.

Source/WebCore:

Test: svg/custom/use-crash-pending-resource.svg

The crash is caused by assumptions in SVGUseElement that xlink:href is the only
pending resource. This patch adds support for dealing with multiple pending resources.

* rendering/svg/RenderSVGResourceContainer.cpp:
(WebCore::RenderSVGResourceContainer::registerResource):
* svg/SVGDocumentExtensions.cpp:
(WebCore::SVGDocumentExtensions::hasPendingResource):
(WebCore::SVGDocumentExtensions::isElementPendingResources):
(WebCore::SVGDocumentExtensions::isElementPendingResource):
(WebCore::SVGDocumentExtensions::removePendingResourceForElement):
* svg/SVGDocumentExtensions.h:
* svg/SVGStyledElement.cpp:
(WebCore::SVGStyledElement::buildPendingResourcesIfNeeded):
(WebCore::SVGStyledElement::clearHasPendingResourcesIfPossible):
Renamed SVGDocumentExtensions::hasPendingResources -> Renamed SVGDocumentExtensions::hasPendingResource.
Renamed SVGDocumentExtensions::isElementInPendingResources -> SVGDocumentExtensions::isElementPendingResources.
Added support for querying and removing pending resources for a specific element.

* svg/SVGUseElement.cpp:
(WebCore::SVGUseElement::svgAttributeChanged):
(WebCore::SVGUseElement::buildPendingResource):
Refactored to support multiple pending resources.

LayoutTests:

* svg/custom/use-crash-pending-resource-expected.txt: Added.
* svg/custom/use-crash-pending-resource.svg: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@105573 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/svg/custom/use-crash-pending-resource-expected.txt [new file with mode: 0644]
LayoutTests/svg/custom/use-crash-pending-resource.svg [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp
Source/WebCore/svg/SVGDocumentExtensions.cpp
Source/WebCore/svg/SVGDocumentExtensions.h
Source/WebCore/svg/SVGStyledElement.cpp
Source/WebCore/svg/SVGUseElement.cpp

index 7aab6abc180f09c1daf3ea7f3aaebd3578c8dc62..474b99db71762131a0a8b24b3420bac63c55cc81 100644 (file)
@@ -1,3 +1,13 @@
+2012-01-21  Florin Malita  <fmalita@google.com>
+
+        Null dereference in SVGDocumentExtensions::removePendingResource when updating <use>'s href
+        https://bugs.webkit.org/show_bug.cgi?id=69284
+
+        Reviewed by Nikolas Zimmermann.
+
+        * svg/custom/use-crash-pending-resource-expected.txt: Added.
+        * svg/custom/use-crash-pending-resource.svg: Added.
+
 2012-01-21  Stephen Chenney  <schenney@chromium.org>
 
         REGRESSION (Safari 5.0.5 - ToT): crash in SVG test http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObjectApproved/animate-elem-39-t.html
diff --git a/LayoutTests/svg/custom/use-crash-pending-resource-expected.txt b/LayoutTests/svg/custom/use-crash-pending-resource-expected.txt
new file mode 100644 (file)
index 0000000..69cfc5a
--- /dev/null
@@ -0,0 +1,2 @@
+PASS
+
diff --git a/LayoutTests/svg/custom/use-crash-pending-resource.svg b/LayoutTests/svg/custom/use-crash-pending-resource.svg
new file mode 100644 (file)
index 0000000..17fd2bc
--- /dev/null
@@ -0,0 +1,27 @@
+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+  <defs>
+    <text id="text1" font-size="20" fill="yellow">PASS</text>
+  </defs>
+
+  <!-- Don't crash when setting the href attribute while a filter resource is pending. -->
+  <use id="crasher" xlink:href="foo" filter="url(#nosuchfilter)"/>
+
+  <!-- Test both updating the href attribute and picking up the pending filter. -->
+  <use id="text_use" y="20" xlink:href="#foo" filter="url(#filter2)"/>
+
+  <filter id="filter2">
+    <!-- Green filter -->
+    <feColorMatrix type="matrix" values="0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0 1 0"/>
+  </filter>
+
+  <script>
+    <!-- Should not crash. -->
+    document.getElementById("crasher").setAttribute("xlink:href", "bar");
+
+    <!-- Should yield the filtered text -->
+    document.getElementById("text_use").setAttribute("xlink:href", "#text1");
+
+    if (window.layoutTestController)
+      layoutTestController.dumpAsText();
+  </script>
+</svg>
index 5868123091e243002e52714df07eeac4a9031900..6797860940664e16cb024f45d6b84ce02b0f0c57 100644 (file)
@@ -1,3 +1,35 @@
+2012-01-21  Florin Malita  <fmalita@google.com>
+
+        Null dereference in SVGDocumentExtensions::removePendingResource when updating <use>'s href
+        https://bugs.webkit.org/show_bug.cgi?id=69284
+
+        Reviewed by Nikolas Zimmermann.
+
+        Test: svg/custom/use-crash-pending-resource.svg
+
+        The crash is caused by assumptions in SVGUseElement that xlink:href is the only
+        pending resource. This patch adds support for dealing with multiple pending resources.
+
+        * rendering/svg/RenderSVGResourceContainer.cpp:
+        (WebCore::RenderSVGResourceContainer::registerResource):
+        * svg/SVGDocumentExtensions.cpp:
+        (WebCore::SVGDocumentExtensions::hasPendingResource):
+        (WebCore::SVGDocumentExtensions::isElementPendingResources):
+        (WebCore::SVGDocumentExtensions::isElementPendingResource):
+        (WebCore::SVGDocumentExtensions::removePendingResourceForElement):
+        * svg/SVGDocumentExtensions.h:
+        * svg/SVGStyledElement.cpp:
+        (WebCore::SVGStyledElement::buildPendingResourcesIfNeeded):
+        (WebCore::SVGStyledElement::clearHasPendingResourcesIfPossible):
+        Renamed SVGDocumentExtensions::hasPendingResources -> Renamed SVGDocumentExtensions::hasPendingResource.
+        Renamed SVGDocumentExtensions::isElementInPendingResources -> SVGDocumentExtensions::isElementPendingResources.
+        Added support for querying and removing pending resources for a specific element.
+
+        * svg/SVGUseElement.cpp:
+        (WebCore::SVGUseElement::svgAttributeChanged):
+        (WebCore::SVGUseElement::buildPendingResource):
+        Refactored to support multiple pending resources.
+
 2012-01-21  Stephen Chenney  <schenney@chromium.org>
 
         REGRESSION (Safari 5.0.5 - ToT): crash in SVG test http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObjectApproved/animate-elem-39-t.html
index 68e60ea78bd9fbb31a710539e781de89da54fbd0..5adb299500fe63135b5cbab459f9e99537d756c5 100644 (file)
@@ -147,7 +147,7 @@ void RenderSVGResourceContainer::removeClient(RenderObject* client)
 void RenderSVGResourceContainer::registerResource()
 {
     SVGDocumentExtensions* extensions = svgExtensionsFromNode(node());
-    if (!extensions->hasPendingResources(m_id)) {
+    if (!extensions->hasPendingResource(m_id)) {
         extensions->addResource(m_id, this);
         return;
     }
index d06feaab379922ed153a7376933f5cdc75218476..1dd61608caa540f3b7d76f14edadb910ebcd7935 100644 (file)
@@ -218,7 +218,7 @@ void SVGDocumentExtensions::addPendingResource(const AtomicString& id, SVGStyled
     element->setHasPendingResources();
 }
 
-bool SVGDocumentExtensions::hasPendingResources(const AtomicString& id) const
+bool SVGDocumentExtensions::hasPendingResource(const AtomicString& id) const
 {
     if (id.isEmpty())
         return false;
@@ -226,7 +226,7 @@ bool SVGDocumentExtensions::hasPendingResources(const AtomicString& id) const
     return m_pendingResources.contains(id);
 }
 
-bool SVGDocumentExtensions::isElementInPendingResources(SVGStyledElement* element) const
+bool SVGDocumentExtensions::isElementPendingResources(SVGStyledElement* element) const
 {
     // This algorithm takes time proportional to the number of pending resources and need not.
     // If performance becomes an issue we can keep a counted set of elements and answer the question efficiently.
@@ -244,6 +244,16 @@ bool SVGDocumentExtensions::isElementInPendingResources(SVGStyledElement* elemen
     return false;
 }
 
+bool SVGDocumentExtensions::isElementPendingResource(SVGStyledElement* element, const AtomicString& id) const
+{
+    ASSERT(element);
+
+    if (!hasPendingResource(id))
+        return false;
+
+    return m_pendingResources.get(id)->contains(element);
+}
+
 void SVGDocumentExtensions::removeElementFromPendingResources(SVGStyledElement* element)
 {
     ASSERT(element);
@@ -277,6 +287,19 @@ PassOwnPtr<SVGDocumentExtensions::SVGPendingElements> SVGDocumentExtensions::rem
     return adoptPtr(m_pendingResources.take(id));
 }
 
+void SVGDocumentExtensions::removePendingResourceForElement(const AtomicString& id, SVGStyledElement* element)
+{
+    ASSERT(element);
+    ASSERT(m_pendingResources.contains(id));
+
+    SVGPendingElements* elements = m_pendingResources.get(id);
+    elements->remove(element);
+    if (elements->isEmpty())
+        removePendingResource(id);
+
+    element->clearHasPendingResourcesIfPossible();
+}
+
 }
 
 #endif
index 4ab5f14e04b30c9734e44af70e45735c8263586e..459ff0647515feacc914a23cbe23aa7c3a021687 100644 (file)
@@ -80,10 +80,12 @@ public:
     // which are referenced by any object in the SVG document, but do NOT exist yet.
     // For instance, dynamically build gradients / patterns / clippers...
     void addPendingResource(const AtomicString& id, SVGStyledElement*);
-    bool hasPendingResources(const AtomicString& id) const;
-    bool isElementInPendingResources(SVGStyledElement*) const;
+    bool hasPendingResource(const AtomicString& id) const;
+    bool isElementPendingResources(SVGStyledElement*) const;
+    bool isElementPendingResource(SVGStyledElement*, const AtomicString& id) const;
     void removeElementFromPendingResources(SVGStyledElement*);
     PassOwnPtr<SVGPendingElements> removePendingResource(const AtomicString& id);
+    void removePendingResourceForElement(const AtomicString& id, SVGStyledElement*);
 };
 
 }
index 14803401c87a4220e6171099fd16c40870030a18..96899f4921180c2a472b8ace10673aa42141b1a0 100644 (file)
@@ -379,7 +379,7 @@ void SVGStyledElement::buildPendingResourcesIfNeeded()
 
     SVGDocumentExtensions* extensions = document->accessSVGExtensions();
     String resourceId = getIdAttribute();
-    if (!extensions->hasPendingResources(resourceId))
+    if (!extensions->hasPendingResource(resourceId))
         return;
 
     OwnPtr<SVGDocumentExtensions::SVGPendingElements> clients(extensions->removePendingResource(resourceId));
@@ -463,7 +463,7 @@ void SVGStyledElement::setHasPendingResources()
 
 void SVGStyledElement::clearHasPendingResourcesIfPossible()
 {
-    if (!document()->accessSVGExtensions()->isElementInPendingResources(this))
+    if (!document()->accessSVGExtensions()->isElementPendingResources(this))
         ensureRareSVGData()->setHasPendingResources(false);
 }
 
index 55f70f4531d2a84a0842c50f6ff687862dce442e..272e01d38f49bf97b49776d6e26ed3636e13a4ff 100644 (file)
@@ -198,18 +198,10 @@ void SVGUseElement::svgAttributeChanged(const QualifiedName& attrName)
         return;
 
     if (SVGURIReference::isKnownAttribute(attrName)) {
-        if (hasPendingResources()) {
-            OwnPtr<SVGDocumentExtensions::SVGPendingElements> clients(document()->accessSVGExtensions()->removePendingResource(m_resourceId));
-            ASSERT(!clients->isEmpty());
-
-            const SVGDocumentExtensions::SVGPendingElements::const_iterator end = clients->end();
-            for (SVGDocumentExtensions::SVGPendingElements::const_iterator it = clients->begin(); it != end; ++it) {
-                ASSERT((*it)->hasPendingResources());
-                (*it)->clearHasPendingResourcesIfPossible();
-            }
-
+        SVGDocumentExtensions* extensions = document()->accessSVGExtensions();
+        if (hasPendingResources() && extensions->isElementPendingResource(this, m_resourceId)) {
+            extensions->removePendingResourceForElement(m_resourceId, this);
             m_resourceId = String();
-            clearHasPendingResourcesIfPossible();
         }
 
         m_targetElementInstance = 0;
@@ -460,19 +452,18 @@ void SVGUseElement::buildPendingResource()
     String id;
     Element* targetElement = SVGURIReference::targetElementFromIRIString(href(), document(), &id);
     ASSERT(!m_targetElementInstance);
+    SVGDocumentExtensions* extensions = document()->accessSVGExtensions();
 
     if (!targetElement) {
-        if (hasPendingResources() || id.isEmpty())
+        if ((hasPendingResources() && extensions->isElementPendingResource(this, id)) || id.isEmpty())
             return;
 
         m_resourceId = id;
-        ASSERT(!hasPendingResources());
-        document()->accessSVGExtensions()->addPendingResource(id, this);
+        extensions->addPendingResource(id, this);
         ASSERT(hasPendingResources());
         return;
     }
 
-
     if (hasPendingResources()) {
         ASSERT(!m_targetElementInstance);
         m_resourceId = String();