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
+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.
--- /dev/null
+If this text is visible and the test did not crash, this test passes
--- /dev/null
+<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()"/>
--- /dev/null
+<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>
+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
{
deleteAllValues(m_animatedElements);
deleteAllValues(m_pendingResources);
+ deleteAllValues(m_pendingResourcesForRemoval);
}
void SVGDocumentExtensions::addTimeContainer(SVGSVGElement* element)
{
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)
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
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;
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&);
};
}
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();
+ }
}
}
git://git.webkit.org / WebKit-https.git / commitdiff
