- Revision
- 96707
- Author
- infe...@chromium.org
- Date
- 2011-10-05 08:40:05 -0700 (Wed, 05 Oct 2011)
Log Message
Crash in SVGTRefElement::updateReferencedText.
https://bugs.webkit.org/show_bug.cgi?id=67555
Reviewed by Rob Buis.
Source/WebCore:
Test: svg/text/tref-event-listener-crash.svg
* svg/SVGTRefElement.cpp:
(WebCore::SubtreeModificationEventListener::clear): This fixes the
crash. there can be cases when our target is removed, but our event
listener is alive. when our tref is going away, we need to disable
those event listeners by nulling out our tref element.
(WebCore::SubtreeModificationEventListener::handleEvent): Adding
null check for tref element, which can happen in cases where
event listener's clear is called, but we could not remove it from
the target.
(WebCore::SVGTRefElement::~SVGTRefElement): There are always
cases when removedFromDocument is not called. So, important to
clear event listener in those cases.
(WebCore::SVGTRefElement::svgAttributeChanged): Use common
function buildPendingResource.
(WebCore::SVGTRefElement::buildPendingResource): Common function
to prevent triple duplication of same code. made it more readable.
(WebCore::SVGTRefElement::insertedIntoDocument): Use common
function buildPendingResource.
(WebCore::SVGTRefElement::removedFromDocument): Use new helper
removeEventListener.
(WebCore::SVGTRefElement::clearEventListener): Helper to clear
event listener. Name chosen to not collide with Node::removeEventListener.
* svg/SVGTRefElement.h: definition of new helpers.
* svg/SVGTextPathElement.cpp:
(WebCore::SVGTextPathElement::insertedIntoDocument): Copy same
logic as tref and use element. more asserts and easy bailouts.
LayoutTests:
* svg/text/tref-event-listener-crash-expected.txt: Added.
* svg/text/tref-event-listener-crash.svg: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (96706 => 96707)
--- trunk/LayoutTests/ChangeLog 2011-10-05 15:18:57 UTC (rev 96706)
+++ trunk/LayoutTests/ChangeLog 2011-10-05 15:40:05 UTC (rev 96707)
@@ -1,3 +1,13 @@
+2011-10-05 Abhishek Arya <infe...@chromium.org>
+
+ Crash in SVGTRefElement::updateReferencedText.
+ https://bugs.webkit.org/show_bug.cgi?id=67555
+
+ Reviewed by Rob Buis.
+
+ * svg/text/tref-event-listener-crash-expected.txt: Added.
+ * svg/text/tref-event-listener-crash.svg: Added.
+
2011-10-05 Balazs Kelemen <kbal...@webkit.org>
[Qt][WK2] First test always fails because of useless xcb dump
Added: trunk/LayoutTests/svg/text/tref-event-listener-crash-expected.txt (0 => 96707)
--- trunk/LayoutTests/svg/text/tref-event-listener-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/svg/text/tref-event-listener-crash-expected.txt 2011-10-05 15:40:05 UTC (rev 96707)
@@ -0,0 +1 @@
+Test passes if it does not crash.
Added: trunk/LayoutTests/svg/text/tref-event-listener-crash.svg (0 => 96707)
--- trunk/LayoutTests/svg/text/tref-event-listener-crash.svg (rev 0)
+++ trunk/LayoutTests/svg/text/tref-event-listener-crash.svg 2011-10-05 15:40:05 UTC (rev 96707)
@@ -0,0 +1,15 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+Test passes if it does not crash.
+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" id="container">
+<script>
+if (window.layoutTestController)
+ layoutTestController.dumpAsText();
+
+window._onload_ = function() {
+ document.getElementById("container").textContent = "1";
+}
+</script>
+<desc id="test-desc"></desc>
+<tref xlink:href=""
+</svg>
+</html>
\ No newline at end of file
Modified: trunk/Source/WebCore/ChangeLog (96706 => 96707)
--- trunk/Source/WebCore/ChangeLog 2011-10-05 15:18:57 UTC (rev 96706)
+++ trunk/Source/WebCore/ChangeLog 2011-10-05 15:40:05 UTC (rev 96707)
@@ -1,3 +1,39 @@
+2011-10-05 Abhishek Arya <infe...@chromium.org>
+
+ Crash in SVGTRefElement::updateReferencedText.
+ https://bugs.webkit.org/show_bug.cgi?id=67555
+
+ Reviewed by Rob Buis.
+
+ Test: svg/text/tref-event-listener-crash.svg
+
+ * svg/SVGTRefElement.cpp:
+ (WebCore::SubtreeModificationEventListener::clear): This fixes the
+ crash. there can be cases when our target is removed, but our event
+ listener is alive. when our tref is going away, we need to disable
+ those event listeners by nulling out our tref element.
+ (WebCore::SubtreeModificationEventListener::handleEvent): Adding
+ null check for tref element, which can happen in cases where
+ event listener's clear is called, but we could not remove it from
+ the target.
+ (WebCore::SVGTRefElement::~SVGTRefElement): There are always
+ cases when removedFromDocument is not called. So, important to
+ clear event listener in those cases.
+ (WebCore::SVGTRefElement::svgAttributeChanged): Use common
+ function buildPendingResource.
+ (WebCore::SVGTRefElement::buildPendingResource): Common function
+ to prevent triple duplication of same code. made it more readable.
+ (WebCore::SVGTRefElement::insertedIntoDocument): Use common
+ function buildPendingResource.
+ (WebCore::SVGTRefElement::removedFromDocument): Use new helper
+ removeEventListener.
+ (WebCore::SVGTRefElement::clearEventListener): Helper to clear
+ event listener. Name chosen to not collide with Node::removeEventListener.
+ * svg/SVGTRefElement.h: definition of new helpers.
+ * svg/SVGTextPathElement.cpp:
+ (WebCore::SVGTextPathElement::insertedIntoDocument): Copy same
+ logic as tref and use element. more asserts and easy bailouts.
+
2011-10-05 Pavel Feldman <pfeld...@google.com>
Not reviewed: restoring : shortcut in inspector.
Modified: trunk/Source/WebCore/svg/SVGTRefElement.cpp (96706 => 96707)
--- trunk/Source/WebCore/svg/SVGTRefElement.cpp 2011-10-05 15:18:57 UTC (rev 96706)
+++ trunk/Source/WebCore/svg/SVGTRefElement.cpp 2011-10-05 15:40:05 UTC (rev 96707)
@@ -73,11 +73,14 @@
virtual bool operator==(const EventListener&);
- void removeFromTarget()
+ void clear()
{
Element* target = m_trefElement->treeScope()->getElementById(m_targetId);
if (target && target->parentNode())
target->parentNode()->removeEventListener(eventNames().DOMSubtreeModifiedEvent, this, false);
+
+ m_trefElement = 0;
+ m_targetId = String();
}
private:
@@ -103,7 +106,7 @@
void SubtreeModificationEventListener::handleEvent(ScriptExecutionContext*, Event* event)
{
- if (event->type() == eventNames().DOMSubtreeModifiedEvent && m_trefElement != event->target())
+ if (m_trefElement && event->type() == eventNames().DOMSubtreeModifiedEvent && m_trefElement != event->target())
m_trefElement->updateReferencedText();
}
@@ -136,6 +139,11 @@
}
}
+SVGTRefElement::~SVGTRefElement()
+{
+ clearEventListener();
+}
+
void SVGTRefElement::updateReferencedText()
{
Element* target = SVGURIReference::targetElementFromIRIString(href(), document());
@@ -182,22 +190,7 @@
SVGElementInstance::InvalidationGuard invalidationGuard(this);
if (SVGURIReference::isKnownAttribute(attrName)) {
- if (m_eventListener) {
- m_eventListener->removeFromTarget();
- m_eventListener = 0;
- }
- String id;
- Element* target = SVGURIReference::targetElementFromIRIString(href(), document(), &id);
- if (!target) {
- document()->accessSVGExtensions()->addPendingResource(id, this);
- return;
- }
- updateReferencedText();
- if (inDocument()) {
- m_eventListener = SubtreeModificationEventListener::create(this, id);
- ASSERT(target->parentNode());
- target->parentNode()->addEventListener(eventNames().DOMSubtreeModifiedEvent, m_eventListener.get(), false);
- }
+ buildPendingResource();
if (RenderObject* renderer = this->renderer())
RenderSVGResource::markForLayoutAndParentResourceInvalidation(renderer);
return;
@@ -233,39 +226,50 @@
void SVGTRefElement::buildPendingResource()
{
- updateReferencedText();
- if (Element* target = SVGURIReference::targetElementFromIRIString(href(), document())) {
- ASSERT(!m_eventListener);
- m_eventListener = SubtreeModificationEventListener::create(this, target->getIdAttribute());
- ASSERT(target->parentNode());
- target->parentNode()->addEventListener(eventNames().DOMSubtreeModifiedEvent, m_eventListener.get(), false);
- }
-}
+ // Remove any existing event listener.
+ clearEventListener();
-void SVGTRefElement::insertedIntoDocument()
-{
- SVGStyledElement::insertedIntoDocument();
String id;
Element* target = SVGURIReference::targetElementFromIRIString(href(), document(), &id);
if (!target) {
+ if (hasPendingResources() || id.isEmpty())
+ return;
+
+ ASSERT(!hasPendingResources());
document()->accessSVGExtensions()->addPendingResource(id, this);
+ ASSERT(hasPendingResources());
return;
}
+
updateReferencedText();
+
+ // We should not add the event listener if we are not in document yet.
+ if (!inDocument())
+ return;
+
m_eventListener = SubtreeModificationEventListener::create(this, id);
ASSERT(target->parentNode());
target->parentNode()->addEventListener(eventNames().DOMSubtreeModifiedEvent, m_eventListener.get(), false);
}
+void SVGTRefElement::insertedIntoDocument()
+{
+ SVGStyledElement::insertedIntoDocument();
+ buildPendingResource();
+}
+
void SVGTRefElement::removedFromDocument()
{
SVGStyledElement::removedFromDocument();
+ clearEventListener();
+}
- if (!m_eventListener)
- return;
-
- m_eventListener->removeFromTarget();
- m_eventListener = 0;
+void SVGTRefElement::clearEventListener()
+{
+ if (m_eventListener) {
+ m_eventListener->clear();
+ m_eventListener = 0;
+ }
}
}
Modified: trunk/Source/WebCore/svg/SVGTRefElement.h (96706 => 96707)
--- trunk/Source/WebCore/svg/SVGTRefElement.h 2011-10-05 15:18:57 UTC (rev 96706)
+++ trunk/Source/WebCore/svg/SVGTRefElement.h 2011-10-05 15:40:05 UTC (rev 96707)
@@ -38,6 +38,7 @@
friend class SubtreeModificationEventListener;
SVGTRefElement(const QualifiedName&, Document*);
+ virtual ~SVGTRefElement();
bool isSupportedAttribute(const QualifiedName&);
virtual void parseMappedAttribute(Attribute*);
@@ -50,6 +51,8 @@
virtual void insertedIntoDocument();
virtual void removedFromDocument();
+ void clearEventListener();
+
void updateReferencedText();
virtual void buildPendingResource();
Modified: trunk/Source/WebCore/svg/SVGTextPathElement.cpp (96706 => 96707)
--- trunk/Source/WebCore/svg/SVGTextPathElement.cpp 2011-10-05 15:18:57 UTC (rev 96706)
+++ trunk/Source/WebCore/svg/SVGTextPathElement.cpp 2011-10-05 15:40:05 UTC (rev 96707)
@@ -145,7 +145,12 @@
String id;
Element* targetElement = SVGURIReference::targetElementFromIRIString(href(), document(), &id);
if (!targetElement) {
+ if (hasPendingResources() || id.isEmpty())
+ return;
+
+ ASSERT(!hasPendingResources());
document()->accessSVGExtensions()->addPendingResource(id, this);
+ ASSERT(hasPendingResources());
return;
}
}
