Stale SVGUseElement reference in CachedResource::checkNotify()
authorfmalita@chromium.org <fmalita@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Dec 2012 19:35:30 +0000 (19:35 +0000)
committerfmalita@chromium.org <fmalita@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Dec 2012 19:35:30 +0000 (19:35 +0000)
https://bugs.webkit.org/show_bug.cgi?id=104004

Reviewed by Eric Seidel.

Source/WebCore:

SVGUseElement tracks one CachedSVGDocument at a time (for external references), but when
the href attribute is updated it fails to unregister with the current CachedSVGDocument
and only updates its CachedSVGDocument with the new instance. This leaves an untracked
reference with the original CachedSVGDocument.

The patch adds the missing removeClient() call on href change, and encapsulates the
CachedSVGDocument manipulation in a helper method which handles the necessary cleanup.

Test: svg/custom/use-href-update-crash.svg

* svg/SVGUseElement.cpp:
(WebCore::SVGUseElement::~SVGUseElement):
(WebCore::SVGUseElement::svgAttributeChanged):
(WebCore::SVGUseElement::setCachedDocument):
(WebCore):
* svg/SVGUseElement.h:
(SVGUseElement):

LayoutTests:

* svg/custom/use-href-update-crash-expected.txt: Added.
* svg/custom/use-href-update-crash.svg: Added.

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

LayoutTests/ChangeLog
LayoutTests/svg/custom/use-href-update-crash-expected.txt [new file with mode: 0644]
LayoutTests/svg/custom/use-href-update-crash.svg [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/svg/SVGUseElement.cpp
Source/WebCore/svg/SVGUseElement.h

index 534398e..32e28b2 100644 (file)
@@ -1,3 +1,13 @@
+2012-12-04  Florin Malita  <fmalita@chromium.org>
+
+        Stale SVGUseElement reference in CachedResource::checkNotify()
+        https://bugs.webkit.org/show_bug.cgi?id=104004
+
+        Reviewed by Eric Seidel.
+
+        * svg/custom/use-href-update-crash-expected.txt: Added.
+        * svg/custom/use-href-update-crash.svg: Added.
+
 2012-12-04  Antoine Quint  <graouts@apple.com>
 
         Missing -expected.txt files for new <track> tests
diff --git a/LayoutTests/svg/custom/use-href-update-crash-expected.txt b/LayoutTests/svg/custom/use-href-update-crash-expected.txt
new file mode 100644 (file)
index 0000000..9b9c972
--- /dev/null
@@ -0,0 +1 @@
+PASS: did not crash.
diff --git a/LayoutTests/svg/custom/use-href-update-crash.svg b/LayoutTests/svg/custom/use-href-update-crash.svg
new file mode 100644 (file)
index 0000000..3d07b16
--- /dev/null
@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<svg xmlns:xlink="http://www.w3.org/1999/xlink" xmlns="http://www.w3.org/2000/svg">
+  <!-- Test for https://bugs.webkit.org/show_bug.cgi?id=104004 -->
+  <use id="use" xlink:href="foo.svg#foo"/>
+  <text>PASS: did not crash.</text>
+  <script>
+    var use = document.getElementById('use');
+    use.setAttribute('xlink:href', 'bar.svg#bar');
+    use.parentNode.removeChild(use);
+    use = null;
+    gc();
+
+    if (window.testRunner)
+      testRunner.dumpAsText();
+  </script>
+</svg>
index 953920d..a84b5db 100644 (file)
@@ -1,3 +1,28 @@
+2012-12-04  Florin Malita  <fmalita@chromium.org>
+
+        Stale SVGUseElement reference in CachedResource::checkNotify()
+        https://bugs.webkit.org/show_bug.cgi?id=104004
+
+        Reviewed by Eric Seidel.
+
+        SVGUseElement tracks one CachedSVGDocument at a time (for external references), but when
+        the href attribute is updated it fails to unregister with the current CachedSVGDocument
+        and only updates its CachedSVGDocument with the new instance. This leaves an untracked
+        reference with the original CachedSVGDocument.
+
+        The patch adds the missing removeClient() call on href change, and encapsulates the
+        CachedSVGDocument manipulation in a helper method which handles the necessary cleanup.
+
+        Test: svg/custom/use-href-update-crash.svg
+
+        * svg/SVGUseElement.cpp:
+        (WebCore::SVGUseElement::~SVGUseElement):
+        (WebCore::SVGUseElement::svgAttributeChanged):
+        (WebCore::SVGUseElement::setCachedDocument):
+        (WebCore):
+        * svg/SVGUseElement.h:
+        (SVGUseElement):
+
 2012-12-04  Yury Semikhatsky  <yurys@chromium.org>
 
         Web Inspector: Can't take a heap snapshot in chromium ("Uncaught ReferenceError")
index dd467d5..f300f14 100644 (file)
@@ -107,8 +107,7 @@ PassRefPtr<SVGUseElement> SVGUseElement::create(const QualifiedName& tagName, Do
 
 SVGUseElement::~SVGUseElement()
 {
-    if (m_cachedDocument)
-        m_cachedDocument->removeClient(this);
+    setCachedDocument(0);
 
     clearResourceReferences();
 }
@@ -257,18 +256,14 @@ void SVGUseElement::svgAttributeChanged(const QualifiedName& attrName)
             if (url.hasFragmentIdentifier()) {
                 CachedResourceRequest request(ResourceRequest(url.string()));
                 request.setInitiator(this);
-                m_cachedDocument = document()->cachedResourceLoader()->requestSVGDocument(request);
-                if (m_cachedDocument)
-                    m_cachedDocument->addClient(this);
+                setCachedDocument(document()->cachedResourceLoader()->requestSVGDocument(request));
             }
-        }
+        } else
+            setCachedDocument(0);
 
-        if (m_cachedDocument && !isExternalReference) {
-            m_cachedDocument->removeClient(this);
-            m_cachedDocument = 0;
-        }
         if (!m_wasInsertedByParser)
             buildPendingResource();
+
         return;
     }
 
@@ -992,6 +987,19 @@ void SVGUseElement::finishParsingChildren()
     }
 }
 
+void SVGUseElement::setCachedDocument(CachedResourceHandle<CachedSVGDocument> cachedDocument)
+{
+    if (m_cachedDocument == cachedDocument)
+        return;
+
+    if (m_cachedDocument)
+        m_cachedDocument->removeClient(this);
+
+    m_cachedDocument = cachedDocument;
+    if (m_cachedDocument)
+        m_cachedDocument->addClient(this);
+}
+
 }
 
 #endif // ENABLE(SVG)
index f6ae162..6bb29ec 100644 (file)
@@ -114,6 +114,7 @@ private:
     bool instanceTreeIsLoading(SVGElementInstance*);
     virtual void notifyFinished(CachedResource*);
     Document* referencedDocument() const;
+    void setCachedDocument(CachedResourceHandle<CachedSVGDocument>);
 
     // SVGTests
     virtual void synchronizeRequiredFeatures() { SVGTests::synchronizeRequiredFeatures(this); }