setAttributeNode() does not set the new value to an existing attribute if specified attribute is in…

setAttributeNode() does not set the new value to an existing attribute if specified attribute is in a different case.

setAttributeNode() now performs a case-insensitive search for an existing attribute
and if found, retrieves the index of such an attribute. For setting the value we
call upon setAttributeInternal() to which we pass both the index as well as the name.
The name passed to this method is the same as the one passed to the setAttributeNode() API from the webpage and thus can be in any case.

However, setAttributeInternal() uses this name to get the corresponding existing node.
Since this retrieval is not case-insensitive, the existing node is not found and thus
the new value is not set on the existing node.
We should instead use the passed index and use that to retrieve the existing node.

Note that obtaining the attribute's value using getAttributeNode() would still
return the correct value, i.e. the new one.

Also, this now makes our behavior similar to that of FF and IE.

BUG=279193, 282922
R= esprehn@chromium.org,eseidel@chromium.org,ojan@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157015

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157508

[ojan, 2013-08-28 01:14:18]

Arv, do you mind reviewing this?

https://codereview.chromium.org/22842013/diff/5001/LayoutTests/fast/dom/Eleme...
File LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html
(right):

https://codereview.chromium.org/22842013/diff/5001/LayoutTests/fast/dom/Eleme...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:17:
document.getElementById('result').innerHTML = "background-color specified with
lowercase style attribute was: "+testElementBgColor+" and specified with
uppercase style attribute is: "+testElementBgColorAfterReset;
Instead of doing this hand-rolled checking, can you use js-test-pre.js and
shouldBe instead?

[arv (Not doing code reviews), 2013-08-28 13:43:37]

The C++ looks correct. The test needs to use js-test-pre and I would also like
there to be a test for an XML document where the case matters. You can do
something like:

var doc = document.createDocument(null, 'root', null);
doc.documentElement.setAttribute('x', 'x');

var attr = doc.createAttribute('X');
attr.value = 'X';
doc.documentElement.setAttributeNode(attr);

shouldBe('doc.documentElement.attributes.length', '2');
shouldBeEqualToString('doc.documentElement.getAttribute("x")', 'x');
shouldBeEqualToString('doc.documentElement.getAttribute("X")', 'X');

var attr2 = doc.createAttribute('X');
attr2.value = 'Y';
doc.documentElement.setAttributeNode(attr2);

shouldBe('doc.documentElement.attributes.length', '2');
shouldBeEqualToString('doc.documentElement.getAttribute("x")', 'x');
shouldBeEqualToString('doc.documentElement.getAttribute("X")', 'Y');

and then exactly the same for an HTML document:

var doc = document.createHTMLDocument('');
doc.documentElement.setAttribute('x', 'x');

var attr = doc.createAttribute('X');
attr.value = 'X';
doc.documentElement.setAttributeNode(attr);

shouldBe('doc.documentElement.attributes.length', '1');
shouldBeEqualToString('doc.documentElement.getAttribute("x")', 'X');
shouldBeEqualToString('doc.documentElement.getAttribute("X")', 'X');

var attr2 = doc.createAttribute('X');
attr2.value = 'Y';
doc.documentElement.setAttributeNode(attr2);

shouldBe('doc.documentElement.attributes.length', '1');
shouldBeEqualToString('doc.documentElement.getAttribute("x")', 'Y');
shouldBeEqualToString('doc.documentElement.getAttribute("X")', 'Y');


I just wrote your test for you :-)

[arpitab_, 2013-08-29 06:02:15]

On 2013/08/28 13:43:37, arv wrote:
> The C++ looks correct. The test needs to use js-test-pre and I would also like
> there to be a test for an XML document where the case matters. You can do
> something like:
> 

Hi arv, thanks a lot for the review and for the testcases. :)

I will upload another patch which has a testcase for XML as well.
The reason why I am not using the getAttribute() API is coz it already returns
the correct attribute value.
The testcases using getAttribute() is here:
LayoutTests/fast/dom/Element/setAttributeNode-case-insensitivity.html
This issue is specifically for verifying that the new attribute value is also
applied (set internally).

[arv (Not doing code reviews), 2013-08-29 13:45:21]

https://codereview.chromium.org/22842013/diff/14001/LayoutTests/fast/dom/Elem...
File LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html
(right):

https://codereview.chromium.org/22842013/diff/14001/LayoutTests/fast/dom/Elem...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:20:
shouldBe("getAttributesLength('test')", '2');
Why not?

shouldBe('testElement.attributes.length', '2');

and

shouldBeEqualToString('testElement.style.background', 'green');

Your functions are only used once and do not add anything for readability.

https://codereview.chromium.org/22842013/diff/14001/LayoutTests/fast/dom/Elem...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:24:
isSuccessfullyParsed();
remove

https://codereview.chromium.org/22842013/diff/14001/LayoutTests/fast/dom/Elem...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:28:
<body onload="runTest();">
remove onload and move the script block to after the testElement.

https://codereview.chromium.org/22842013/diff/14001/LayoutTests/fast/dom/Elem...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:29:
<div id="description"></div>
remove

https://codereview.chromium.org/22842013/diff/14001/LayoutTests/fast/dom/Elem...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:31:
<div id="console"></div>
remove

https://codereview.chromium.org/22842013/diff/14001/LayoutTests/fast/dom/Elem...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:32:
</body>
Missing 

<script src="../../js/resources/js-test-post.js"></script>

[arpitab_, 2013-08-29 14:11:00]

https://codereview.chromium.org/22842013/diff/14001/LayoutTests/fast/dom/Elem...
File LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html
(right):

https://codereview.chromium.org/22842013/diff/14001/LayoutTests/fast/dom/Elem...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:20:
shouldBe("getAttributesLength('test')", '2');
On 2013/08/29 13:45:21, arv wrote:
> Why not?
> 
> shouldBe('testElement.attributes.length', '2');
> 
> and
> 
> shouldBeEqualToString('testElement.style.background', 'green');
> 
> Your functions are only used once and do not add anything for readability.

Done.

https://codereview.chromium.org/22842013/diff/14001/LayoutTests/fast/dom/Elem...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:24:
isSuccessfullyParsed();
On 2013/08/29 13:45:21, arv wrote:
> remove

Done.

https://codereview.chromium.org/22842013/diff/14001/LayoutTests/fast/dom/Elem...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:28:
<body onload="runTest();">
On 2013/08/29 13:45:21, arv wrote:
> remove onload and move the script block to after the testElement.

Done.

https://codereview.chromium.org/22842013/diff/14001/LayoutTests/fast/dom/Elem...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:29:
<div id="description"></div>
On 2013/08/29 13:45:21, arv wrote:
> remove

Done.

https://codereview.chromium.org/22842013/diff/14001/LayoutTests/fast/dom/Elem...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:31:
<div id="console"></div>
On 2013/08/29 13:45:21, arv wrote:
> remove

Done.

https://codereview.chromium.org/22842013/diff/14001/LayoutTests/fast/dom/Elem...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:32:
</body>
On 2013/08/29 13:45:21, arv wrote:
> Missing 
> 
> <script src="../../js/resources/js-test-post.js"></script>

Done.

[arv (Not doing code reviews), 2013-08-29 14:18:16]

https://chromiumcodereview.appspot.com/22842013/diff/19001/LayoutTests/fast/d...
File
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute-html.html
(right):

https://chromiumcodereview.appspot.com/22842013/diff/19001/LayoutTests/fast/d...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute-html.html:28:
<body>
This should also have a js-test-post

https://chromiumcodereview.appspot.com/22842013/diff/19001/LayoutTests/fast/d...
File
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute-xml.html
(right):

https://chromiumcodereview.appspot.com/22842013/diff/19001/LayoutTests/fast/d...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute-xml.html:27:
</head>
js-test-post

https://chromiumcodereview.appspot.com/22842013/diff/19001/LayoutTests/fast/d...
File LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html
(right):

https://chromiumcodereview.appspot.com/22842013/diff/19001/LayoutTests/fast/d...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:7:
<div id="test" style="background-color: red;"/>&nbsp;</div>
Invalid HTML.

Instead of using &nbsp; you could set width and height.

https://chromiumcodereview.appspot.com/22842013/diff/19001/LayoutTests/fast/d...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:19:
testElement.style.display = 'none';
If you hide this your description is not accurate. You can no longer manually
very that there is a green element.

[arpitab_, 2013-08-29 14:51:40]

On 2013/08/29 14:18:16, arv wrote:
>
https://chromiumcodereview.appspot.com/22842013/diff/19001/LayoutTests/fast/d...
>
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute-html.html:28:
> <body>
> This should also have a js-test-post
> 
> 
https://chromiumcodereview.appspot.com/22842013/diff/19001/LayoutTests/fast/d...
>
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute-xml.html:27:
> </head>
> js-test-post
> 
Done.
> 
>
https://chromiumcodereview.appspot.com/22842013/diff/19001/LayoutTests/fast/d...
> LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:7:
> <div id="test" style="background-color: red;"/>&nbsp;</div>
> Invalid HTML.
> 
> Instead of using &nbsp; you could set width and height.
> 
Done.
>
https://chromiumcodereview.appspot.com/22842013/diff/19001/LayoutTests/fast/d...
> LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:19:
> testElement.style.display = 'none';
> If you hide this your description is not accurate. You can no longer manually
> very that there is a green element.
Done. Not setting the display to none.

[arv (Not doing code reviews), 2013-08-29 15:46:36]

Sorry for all the nit picking. Hopefully you will get it right this time :-)

https://codereview.chromium.org/22842013/diff/21001/LayoutTests/fast/dom/Elem...
File LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html
(right):

https://codereview.chromium.org/22842013/diff/21001/LayoutTests/fast/dom/Elem...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:10:
<div id="test" style="background-color: red;"/></div>
Still invalid HTML.

<div id="test" style="background-color: red;"></div>

https://codereview.chromium.org/22842013/diff/21001/LayoutTests/fast/dom/Elem...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:19:
shouldBeEqualToString('testElement.attributes.length.toString()', '2');
No need to use a string comparison.

shouldBe('testElement.attributes.length', '2');

https://codereview.chromium.org/22842013/diff/21001/LayoutTests/fast/dom/Elem...
LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:20:
shouldBeEqualToString('testElement.style.background', 'green');
Should be backgroundColor

[arpitab_, 2013-08-30 06:26:48]

On 2013/08/29 15:46:36, arv wrote:
> Sorry for all the nit picking. Hopefully you will get it right this time :-)
> 
On the contrary, thanks for being so patient with the reviews!
Hope I do get it right this time, sorry for the careless errors. :)

>
https://codereview.chromium.org/22842013/diff/21001/LayoutTests/fast/dom/Elem...
> File LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html
> (right):
> 
>
https://codereview.chromium.org/22842013/diff/21001/LayoutTests/fast/dom/Elem...
> LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:10:
> <div id="test" style="background-color: red;"/></div>
> Still invalid HTML.
> 
> <div id="test" style="background-color: red;"></div>
> 
Done.

>
https://codereview.chromium.org/22842013/diff/21001/LayoutTests/fast/dom/Elem...
> LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:19:
> shouldBeEqualToString('testElement.attributes.length.toString()', '2');
> No need to use a string comparison.
> 
> shouldBe('testElement.attributes.length', '2');
> 
Done.

>
https://codereview.chromium.org/22842013/diff/21001/LayoutTests/fast/dom/Elem...
> LayoutTests/fast/dom/Element/setAttributeNode-for-existing-attribute.html:20:
> shouldBeEqualToString('testElement.style.background', 'green');
> Should be backgroundColor
Done.

[arv (Not doing code reviews), 2013-08-30 14:19:08]

LGTM

[commit-bot: I haz the power, 2013-08-30 18:37:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.bah@samsung.com/22842013/29001

[commit-bot: I haz the power, 2013-08-30 20:18:46]

Change committed as 157015

[arpitab_, 2013-09-02 09:59:35]

Reopening issue as it causes regression:
https://code.google.com/p/chromium/issues/detail?id=282922

[arpitab_, 2013-09-02 10:00:58]

Patch reverted with:
https://src.chromium.org/viewvc/chrome?view=rev&revision=157033

[arpitab_, 2013-09-02 10:47:21]

https://code.google.com/p/chromium/issues/detail?id=282922 caused due to garbage
collection. 

Patch-set #7 retrieves the existing attribute node by calling attributeItem()
directly without first calling on ensureUniqueElementData().

This causes erroneous behavior when garbage collection is triggered causing the
elementData to be destroyed and thereby returning an already freed value for the
attribute's name. Its use thereafter causes crashes.

Submitted patch-set #8 for fixing this case.

[arpitab_, 2013-09-06 14:19:53]

Hi arv!

Have uploaded another patch that fixes the regression/crash caused by this
patch.

Instead of a const reference for QualifiedName, using a local copy fixes the
issue.

Could you please kick this in again?

Thanks!

[arv (Not doing code reviews), 2013-09-06 15:40:48]

LGTM

I feel like the test could be a lot cleaner using js-test-pre + js-test-post but
it is no big deal.

[commit-bot: I haz the power, 2013-09-10 05:31:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.bah@samsung.com/22842013/49001

[commit-bot: I haz the power, 2013-09-10 07:01:31]

Change committed as 157508