Null deref when no use element exists for SVG element instance
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Nov 2011 15:23:40 +0000 (15:23 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Nov 2011 15:23:40 +0000 (15:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=59136

Source/WebCore:

Cleans up the object when it is detached from the shadow DOM tree,
rather than leaving it in a half-clean state with some nulled
references but not others.

Patch by Stephen Chenney <schenney@chromium.org> on 2011-11-10
Reviewed by Nikolas Zimmermann.

Test: svg/custom/element-instance-held-by-js-crash.svg

* svg/SVGElementInstance.cpp:
(WebCore::SVGElementInstance::~SVGElementInstance): Added a call to
detach to clean up if deletion happens without a prior detach call (as
when an entire tree is removed).
(WebCore::SVGElementInstance::detach): New method that replaces other
clean-up methods when the instance is removed from the shadow DOM.
* svg/SVGElementInstance.h: Added new detach method and removed
unnecessary clear methods.
* svg/SVGUseElement.cpp:
(WebCore::SVGUseElement::detachInstance): Changed calls to clear to
instead detach.

LayoutTests:

Patch by Stephen Chenney <schenney@chromium.org> on 2011-11-10
Reviewed by Nikolas Zimmermann.

* svg/custom/element-instance-held-by-js-crash-expected.txt: Added.
* svg/custom/element-instance-held-by-js-crash.svg: Added.

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

LayoutTests/ChangeLog
LayoutTests/svg/custom/element-instance-held-by-js-crash-expected.txt [new file with mode: 0644]
LayoutTests/svg/custom/element-instance-held-by-js-crash.svg [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/svg/SVGElementInstance.cpp
Source/WebCore/svg/SVGElementInstance.h
Source/WebCore/svg/SVGUseElement.cpp

index 121e2ea..1a00e70 100644 (file)
@@ -1,3 +1,13 @@
+2011-11-10  Stephen Chenney  <schenney@chromium.org>
+
+        Null deref when no use element exists for SVG element instance
+        https://bugs.webkit.org/show_bug.cgi?id=59136
+
+        Reviewed by Nikolas Zimmermann.
+
+        * svg/custom/element-instance-held-by-js-crash-expected.txt: Added.
+        * svg/custom/element-instance-held-by-js-crash.svg: Added.
+
 2011-11-10  Alexander Pavlov  <apavlov@chromium.org>
 
         Web Inspector: Show media queries associated with specific CSS rules
diff --git a/LayoutTests/svg/custom/element-instance-held-by-js-crash-expected.txt b/LayoutTests/svg/custom/element-instance-held-by-js-crash-expected.txt
new file mode 100644 (file)
index 0000000..bc5d7f7
--- /dev/null
@@ -0,0 +1,2 @@
+PASS - Null corresponding element dereference does not crash.
+
diff --git a/LayoutTests/svg/custom/element-instance-held-by-js-crash.svg b/LayoutTests/svg/custom/element-instance-held-by-js-crash.svg
new file mode 100644 (file)
index 0000000..ddf4690
--- /dev/null
@@ -0,0 +1,27 @@
+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+    <script>
+    <![CDATA[
+    window.onload = function() {
+        // Grab a reference to an SVGElementInstance native object. This reference will prevent the
+        // object from deletion when the shadow DOM is removed due to a style change.
+        instance = document.getElementById("use_elem").instanceRoot;
+
+        // Setting an attribute forces re-creation of the shadow DOM
+        document.getElementById("circleID").setAttribute("cx", 30);
+
+        // The animate element tries to modify the element, which tries to update the
+        // instances in the circle, which crashes if it holds a pointer to a non-existent element.
+
+        if (window.layoutTestController)
+            layoutTestController.dumpAsText();
+    }
+    //]]>
+    </script>
+    <circle transform="translate(1)" id="circleID" fill="green" cy="15" cx="15" r="10" >
+        <animate attributeName="cy" />
+    </circle>
+    <text id="resultText" y="20" x="50" >
+      PASS - Null corresponding element dereference does not crash.
+    </text>
+    <use id="use_elem" xlink:href="#circleID" />
+</svg>
index 3f48a51..2b9441e 100644 (file)
@@ -1,3 +1,28 @@
+2011-11-10  Stephen Chenney  <schenney@chromium.org>
+
+        Null deref when no use element exists for SVG element instance
+        https://bugs.webkit.org/show_bug.cgi?id=59136
+
+        Cleans up the object when it is detached from the shadow DOM tree,
+        rather than leaving it in a half-clean state with some nulled
+        references but not others.
+
+        Reviewed by Nikolas Zimmermann.
+
+        Test: svg/custom/element-instance-held-by-js-crash.svg
+
+        * svg/SVGElementInstance.cpp:
+        (WebCore::SVGElementInstance::~SVGElementInstance): Added a call to
+        detach to clean up if deletion happens without a prior detach call (as
+        when an entire tree is removed).
+        (WebCore::SVGElementInstance::detach): New method that replaces other
+        clean-up methods when the instance is removed from the shadow DOM.
+        * svg/SVGElementInstance.h: Added new detach method and removed
+        unnecessary clear methods.
+        * svg/SVGUseElement.cpp:
+        (WebCore::SVGUseElement::detachInstance): Changed calls to clear to
+        instead detach.
+
 2011-11-10  Alexander Pavlov  <apavlov@chromium.org>
 
         Web Inspector: Show media queries associated with specific CSS rules
index d081d9f..4ad8a43 100644 (file)
@@ -61,18 +61,30 @@ SVGElementInstance::SVGElementInstance(SVGUseElement* correspondingUseElement, S
 
 SVGElementInstance::~SVGElementInstance()
 {
+    // Call detach because we may be deleted directly if we are a child of a detached instance.
+    detach();
+
 #ifndef NDEBUG
     instanceCounter.decrement();
 #endif
 
-    // Deregister as instance for passed element.
-    m_element->removeInstanceMapping(this);
-
-    clearChildren();
 }
 
-void SVGElementInstance::clearChildren()
+void SVGElementInstance::detach()
 {
+    // Clear all pointers. When the node is detached from the shadow DOM it should be removed but,
+    // due to ref counting, it may not be. So clear everything to avoid dangling pointers.
+
+    // Deregister as instance for passed element.
+    if (m_element)
+        m_element->removeInstanceMapping(this);
+    m_element = 0;
+
+    m_shadowTreeElement = 0;
+
+    m_directUseElement = 0;
+    m_correspondingUseElement = 0;
+
     removeAllChildrenInContainer<SVGElementInstance, SVGElementInstance>(this);
 }
 
index 3951343..a9ae74b 100644 (file)
@@ -60,12 +60,8 @@ public:
     SVGUseElement* correspondingUseElement() const { return m_correspondingUseElement; }
     SVGUseElement* directUseElement() const { return m_directUseElement; }
     SVGElement* shadowTreeElement() const { return m_shadowTreeElement.get(); }
-    void clearChildren();
-    void clearUseElements()
-    {
-        m_directUseElement = 0;
-        m_correspondingUseElement = 0;
-    }
+
+    void detach();
 
     SVGElementInstance* parentNode() const { return parent(); }
     PassRefPtr<SVGElementInstanceList> childNodes();
index d8422e2..a349123 100644 (file)
@@ -624,8 +624,7 @@ void SVGUseElement::detachInstance()
 {
     if (!m_targetElementInstance)
         return;
-    m_targetElementInstance->clearUseElements();
-    m_targetElementInstance->clearChildren();
+    m_targetElementInstance->detach();
     m_targetElementInstance = 0;
 }