Closed Bug 1075702 Opened 10 years ago Closed 9 years ago

Element.setAttributeNode() is wrong implemented

Categories

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

34 Branch
x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: anujagarwal464, Assigned: anujagarwal464, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(2 files, 24 obsolete files)

14.74 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.62 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
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
Attached patch bug1075702.diff (obsolete) — Splinter Review
Attachment #8498906 - Flags: feedback?(bzbarsky)
Comment on attachment 8498906 [details] [diff] [review]
bug1075702.diff

>+  nsRefPtr<Attr> attribute = GetAttribute(oldNi, true);

Maybe call this oldAttr?

>+  if(attribute == aAttr) {

Space before "(", please.

Also, does this actually compile?  I would have thought you need "attribute == &aAttr".

>+  if(attribute) {

Also, space before "(".

>+    attr = RemoveAttribute(oldNi);

This should return the same thing as oldAttr, right?  Please assert that?

>+  ni = aAttr.NodeInfo();

Move the declaration of "ni" down here too?

Does this fix the testcase?  Please add tests!
Attachment #8498906 - Flags: feedback?(bzbarsky) → feedback+
Attached patch bug1075702.diff (obsolete) — Splinter Review
When I apply this patch and try to setAttributeNode, tab crashes. Can you check that?
Attachment #8498906 - Attachment is obsolete: true
Attachment #8499093 - Flags: feedback?(bzbarsky)
Comment on attachment 8499093 [details] [diff] [review]
bug1075702.diff

>+  nsRefPtr<Attr> attribute = GetAttribute(oldAttr, true);

No, I meant naming the nsRefPtr<Attr> oldAttr.

In any case, this is presumably crashing if GetExistingAttrNameFromQName() returned null.  Which it can do.  If that happens, that means there is no oldAttr.

So all this stuff about GetAttribute and RemoveAttribute() can be conditioned on the nodeinfo being non-null.
Attachment #8499093 - Flags: feedback?(bzbarsky) → feedback+
Attached patch bug1075702.diff (obsolete) — Splinter Review
For testcase mentioned in description:

//Patch throws lowercase instead of null
alert(document.documentElement.getAttributeNS("", "aa"));

//Patch throws uppercase
alert(document.documentElement.getAttributeNS("", "AA"));
Attachment #8499093 - Attachment is obsolete: true
Attachment #8499555 - Flags: feedback?(bzbarsky)
Comment on attachment 8499555 [details] [diff] [review]
bug1075702.diff

>//Patch throws lowercase instead of null

Er, why?  Are you not removing the lowercase version of the attribute?

Oh, I see.  RemoveAttribute() doesn't remove it from the _element_.  You need to UnsetAttr on mContent as well.

Perhaps create a RemoveNamedItem version that takes a nodeinfo and call it from here, RemoveNamedItem, and RemoveNamedItemNS?
Attachment #8499555 - Flags: feedback?(bzbarsky) → feedback+
Attached patch bug1075702.diff (obsolete) — Splinter Review
Now I'm calling UnsetAttr on mContent as well. But still it's showing same behavior as comment 5.
Attachment #8499555 - Attachment is obsolete: true
Attachment #8499777 - Flags: feedback?(bzbarsky)
Hmm.

Do aNodeInfo and attrNi actually match?

You really need to get a C++ debugger working here and figure out what is being passed to the RemoveAttr call and whether it matches what you expect to get passed.
Attachment #8499777 - Flags: feedback?(bzbarsky)
Attached patch bug1075702.diff (obsolete) — Splinter Review
@bz: Thanks! Now it works! :D
Attachment #8499777 - Attachment is obsolete: true
Attachment #8505628 - Flags: review?(bzbarsky)
Comment on attachment 8505628 [details] [diff] [review]
bug1075702.diff

You don't need the "mozilla::dom" bits, by the way, given the "uses" declarations at the top of the file.

Please add a test?

r=me with that.  Thank you!
Attachment #8505628 - Flags: review?(bzbarsky) → review+
Attached patch bug1075702.diff r=bz (obsolete) — Splinter Review
Attachment #8505628 - Attachment is obsolete: true
Comment on attachment 8506276 [details] [diff] [review]
test_bug1075702.diff

Did you run this test?  Does it actually pass?

There is no "NULL" in JS.  There is "null".

Apart from that, this looks reasonable.
Attachment #8506276 - Flags: feedback?(bzbarsky) → feedback+
Attached patch test_bug1075702.diff (obsolete) — Splinter Review
Attachment #8506276 - Attachment is obsolete: true
Attachment #8507038 - Flags: review?(bzbarsky)
Attached patch bug1075702-part2.diff (obsolete) — Splinter Review
Attachment #8510154 - Flags: feedback?(bzbarsky)
Comment on attachment 8510154 [details] [diff] [review]
bug1075702-part2.diff

>+  is(document.body.getAttribute('aa'), null, "Wrong value (1)");

Maybe change the string to indicate why null is expected?  Because the property that's set has the localName "AA" and not "aa".

Also add getAttributeNS tests here as well?

>+  is(s, "AA=", "Wrong attribute!");

This part doesn't look right to me.  It should be "AA=UPPERCASE", I'd think.  Why isn't it?

>-  document.body.getAttributeNode("AA").nodeValue = "FOO";
>-  is(document.body.getAttribute("AA"), "FOO", "Wrong value!");

This test should stay, but replace getAttribute with getAttributeNS, no?

> ok(!document.body.hasAttribute("AA"), "Should not have attribute!");
> ok(!document.body.hasAttribute("aa"), "Should not have attribute!");

These tests are meaningless, since getAttribute returned null even when the attribute was there.  Add tests here using getAttributeNS as well, please.
Attachment #8510154 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 8507038 [details] [diff] [review]
test_bug1075702.diff

r=me
Attachment #8507038 - Flags: review?(bzbarsky) → review+
Attached patch bug1075702-part3.diff (obsolete) — Splinter Review
Attached patch bug1075702-part1.diff (obsolete) — Splinter Review
Attachment #8506274 - Attachment is obsolete: true
Attachment #8507038 - Attachment is obsolete: true
Attachment #8510154 - Attachment is obsolete: true
Attachment #8513638 - Attachment is obsolete: true
Attached patch bug1075702-part2.diff (obsolete) — Splinter Review
Attached patch bug1075702-part3.diff (obsolete) — Splinter Review
Attached patch test_bug1075702.diff (obsolete) — Splinter Review
Attached patch bug1075702.diff (obsolete) — Splinter Review
Attachment #8526044 - Attachment is obsolete: true
Attachment #8526045 - Attachment is obsolete: true
Attachment #8526046 - Attachment is obsolete: true
Attachment #8526047 - Attachment is obsolete: true
Attachment #8526048 - Attachment is obsolete: true
Attachment #8532765 - Flags: review?(bzbarsky)
Attached patch bug1075702-part2.diff (obsolete) — Splinter Review
Attachment #8532766 - Flags: review?(bzbarsky)
Attached patch bug1075702-part3.diff (obsolete) — Splinter Review
Attachment #8532767 - Flags: review?(bzbarsky)
Attached patch test_bug1075702.diff (obsolete) — Splinter Review
Attachment #8532768 - Flags: review?(bzbarsky)
Attached patch test_fix.diff (obsolete) — Splinter Review
Attachment #8532769 - Flags: review?(bzbarsky)
Comment on attachment 8532765 [details] [diff] [review]
bug1075702.diff

>+      if (attrNS == aAttr.NodeInfo()->NamespaceID() &&
>+          aAttr.NodeInfo()->Equals(nameAtom)) {

  if (aAttr.NodeInfo()->Equals(nameAtom, attrNS)) {

and probably add a comment that we're purposefully ignoring the prefix.

>+          GetNodeInfo(nameAtom, name->GetPrefix(), aAttr.NodeInfo()->NamespaceID(),

I'd prefer you use attrNS instead of aAttr.NodeInfo()->NamespaceID() for the third arg here, just so we're consistent and using |name| for everything.

>+      NS_ASSERTION(attr->NodeInfo()->NameAndNamespaceEquals(oldNi), "RemoveNamedItem() called, attr->NodeInfo() should be equal to oldNi!");

Please wrap to 80 columns.

>+nsDOMAttributeMap::RemoveNamedItem(NodeInfo* aNodeInfo)

This function doesn't look quite right to me: it'll fire an incorrect mutation event from inside the UnsetAttr, because it has already removed the Attr, no?

Seems to me like this should be doing the GetAttribute() bit and we should still be calling it from nsDOMAttributeMap::RemoveNamedItemNS.

Please add a test for this mutation event case.

Also, this needs a test (that fails without these patches) for the incorrect mutation event we fire right now on trunk with nsDOMAttributeMap::RemoveNamedItemNS.

r=me with those fixed.
Attachment #8532765 - Flags: review?(bzbarsky) → review+
Comment on attachment 8532766 [details] [diff] [review]
bug1075702-part2.diff

The checkin comment here should mention "on Attr nodes" or something.

But this needs to be folded with part 1 to not have an intermediate build that fails tests, right?

r=me with that done.
Attachment #8532766 - Flags: review?(bzbarsky) → review+
Comment on attachment 8532767 [details] [diff] [review]
bug1075702-part3.diff

r=me, but again need to fold this into part 1.
Attachment #8532767 - Flags: review?(bzbarsky) → review+
Comment on attachment 8532768 [details] [diff] [review]
test_bug1075702.diff

r=me
Attachment #8532768 - Flags: review?(bzbarsky) → review+
Comment on attachment 8532769 [details] [diff] [review]
test_fix.diff

>+  is(document.body.getAttribute('aa'), null, "property has the localName AA and not aa.");
>+  is(document.body.getAttribute('AA'), null, "property has the localName AA and not aa.");

Would be nice to have different description things there.  Also, "attribute" instead of "property".

>+  is(document.body.getAttributeNS("", "aa"), null, "Should be NULL!");

And here s/NULL/null/, or better yet "Attribute should have localName AA".  ;)

>+  is(document.body.getAttributeNS("", "aa"), null, "Should be NULL!");

Again, "null".  And again, unique descriptive strings preferred.

This also needs to be folded into part 1.  r=me with those bits fixed.
Attachment #8532769 - Flags: review?(bzbarsky) → review+
Attached patch bug1075702.diff (obsolete) — Splinter Review
Attachment #8532765 - Attachment is obsolete: true
Attachment #8532766 - Attachment is obsolete: true
Attachment #8532767 - Attachment is obsolete: true
Attachment #8532768 - Attachment is obsolete: true
Attachment #8532769 - Attachment is obsolete: true
Attachment #8548201 - Flags: feedback?(bzbarsky)
Comment on attachment 8548201 [details] [diff] [review]
bug1075702.diff

This part of comment 31 wasn't addressed and should be:

>  if (aAttr.NodeInfo()->Equals(nameAtom, attrNS)) {

And you still need to add the mutation event tests for removeNamedItem/removeNamedItemNS.  The actual behavior looks good in this patch.

The rest looks good, thanks!
Attachment #8548201 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 8548202 [details] [diff] [review]
test_bug1075702.diff

Looks great.
Attachment #8548202 - Flags: feedback?(bzbarsky) → feedback+
Attached patch bug1075702.diff (obsolete) — Splinter Review
Attachment #8548201 - Attachment is obsolete: true
Attachment #8548202 - Attachment is obsolete: true
Attachment #8548820 - Flags: review?(bzbarsky)
Comment on attachment 8548820 [details] [diff] [review]
bug1075702.diff

This didn't address the review comment from comment 38.
Attachment #8548820 - Flags: review?(bzbarsky) → review-
Comment on attachment 8548821 [details] [diff] [review]
test_bug1075702.diff

>+    ok(removedNodeAccordingToEvent == removedNodeAccordingToRemoveNamedItemNS, "Node removed according to event is not same as node removed by removeNamedItemNS.");

  ise(removedNodeAccordingToEvent, removedNodeAccordingToRemoveNamedItemNS, "...")

instead of ok().

Same in runTest2().

Also, runTest1() and runTest2() need better names.

r=me with that fixed.
Attachment #8548821 - Flags: review?(bzbarsky) → review+
Attached patch bug1075702.diffSplinter Review
Attachment #8548820 - Attachment is obsolete: true
Attachment #8548821 - Attachment is obsolete: true
Attachment #8549599 - Flags: review?(bzbarsky)
Attachment #8549600 - Flags: review?(bzbarsky)
Comment on attachment 8549600 [details] [diff] [review]
test_bug1075702.diff

r=me
Attachment #8549600 - Flags: review?(bzbarsky) → review+
Comment on attachment 8549599 [details] [diff] [review]
bug1075702.diff

r=me
Attachment #8549599 - Flags: review?(bzbarsky) → review+
Try run's looking pretty good!
Keywords: checkin-needed
Depends on: 1165851
Depends on: 1167067
Depends on: 1169384
Depends on: 1169645
You need to log in before you can comment on or make changes to this bug.