Bug 13611 - Crash in setAttributeNS setting href of SVG <use> to nonexistent symbol
Summary: Crash in setAttributeNS setting href of SVG <use> to nonexistent symbol
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-05-07 08:48 PDT by Gera Weiss
Modified: 2007-07-16 12:19 PDT (History)
0 users

See Also:


Attachments
Better test case (781 bytes, image/svg+xml)
2007-05-08 04:17 PDT, Eric Seidel (no email)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Gera Weiss 2007-05-07 08:48:20 PDT
<?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>
Comment 1 Alexey Proskuryakov 2007-05-07 11:46:14 PDT
ASSERTION FAILED: target
(/Users/ap/WebKit/WebCore/ksvg2/svg/SVGUseElement.cpp:242 virtual void WebCore::SVGUseElement::buildPendingResource())

Crashes release build.
Comment 2 Eric Seidel (no email) 2007-05-08 04:06:03 PDT
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();
Comment 3 Eric Seidel (no email) 2007-05-08 04:17:31 PDT
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.
Comment 4 Eric Seidel (no email) 2007-05-08 04:19:58 PDT
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.
Comment 5 Eric Seidel (no email) 2007-05-31 13:52:57 PDT
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();
Comment 6 Nikolas Zimmermann 2007-06-11 17:44:31 PDT
Investigate to find a real fix for the xlink:href problems, which don't involve detach/attach.
Comment 7 Nikolas Zimmermann 2007-07-16 06:34:35 PDT
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.
Comment 8 Nikolas Zimmermann 2007-07-16 12:19:41 PDT
Landed in r24320.