Closed Bug 1060938 Opened 6 years ago Closed 5 years ago

Element.removeAttributeNode() is wrong implemented

Categories

(Core :: DOM: Core & HTML, defect)

34 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: crimsteam, Assigned: anujagarwal464, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 6 obsolete files)

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.
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."
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.
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
Whiteboard: [lang=c++]
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.2?
Nalin, why would this block anything?
Flags: needinfo?(naling1994)
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)
blocking-b2g: 2.2? → ---
@bz: I would like to work on this bug. Can you give me few pointers.
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).
Attached patch bug1060938.diff (obsolete) — Splinter Review
Assignee: nobody → anujagarwal464
Attachment #8490132 - Flags: feedback?(bzbarsky)
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+
Attached patch bug1060938.diff (obsolete) — Splinter Review
Attachment #8490132 - Attachment is obsolete: true
Attached patch test_bug1060938.diff (obsolete) — Splinter Review
Attachment #8491652 - Flags: feedback?
Attachment #8491652 - Flags: feedback? → feedback?(bzbarsky)
Attachment #8490806 - Flags: feedback?(bzbarsky)
Comment on attachment 8490806 [details] [diff] [review]
bug1060938.diff

r=me
Attachment #8490806 - Flags: review+
Attachment #8490806 - Flags: feedback?(bzbarsky)
Attachment #8490806 - Flags: feedback+
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+
Attached patch test_bug1060938.diff (obsolete) — Splinter Review
Try: https://tbpl.mozilla.org/?tree=Try&rev=6b991a3d652c
Attachment #8491652 - Attachment is obsolete: true
Attachment #8492252 - Flags: review?(bzbarsky)
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+
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
Flags: needinfo?(bzbarsky)
Or fix the code, possibly.  The failures in test_bug469304.html indicate removeAttributeNode is just not working right?
Flags: needinfo?(bzbarsky)
Blocks: 1070015
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
Depends on: 1075702
Attached patch test_bug1060938.diff (obsolete) — Splinter Review
@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 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+
Attached patch test_bug1060938.diff (obsolete) — Splinter Review
Attachment #8550708 - Attachment is obsolete: true
Attachment #8552537 - Flags: review?(bzbarsky)
Comment on attachment 8552537 [details] [diff] [review]
test_bug1060938.diff

r=me
Attachment #8552537 - Flags: review?(bzbarsky) → review+
Comment on attachment 8553085 [details] [diff] [review]
test_bug1060938.diff

r=me
Attachment #8553085 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/23335411e134
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.