Element.setAttributeNode() is wrong implemented

RESOLVED FIXED in mozilla38

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: anujagarwal464, Assigned: anujagarwal464, Mentored)

Tracking

34 Branch
mozilla38
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++])

Attachments

(2 attachments, 24 obsolete attachments)

14.74 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.62 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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 1

4 years ago
Created attachment 8498906 [details] [diff] [review]
bug1075702.diff
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+
(Assignee)

Comment 3

4 years ago
Created attachment 8499093 [details] [diff] [review]
bug1075702.diff

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+
(Assignee)

Comment 5

4 years ago
Created attachment 8499555 [details] [diff] [review]
bug1075702.diff

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+
(Assignee)

Comment 7

4 years ago
Created attachment 8499777 [details] [diff] [review]
bug1075702.diff

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.
(Assignee)

Comment 9

4 years ago
Created attachment 8505628 [details] [diff] [review]
bug1075702.diff

@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+
(Assignee)

Comment 11

4 years ago
Created attachment 8506274 [details] [diff] [review]
bug1075702.diff r=bz
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+
(Assignee)

Comment 14

4 years ago
Created attachment 8507038 [details] [diff] [review]
test_bug1075702.diff
Attachment #8506276 - Attachment is obsolete: true
Attachment #8507038 - Flags: review?(bzbarsky)
(Assignee)

Comment 16

4 years ago
Created attachment 8510154 [details] [diff] [review]
bug1075702-part2.diff
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+
(Assignee)

Comment 20

4 years ago
Created attachment 8526044 [details] [diff] [review]
bug1075702-part1.diff
Attachment #8506274 - Attachment is obsolete: true
Attachment #8507038 - Attachment is obsolete: true
Attachment #8510154 - Attachment is obsolete: true
Attachment #8513638 - Attachment is obsolete: true
(Assignee)

Comment 23

4 years ago
Created attachment 8526047 [details] [diff] [review]
test_bug1075702.diff
(Assignee)

Comment 25

4 years ago
Created attachment 8532765 [details] [diff] [review]
bug1075702.diff
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)
(Assignee)

Comment 26

4 years ago
Created attachment 8532766 [details] [diff] [review]
bug1075702-part2.diff
Attachment #8532766 - Flags: review?(bzbarsky)
(Assignee)

Comment 27

4 years ago
Created attachment 8532767 [details] [diff] [review]
bug1075702-part3.diff
Attachment #8532767 - Flags: review?(bzbarsky)
(Assignee)

Comment 28

4 years ago
Created attachment 8532768 [details] [diff] [review]
test_bug1075702.diff
Attachment #8532768 - Flags: review?(bzbarsky)
(Assignee)

Comment 29

4 years ago
Created attachment 8532769 [details] [diff] [review]
test_fix.diff
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+
(Assignee)

Comment 36

4 years ago
Created attachment 8548201 [details] [diff] [review]
bug1075702.diff
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+
(Assignee)

Comment 40

4 years ago
Created attachment 8548820 [details] [diff] [review]
bug1075702.diff
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+
(Assignee)

Comment 44

4 years ago
Created attachment 8549599 [details] [diff] [review]
bug1075702.diff
Attachment #8548820 - Attachment is obsolete: true
Attachment #8548821 - Attachment is obsolete: true
Attachment #8549599 - Flags: review?(bzbarsky)
(Assignee)

Comment 45

4 years ago
Created attachment 8549600 [details] [diff] [review]
test_bug1075702.diff
Attachment #8549600 - Flags: review?(bzbarsky)
Comment on attachment 8549600 [details] [diff] [review]
test_bug1075702.diff

r=me
Attachment #8549600 - Flags: review?(bzbarsky) → review+
Try run's looking pretty good!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/acc9dcd07546
https://hg.mozilla.org/mozilla-central/rev/bafcd4598f5d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38

Updated

3 years ago
Depends on: 1165851

Updated

3 years ago
Depends on: 1167067
You need to log in before you can comment on or make changes to this bug.