Title: [122278] trunk
- Revision
- 122278
- Author
- [email protected]
- Date
- 2012-07-10 16:36:07 -0700 (Tue, 10 Jul 2012)
Log Message
Crash due to SVG animation element not removed from target (before reset)
https://bugs.webkit.org/show_bug.cgi?id=90750
Reviewed by Abhishek Arya.
Source/WebCore:
Previously we were not removing an animation element from
SVGDocumentExtensions::m_animatedElements which led to a crash.
This change properly removes animation elements in resetTargetElement
which both fixes this bug and will prevent others from hitting it in
the future.
Test: svg/animations/dynamic-modify-attributename-crash2.svg
* svg/SVGDocumentExtensions.cpp:
(WebCore::SVGDocumentExtensions::removeAllAnimationElementsFromTarget):
removeAllAnimationElementsFromTarget now adds all the animation elements
to a vector and iterates over it because the changes to resetTargetElement
would have caused us to modify the underlying hashset as we iterated. Note that
before we deleted animationElementsForTarget in removeAllAnimationElementsFromTarget
but that logic is now handled in removeAnimationElementFromTarget which is called
during resetTargetElement.
* svg/animation/SVGSMILElement.cpp:
(WebCore::SVGSMILElement::removedFrom):
Because of the changes in resetTargetElement, removedFrom was able to be
refactored. This patch changes removedFrom to call resetTargetElement rather
than have duplicated logic. There is a very small change in logic here:
animationAttributeChanged() is now called in removedFrom().
(WebCore::SVGSMILElement::resetTargetElement):
resetTargetElement now fully resets the target, including removing it from
m_animatedElements. This will prevent future instances of this bug.
LayoutTests:
* svg/animations/dynamic-modify-attributename-crash2-expected.txt: Added.
* svg/animations/dynamic-modify-attributename-crash2.svg: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (122277 => 122278)
--- trunk/LayoutTests/ChangeLog 2012-07-10 23:22:32 UTC (rev 122277)
+++ trunk/LayoutTests/ChangeLog 2012-07-10 23:36:07 UTC (rev 122278)
@@ -1,3 +1,13 @@
+2012-07-10 Philip Rogers <[email protected]>
+
+ Crash due to SVG animation element not removed from target (before reset)
+ https://bugs.webkit.org/show_bug.cgi?id=90750
+
+ Reviewed by Abhishek Arya.
+
+ * svg/animations/dynamic-modify-attributename-crash2-expected.txt: Added.
+ * svg/animations/dynamic-modify-attributename-crash2.svg: Added.
+
2012-07-10 Dean Jackson <[email protected]>
REGRESSION (r109610): Order of values in shorthand animation makes a difference
Added: trunk/LayoutTests/svg/animations/dynamic-modify-attributename-crash2-expected.txt (0 => 122278)
--- trunk/LayoutTests/svg/animations/dynamic-modify-attributename-crash2-expected.txt (rev 0)
+++ trunk/LayoutTests/svg/animations/dynamic-modify-attributename-crash2-expected.txt 2012-07-10 23:36:07 UTC (rev 122278)
@@ -0,0 +1 @@
+PASS
Added: trunk/LayoutTests/svg/animations/dynamic-modify-attributename-crash2.svg (0 => 122278)
--- trunk/LayoutTests/svg/animations/dynamic-modify-attributename-crash2.svg (rev 0)
+++ trunk/LayoutTests/svg/animations/dynamic-modify-attributename-crash2.svg 2012-07-10 23:36:07 UTC (rev 122278)
@@ -0,0 +1,26 @@
+<!-- Test for WK90750 - this should not crash or say FAIL. -->
+<svg version="1.1" xmlns="http://www.w3.org/2000/svg">
+ <set id="set"></set>
+ <text id="status" x="100" y="100">FAIL</text>
+ <script>
+ if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+ }
+
+ if (location.hash != "#done") {
+ setTimeout(function() {
+ document.getElementById("set").setAttribute("attributeName", "points");
+ document.implementation.createDocument("", "", null).adoptNode(document.getElementById("set"));
+ location.hash = "#done";
+ window.location.reload();
+ }, 0);
+ } else {
+ if (window.testRunner)
+ gc();
+ document.getElementById("status").textContent = "PASS";
+ if (window.testRunner)
+ testRunner.notifyDone();
+ }
+ </script>
+</svg>
Modified: trunk/Source/WebCore/ChangeLog (122277 => 122278)
--- trunk/Source/WebCore/ChangeLog 2012-07-10 23:22:32 UTC (rev 122277)
+++ trunk/Source/WebCore/ChangeLog 2012-07-10 23:36:07 UTC (rev 122278)
@@ -1,3 +1,41 @@
+2012-07-10 Philip Rogers <[email protected]>
+
+ Crash due to SVG animation element not removed from target (before reset)
+ https://bugs.webkit.org/show_bug.cgi?id=90750
+
+ Reviewed by Abhishek Arya.
+
+ Previously we were not removing an animation element from
+ SVGDocumentExtensions::m_animatedElements which led to a crash.
+ This change properly removes animation elements in resetTargetElement
+ which both fixes this bug and will prevent others from hitting it in
+ the future.
+
+ Test: svg/animations/dynamic-modify-attributename-crash2.svg
+
+ * svg/SVGDocumentExtensions.cpp:
+ (WebCore::SVGDocumentExtensions::removeAllAnimationElementsFromTarget):
+
+ removeAllAnimationElementsFromTarget now adds all the animation elements
+ to a vector and iterates over it because the changes to resetTargetElement
+ would have caused us to modify the underlying hashset as we iterated. Note that
+ before we deleted animationElementsForTarget in removeAllAnimationElementsFromTarget
+ but that logic is now handled in removeAnimationElementFromTarget which is called
+ during resetTargetElement.
+
+ * svg/animation/SVGSMILElement.cpp:
+ (WebCore::SVGSMILElement::removedFrom):
+
+ Because of the changes in resetTargetElement, removedFrom was able to be
+ refactored. This patch changes removedFrom to call resetTargetElement rather
+ than have duplicated logic. There is a very small change in logic here:
+ animationAttributeChanged() is now called in removedFrom().
+
+ (WebCore::SVGSMILElement::resetTargetElement):
+
+ resetTargetElement now fully resets the target, including removing it from
+ m_animatedElements. This will prevent future instances of this bug.
+
2012-07-10 Helder Correia <[email protected]>
[Qt] Repaint counter for accelerated compositing
Modified: trunk/Source/WebCore/svg/SVGDocumentExtensions.cpp (122277 => 122278)
--- trunk/Source/WebCore/svg/SVGDocumentExtensions.cpp 2012-07-10 23:22:32 UTC (rev 122277)
+++ trunk/Source/WebCore/svg/SVGDocumentExtensions.cpp 2012-07-10 23:36:07 UTC (rev 122278)
@@ -169,14 +169,20 @@
void SVGDocumentExtensions::removeAllAnimationElementsFromTarget(SVGElement* targetElement)
{
ASSERT(targetElement);
- HashSet<SVGSMILElement*>* animationElementsForTarget = m_animatedElements.take(targetElement);
- if (!animationElementsForTarget)
+ HashMap<SVGElement*, HashSet<SVGSMILElement*>* >::iterator it = m_animatedElements.find(targetElement);
+ if (it == m_animatedElements.end())
return;
- HashSet<SVGSMILElement*>::iterator it = animationElementsForTarget->begin();
+
+ HashSet<SVGSMILElement*>* animationElementsForTarget = it->second;
+ Vector<SVGSMILElement*> toBeReset;
+
HashSet<SVGSMILElement*>::iterator end = animationElementsForTarget->end();
- for (; it != end; ++it)
- (*it)->resetTargetElement();
- delete animationElementsForTarget;
+ for (HashSet<SVGSMILElement*>::iterator it = animationElementsForTarget->begin(); it != end; ++it)
+ toBeReset.append(*it);
+
+ Vector<SVGSMILElement*>::iterator vectorEnd = toBeReset.end();
+ for (Vector<SVGSMILElement*>::iterator vectorIt = toBeReset.begin(); vectorIt != vectorEnd; ++vectorIt)
+ (*vectorIt)->resetTargetElement();
}
// FIXME: Callers should probably use ScriptController::eventHandlerLineNumber()
Modified: trunk/Source/WebCore/svg/animation/SVGSMILElement.cpp (122277 => 122278)
--- trunk/Source/WebCore/svg/animation/SVGSMILElement.cpp 2012-07-10 23:22:32 UTC (rev 122277)
+++ trunk/Source/WebCore/svg/animation/SVGSMILElement.cpp 2012-07-10 23:36:07 UTC (rev 122278)
@@ -233,11 +233,8 @@
disconnectConditions();
// Clear target now, because disconnectConditions calls targetElement() which will recreate the target if we removed it sooner.
- if (m_targetElement) {
- document()->accessSVGExtensions()->removeAnimationElementFromTarget(this, m_targetElement);
- targetElementWillChange(m_targetElement, 0);
- m_targetElement = 0;
- }
+ if (m_targetElement)
+ resetTargetElement();
m_attributeName = anyQName();
}
@@ -582,6 +579,7 @@
void SVGSMILElement::resetTargetElement()
{
+ document()->accessSVGExtensions()->removeAnimationElementFromTarget(this, m_targetElement);
targetElementWillChange(m_targetElement, 0);
m_targetElement = 0;
animationAttributeChanged();
_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo/webkit-changes
