2010-07-05 Nikolas Zimmermann <nzimmermann@rim.com>
authorzimmermann@webkit.org <zimmermann@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 5 Jul 2010 12:27:35 +0000 (12:27 +0000)
committerzimmermann@webkit.org <zimmermann@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 5 Jul 2010 12:27:35 +0000 (12:27 +0000)
        Reviewed by Darin Adler.

        Memory corruption with SVG <use> element
        https://bugs.webkit.org/show_bug.cgi?id=40994

        Fix race condition in svgAttributeChanged. Never call svgAttributeChanged() from attributeChanged()
        when we're synchronizing SVG attributes. It leads to either unnecessary extra work being done or
        crashes. Especially together with <polyline>/<polygon> which always synchronize the SVGAnimatedPoints
        datastructure with the points attribute, no matter if there are changes are not. This should be
        furhter optimized, but this fix is sane and fixes the root of the evil races.

        Test: svg/custom/use-property-synchronization-crash.svg

        * svg/SVGElement.cpp:
        (WebCore::SVGElement::attributeChanged):

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

LayoutTests/ChangeLog
LayoutTests/platform/mac/svg/custom/use-property-synchronization-crash-expected.checksum [new file with mode: 0644]
LayoutTests/platform/mac/svg/custom/use-property-synchronization-crash-expected.png [new file with mode: 0644]
LayoutTests/platform/mac/svg/custom/use-property-synchronization-crash-expected.txt [new file with mode: 0644]
LayoutTests/svg/custom/use-property-synchronization-crash.svg [new file with mode: 0644]
WebCore/ChangeLog
WebCore/svg/SVGElement.cpp

index 165de3354b6cfa132e93291e041e1d146d6bbbd0..e663ba665983e166c6234a8ce567afce3d2b8353 100644 (file)
@@ -1,3 +1,15 @@
+2010-07-05  Nikolas Zimmermann  <nzimmermann@rim.com>
+
+        Reviewed by Darin Adler.
+
+        Memory corruption with SVG <use> element
+        https://bugs.webkit.org/show_bug.cgi?id=40994
+
+        * platform/mac/svg/custom/use-property-synchronization-crash-expected.checksum: Added.
+        * platform/mac/svg/custom/use-property-synchronization-crash-expected.png: Added.
+        * platform/mac/svg/custom/use-property-synchronization-crash-expected.txt: Added.
+        * svg/custom/use-property-synchronization-crash.svg: Added.
+
 2010-07-04  Rob Buis  <rwlbuis@gmail.com>
 
         Reviewed by Dirk Schulze.
diff --git a/LayoutTests/platform/mac/svg/custom/use-property-synchronization-crash-expected.checksum b/LayoutTests/platform/mac/svg/custom/use-property-synchronization-crash-expected.checksum
new file mode 100644 (file)
index 0000000..d69a371
--- /dev/null
@@ -0,0 +1 @@
+853de00567d121bea0b7bece66a5d61c
\ No newline at end of file
diff --git a/LayoutTests/platform/mac/svg/custom/use-property-synchronization-crash-expected.png b/LayoutTests/platform/mac/svg/custom/use-property-synchronization-crash-expected.png
new file mode 100644 (file)
index 0000000..12fde95
Binary files /dev/null and b/LayoutTests/platform/mac/svg/custom/use-property-synchronization-crash-expected.png differ
diff --git a/LayoutTests/platform/mac/svg/custom/use-property-synchronization-crash-expected.txt b/LayoutTests/platform/mac/svg/custom/use-property-synchronization-crash-expected.txt
new file mode 100644 (file)
index 0000000..8523806
--- /dev/null
@@ -0,0 +1,13 @@
+CONSOLE MESSAGE: line 5: Error: Problem parsing points="0"
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x600
+  RenderSVGRoot {svg} at (0,0) size 800x600
+    RenderSVGContainer {g} at (0,0) size 0x0
+      RenderSVGContainer {g} at (0,0) size 0x0
+        RenderPath {polyline} at (0,0) size 0x0 [fill={[type=SOLID] [color=#000000]}] [data=""]
+      RenderSVGContainer {use} at (0,0) size 0x0
+        RenderSVGContainer {g} at (0,0) size 0x0
+          RenderSVGContainer {g} at (0,0) size 0x0
+            RenderPath {polyline} at (0,0) size 0x0 [fill={[type=SOLID] [color=#000000]}] [data=""]
+    RenderSVGContainer {g} at (0,0) size 0x0
diff --git a/LayoutTests/svg/custom/use-property-synchronization-crash.svg b/LayoutTests/svg/custom/use-property-synchronization-crash.svg
new file mode 100644 (file)
index 0000000..6c3f4a2
--- /dev/null
@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1">
+  <g>
+    <g id="setOneRef">
+        <polyline points="0"/>
+    </g>
+    <use xlink:href="#setOneRef"/>
+  </g>
+  <g/>
+</svg>
index aa703f19aae3763f4037a184d2dc9b80195d430b..770fb4ab95188b60c6c7ee60b5a85b4cfe80c08c 100644 (file)
@@ -1,3 +1,21 @@
+2010-07-05  Nikolas Zimmermann  <nzimmermann@rim.com>
+
+        Reviewed by Darin Adler.
+
+        Memory corruption with SVG <use> element
+        https://bugs.webkit.org/show_bug.cgi?id=40994
+
+        Fix race condition in svgAttributeChanged. Never call svgAttributeChanged() from attributeChanged()
+        when we're synchronizing SVG attributes. It leads to either unnecessary extra work being done or
+        crashes. Especially together with <polyline>/<polygon> which always synchronize the SVGAnimatedPoints
+        datastructure with the points attribute, no matter if there are changes are not. This should be
+        furhter optimized, but this fix is sane and fixes the root of the evil races.
+
+        Test: svg/custom/use-property-synchronization-crash.svg
+
+        * svg/SVGElement.cpp:
+        (WebCore::SVGElement::attributeChanged):
+
 2010-07-05  Yury Semikhatsky  <yurys@chromium.org>
 
         Reviewed by Pavel Feldman.
index 293e4d6cab7b0ea986217e875cf2be0cb39249f3..b262e45331e91d9bbb39bb918d0c02dbe99bebeb 100644 (file)
@@ -301,6 +301,15 @@ void SVGElement::attributeChanged(Attribute* attr, bool preserveDecls)
         return;
 
     StyledElement::attributeChanged(attr, preserveDecls);
+
+    // When an animated SVG property changes through SVG DOM, svgAttributeChanged() is called, not attributeChanged().
+    // Next time someone tries to access the XML attributes, the synchronization code starts. During that synchronization
+    // SVGAnimatedPropertySynchronizer may call NamedNodeMap::removeAttribute(), which in turn calls attributeChanged().
+    // At this point we're not allowed to call svgAttributeChanged() again - it may lead to extra work being done, or crashes
+    // see bug https://bugs.webkit.org/show_bug.cgi?id=40994.
+    if (isSynchronizingSVGAttributes())
+        return;
+
     svgAttributeChanged(attr->name());
 }