Avoid race condition when iterating over pending resources
authorschenney@chromium.org <schenney@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 25 Mar 2012 17:41:18 +0000 (17:41 +0000)
committerschenney@chromium.org <schenney@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 25 Mar 2012 17:41:18 +0000 (17:41 +0000)
https://bugs.webkit.org/show_bug.cgi?id=82115

Patch by Philip Rogers <pdr@google.com> on 2012-03-25
Reviewed by Nikolas Zimmermann.

Source/WebCore:

We can hit a race condition in SVGStyledElement::buildPendingResourcesIfNeeded
where pending elements can become non-pending while we iterate over them.

This patch cleans up buildPendingResourcesIfNeeded and re-works how pending
resources are removed. Because pending resources can be modified while
iterating over them, we introduce m_pendingResourcesForRemoval that
holds pending resources that are marked for removal. Instead of iterating
over this list we simply remove each pending resource from
m_pendingResourcesForRemoval; if a pending resource is modified or removed
during the processing of another pending resource this list is updated before
the next element can be accessed.

This change also removes removePendingResourceForElement which is no longer
referenced.

Test: http/tests/svg/change-id-with-pending-resources.html

* svg/SVGDocumentExtensions.cpp:
(WebCore::SVGDocumentExtensions::~SVGDocumentExtensions):
(WebCore::SVGDocumentExtensions::removeElementFromPendingResources):
(WebCore::SVGDocumentExtensions::removePendingResourceForRemoval):
(WebCore):
(WebCore::SVGDocumentExtensions::markPendingResourcesForRemoval):
(WebCore::SVGDocumentExtensions::removeElementFromPendingResourcesForRemoval):
* svg/SVGDocumentExtensions.h:
(SVGDocumentExtensions):
* svg/SVGStyledElement.cpp:
(WebCore::SVGStyledElement::buildPendingResourcesIfNeeded):

LayoutTests:

* http/tests/svg/change-id-with-pending-resources-expected.txt: Added.
* http/tests/svg/change-id-with-pending-resources.html: Added.
* http/tests/svg/resources/svg-use-defs-rect.svg: Added.

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

LayoutTests/ChangeLog
LayoutTests/http/tests/svg/change-id-with-pending-resources-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/svg/change-id-with-pending-resources.html [new file with mode: 0644]
LayoutTests/http/tests/svg/resources/svg-use-defs-rect.svg [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/svg/SVGDocumentExtensions.cpp
Source/WebCore/svg/SVGDocumentExtensions.h
Source/WebCore/svg/SVGStyledElement.cpp

index 87f866112f73247bf2110961442785cc24ce2df4..56eab3f023a9df4f03fc34376507d4989d88d2e0 100644 (file)
@@ -1,3 +1,14 @@
+2012-03-25  Philip Rogers  <pdr@google.com>
+
+        Avoid race condition when iterating over pending resources
+        https://bugs.webkit.org/show_bug.cgi?id=82115
+
+        Reviewed by Nikolas Zimmermann.
+
+        * http/tests/svg/change-id-with-pending-resources-expected.txt: Added.
+        * http/tests/svg/change-id-with-pending-resources.html: Added.
+        * http/tests/svg/resources/svg-use-defs-rect.svg: Added.
+
 2012-03-25  Nikolas Zimmermann  <nzimmermann@rim.com>
 
         Not reviewed. Rebaseline foreignObject results, after Florins changes.
diff --git a/LayoutTests/http/tests/svg/change-id-with-pending-resources-expected.txt b/LayoutTests/http/tests/svg/change-id-with-pending-resources-expected.txt
new file mode 100644 (file)
index 0000000..4fe92ec
--- /dev/null
@@ -0,0 +1 @@
+If this text is visible and the test did not crash, this test passes
diff --git a/LayoutTests/http/tests/svg/change-id-with-pending-resources.html b/LayoutTests/http/tests/svg/change-id-with-pending-resources.html
new file mode 100644 (file)
index 0000000..1c0e5a1
--- /dev/null
@@ -0,0 +1,19 @@
+<script>
+// Test passes if it does not crash.
+// Note: this test is located under Layouttests/http in order to load an external
+//       document and modify it without hitting security restrictions.
+    if (window.layoutTestController) {
+        layoutTestController.waitUntilDone();
+        layoutTestController.dumpAsText();
+    }
+
+    function crash() {
+        q = document.getElementById('root').contentDocument;
+        document.write("");
+        q.getElementById('d').id = "maskedGradient";
+        document.write("If this text is visible and the test did not crash, this test passes");
+        if (window.layoutTestController)
+            layoutTestController.notifyDone();
+    }
+</script>
+<object data="http://127.0.0.1:8000/svg/resources/svg-use-defs-rect.svg" id="root" onload="crash()"/>
diff --git a/LayoutTests/http/tests/svg/resources/svg-use-defs-rect.svg b/LayoutTests/http/tests/svg/resources/svg-use-defs-rect.svg
new file mode 100644 (file)
index 0000000..b4275f0
--- /dev/null
@@ -0,0 +1,8 @@
+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+    <defs id="d">
+        <linearGradient id="maskedGradient"></linearGradient>
+        <rect id="masked" fill="url(#maskedGradient)"/>
+        <mask id="mask"></mask>
+    </defs>
+    <use xlink:href="#masked" mask="url(#mask)"/>
+</svg>
index e50c6237ea49406fa4586738ab75bb05c7a22b46..bf2bef2d941da65933f86205d38dae06e37edece 100644 (file)
@@ -1,3 +1,39 @@
+2012-03-25  Philip Rogers  <pdr@google.com>
+
+        Avoid race condition when iterating over pending resources
+        https://bugs.webkit.org/show_bug.cgi?id=82115
+
+        Reviewed by Nikolas Zimmermann.
+
+        We can hit a race condition in SVGStyledElement::buildPendingResourcesIfNeeded
+        where pending elements can become non-pending while we iterate over them.
+
+        This patch cleans up buildPendingResourcesIfNeeded and re-works how pending
+        resources are removed. Because pending resources can be modified while
+        iterating over them, we introduce m_pendingResourcesForRemoval that
+        holds pending resources that are marked for removal. Instead of iterating
+        over this list we simply remove each pending resource from
+        m_pendingResourcesForRemoval; if a pending resource is modified or removed
+        during the processing of another pending resource this list is updated before
+        the next element can be accessed.
+
+        This change also removes removePendingResourceForElement which is no longer
+        referenced.
+
+        Test: http/tests/svg/change-id-with-pending-resources.html
+
+        * svg/SVGDocumentExtensions.cpp:
+        (WebCore::SVGDocumentExtensions::~SVGDocumentExtensions):
+        (WebCore::SVGDocumentExtensions::removeElementFromPendingResources):
+        (WebCore::SVGDocumentExtensions::removePendingResourceForRemoval):
+        (WebCore):
+        (WebCore::SVGDocumentExtensions::markPendingResourcesForRemoval):
+        (WebCore::SVGDocumentExtensions::removeElementFromPendingResourcesForRemoval):
+        * svg/SVGDocumentExtensions.h:
+        (SVGDocumentExtensions):
+        * svg/SVGStyledElement.cpp:
+        (WebCore::SVGStyledElement::buildPendingResourcesIfNeeded):
+
 2012-03-25  Arvid Nilsson  <anilsson@rim.com>
 
         [BlackBerry] Accelerated compositing layers fail to render when using WebPageCompositor
index 888bd580081e73f04b2251c968feb54e9507be8a..35ea6402ef07e1825b63d7e7c14f7d118b414590 100644 (file)
@@ -52,6 +52,7 @@ SVGDocumentExtensions::~SVGDocumentExtensions()
 {
     deleteAllValues(m_animatedElements);
     deleteAllValues(m_pendingResources);
+    deleteAllValues(m_pendingResourcesForRemoval);
 }
 
 void SVGDocumentExtensions::addTimeContainer(SVGSVGElement* element)
@@ -262,27 +263,47 @@ void SVGDocumentExtensions::removeElementFromPendingResources(SVGStyledElement*
 {
     ASSERT(element);
 
-    if (m_pendingResources.isEmpty() || !element->hasPendingResources())
-        return;
-
-    Vector<AtomicString> toBeRemoved;
-    HashMap<AtomicString, SVGPendingElements*>::iterator end = m_pendingResources.end();
-    for (HashMap<AtomicString, SVGPendingElements*>::iterator it = m_pendingResources.begin(); it != end; ++it) {
-        SVGPendingElements* elements = it->second;
-        ASSERT(elements);
-        ASSERT(!elements->isEmpty());
-
-        elements->remove(element);
-        if (elements->isEmpty())
-            toBeRemoved.append(it->first);
+    // Remove the element from pending resources.
+    if (!m_pendingResources.isEmpty() && element->hasPendingResources()) {
+        Vector<AtomicString> toBeRemoved;
+        HashMap<AtomicString, SVGPendingElements*>::iterator end = m_pendingResources.end();
+        for (HashMap<AtomicString, SVGPendingElements*>::iterator it = m_pendingResources.begin(); it != end; ++it) {
+            SVGPendingElements* elements = it->second;
+            ASSERT(elements);
+            ASSERT(!elements->isEmpty());
+
+            elements->remove(element);
+            if (elements->isEmpty())
+                toBeRemoved.append(it->first);
+        }
+
+        element->clearHasPendingResourcesIfPossible();
+
+        // We use the removePendingResource function here because it deals with set lifetime correctly.
+        Vector<AtomicString>::iterator vectorEnd = toBeRemoved.end();
+        for (Vector<AtomicString>::iterator it = toBeRemoved.begin(); it != vectorEnd; ++it)
+            removePendingResource(*it);
     }
 
-    element->clearHasPendingResourcesIfPossible();
-
-    // We use the removePendingResource function here because it deals with set lifetime correctly.
-    Vector<AtomicString>::iterator vectorEnd = toBeRemoved.end();
-    for (Vector<AtomicString>::iterator it = toBeRemoved.begin(); it != vectorEnd; ++it)
-        removePendingResource(*it);
+    // Remove the element from pending resources that were scheduled for removal.
+    if (!m_pendingResourcesForRemoval.isEmpty()) {
+        Vector<AtomicString> toBeRemoved;
+        HashMap<AtomicString, SVGPendingElements*>::iterator end = m_pendingResourcesForRemoval.end();
+        for (HashMap<AtomicString, SVGPendingElements*>::iterator it = m_pendingResourcesForRemoval.begin(); it != end; ++it) {
+            SVGPendingElements* elements = it->second;
+            ASSERT(elements);
+            ASSERT(!elements->isEmpty());
+
+            elements->remove(element);
+            if (elements->isEmpty())
+                toBeRemoved.append(it->first);
+        }
+
+        // We use the removePendingResourceForRemoval function here because it deals with set lifetime correctly.
+        Vector<AtomicString>::iterator vectorEnd = toBeRemoved.end();
+        for (Vector<AtomicString>::iterator it = toBeRemoved.begin(); it != vectorEnd; ++it)
+            removePendingResourceForRemoval(*it);
+    }
 }
 
 PassOwnPtr<SVGDocumentExtensions::SVGPendingElements> SVGDocumentExtensions::removePendingResource(const AtomicString& id)
@@ -291,17 +312,42 @@ PassOwnPtr<SVGDocumentExtensions::SVGPendingElements> SVGDocumentExtensions::rem
     return adoptPtr(m_pendingResources.take(id));
 }
 
-void SVGDocumentExtensions::removePendingResourceForElement(const AtomicString& id, SVGStyledElement* element)
+PassOwnPtr<SVGDocumentExtensions::SVGPendingElements> SVGDocumentExtensions::removePendingResourceForRemoval(const AtomicString& id)
 {
-    ASSERT(element);
-    ASSERT(m_pendingResources.contains(id));
+    ASSERT(m_pendingResourcesForRemoval.contains(id));
+    return adoptPtr(m_pendingResourcesForRemoval.take(id));
+}
+
+void SVGDocumentExtensions::markPendingResourcesForRemoval(const AtomicString& id)
+{
+    if (id.isEmpty())
+        return;
+
+    ASSERT(!m_pendingResourcesForRemoval.contains(id));
+
+    SVGPendingElements* existing = m_pendingResources.take(id);
+    if (existing && !existing->isEmpty())
+        m_pendingResourcesForRemoval.add(id, existing);
+}
+
+SVGStyledElement* SVGDocumentExtensions::removeElementFromPendingResourcesForRemoval(const AtomicString& id)
+{
+    if (id.isEmpty())
+        return 0;
+
+    SVGPendingElements* resourceSet = m_pendingResourcesForRemoval.get(id);
+    if (!resourceSet || resourceSet->isEmpty())
+        return 0;
+
+    SVGPendingElements::iterator firstElement = resourceSet->begin();
+    SVGStyledElement* element = *firstElement;
+
+    resourceSet->remove(firstElement);
 
-    SVGPendingElements* elements = m_pendingResources.get(id);
-    elements->remove(element);
-    if (elements->isEmpty())
-        removePendingResource(id);
+    if (resourceSet->isEmpty())
+        removePendingResourceForRemoval(id);
 
-    element->clearHasPendingResourcesIfPossible();
+    return element;
 }
 
 HashSet<SVGElement*>* SVGDocumentExtensions::setOfElementsReferencingTarget(SVGElement* referencedElement) const
index 48dae973727475b9d186a85372005c47987727ff..b0c60259430aa46984396409e530c739a1bde0a5 100644 (file)
@@ -77,7 +77,8 @@ private:
     HashSet<SVGSVGElement*> m_timeContainers; // For SVG 1.2 support this will need to be made more general.
     HashMap<SVGElement*, HashSet<SVGSMILElement*>* > m_animatedElements;
     HashMap<AtomicString, RenderSVGResourceContainer*> m_resources;
-    HashMap<AtomicString, SVGPendingElements*> m_pendingResources;
+    HashMap<AtomicString, SVGPendingElements*> m_pendingResources; // Resources that are pending.
+    HashMap<AtomicString, SVGPendingElements*> m_pendingResourcesForRemoval; // Resources that are pending and scheduled for removal.
     HashMap<SVGElement*, OwnPtr<HashSet<SVGElement*> > > m_elementDependencies;
     OwnPtr<SVGResourcesCache> m_resourcesCache;
 
@@ -91,7 +92,13 @@ public:
     bool isElementPendingResource(SVGStyledElement*, const AtomicString& id) const;
     void removeElementFromPendingResources(SVGStyledElement*);
     PassOwnPtr<SVGPendingElements> removePendingResource(const AtomicString& id);
-    void removePendingResourceForElement(const AtomicString& id, SVGStyledElement*);
+
+    // The following two functions are used for scheduling a pending resource to be removed.
+    void markPendingResourcesForRemoval(const AtomicString&);
+    SVGStyledElement* removeElementFromPendingResourcesForRemoval(const AtomicString&);
+
+private:
+    PassOwnPtr<SVGPendingElements> removePendingResourceForRemoval(const AtomicString&);
 };
 
 }
index a573b8dce2decede7ed137ae798d53b65910a3de..9c427d1ed6d3e45c83750e375d4fa88e0c9ad2ff 100644 (file)
@@ -375,14 +375,16 @@ void SVGStyledElement::buildPendingResourcesIfNeeded()
     if (!extensions->hasPendingResource(resourceId))
         return;
 
-    OwnPtr<SVGDocumentExtensions::SVGPendingElements> clients(extensions->removePendingResource(resourceId));
-    ASSERT(!clients->isEmpty());
-
-    const SVGDocumentExtensions::SVGPendingElements::const_iterator end = clients->end();
-    for (SVGDocumentExtensions::SVGPendingElements::const_iterator it = clients->begin(); it != end; ++it) {
-        ASSERT((*it)->hasPendingResources());
-        (*it)->buildPendingResource();
-        (*it)->clearHasPendingResourcesIfPossible();
+    // Mark pending resources as pending for removal.
+    extensions->markPendingResourcesForRemoval(resourceId);
+
+    // Rebuild pending resources for each client of a pending resource that is being removed.
+    while (SVGStyledElement* clientElement = extensions->removeElementFromPendingResourcesForRemoval(resourceId)) {
+        ASSERT(clientElement->hasPendingResources());
+        if (clientElement->hasPendingResources()) {
+            clientElement->buildPendingResource();
+            clientElement->clearHasPendingResourcesIfPossible();
+        }
     }
 }