Crash when ImageLoader deletes Element inside SVGImageElement
authorschenney@chromium.org <schenney@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Mar 2013 22:53:57 +0000 (22:53 +0000)
committerschenney@chromium.org <schenney@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Mar 2013 22:53:57 +0000 (22:53 +0000)
https://bugs.webkit.org/show_bug.cgi?id=111085

Reviewed by Abhishek Arya.

Source/WebCore:

Elements with ImageLoader objects associated with them may have their
final reference held by the ImageLoader (to allow events to be sent
and handled). Any call on Element that causes the ImageLoader to
dispatch events might then result in the final deref of the Element
itself, thus leaving all the Element's "this" pointers up the stack
pointing to invalid memory.

This change puts the deref of the Element on a timer so that, if the
deref is called via a method on Element, the call stack will unwind
before the deref occurs.

Test: svg/custom/image-with-attr-change-after-delete-crash.html

* loader/ImageLoader.cpp:
(WebCore::ImageLoader::ImageLoader): Initialize a timer
(WebCore::ImageLoader::updatedHasPendingEvent): Put deref of the
  element on a oneShotTimer, with appropriate assertions and checks to
  ensure we only ref/deref once.
(WebCore::ImageLoader::timerFired): Deref the element when the timer fires.
* loader/ImageLoader.h:
(ImageLoader): Define a timer for controlling deref of the element.

LayoutTests:

* svg/custom/image-with-attr-change-after-delete-crash-expected.txt: Added.
* svg/custom/image-with-attr-change-after-delete-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/svg/custom/image-with-attr-change-after-delete-crash-expected.txt [new file with mode: 0644]
LayoutTests/svg/custom/image-with-attr-change-after-delete-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/ImageLoader.cpp
Source/WebCore/loader/ImageLoader.h

index c64ed81..98596eb 100644 (file)
@@ -1,3 +1,13 @@
+2013-03-05  Stephen Chenney  <schenney@chromium.org>
+
+        Crash when ImageLoader deletes Element inside SVGImageElement
+        https://bugs.webkit.org/show_bug.cgi?id=111085
+
+        Reviewed by Abhishek Arya.
+
+        * svg/custom/image-with-attr-change-after-delete-crash-expected.txt: Added.
+        * svg/custom/image-with-attr-change-after-delete-crash.html: Added.
+
 2013-03-05  Antoine Quint  <graouts@apple.com>
 
         Web Inspector: identify reflection layers in LayerTreeAgent
diff --git a/LayoutTests/svg/custom/image-with-attr-change-after-delete-crash-expected.txt b/LayoutTests/svg/custom/image-with-attr-change-after-delete-crash-expected.txt
new file mode 100644 (file)
index 0000000..1143faf
--- /dev/null
@@ -0,0 +1 @@
+Test Passes when there is no crash
diff --git a/LayoutTests/svg/custom/image-with-attr-change-after-delete-crash.html b/LayoutTests/svg/custom/image-with-attr-change-after-delete-crash.html
new file mode 100644 (file)
index 0000000..228b4ae
--- /dev/null
@@ -0,0 +1,38 @@
+<!DOCTYPE html>
+<html xmlns="http://www.w3.org/1999/xhtml">
+  <body id="body">
+    <svg id="svgRoot" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns="http://www.w3.org/2000/svg">
+      <image height="410" id="svgImage" width="503" x="0" y="0" xlink:href="resources/green-checker.png"/>
+      <script>
+        if (window.testRunner) {
+          testRunner.dumpAsText();
+          testRunner.waitUntilDone();
+        }
+
+        function forceGC() {
+          if (window.GCController)
+            GCController.collect();
+          else
+            gc();
+        }
+
+        svgImage = document.getElementById("svgImage");
+        docElement = document.getElementById("svgRoot");
+        newTextNode = document.createTextNode("\7f Test Passes");
+        setTimeout("crash()", 0);
+
+        function crash() {
+          svgImage.attributes[5].replaceChild(newTextNode, svgImage.attributes[5].childNodes[0]);
+          svgImage.parentNode.removeChild(svgImage);
+          delete svgImage;
+          forceGC();
+          docElement.appendChild(newTextNode);
+          if (window.testRunner) {
+            document.getElementById("body").innerHTML = "Test Passes when there is no crash";
+            testRunner.notifyDone();
+          }
+        }
+      </script>
+    </svg>
+  </body>
+</html>
index 2b7c4aa..f711cfa 100644 (file)
@@ -1,3 +1,32 @@
+2013-03-05  Stephen Chenney  <schenney@chromium.org>
+
+        Crash when ImageLoader deletes Element inside SVGImageElement
+        https://bugs.webkit.org/show_bug.cgi?id=111085
+
+        Reviewed by Abhishek Arya.
+
+        Elements with ImageLoader objects associated with them may have their
+        final reference held by the ImageLoader (to allow events to be sent
+        and handled). Any call on Element that causes the ImageLoader to
+        dispatch events might then result in the final deref of the Element
+        itself, thus leaving all the Element's "this" pointers up the stack
+        pointing to invalid memory.
+
+        This change puts the deref of the Element on a timer so that, if the
+        deref is called via a method on Element, the call stack will unwind
+        before the deref occurs.
+
+        Test: svg/custom/image-with-attr-change-after-delete-crash.html
+
+        * loader/ImageLoader.cpp:
+        (WebCore::ImageLoader::ImageLoader): Initialize a timer
+        (WebCore::ImageLoader::updatedHasPendingEvent): Put deref of the
+          element on a oneShotTimer, with appropriate assertions and checks to
+          ensure we only ref/deref once.
+        (WebCore::ImageLoader::timerFired): Deref the element when the timer fires.
+        * loader/ImageLoader.h:
+        (ImageLoader): Define a timer for controlling deref of the element.
+
 2013-03-05  Antoine Quint  <graouts@apple.com>
 
         Web Inspector: identify reflection layers in LayerTreeAgent
index 81a129d..186ce94 100644 (file)
@@ -92,6 +92,7 @@ static inline bool pageIsBeingDismissed(Document* document)
 ImageLoader::ImageLoader(Element* element)
     : m_element(element)
     , m_image(0)
+    , m_derefElementTimer(this, &ImageLoader::timerFired)
     , m_hasPendingBeforeLoadEvent(false)
     , m_hasPendingLoadEvent(false)
     , m_hasPendingErrorEvent(false)
@@ -366,10 +367,20 @@ void ImageLoader::updatedHasPendingEvent()
     if (wasProtected == m_elementIsProtected)
         return;
 
-    if (m_elementIsProtected)
-        m_element->ref();
-    else
-        m_element->deref();
+    if (m_elementIsProtected) {
+        if (m_derefElementTimer.isActive())
+            m_derefElementTimer.stop();
+        else
+            m_element->ref();
+    } else {
+        ASSERT(!m_derefElementTimer.isActive());
+        m_derefElementTimer.startOneShot(0);
+    }   
+}
+
+void ImageLoader::timerFired(Timer<ImageLoader>*)
+{
+    m_element->deref();
 }
 
 void ImageLoader::dispatchPendingEvent(ImageEventSender* eventSender)
index f59d4d5..3c5d1c4 100644 (file)
@@ -88,8 +88,11 @@ private:
     void setImageWithoutConsideringPendingLoadEvent(CachedImage*);
     void clearFailedLoadURL();
 
+    void timerFired(Timer<ImageLoader>*);
+
     Element* m_element;
     CachedResourceHandle<CachedImage> m_image;
+    Timer<ImageLoader> m_derefElementTimer;
     AtomicString m_failedLoadURL;
     bool m_hasPendingBeforeLoadEvent : 1;
     bool m_hasPendingLoadEvent : 1;