Crash in setAttributeNS setting href of SVG <use> to nonexistent symbol

• Component: SVG
• Status: RESOLVED FIXED

[Gera Weiss]

<?xml version="1.0"?>
<svg xmlns="http://www.w3.org/2000/svg"
 xmlns:xlink="http://www.w3.org/1999/xlink" onload="init()">
 <script><![CDATA[
 function init() {
 	alert("Wait")
  	var c= document.getElementById("tt")
  	c.setAttributeNS("http://www.w3.org/1999/xlink","href","#no")
  	alert("Done")
 }
]]>
</script>

<use id="tt" xlink:href="#no"/> 

</svg>

[Alexey Proskuryakov]

ASSERTION FAILED: target
(/Users/ap/WebKit/WebCore/ksvg2/svg/SVGUseElement.cpp:242 virtual void WebCore::SVGUseElement::buildPendingResource())

Crashes release build.

[Eric Seidel (no email)]

Ok, well this is a brute-force fix:

Index: ksvg2/svg/SVGUseElement.cpp
===================================================================
--- ksvg2/svg/SVGUseElement.cpp	(revision 21299)
+++ ksvg2/svg/SVGUseElement.cpp	(working copy)
@@ -142,10 +142,14 @@
     if (!attached())
        return;
 
+    // do a complete rebuild of the renderer if the target changed.
+    if (attr->name().matches(XLinkNames::hrefAttr)) {
+        detach();
+        attach();
+    }
     // Only update the tree if x/y/width/height or xlink:href changed.
-    if (attr->name() == SVGNames::xAttr || attr->name() == SVGNames::yAttr ||
-        attr->name() == SVGNames::widthAttr || attr->name() == SVGNames::heightAttr ||
-        attr->name().matches(XLinkNames::hrefAttr))
+    else if (attr->name() == SVGNames::xAttr || attr->name() == SVGNames::yAttr ||
+        attr->name() == SVGNames::widthAttr || attr->name() == SVGNames::heightAttr)
         buildPendingResource();
     else if (m_shadowTreeRootElement)
         m_shadowTreeRootElement->setChanged();

[Eric Seidel (no email)]

Created attachment 14409 [details]
Better test case

This test case tests both the crash, as well as testing to make sure that the id gets added to the pending list if appropriate.  (My above simple fix does not handle that case properly).

This example shows how fragile the current pending list system is... but that's a subject for another bug.

[Eric Seidel (no email)]

Actually, the pending list issue may need to be split out.  Even if "#green" is part of the initial source text, WebKit still doesn't notice when "wrongname" changes to "green" and properly update the document.

[Eric Seidel (no email)]

A slightly more complete brute-force fix:

Index: ksvg2/svg/SVGUseElement.cpp
===================================================================
--- ksvg2/svg/SVGUseElement.cpp (revision 21912)
+++ ksvg2/svg/SVGUseElement.cpp (working copy)
@@ -142,10 +142,15 @@
     if (!attached())
        return;
 
-    // Only update the tree if x/y/width/height or xlink:href changed.
-    if (attr->name() == SVGNames::xAttr || attr->name() == SVGNames::yAttr ||
-        attr->name() == SVGNames::widthAttr || attr->name() == SVGNames::heightAttr ||
-        attr->name().matches(XLinkNames::hrefAttr))
+    if (attr->name().matches(XLinkNames::hrefAttr)) {
+        // if the target changed recreate the entire rendering sub-tree
+        detach();
+        m_targetElementInstance = 0;
+        m_shadowTreeRootElement = 0;
+        attach();
+    } else if (attr->name() == SVGNames::xAttr || attr->name() == SVGNames::yAttr ||
+        attr->name() == SVGNames::widthAttr || attr->name() == SVGNames::heightAttr)
+        // otherwise only update the tree if x/y/width/height changed.
         buildPendingResource();
     else if (m_shadowTreeRootElement)
         m_shadowTreeRootElement->setChanged();

[Nikolas Zimmermann]

Investigate to find a real fix for the xlink:href problems, which don't involve detach/attach.

[Nikolas Zimmermann]

This doesn't crash anymore in feature-branch. Though it just silently ignores the request, instead of cleaning up the use element, to not show anything anymore. About to fix that.

[Nikolas Zimmermann]

Landed in r24320.