use after free in WebCore::DocumentOrderedMap::remove / WebCore::TreeScope::removeEle...
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Nov 2013 06:01:45 +0000 (06:01 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Nov 2013 06:01:45 +0000 (06:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=121324

Reviewed by Ryosuke Niwa.

Update the document ordered map for an image element before dispatching load or error events
when it's inserted into a document.

Source/WebCore:

Test: fast/dom/modify-node-and-while-in-the-callback-too-crash.html

* dom/DocumentOrderedMap.cpp: defensive fix to avoid use after free issues.
(WebCore::DocumentOrderedMap::remove):
* html/HTMLImageElement.cpp:
(WebCore::HTMLImageElement::insertedInto):
* loader/ImageLoader.cpp:
(WebCore::ImageLoader::updateFromElement): setting m_failedLoadURL makes
repeated updateFromElement calls return early.

LayoutTests:

* fast/dom/modify-node-and-while-in-the-callback-too-crash-expected.txt: Added.
* fast/dom/modify-node-and-while-in-the-callback-too-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/dom/modify-node-and-while-in-the-callback-too-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/modify-node-and-while-in-the-callback-too-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/DocumentOrderedMap.cpp
Source/WebCore/html/HTMLImageElement.cpp
Source/WebCore/loader/ImageLoader.cpp

index 5ed837c..e03be10 100644 (file)
@@ -1,3 +1,16 @@
+2013-11-18  Zalan Bujtas  <zalan@apple.com>
+
+        use after free in WebCore::DocumentOrderedMap::remove / WebCore::TreeScope::removeElementById
+        https://bugs.webkit.org/show_bug.cgi?id=121324
+
+        Reviewed by Ryosuke Niwa.
+
+        Update the document ordered map for an image element before dispatching load or error events
+        when it's inserted into a document.
+
+        * fast/dom/modify-node-and-while-in-the-callback-too-crash-expected.txt: Added.
+        * fast/dom/modify-node-and-while-in-the-callback-too-crash.html: Added.
+
 2013-11-18  Sun-woo Nam  <sunny.nam@samsung.com>
 
         [EFL] Layout tests need to be rebaselined.
diff --git a/LayoutTests/fast/dom/modify-node-and-while-in-the-callback-too-crash-expected.txt b/LayoutTests/fast/dom/modify-node-and-while-in-the-callback-too-crash-expected.txt
new file mode 100644 (file)
index 0000000..9ee018f
--- /dev/null
@@ -0,0 +1,3 @@
+This tests that making changes on a node that triggers a callback where we make changes again on the same node does not result in an assert/crash. Test passes if no crash is observed.
+
+
diff --git a/LayoutTests/fast/dom/modify-node-and-while-in-the-callback-too-crash.html b/LayoutTests/fast/dom/modify-node-and-while-in-the-callback-too-crash.html
new file mode 100644 (file)
index 0000000..249c681
--- /dev/null
@@ -0,0 +1,24 @@
+<!DOCTYPE html>\r
+<html>\r
+<body>\r
+    <p>This tests that making changes on a node that triggers a callback where we make changes \r
+    again on the same node does not result in an assert/crash.\r
+    Test passes if no crash is observed.</p>  \r
+    <img id="error-image" src="">\r
+    <div id="container"></div>\r
+\r
+    <script> \r
+        if (window.testRunner)\r
+            testRunner.dumpAsText();\r
+        \r
+        var img = document.getElementById('error-image');\r
+        var container = document.getElementById('container');\r
+\r
+        img.onerror = function() { \r
+            container.parentNode.removeChild(container);\r
+        }\r
+        \r
+        container.appendChild(img);\r
+    </script>\r
+</body>\r
+</html>\r
index 3bf099e..aa9899f 100644 (file)
@@ -1,3 +1,23 @@
+2013-11-18  Zalan Bujtas  <zalan@apple.com>
+
+        use after free in WebCore::DocumentOrderedMap::remove / WebCore::TreeScope::removeElementById
+        https://bugs.webkit.org/show_bug.cgi?id=121324
+
+        Reviewed by Ryosuke Niwa.
+
+        Update the document ordered map for an image element before dispatching load or error events
+        when it's inserted into a document.
+
+        Test: fast/dom/modify-node-and-while-in-the-callback-too-crash.html
+
+        * dom/DocumentOrderedMap.cpp: defensive fix to avoid use after free issues.
+        (WebCore::DocumentOrderedMap::remove):
+        * html/HTMLImageElement.cpp:
+        (WebCore::HTMLImageElement::insertedInto):
+        * loader/ImageLoader.cpp:
+        (WebCore::ImageLoader::updateFromElement): setting m_failedLoadURL makes
+        repeated updateFromElement calls return early.
+
 2013-11-18  Benjamin Poulain  <bpoulain@apple.com>
 
         Upstream iOS's ResourceHandle implementation
index c3325c0..2fb0f25 100644 (file)
@@ -105,6 +105,9 @@ void DocumentOrderedMap::remove(const AtomicStringImpl& key, Element& element)
     m_map.checkConsistency();
     auto it = m_map.find(&key);
     ASSERT(it != m_map.end());
+    if (it == m_map.end())
+        return;
+
     MapEntry& entry = it->value;
 
     ASSERT(entry.count);
index e49d8d9..e231a2e 100644 (file)
@@ -221,12 +221,16 @@ Node::InsertionNotificationRequest HTMLImageElement::insertedInto(ContainerNode&
     if (insertionPoint.inDocument() && !m_lowercasedUsemap.isNull())
         document().addImageElementByLowercasedUsemap(*m_lowercasedUsemap.impl(), *this);
 
+    // Insert needs to complete first, before we start updating the loader. Loader dispatches events which could result
+    // in callbacks back to this node.
+    Node::InsertionNotificationRequest insertNotificationRequest = HTMLElement::insertedInto(insertionPoint);
+
     // If we have been inserted from a renderer-less document,
     // our loader may have not fetched the image, so do it now.
     if (insertionPoint.inDocument() && !m_imageLoader.image())
         m_imageLoader.updateFromElement();
 
-    return HTMLElement::insertedInto(insertionPoint);
+    return insertNotificationRequest;
 }
 
 void HTMLImageElement::removedFrom(ContainerNode& insertionPoint)
index c54ca25..011bcda 100644 (file)
@@ -212,8 +212,9 @@ void ImageLoader::updateFromElement()
             clearFailedLoadURL();
     } else if (!attr.isNull()) {
         // Fire an error event if the url is empty.
-        // FIXME: Should we fire this event asynchronoulsy via errorEventSender()?
-        m_element->dispatchEvent(Event::create(eventNames().errorEvent, false, false));
+        m_failedLoadURL = attr;
+        m_hasPendingErrorEvent = true;
+        errorEventSender().dispatchEventSoon(this);
     }
     
     CachedImage* oldImage = m_image.get();