Closed
Bug 1060938
Opened 10 years ago
Closed 10 years ago
Element.removeAttributeNode() is wrong implemented
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: crimsteam, Assigned: anujagarwal464, Mentored)
References
Details
(Whiteboard: [lang=c++])
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446
Steps to reproduce:
Firefox has wrong behaviour for Element.removeAttributeNode(). See algo from DOM:
http://dom.spec.whatwg.org/#dom-element-removeattributenode
And small test:
<!DOCTYPE html>
<body>
<p>Some P with Attrs setting in script:</p>
<ul>
<li>p.setAttributeNS("www.test1.com", "ID", "Test1");</li>
<li>p.setAttributeNS("www.test2.com", "Class", "Test2");</li>
<li>p.setAttribute("id", "Identyfikator");</li>
<li>p.className = "Klasa1 Klasa2 Klasa3";</li>
</ul>
<p>Press button to remove first Attr from P.</p>
<input type="button" value="removeAttributeNode(attr[0] from P)" onclick="removeAttr()">
<p id="info"></p>
<script>
p = document.getElementsByTagName("p")[0];
p.setAttributeNS("www.test1.com", "ID", "Test1");
p.setAttributeNS("www.test2.com", "Class", "Test2");
p.setAttribute("id", "Identyfikator");
p.className = "Klasa1 Klasa2 Klasa3";
var info = document.getElementById("info");
allAttr = p.attributes;
function removeAttr(){
try{
info.innerHTML = "allAttr.length (before remove): " + allAttr.length + "<br>";
var remove_attr = allAttr[0];
var return_attr = p.removeAttributeNode(remove_attr);
var compare = remove_attr == return_attr;
info.innerHTML += "allAttr.length (after remove): " + allAttr.length
+ "<br>" + "remove_attr.name: " + remove_attr.name
+ "<br>" + "return_attr.name: " + return_attr.name
+ "<br>" + "remove_attr == return_attr: " + compare;
}
catch(e){
info.innerHTML = "Exception: " + e;
}
}
</script>
</body>
When we try remove first attribute (.name "ID") Firefox delete third attribute (.name "id") and another click on button throw ("Exception: NotFoundError: Node was not found"). The same problem is when wy try pass to removeAttributeNode() attr object, which belongs to the different element, Firefox doesn't throw "NotFoundError" exception.
Chrome and IE don't have this problem, we can remove all attribute and can't pass attr from other (or without) element.
Comment 1•10 years ago
|
||
From https://developer.mozilla.org/en-US/docs/Web/API/Node.attributes : ".attributes is a collection of all attribute nodes registered to the specified node. It is a NamedNodeMap,not an Array, so it has no Array methods and the Attr nodes' indexes may differ among browsers."
Reporter | ||
Comment 2•10 years ago
|
||
DOM define how indexing looks like for NameNodeMap (and order for attrs) and Firefox doesn't have problem in this area. Backing to above example, looks like removeAttributeNode() has problem when we pass attr which name has at least one uppercase letter:
- passing p.setAttributeNS("", "aaa", "Test1"); << works
- passing p.setAttributeNS("", "aaA", "Test1"); << not works
- passing p.setAttributeNS("", "AAA", "Test1"); << not works
- passing p.setAttributeNS("", "b:aaa", "Test1"); << works
- passing p.setAttributeNS("", "b:aAa", "Test1"); << not works
But still this not explain this: pass to removeAttributeNode() attr object, which belongs to the different element, Firefox doesn't throw "NotFoundError" exception.
Around all the commands associated with the attributes I detected that only this method has some problem (comparing via new DOM). Not big problem when I see how Chrome and (especially) IE works in this area.
Comment 3•10 years ago
|
||
RemoveAttributeNode calls RemoveNamedItem on the .attributes, passing the .nodeName. This ends up looking for an existing attr name nodeinfo, etc.
We could probably simplify this to passing in the relevant nodeinfo from the provided Attr object instead of looking it up in the element.
And yes, probably checking the owner element of the Attr.
Mentor: bzbarsky
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•10 years ago
|
Whiteboard: [lang=c++]
Comment 6•10 years ago
|
||
I am really sorry
But it is my first time working on bugzilla,so i asked that by mistake.
I apologise if there is any inconvenience caused by it.
Flags: needinfo?(naling1994)
Updated•10 years ago
|
blocking-b2g: 2.2? → ---
Assignee | ||
Comment 7•10 years ago
|
||
@bz: I would like to work on this bug. Can you give me few pointers.
Comment 8•10 years ago
|
||
Anuj Agarwal, I assume you've pulled the Firefox source and compiled it?
The next pointer is comment 3. The relevant code is in content/base/src/Element.cpp and content/base/src/nsDOMAttributeMap.cpp.
The simplest thing to do is to add a variant of RemoveNamedItem that takes a nodeinfo and call it from both the existing RemoveNamedItem (after looking up the nodeinfo, as now) and from removeAttributeNode (using the nodeinfo of the passed-in attribute).
Assignee | ||
Comment 9•10 years ago
|
||
Assignee: nobody → anujagarwal464
Attachment #8490132 -
Flags: feedback?(bzbarsky)
Comment 10•10 years ago
|
||
Comment on attachment 8490132 [details] [diff] [review]
bug1060938.diff
Yep, this is a good start! You probably want to name the argument "aAttrNodeInfo" instead of "attrNi", and add a declaration for the new RemoveNamedItem overload to the header.
And of course add tests.
Attachment #8490132 -
Flags: feedback?(bzbarsky) → feedback+
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8490132 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8491652 -
Flags: feedback?
Assignee | ||
Updated•10 years ago
|
Attachment #8491652 -
Flags: feedback? → feedback?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8490806 -
Flags: feedback?(bzbarsky)
Comment 13•10 years ago
|
||
Attachment #8490806 -
Flags: review+
Attachment #8490806 -
Flags: feedback?(bzbarsky)
Attachment #8490806 -
Flags: feedback+
Comment 14•10 years ago
|
||
Comment on attachment 8491652 [details] [diff] [review]
test_bug1060938.diff
>+ SimpleTest.waitForExplicitFinish();
You don't need this, or the finish() call, since your test never does anything async.
>+ function removeAttr(){
this should probably just be:
var removed_attribute = allAttributes[0];
is(removed_attribute, parent.removeAttributeNode(removed_attribute),
"Returned attribute and remove attribute should be same.");
and return void.
And the fifth test (that an exception is thrown once allAttributes[0] is undefined) is not really needed.
If you wanted to, you could beef this up a bit by checking that the state of the remaining attributes is correct after each removeAttr() call.
Also, please file a followup bug on the "passing in some other element's Attr doesn't throw" bit from comment 0.
Attachment #8491652 -
Flags: feedback?(bzbarsky) → feedback+
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8491652 -
Attachment is obsolete: true
Attachment #8492252 -
Flags: review?(bzbarsky)
Comment 16•10 years ago
|
||
Comment on attachment 8492252 [details] [diff] [review]
test_bug1060938.diff
r=me
Do you have try push access, or do you need someone to push this to try?
Attachment #8492252 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 17•10 years ago
|
||
Yeah, I already pushed it to try. From results it seems I need to fix some existing tests.
Try: https://tbpl.mozilla.org/?tree=Try&rev=6b991a3d652c
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bzbarsky)
Comment 18•10 years ago
|
||
Or fix the code, possibly. The failures in test_bug469304.html indicate removeAttributeNode is just not working right?
Updated•10 years ago
|
Flags: needinfo?(bzbarsky)
Comment 19•10 years ago
|
||
OK, so I think what's going on with that test is that it's showing a bug in setAttributeNode.
In particular, after these two lines:
document.body.setAttributeNode(a1);
document.body.setAttributeNode(a2);
the element has a single attribute named "aa", not "AA".
This part of the test:
var s = "";
for (var i = 0; i < document.body.attributes.length; ++i) {
s += document.body.attributes[i].nodeName + "=" +
document.body.attributes[i].nodeValue;
}
is(s, "AA=UPPERCASE", "Wrong attribute!");
is not catching this, because the hashtable in the nsDOMAttributeMap is storing an Attr with localName "AA", keyed by localName "aa" (which seems very very bogus).
So when we do getAttributeNode("AA"), that finds the existing name "aa" and looks up the Attr by that name, which it finds. But when we do the removeAttributeNode() with that Attr, we fail with the patch in this bug, because there is no attribute named "AA".
You can see the inconsistency pretty trivially because document.body.attributes[0].localName is "AA", but getAttributeNS needs to be passed "aa", not "AA" to find the value.
This is somewhat similar to the issues described in bug 1061234, actually, but for the non-NS-aware codepath in nsDOMAttributeMap::SetNamedItemInternal.
We should fix this setAttributeNode bug before we land this patch, I guess. Anuj, are you willing to give that a shot? Simple testcase for that bug:
var a1 = document.createAttribute("aa");
a1.nodeValue = "lowercase";
var a2 = document.createAttribute("AA");
a2.nodeValue = "UPPERCASE";
document.documentElement.setAttributeNode(a1);
document.documentElement.setAttributeNode(a2);
alert(document.documentElement.getAttributeNS("", "aa")); // should alert null per spec
alert(document.documentElement.getAttributeNS("", "AA")); // should alert UPPERCASE
Assignee | ||
Comment 20•10 years ago
|
||
@bz: Element.removeAttributeNode() is showing right behavior after patch for Bug 1075702 landed in.
Try build with tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0b1cba859f1
Attachment #8490806 -
Attachment is obsolete: true
Attachment #8492252 -
Attachment is obsolete: true
Attachment #8550708 -
Flags: feedback?(bzbarsky)
Comment 21•10 years ago
|
||
Comment on attachment 8550708 [details] [diff] [review]
test_bug1060938.diff
Would be good to have the test message indicate which of the removeAttr calls is involved, in case it ever fails.
Apart from that, looks great.
Attachment #8550708 -
Flags: feedback?(bzbarsky) → feedback+
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8550708 -
Attachment is obsolete: true
Attachment #8552537 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 23•10 years ago
|
||
Comment 24•10 years ago
|
||
Comment on attachment 8552537 [details] [diff] [review]
test_bug1060938.diff
r=me
Attachment #8552537 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8552537 -
Attachment is obsolete: true
Attachment #8553085 -
Flags: review?(bzbarsky)
Comment 26•10 years ago
|
||
Comment on attachment 8553085 [details] [diff] [review]
test_bug1060938.diff
r=me
Attachment #8553085 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 27•10 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 28•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•