<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[197125] trunk/Source/WebCore</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/197125">197125</a></dd>
<dt>Author</dt> <dd>said@apple.com</dd>
<dt>Date</dt> <dd>2016-02-25 11:55:55 -0800 (Thu, 25 Feb 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>REGRESSION (<a href="http://trac.webkit.org/projects/webkit/changeset/196268">r196268</a>): Many assertion failures and crashes on SVG path animation tests when JS garbage collection happens quickly
https://bugs.webkit.org/show_bug.cgi?id=154331
Reviewed by Darin Adler.
This is not an actual regression. The bug did exist before <a href="http://trac.webkit.org/projects/webkit/changeset/196268">r196268</a> but
the whole document was leaking once an SVGAnimatedProperty was created
so there was no way to produce this bug. After fixing the leak, one crash
and one assert got uncovered. Both of them happen because of the fact:
"if an SVGAnimatedProperty is not referenced it will be deleted."
* svg/SVGPathElement.cpp:
(WebCore::SVGPathElement::lookupOrCreateDWrapper):
The code in this function was assuming that the wrapper will be created
only once which happens when SVGAnimatedProperty::lookupOrCreateWrapper()
is called. Before making this single call, lookupOrCreateDWrapper() was
building an initial SVGPathSegList from byte stream. But now
SVGAnimatedProperty::lookupWrapper() can return false even after creating
the SVGAnimatedProperty because it was deleted later. Calling
buildSVGPathSegListFromByteStream() more than once was causing
SVGAnimatedListPropertyTearOff::animationStarted() to fire the assertion
ASSERT(m_values.size() == m_wrappers.size()) because the path segments were
appended twice to m_values which is in fact SVGPathElement::m_pathSegList.value.
The fix is to build the initial SVGPathSegList only once which should happen
when m_pathSegList.value.isEmpty().
(WebCore::SVGPathElement::animatedPropertyWillBeDeleted):
* svg/SVGPathElement.h:
* svg/properties/SVGAnimatedPathSegListPropertyTearOff.h:
(WebCore::SVGAnimatedPathSegListPropertyTearOff::~SVGAnimatedPathSegListPropertyTearOff):
SVGPathElement is assuming the following equivalence relation:
m_pathSegList.shouldSynchronize ~ SVGAnimatedProperty_is_created_and_not_null.
SVGPathElement::animatedPathSegList() and animatedNormalizedPathSegList()
set m_pathSegList.shouldSynchronize to true when SVGAnimatedProperty is
created but nothing sets m_pathSegList.shouldSynchronize back to false.
This was not a problem when the SVGAnimatedProperty was leaking but after
ensuring it is deleted when it is not referenced this equivalence relation
becomes untrue sometimes. This caused SVGPathElement::svgAttributeChanged()
to crash when we check m_pathSegList.shouldSynchronize and if it is true we
assume that SVGAnimatedProperty::lookupWrapper() will return a non-null pointer
and therefore we deference this pointer and call SVGAnimatedProperty::isAnimating().
To fix this crash we need to set m_pathSegList.shouldSynchronize back to false
when the associated SVGAnimatedProperty is deleted.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoresvgSVGPathElementcpp">trunk/Source/WebCore/svg/SVGPathElement.cpp</a></li>
<li><a href="#trunkSourceWebCoresvgSVGPathElementh">trunk/Source/WebCore/svg/SVGPathElement.h</a></li>
<li><a href="#trunkSourceWebCoresvgpropertiesSVGAnimatedPathSegListPropertyTearOffh">trunk/Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.h</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (197124 => 197125)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog 2016-02-25 19:36:52 UTC (rev 197124)
+++ trunk/Source/WebCore/ChangeLog 2016-02-25 19:55:55 UTC (rev 197125)
</span><span class="lines">@@ -1,3 +1,49 @@
</span><ins>+2016-02-25 Said Abou-Hallawa <sabouhallawa@apple.com>
+
+ REGRESSION (r196268): Many assertion failures and crashes on SVG path animation tests when JS garbage collection happens quickly
+ https://bugs.webkit.org/show_bug.cgi?id=154331
+
+ Reviewed by Darin Adler.
+
+ This is not an actual regression. The bug did exist before r196268 but
+ the whole document was leaking once an SVGAnimatedProperty was created
+ so there was no way to produce this bug. After fixing the leak, one crash
+ and one assert got uncovered. Both of them happen because of the fact:
+ "if an SVGAnimatedProperty is not referenced it will be deleted."
+
+ * svg/SVGPathElement.cpp:
+ (WebCore::SVGPathElement::lookupOrCreateDWrapper):
+ The code in this function was assuming that the wrapper will be created
+ only once which happens when SVGAnimatedProperty::lookupOrCreateWrapper()
+ is called. Before making this single call, lookupOrCreateDWrapper() was
+ building an initial SVGPathSegList from byte stream. But now
+ SVGAnimatedProperty::lookupWrapper() can return false even after creating
+ the SVGAnimatedProperty because it was deleted later. Calling
+ buildSVGPathSegListFromByteStream() more than once was causing
+ SVGAnimatedListPropertyTearOff::animationStarted() to fire the assertion
+ ASSERT(m_values.size() == m_wrappers.size()) because the path segments were
+ appended twice to m_values which is in fact SVGPathElement::m_pathSegList.value.
+ The fix is to build the initial SVGPathSegList only once which should happen
+ when m_pathSegList.value.isEmpty().
+
+ (WebCore::SVGPathElement::animatedPropertyWillBeDeleted):
+ * svg/SVGPathElement.h:
+ * svg/properties/SVGAnimatedPathSegListPropertyTearOff.h:
+ (WebCore::SVGAnimatedPathSegListPropertyTearOff::~SVGAnimatedPathSegListPropertyTearOff):
+ SVGPathElement is assuming the following equivalence relation:
+ m_pathSegList.shouldSynchronize ~ SVGAnimatedProperty_is_created_and_not_null.
+ SVGPathElement::animatedPathSegList() and animatedNormalizedPathSegList()
+ set m_pathSegList.shouldSynchronize to true when SVGAnimatedProperty is
+ created but nothing sets m_pathSegList.shouldSynchronize back to false.
+ This was not a problem when the SVGAnimatedProperty was leaking but after
+ ensuring it is deleted when it is not referenced this equivalence relation
+ becomes untrue sometimes. This caused SVGPathElement::svgAttributeChanged()
+ to crash when we check m_pathSegList.shouldSynchronize and if it is true we
+ assume that SVGAnimatedProperty::lookupWrapper() will return a non-null pointer
+ and therefore we deference this pointer and call SVGAnimatedProperty::isAnimating().
+ To fix this crash we need to set m_pathSegList.shouldSynchronize back to false
+ when the associated SVGAnimatedProperty is deleted.
+
</ins><span class="cx"> 2016-02-25 Brady Eidson <beidson@apple.com>
</span><span class="cx">
</span><span class="cx"> Modern IDB: WebKit 2 IPC layer.
</span></span></pre></div>
<a id="trunkSourceWebCoresvgSVGPathElementcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/svg/SVGPathElement.cpp (197124 => 197125)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/svg/SVGPathElement.cpp 2016-02-25 19:36:52 UTC (rev 197124)
+++ trunk/Source/WebCore/svg/SVGPathElement.cpp 2016-02-25 19:55:55 UTC (rev 197125)
</span><span class="lines">@@ -313,8 +313,8 @@
</span><span class="cx"> if (auto property = SVGAnimatedProperty::lookupWrapper<SVGPathElement, SVGAnimatedPathSegListPropertyTearOff>(&ownerType, dPropertyInfo()))
</span><span class="cx"> return *property;
</span><span class="cx">
</span><del>- // Build initial SVGPathSegList.
- buildSVGPathSegListFromByteStream(ownerType.m_pathByteStream, ownerType, ownerType.m_pathSegList.value, UnalteredParsing);
</del><ins>+ if (ownerType.m_pathSegList.value.isEmpty())
+ buildSVGPathSegListFromByteStream(ownerType.m_pathByteStream, ownerType, ownerType.m_pathSegList.value, UnalteredParsing);
</ins><span class="cx">
</span><span class="cx"> return SVGAnimatedProperty::lookupOrCreateWrapper<SVGPathElement, SVGAnimatedPathSegListPropertyTearOff, SVGPathSegList>
</span><span class="cx"> (&ownerType, dPropertyInfo(), ownerType.m_pathSegList.value);
</span><span class="lines">@@ -329,6 +329,15 @@
</span><span class="cx"> ownerType.m_pathSegList.synchronize(&ownerType, dPropertyInfo()->attributeName, ownerType.m_pathSegList.value.valueAsString());
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+void SVGPathElement::animatedPropertyWillBeDeleted()
+{
+ // m_pathSegList.shouldSynchronize is set to true when the 'd' wrapper for m_pathSegList
+ // is created and cached. We need to reset it back to false when this wrapper is deleted
+ // so we can be sure if shouldSynchronize is true, SVGAnimatedProperty::lookupWrapper()
+ // will return a valid cached 'd' wrapper for the m_pathSegList.
+ m_pathSegList.shouldSynchronize = false;
+}
+
</ins><span class="cx"> RefPtr<SVGPathSegListPropertyTearOff> SVGPathElement::pathSegList()
</span><span class="cx"> {
</span><span class="cx"> m_pathSegList.shouldSynchronize = true;
</span></span></pre></div>
<a id="trunkSourceWebCoresvgSVGPathElementh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/svg/SVGPathElement.h (197124 => 197125)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/svg/SVGPathElement.h 2016-02-25 19:36:52 UTC (rev 197124)
+++ trunk/Source/WebCore/svg/SVGPathElement.h 2016-02-25 19:55:55 UTC (rev 197125)
</span><span class="lines">@@ -99,6 +99,8 @@
</span><span class="cx">
</span><span class="cx"> WeakPtr<SVGPathElement> createWeakPtr() const { return m_weakPtrFactory.createWeakPtr(); }
</span><span class="cx">
</span><ins>+ void animatedPropertyWillBeDeleted();
+
</ins><span class="cx"> private:
</span><span class="cx"> SVGPathElement(const QualifiedName&, Document&);
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceWebCoresvgpropertiesSVGAnimatedPathSegListPropertyTearOffh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.h (197124 => 197125)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.h 2016-02-25 19:36:52 UTC (rev 197124)
+++ trunk/Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.h 2016-02-25 19:55:55 UTC (rev 197125)
</span><span class="lines">@@ -112,8 +112,15 @@
</span><span class="cx"> : SVGAnimatedListPropertyTearOff<SVGPathSegList>(contextElement, attributeName, animatedPropertyType, values)
</span><span class="cx"> , m_animatedPathByteStream(nullptr)
</span><span class="cx"> {
</span><ins>+ ASSERT(contextElement);
+ ASSERT(is<SVGPathElement>(contextElement));
</ins><span class="cx"> }
</span><span class="cx">
</span><ins>+ virtual ~SVGAnimatedPathSegListPropertyTearOff()
+ {
+ downcast<SVGPathElement>(contextElement())->animatedPropertyWillBeDeleted();
+ }
+
</ins><span class="cx"> SVGPathByteStream* m_animatedPathByteStream;
</span><span class="cx"> };
</span><span class="cx">
</span></span></pre>
</div>
</div>
</body>
</html>