- Revision
- 112030
- Author
- [email protected]
- Date
- 2012-03-25 10:41:18 -0700 (Sun, 25 Mar 2012)
Log Message
Avoid race condition when iterating over pending resources
https://bugs.webkit.org/show_bug.cgi?id=82115
Patch by Philip Rogers <[email protected]> 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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (112029 => 112030)
--- trunk/LayoutTests/ChangeLog 2012-03-25 17:18:46 UTC (rev 112029)
+++ trunk/LayoutTests/ChangeLog 2012-03-25 17:41:18 UTC (rev 112030)
@@ -1,3 +1,14 @@
+2012-03-25 Philip Rogers <[email protected]>
+
+ 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 <[email protected]>
Not reviewed. Rebaseline foreignObject results, after Florins changes.
Added: trunk/LayoutTests/http/tests/svg/change-id-with-pending-resources-expected.txt (0 => 112030)
--- trunk/LayoutTests/http/tests/svg/change-id-with-pending-resources-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/svg/change-id-with-pending-resources-expected.txt 2012-03-25 17:41:18 UTC (rev 112030)
@@ -0,0 +1 @@
+If this text is visible and the test did not crash, this test passes
Added: trunk/LayoutTests/http/tests/svg/change-id-with-pending-resources.html (0 => 112030)
--- trunk/LayoutTests/http/tests/svg/change-id-with-pending-resources.html (rev 0)
+++ trunk/LayoutTests/http/tests/svg/change-id-with-pending-resources.html 2012-03-25 17:41:18 UTC (rev 112030)
@@ -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="" id="root" _onload_="crash()"/>
Added: trunk/LayoutTests/http/tests/svg/resources/svg-use-defs-rect.svg (0 => 112030)
--- trunk/LayoutTests/http/tests/svg/resources/svg-use-defs-rect.svg (rev 0)
+++ trunk/LayoutTests/http/tests/svg/resources/svg-use-defs-rect.svg 2012-03-25 17:41:18 UTC (rev 112030)
@@ -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="" mask="url(#mask)"/>
+</svg>
Modified: trunk/Source/WebCore/ChangeLog (112029 => 112030)
--- trunk/Source/WebCore/ChangeLog 2012-03-25 17:18:46 UTC (rev 112029)
+++ trunk/Source/WebCore/ChangeLog 2012-03-25 17:41:18 UTC (rev 112030)
@@ -1,3 +1,39 @@
+2012-03-25 Philip Rogers <[email protected]>
+
+ 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 <[email protected]>
[BlackBerry] Accelerated compositing layers fail to render when using WebPageCompositor
Modified: trunk/Source/WebCore/svg/SVGDocumentExtensions.cpp (112029 => 112030)
--- trunk/Source/WebCore/svg/SVGDocumentExtensions.cpp 2012-03-25 17:18:46 UTC (rev 112029)
+++ trunk/Source/WebCore/svg/SVGDocumentExtensions.cpp 2012-03-25 17:41:18 UTC (rev 112030)
@@ -52,6 +52,7 @@
{
deleteAllValues(m_animatedElements);
deleteAllValues(m_pendingResources);
+ deleteAllValues(m_pendingResourcesForRemoval);
}
void SVGDocumentExtensions::addTimeContainer(SVGSVGElement* element)
@@ -262,27 +263,47 @@
{
ASSERT(element);
- if (m_pendingResources.isEmpty() || !element->hasPendingResources())
- return;
+ // 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());
- 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);
+ }
- 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();
+ // 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());
- // 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);
+ 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,19 +312,44 @@
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));
+}
- SVGPendingElements* elements = m_pendingResources.get(id);
- elements->remove(element);
- if (elements->isEmpty())
- removePendingResource(id);
+void SVGDocumentExtensions::markPendingResourcesForRemoval(const AtomicString& id)
+{
+ if (id.isEmpty())
+ return;
- element->clearHasPendingResourcesIfPossible();
+ 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);
+
+ if (resourceSet->isEmpty())
+ removePendingResourceForRemoval(id);
+
+ return element;
+}
+
HashSet<SVGElement*>* SVGDocumentExtensions::setOfElementsReferencingTarget(SVGElement* referencedElement) const
{
ASSERT(referencedElement);
Modified: trunk/Source/WebCore/svg/SVGDocumentExtensions.h (112029 => 112030)
--- trunk/Source/WebCore/svg/SVGDocumentExtensions.h 2012-03-25 17:18:46 UTC (rev 112029)
+++ trunk/Source/WebCore/svg/SVGDocumentExtensions.h 2012-03-25 17:41:18 UTC (rev 112030)
@@ -77,7 +77,8 @@
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 @@
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&);
};
}
Modified: trunk/Source/WebCore/svg/SVGStyledElement.cpp (112029 => 112030)
--- trunk/Source/WebCore/svg/SVGStyledElement.cpp 2012-03-25 17:18:46 UTC (rev 112029)
+++ trunk/Source/WebCore/svg/SVGStyledElement.cpp 2012-03-25 17:41:18 UTC (rev 112030)
@@ -375,14 +375,16 @@
if (!extensions->hasPendingResource(resourceId))
return;
- OwnPtr<SVGDocumentExtensions::SVGPendingElements> clients(extensions->removePendingResource(resourceId));
- ASSERT(!clients->isEmpty());
+ // Mark pending resources as pending for removal.
+ extensions->markPendingResourcesForRemoval(resourceId);
- 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();
+ // 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();
+ }
}
}
