<!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>[183706] trunk</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/183706">183706</a></dd>
<dt>Author</dt> <dd>akling@apple.com</dd>
<dt>Date</dt> <dd>2015-05-01 18:29:29 -0700 (Fri, 01 May 2015)</dd>
</dl>
<h3>Log Message</h3>
<pre>Reproducible crash removing name attribute from <img> node
<https://webkit.org/b/144371>
<rdar://problem/17198583>
Reviewed by Darin Adler.
Source/WebCore:
The problem here was with HTMLImageElement::getNameAttribute(), which relies
on Element::hasName() to avoid slow attribute lookups when the attribute
is already known not to be present. Unfortunately hasName() uses an ElementData
flag that wasn't getting updated until after the call to parseAttribute().
This patch fixes the issue by moving the code that updates the hasName() flag
before the parseAttribute() virtual dispatch.
Test: fast/dom/HTMLImageElement/remove-name-id-attribute-from-image.html
* dom/Element.cpp:
(WebCore::Element::attributeChanged):
LayoutTests:
* fast/dom/HTMLImageElement/remove-name-id-attribute-from-image-expected.txt: Added.
* fast/dom/HTMLImageElement/remove-name-id-attribute-from-image.html: Added.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsChangeLog">trunk/LayoutTests/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoredomElementcpp">trunk/Source/WebCore/dom/Element.cpp</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsfastdomHTMLImageElementremovenameidattributefromimageexpectedtxt">trunk/LayoutTests/fast/dom/HTMLImageElement/remove-name-id-attribute-from-image-expected.txt</a></li>
<li><a href="#trunkLayoutTestsfastdomHTMLImageElementremovenameidattributefromimagehtml">trunk/LayoutTests/fast/dom/HTMLImageElement/remove-name-id-attribute-from-image.html</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkLayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/ChangeLog (183705 => 183706)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/ChangeLog        2015-05-02 01:11:28 UTC (rev 183705)
+++ trunk/LayoutTests/ChangeLog        2015-05-02 01:29:29 UTC (rev 183706)
</span><span class="lines">@@ -1,3 +1,14 @@
</span><ins>+2015-05-01 Andreas Kling <akling@apple.com>
+
+ Reproducible crash removing name attribute from <img> node
+ <https://webkit.org/b/144371>
+ <rdar://problem/17198583>
+
+ Reviewed by Darin Adler.
+
+ * fast/dom/HTMLImageElement/remove-name-id-attribute-from-image-expected.txt: Added.
+ * fast/dom/HTMLImageElement/remove-name-id-attribute-from-image.html: Added.
+
</ins><span class="cx"> 2015-05-01 Eric Carlson <eric.carlson@apple.com>
</span><span class="cx">
</span><span class="cx"> Postpone caption style sheet creation
</span></span></pre></div>
<a id="trunkLayoutTestsfastdomHTMLImageElementremovenameidattributefromimageexpectedtxt"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/fast/dom/HTMLImageElement/remove-name-id-attribute-from-image-expected.txt (0 => 183706)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/fast/dom/HTMLImageElement/remove-name-id-attribute-from-image-expected.txt         (rev 0)
+++ trunk/LayoutTests/fast/dom/HTMLImageElement/remove-name-id-attribute-from-image-expected.txt        2015-05-02 01:29:29 UTC (rev 183706)
</span><span class="lines">@@ -0,0 +1,14 @@
</span><ins>+This test checks that there's no crash when removing the name or id attribute from a HTMLImageElement.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS doc.body.firstChild.getAttribute('name') is 'bar'
+PASS doc.body.firstChild.getAttribute('id') is 'foo'
+Removing name and id attributes...
+PASS doc.body.firstChild.getAttribute('name') is null
+PASS doc.body.firstChild.getAttribute('id') is null
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
</ins></span></pre></div>
<a id="trunkLayoutTestsfastdomHTMLImageElementremovenameidattributefromimagehtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/fast/dom/HTMLImageElement/remove-name-id-attribute-from-image.html (0 => 183706)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/fast/dom/HTMLImageElement/remove-name-id-attribute-from-image.html         (rev 0)
+++ trunk/LayoutTests/fast/dom/HTMLImageElement/remove-name-id-attribute-from-image.html        2015-05-02 01:29:29 UTC (rev 183706)
</span><span class="lines">@@ -0,0 +1,25 @@
</span><ins>+<!DOCTYPE html>
+<html>
+<head>
+<meta charset="utf-8">
+<script src="../../../resources/js-test-pre.js"></script>
+</head>
+<body>
+<script>
+
+description("This test checks that there's no crash when removing the name or id attribute from a HTMLImageElement.");
+
+var doc = document.implementation.createHTMLDocument( '' );
+doc.body.innerHTML = '<img name="bar" id="foo">';
+shouldBe("doc.body.firstChild.getAttribute('name')", "'bar'");
+shouldBe("doc.body.firstChild.getAttribute('id')", "'foo'");
+debug("Removing name and id attributes...");
+doc.body.firstChild.removeAttribute("name");
+doc.body.firstChild.removeAttribute("id");
+shouldBe("doc.body.firstChild.getAttribute('name')", "null");
+shouldBe("doc.body.firstChild.getAttribute('id')", "null");
+
+</script>
+<script src="../../../resources/js-test-post.js"></script>
+</body>
+</html>
</ins></span></pre></div>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (183705 => 183706)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog        2015-05-02 01:11:28 UTC (rev 183705)
+++ trunk/Source/WebCore/ChangeLog        2015-05-02 01:29:29 UTC (rev 183706)
</span><span class="lines">@@ -1,3 +1,24 @@
</span><ins>+2015-05-01 Andreas Kling <akling@apple.com>
+
+ Reproducible crash removing name attribute from <img> node
+ <https://webkit.org/b/144371>
+ <rdar://problem/17198583>
+
+ Reviewed by Darin Adler.
+
+ The problem here was with HTMLImageElement::getNameAttribute(), which relies
+ on Element::hasName() to avoid slow attribute lookups when the attribute
+ is already known not to be present. Unfortunately hasName() uses an ElementData
+ flag that wasn't getting updated until after the call to parseAttribute().
+
+ This patch fixes the issue by moving the code that updates the hasName() flag
+ before the parseAttribute() virtual dispatch.
+
+ Test: fast/dom/HTMLImageElement/remove-name-id-attribute-from-image.html
+
+ * dom/Element.cpp:
+ (WebCore::Element::attributeChanged):
+
</ins><span class="cx"> 2015-05-01 Eric Carlson <eric.carlson@apple.com>
</span><span class="cx">
</span><span class="cx"> Postpone caption style sheet creation
</span></span></pre></div>
<a id="trunkSourceWebCoredomElementcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/dom/Element.cpp (183705 => 183706)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/dom/Element.cpp        2015-05-02 01:11:28 UTC (rev 183705)
+++ trunk/Source/WebCore/dom/Element.cpp        2015-05-02 01:29:29 UTC (rev 183706)
</span><span class="lines">@@ -1216,37 +1216,41 @@
</span><span class="cx">
</span><span class="cx"> void Element::attributeChanged(const QualifiedName& name, const AtomicString& oldValue, const AtomicString& newValue, AttributeModificationReason)
</span><span class="cx"> {
</span><del>- parseAttribute(name, newValue);
</del><ins>+ bool valueIsSameAsBefore = oldValue == newValue;
</ins><span class="cx">
</span><del>- document().incDOMTreeVersion();
-
- if (oldValue == newValue)
- return;
-
</del><span class="cx"> StyleResolver* styleResolver = document().styleResolverIfExists();
</span><span class="cx"> bool testShouldInvalidateStyle = inRenderedDocument() && styleResolver && styleChangeType() < FullStyleChange;
</span><ins>+
</ins><span class="cx"> bool shouldInvalidateStyle = false;
</span><span class="cx">
</span><del>- if (name == HTMLNames::idAttr) {
- if (!oldValue.isEmpty())
- treeScope().idTargetObserverRegistry().notifyObservers(*oldValue.impl());
- if (!newValue.isEmpty())
- treeScope().idTargetObserverRegistry().notifyObservers(*newValue.impl());
</del><ins>+ if (!valueIsSameAsBefore) {
+ if (name == HTMLNames::idAttr) {
+ if (!oldValue.isEmpty())
+ treeScope().idTargetObserverRegistry().notifyObservers(*oldValue.impl());
+ if (!newValue.isEmpty())
+ treeScope().idTargetObserverRegistry().notifyObservers(*newValue.impl());
</ins><span class="cx">
</span><del>- AtomicString oldId = elementData()->idForStyleResolution();
- AtomicString newId = makeIdForStyleResolution(newValue, document().inQuirksMode());
- if (newId != oldId) {
- elementData()->setIdForStyleResolution(newId);
- shouldInvalidateStyle = testShouldInvalidateStyle && checkNeedsStyleInvalidationForIdChange(oldId, newId, styleResolver);
- }
- } else if (name == classAttr)
- classAttributeChanged(newValue);
- else if (name == HTMLNames::nameAttr)
- elementData()->setHasNameAttribute(!newValue.isNull());
- else if (name == HTMLNames::pseudoAttr)
- shouldInvalidateStyle |= testShouldInvalidateStyle && isInShadowTree();
</del><ins>+ AtomicString oldId = elementData()->idForStyleResolution();
+ AtomicString newId = makeIdForStyleResolution(newValue, document().inQuirksMode());
+ if (newId != oldId) {
+ elementData()->setIdForStyleResolution(newId);
+ shouldInvalidateStyle = testShouldInvalidateStyle && checkNeedsStyleInvalidationForIdChange(oldId, newId, styleResolver);
+ }
+ } else if (name == classAttr)
+ classAttributeChanged(newValue);
+ else if (name == HTMLNames::nameAttr)
+ elementData()->setHasNameAttribute(!newValue.isNull());
+ else if (name == HTMLNames::pseudoAttr)
+ shouldInvalidateStyle |= testShouldInvalidateStyle && isInShadowTree();
+ }
</ins><span class="cx">
</span><ins>+ parseAttribute(name, newValue);
</ins><span class="cx">
</span><ins>+ document().incDOMTreeVersion();
+
+ if (valueIsSameAsBefore)
+ return;
+
</ins><span class="cx"> invalidateNodeListAndCollectionCachesInAncestors(&name, this);
</span><span class="cx">
</span><span class="cx"> // If there is currently no StyleResolver, we can't be sure that this attribute change won't affect style.
</span></span></pre>
</div>
</div>
</body>
</html>