235512 - Fix up nsDOMAttributeMap

Status: RESOLVED FIXED

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The nsDOMAttributeMap is currently somewhat buggy. The biggest problem is that
it always creates a new attributenode every time GetNamedItem is called. This
means that:

elem.attributes.getNamedItem("id") == elem.attributes.getNamedItem("id")
and
myElement.attributes.setNamedItem(myAttr) == myAttr

both return false when they shouldn't.

There probably are other bugs in there too. Feel free to comment if you know of
any more problems.

Comment 1

[Nicholas Allen]

Is having these canonical attrs a requirement for a sensibly working isEqualNode?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No, that shouldn't be affected by this bug.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Allan: Peterv told me you might be interested in fixing this one and needed some
info to get you started.

So what's needed is a hash that keeps references to all nsDOMAttributes returned
from the map. The hash is keyed on a localname (nsIAtom) and a namespace
(PRInt32). You could probably use an nsInterfaceHashtable<KeyClass, nsIDOMAttr>
where KeyClass is an

struct AttrName
{
  PRInt32 namespaceID;
  nsCOMPtr<nsIAtom> localName;
};

So whenever an nsDOMAttribute is created in the current code we need to first
look in the hash for an existing one, and if none exists create one and stick it
in the hash. You also need to stick a node in the hash in SetNamedItem(NS).

You also need to make nsDOMAttribute call it's owner map whenever it's inserted
into another map so that the first map can remove it from its hash. There are
probably other places where the attribute needs to call the map and tell it to
remove the attribute from the hash too.
The basic idea is that we want to do changes in the map lazily since we don't
want to subscribe to notifications of all attribute changes since that would
slow down performance.
So for example in nsDOMAttribute::GetOwnerElement you should call
mContent->HasAttr to make sure that the attribute is still set in the element
and if it isn't you should drop the mContent reference and remove the attribute
from the hash. Same thing in GetValue.

Note that the attribute needs to hold a week reference to the map since the map
holds an owning reference to the attribute. Hmm.. or you could just get the
reference to the map through mContent...


I hope that makes some sense? :)

Oh, and once you've done this you can make IsSameNode do a simple pointercompare
rather then what it does now.



Other things that should be done, but doesn't need to be done here now is to
reduce the codeduplication in nsDOMAttributeMap, seems like there's a good deal
of it. You should probably consider a few utility functions that return an
attributenode given a atom+nsid for example, it'd be usefull in a lot of places
i'm sure.

We might also want to deal with elements being moved between documents. Then we
chould check if the element has an nsDOMAttributeMap and if so tell it to change
the mNodeInfos in all its attributes to the new document.

Ugh, and RemoveNamedItem(NS) needs to call SetContent(nsnull) on the returned node.

Comment 4

[Allan Beaufour]

(In reply to comment #3)
> I hope that makes some sense? :)

It does! Thanks for the info. Jonas, I'll start looking at it this week.

Comment 5

[Allan Beaufour]

Attachment: Testcase

Here's a testcase. It might not be 100% correct... could somebody verify it?

Comment 6

[Allan Beaufour]

Attachment: Patch

Here's a go at what you suggested Jonas. The hashmap is maybe a bit overkill?

Comment 7

[Allan Beaufour]

Attachment: Revised testcase

Here's a revised version of the testcase that both works in IE (mostly by
disabling checks....) and Mozilla. IE does not agree with my assertions...

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 177478 [details] [diff] [review]
Patch

>+  if (!mAttributeCache.Get(attr, &domAttr)) {
>+    domAttr = new nsDOMAttribute(aContent, aNodeInfo, aValue, this);
>+    if (domAttr && !mAttributeCache.Put(attr, domAttr))
>+      delete domAttr;
Add domAttr = nsnull;


>   nsAttributeChildList* mChildList;
>+  nsWeakPtr<nsDOMAttributeMap> mAttributeMap;
> };
Is the mAttributeMap needed? I think you could get it from
the mContent.


>+    } else {
>+      if (mAttributeMap) {
>+        mValue = EmptyString();
SetDOMStringToNull(mValue)


What should be the value of the attribute node in this case:
aElement.setAttribute('foo', 'bar1');
var attr = aElement.getAttributeNode('foo');
aElement.setAttribute('foo', 'bar2');
aElement.removeAttribute('foo');
Now which one is right?
  attr.value == "bar1"
  attr.value == "bar2"
  attr.value == null

I'd say attr.value == "bar2";
And in that case I think *::SetAttr(...) or rather *::UnsetAttr() 
should update the mValue of the nsDOMAttribute.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> What should be the value of the attribute node in this case:
> ...

Yes, this is a tricky case. I do agree that the logical value would be "bar2".
However, I'm not sure we can do that without getting a performance hit. I'm not
too keep on hitting the attrmap-hash on every attribute-set.

Though if we do it on unset it might not be too bad since attributes are rarly
removed.

Sorry, havn't had time to look properly at the patch yet, but on a quick
scanthrough it looked about right. I'm surpriced you need so much code to get
the hash working though :(

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, and if you do need the mAttributeMap pointer, don't make it an nsWeakRef.
Instead use a normal |nsDOMAttributeMap* mAttributeMap|. You just need to call
into the attribute to have it nulled out in the nsDOMAttributeMap dtor (add a
function to nsIAttribute).

Comment 11

[Allan Beaufour]

(In reply to comment #8)

Thanks for the comments.

> >   nsAttributeChildList* mChildList;
> >+  nsWeakPtr<nsDOMAttributeMap> mAttributeMap;
> > };
> Is the mAttributeMap needed? I think you could get it from
> the mContent.

Then I would either need to expose it/the function (through what interface
then?) or do an ugly pointer conversion?

> What should be the value of the attribute node in this case:
> [...]

I'm hoping somebody will shed some light on that one....

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > Is the mAttributeMap needed? I think you could get it from
> > the mContent.
> 
> Then I would either need to expose it/the function (through what interface
> then?) or do an ugly pointer conversion?

You could either add |nsDOMAttributeMap* nsIContent::GetAttrMap()| or keep a
'redundant' pointer in nsDOMAttribute. Neither is a really nice solution but i'd
probably go with the latter.

Actually, what you could do is to remove mContent instead and get to that
through the map...

> > What should be the value of the attribute node in this case:
> > [...]
> 
> I'm hoping somebody will shed some light on that one....

Feel free to leave this issue to a separate bug. That way you won't have to
worry about perfhits from that change either.


What you do need to do though is to deal with SetNamedItem(NS). There you need
to insert the passed node into the hash as well as make sure it gets removed
from its previous hash if it had one. I.e. you should actually unset the
attribute from its previous ownerElement.

Comment 13

[Johnny Stenback (:jst)]

(In reply to comment #8)
> I'd say attr.value == "bar2";

FWIW, I agree that this is the desired behaviour, but I haven't had a chance to
think about how to best implement that yet...

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If we in UnsetAttr call into the attrmap and tell it the attribute is going away
it should be doable without a too big perf hit. At the most it'll be a
hashlookup on every UnsetAttr, and I don't think we call that that often. And
most of the time there won't be an attrmap so then it's practically free.

Comment 15

[Allan Beaufour]

(In reply to comment #12)
> What you do need to do though is to deal with SetNamedItem(NS). There you need
> to insert the passed node into the hash as well as make sure it gets removed
> from its previous hash if it had one. I.e. you should actually unset the
> attribute from its previous ownerElement.

Doesn't that contradict the current code, that only allows you to insert
non-owned attributes?
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDOMAttributeMap.cpp#131

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

setAttributeNode / setNamedItem:
"INUSE_ATTRIBUTE_ERR: Raised if newAttr is already an attribute of another
Element object. The DOM user must explicitly clone Attr nodes to re-use them in
other elements."

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, i had forgotten attrs behave that way.

Allan, so do you want a review of the patch as is, or are you going to make more
changes to it?

Comment 18

[Allan Beaufour]

Attachment: Testcase v3

Simplified the testcase, and made it match the comments on this bug.

Comment 19

[Allan Beaufour]

Attachment: Patch v2

Includes fixes for above comments, etc., except smaug's test.

Comment 20

[Allan Beaufour]

Attachment: Patch v3

Oops, forgot patch for nsIAttribute.h in v2 patch.

Comment 21

[Allan Beaufour]

(In reply to comment #20)
> Created an attachment (id=177734) [edit]
> Patch v3
> 
> Oops, forgot patch for nsIAttribute.h in v2 patch.

BTW. It's on purpose that I have not touched the remove-functions, as they
should probably be fixed through UnsetAttr().

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 177734 [details] [diff] [review]
Patch v3

I'm not super exited about the attributes holding on to both an nsIContent and
an nsDOMAttributeMap. Could you hold on to just the nsDOMAttributeMap and get
the content from that? You'd have to remove the inline, but that's IMHO ok.
At least make it so that setting and clearing the two members is done through a
single functioncall (RemoveMapRef currently misses a SetContent(nsnull) call
for example).

It would be great if you added code to nsDOMAttributeMap::DropReference so that
it enumarated all the attributes in the hash and made them drop their
references. In general I feel a little bit unconfortable about the weak
nsIContent and nsDOMAttrMap references in the attributes. The management code
for them is spread out all over the place. But it was bad before too.

Also, if you could make sure to call RemoveMap/SetContent(nsnull) on all
attributes that are removed from the hash when they are replaced by new
attributes.


>Index: content/base/src/nsDOMAttributeMap.h
...
>+class nsAttrKey

Hmm... come to think of it. You could maybe use nsAttrName as key-class. Not
sure what's cleanest, do whichever you think is best.

>+{
>+public:
>+  /** The namespace of the attribute */
>+  PRInt32  mNamespaceID;
>+  /**
>+   * The atom for attribute, weak ref. is fine as we only use it for the
>+   * hashcode, we never dereference it.
>+   */
>+  nsIAtom* mLocalName;   
>+
>+  nsAttrKey(PRInt32 aNs, nsIAtom* aName)
>+    : mNamespaceID(aNs), mLocalName(aName) {}
>+
>+  nsAttrKey(const nsAttrKey& aAttr)
>+    : mNamespaceID(aAttr.mNamespaceID), mLocalName(aAttr.mLocalName) {}
>+
>+  friend PRBool operator==(const nsAttrKey& aAttr1, const nsAttrKey& aAttr2);

I personally prefer operators as member functions. That way you can inline it
too. No biggie though.

...
>+  static PLDHashNumber HashKey(KeyTypePointer aKey)
>+    {
>+      if (!aKey)
>+        return 0;
>+
>+      // This could go into /xpcom/ds/nsCRT.cpp, as it only handles strings for
>+      // the moment. (XXX)
>+      PRUint32 h = 0;
>+      PRUint8* buf = (PRUint8*) aKey;
>+      for (PRUint32 i = 0; i < sizeof(nsAttrKey); ++i)
>+        h = (h>>28) ^ (h<<4) ^ buf[i];
>+
>+      return h;

Just return |aKey->mNamespaceID + NS_PTR_TO_INT32(aKey->mLocalName)|


>Index: content/base/src/nsDOMAttributeMap.cpp
...
>+nsIDOMNode*
>+nsDOMAttributeMap::GetAttribute(nsINodeInfo*     aNodeInfo,
>+                                const nsAString& aValue,
>+                                PRBool aRemove)
>+{
>+  if (!aNodeInfo)
>+    return nsnull;

Is this needed? Seems like a bug if so. You could always just assert (works as
documentation if nothing else).

>+
>+  nsIDOMNode* domAttr = nsnull;
>+
>+  nsAttrHashKey::KeyType attr(aNodeInfo->NamespaceID(),
>+                              aNodeInfo->NameAtom());
>+  nsAttrHashKey attrHash(attr);

attrHash is unused.

>+  if (!mAttributeCache.Get(attr, &domAttr)) {
>+    domAttr = new nsDOMAttribute(aRemove ? nsnull : mContent,
>+                                 aNodeInfo,
>+                                 aValue,
>+                                 aRemove ? nsnull : this);
>+    if (domAttr && !aRemove && !mAttributeCache.Put(attr, domAttr)) {
>+      delete domAttr;
>+      domAttr = nsnull;
>+    }
>+  } else if (aRemove) {
>+    nsCOMPtr<nsIAttribute> iAttr = do_QueryInterface(domAttr);
>+
>+    /// @todo I'm not sure what to do if a node does not implement
>+    /// nsIAttribute... (XXX)

Actually, all real attributes should now implement nsIAttribute. So we should
bail early (in SetNamedItem(NS)) for attributes that don't so that they never
get into the hash.

>+nsDOMAttributeMap::RemoveFromCache(nsDOMAttribute* aAttr) 
>+{
>+  if (!aAttr)
>+    return;

Again, is this needed?

>+  nsINodeInfo* ni = aAttr->NodeInfo();
>+
>+  nsAttrHashKey::KeyType attr(ni->NamespaceID(),
>+                              ni->NameAtom());
>+  mAttributeCache.Remove(attr);

Don't you need to call RemoveMap() and SetContent(nsnull) on the attribute
here?


>@@ -123,16 +207,17 @@ nsDOMAttributeMap::SetNamedItem(nsIDOMNo
>     // nsContentUtils::CheckSameOrigin can't deal with attributenodes yet
>     
>     nsCOMPtr<nsIDOMAttr> attribute(do_QueryInterface(aNode));

QI to nsIAttribute here instead. Same in SetNamedItemNS.


In IsSameNode, no need to check that the returnvalue-pointer isn't null (assert
if you really want to)

Comment 23

[Allan Beaufour]

(In reply to comment #22)
> >+  static PLDHashNumber HashKey(KeyTypePointer aKey)
> >+    {
> >+      if (!aKey)
> >+        return 0;
> >+
> >+      // This could go into /xpcom/ds/nsCRT.cpp, as it only handles strings for
> >+      // the moment. (XXX)
> >+      PRUint32 h = 0;
> >+      PRUint8* buf = (PRUint8*) aKey;
> >+      for (PRUint32 i = 0; i < sizeof(nsAttrKey); ++i)
> >+        h = (h>>28) ^ (h<<4) ^ buf[i];
> >+
> >+      return h;
> 
> Just return |aKey->mNamespaceID + NS_PTR_TO_INT32(aKey->mLocalName)|

That would make two attributes created right after each other with an "unlucky"
combination of namespaces clash pretty easily, would it not?

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Being XPCOM objects atoms are fairly large (refcounter + vtable + members) so
the chances that the namespaceID would compensate for it exactly is pretty slim.
Especially given that in most cases you'll only have attributes of one or two
namespaces in the same element. And even if they did a collision isn't that big
a deal since this code isn't performance critical anyway.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Though if you really want to you can always do

(aKey->mNamespaceID>>28)^(aKey->mNamespaceID<<4)^NS_PTR_TO_INT32(aKey->mLocalName)

which is what you're doing now. It was mostly the loop I thought was unneccesary.

Comment 26

[Allan Beaufour]

Attachment: Patch v4

Should include fixes for all your comments. I've cleaned up some redundant code
in nsDOMAttributeMap too.

I'm not too happy about the change to nsGenericElement, but it fixes smaug's
comment... as said before, we could do that in another bug.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 179836 [details] [diff] [review]
Patch v4


>Index: content/base/src/nsGenericElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v
>retrieving revision 3.381
>diff -u -p -U8 -r3.381 nsGenericElement.cpp
>--- content/base/src/nsGenericElement.cpp	5 Apr 2005 23:54:26 -0000	3.381
>+++ content/base/src/nsGenericElement.cpp	6 Apr 2005 12:03:19 -0000
>@@ -3598,31 +3598,32 @@ nsGenericElement::UnsetAttr(PRInt32 aNam
> {
>   NS_ASSERTION(nsnull != aName, "must have attribute name");
> 
>   PRInt32 index = mAttrsAndChildren.IndexOfAttr(aName, aNameSpaceID);
>   if (index < 0) {
>     return NS_OK;
>   }
> 
>+  nsAutoString attrName;
>+  aName->ToString(attrName);
>+  nsCOMPtr<nsIDOMAttr> attrNode;
>+  GetAttributeNode(attrName, getter_AddRefs(attrNode));
>+

Put this somehow inside an if. Something like

nsCOMPtr<nsIDOMAttr> attrNode;
nsDOMSlots * slots = GetExistingDOMSlots();
if(slots && slots->mAttributeMap) {  
  nsAutoString attrName;
  aName->ToString(attrName);
  GetAttributeNode(attrName, getter_AddRefs(attrNode));
}

>   nsIDocument *document = GetCurrentDoc();    
>   mozAutoDocUpdate updateBatch(document, UPDATE_CONTENT_MODEL, aNotify);
>   if (document) {
>     if (aNotify) {
>       document->AttributeWillChange(this, aNameSpaceID, aName);
>     }
> 
>     if (HasMutationListeners(this, NS_EVENT_BITS_MUTATION_ATTRMODIFIED)) {
>       nsCOMPtr<nsIDOMEventTarget> node(do_QueryInterface(NS_STATIC_CAST(nsIContent *, this)));
>       nsMutationEvent mutation(NS_MUTATION_ATTRMODIFIED, node);
> 
>-      nsAutoString attrName;
>-      aName->ToString(attrName);
>-      nsCOMPtr<nsIDOMAttr> attrNode;
>-      GetAttributeNode(attrName, getter_AddRefs(attrNode));

And then here
if (!attrNode) {
  nsAutoString attrName;
  aName->ToString(attrName);
  GetAttributeNode(attrName, getter_AddRefs(attrNode));
}

>       mutation.mRelatedNode = attrNode;
>       mutation.mAttrName = aName;
> 
>       nsAutoString value;
>       // It sucks that we have to call GetAttr here, but HTML can't always
>       // get the value from the nsAttrAndChildArray. Specifically enums and
>       // nsISupports can't be converted to strings.
>       GetAttr(aNameSpaceID, aName, value);
>@@ -3631,16 +3632,22 @@ nsGenericElement::UnsetAttr(PRInt32 aNam
>       mutation.mAttrChange = nsIDOMMutationEvent::REMOVAL;
> 
>       nsEventStatus status = nsEventStatus_eIgnore;
>       HandleDOMEvent(nsnull, &mutation, nsnull,
>                      NS_EVENT_FLAG_INIT, &status);
>     }
>   }
> 
>+  // Clear binding to nsIDOMNamedNodeMap
>+  nsCOMPtr<nsIAttribute> iAttr(do_QueryInterface(attrNode));
>+  if (iAttr) {
>+    iAttr->SetMap(nsnull);
>+  }
>+

Comment 28

[Allan Beaufour]

(In reply to comment #27)
> (From update of attachment 179836 [details] [diff] [review] [edit])
> Put this somehow inside an if. Something like

As in: Do not create attribute node if we do not need it, yes. I'll fix that
together with sickings review comments.

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 179836 [details] [diff] [review]
Patch v4

Yay! All in all this looks great! The code should behave a lot better then
what's on the trunk now! And I feel a lot less worried about dangling pointers
too.


Style nit: You're inconsistent about doing

} else {

vs.

}
else {

Usually you should follow the style of the file, but it seems inconsistent in
this code :). Choose the one you like best. Personally I prefer the latter.


>Index: content/base/src/nsDOMAttributeMap.cpp

>+PRBool operator==(const nsAttrKey& aAttr1, const nsAttrKey& aAttr2)
>+{
>+  return aAttr1.mNamespaceID == aAttr2.mNamespaceID &&
>+         aAttr1.mLocalName == aAttr2.mLocalName;
>+}

I take it you prefer this as a non-memberfunction? Would you mind inlineing it
at least? Especially since we'll be hitting this stuff on UnsetAttr which makes
it at least somewhat performance sensitive.

> nsDOMAttributeMap::~nsDOMAttributeMap()
> {
>+  mAttributeCache.Enumerate(RemoveMapRef, nsnull);
> }
> 
> void
> nsDOMAttributeMap::DropReference()
> {
>+  mAttributeCache.Enumerate(RemoveMapRef, nsnull);
>   mContent = nsnull;
> }

Add a mAttributeCache.Clear() call here, it feels like a bad thing to have the
attributes in the hash once the attributes have dropped the reference back. If
nothing else it'll give the attributes a chance to get deleted.


+nsDOMAttributeMap::GetAttribute(nsINodeInfo* aNodeInfo,
...
>+    if (!aRemove && !mAttributeCache.Put(attr, newAttr)) {
>+      delete newAttr;
>+      return NS_ERROR_FAILURE;
>+    }

Return NS_ERROR_OUT_OF_MEMORY here, that's the only reason Put can fail.


>+nsDOMAttributeMap::SetNamedItemInternal(nsIDOMNode *aNode,
...
>+      // Set the new attribute value
>+      nsAutoString value;
>+      attribute->GetValue(value);
>+      if (ni->NamespaceID() == kNameSpaceID_None &&
>+          mContent->IsContentOfType(nsIContent::eHTML)) {
>+        // Set via setAttribute(), which may do normalization on the
>+        // attribute name for HTML
>+        nsCOMPtr<nsIDOMElement> ourElement(do_QueryInterface(mContent));
>+        NS_ASSERTION(ourElement, "HTML content that's not an element?");
>+        ourElement->SetAttribute(name, value);
>+      } else {
>+        // It's OK to just use SetAttr
>+        rv = mContent->SetAttr(ni->NamespaceID(), ni->NameAtom(),
>+                               ni->GetPrefixAtom(), value, PR_TRUE);
>+      }

If you add an |aWithNS &&| test here you can move this out of the big |if
(aWithNS)| test and get a bit more codereuse.

Also, rather then using |aReturn| directly in the calls to GetAttribute use a
temporary local and set aReturn towards the end of the function. Otherwise you
risk returning an errorcode while still having aReturn point to an addrefed
object, which breaks XPCOM rules.

The risk is that the caller will bail without releasing aReturn, which is ok to
do when an errorcode is returned (when an error is returned none of the
outvalues are to be considered valid).


I'd like to see some other solution to the UnsetAttr change too since here
performance could matter a lot. The fastest solution would probably be to have
a function like nsDOMAttributeMap::DropAttribute(PRInt32 aNamespaceID, nsIAtom*
aLocalName) that would check the hash and if it exists call SetMap(nsnull) and
remove it from the hash. If you do that you could also remove that code from
nsDOMAttributeMap::GetAttribute.


I think it would be worth adding an inlined GetContentInternal() function to
nsDOMAttribute that you can use instead of all those GetContent() calls inside
nsDOMAttribute which ends up being virtual calls now.

Comment 30

[Allan Beaufour]

Attachment: Patch v4 w/sicking's comments

(In reply to comment #29)
> (From update of attachment 179836 [details] [diff] [review] [edit])
> >+PRBool operator==(const nsAttrKey& aAttr1, const nsAttrKey& aAttr2)
...
> I take it you prefer this as a non-memberfunction? Would you mind inlineing
it
> at least? Especially since we'll be hitting this stuff on UnsetAttr which
makes
> it at least somewhat performance sensitive.

Sorry about that, I simply forgot about it. Inlined it in nsAttrHashKey
instead...
 
> >+nsDOMAttributeMap::SetNamedItemInternal(nsIDOMNode *aNode,
> ...
> If you add an |aWithNS &&| test here you can move this out of the big |if
> (aWithNS)| test and get a bit more codereuse.

Ooops. I merged the two functions a bit too quickly apparently, GetName() was
also duplicated.
 
> I'd like to see some other solution to the UnsetAttr change too since here
> performance could matter a lot. The fastest solution would probably be to
have
> a function like nsDOMAttributeMap::DropAttribute(PRInt32 aNamespaceID,
nsIAtom*
> aLocalName) that would check the hash and if it exists call SetMap(nsnull)
and
> remove it from the hash. If you do that you could also remove that code from
> nsDOMAttributeMap::GetAttribute.

Done that, but kept the code in GetAttribute(), to save creation of KeyType and
cache check.

The rest of the comments incorporated.

I also got tired of seeing the compiler warnings in nsGenericElement.cpp, so I
added two fixes for that. Naturally, they can be ignored if they are
problematic in some way?

BTW: Should I cycle the IID for nsIAttribute?

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 180352 [details] [diff] [review]
Patch v4 w/sicking's comments

Yep, good catch, you should get nsIAttribute a new IID.

>+nsDOMAttributeMap::SetNamedItemInternal(nsIDOMNode *aNode,
>+                                        nsIDOMNode **aReturn,
>+                                        PRBool aWithNS)
>+{
>+  NS_ENSURE_ARG_POINTER(aNode);
>+  NS_ENSURE_ARG_POINTER(aReturn);
> 
>   nsresult rv = NS_OK;
>   *aReturn = nsnull;
>+  nsIDOMNode* tmpReturn = nsnull;

Make this an nsCOMPtr to make sure it gets released if there's an error after
it gets set. You can use tmpReturn.swap(*aReturn) at the end to keep the
refcounting simple.


>+    if (aWithNS && ni->NamespaceID() == kNameSpaceID_None &&
>         mContent->IsContentOfType(nsIContent::eHTML)) {

That should be !aWithNS. My bad :)

With that, r=sicking

This is great work, I think it's worth getting this into 1.8b2 since it fixes a
few dangling-pointer issues with mContent in nsDOMAttribute that could cause
crashes.

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Eep, missed a problem in the warnins fix. That should be
aIndex == GetChildCount() - 1
to account for the fact that the child has been inserted now.

Comment 33

[Allan Beaufour]

Attachment: Patch v5

Gave nsIAttribute a new IID and fixed the pretty st*pid errors in the previous
patch.

For an excuse, take a pick:
* it's Monday
* my brain's asleep
* I left my head in extensions/xforms
* I was distracted by hordes of Swedish girls marching by (in my dreams)

:)

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Hmm, forgot to mention....
I think nsGenericElement::UnsetAttr is not quite enough, 
also nsXULElement::UnsetAttr must be modified, unfortunately.

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Doh! nsXTFElementWrapper::UnsetAttr as well.

Btw, i'm totally going to reuse that last excuse :)

Comment 36

[Allan Beaufour]

(In reply to comment #35)
> Doh! nsXTFElementWrapper::UnsetAttr as well.

Sure? If the implementation is not an attribute handler it just calls its base,
nsXMLElement -> nsGenericElement does it not?

> Btw, i'm totally going to reuse that last excuse :)

:)

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Right, but the attrnode needs to be dropped in the case where the attribute is
in the attributehandler too, so you need a call inside that |if|

Comment 38

[Allan Beaufour]

Attachment: Patcv v5 w/support for XUL and XTF

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 180681 [details] [diff] [review]
Patcv v5 w/support for XUL and XTF

Don't you need to do the DropAttribute call before
mAttrsAndChildren.RemoveAttrAt is called in nsXULElement so that the node can
grab the value?

r=me with that

Comment 40

[Allan Beaufour]

Attachment: Patch v6

(In reply to comment #39)
> (From update of attachment 180681 [details] [diff] [review] [edit])
> Don't you need to do the DropAttribute call before
> mAttrsAndChildren.RemoveAttrAt is called in nsXULElement so that the node can

> grab the value?

Correct. The same goes for the XTF handling I guess. Problem is that if the
attribute removal fails or is actually not removed, it does not work correctly
though. But one could say that we follow the same "faulty design line", because
AttributeRemoved() is also called, no matter what.

Comment 41

[Peter Van der Beken [:peterv]]

Comment on attachment 180692 [details] [diff] [review]
Patch v6

Resolve these issues:
http://people.mozilla-europe.org/peterv/jst-review/jst-review-cgi.pl?patch_file
=&patch_url=https%3A%2F%2Fbugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D180692&p
atch_text=&reason_type=%21&reason_type=A&reason_type=B&reason_type=E&reason_typ
e=F&reason_type=L&reason_type=N&reason_type=P&reason_type=Q&reason_type=S&reaso
n_type=W&reason_type=X&reason_type=%7B
Please be consistent about your coding style.

>Index: content/base/public/nsIAttribute.h
>===================================================================

>+  virtual void SetMap(nsDOMAttributeMap *aMap) = 0;
>+  
>+  nsDOMAttributeMap *GetMap()
>+    { return mAttrMap; };

Please follow the style of the file:

  nsDOMAttributeMap *GetMap()
  {
    return mAttrMap;
  }

>Index: content/base/src/nsDOMAttributeMap.h
>===================================================================

>+/**
>+ * Structure used to cache nsDOMAttributes with.
>+ */
>+class nsAttrKey
>+{
>+public:
>+  /** The namespace of the attribute */

Use

/**
 *
 */

or

//

>+  PRInt32  mNamespaceID;

Insert a newline here.

>+  /**
>+   * The atom for attribute, weak ref. is fine as we only use it for the
>+   * hashcode, we never dereference it.
>+   */

>+class NS_COM nsAttrHashKey : public PLDHashEntryHdr
>+{
>+public:
>+  typedef nsAttrKey KeyType;
>+  typedef const nsAttrKey* KeyTypePointer;
>+
>+  nsAttrHashKey(KeyType aKey) : mKey(aKey) {}

Is this needed?

>+  nsAttrHashKey(KeyTypePointer aKey) : mKey(*aKey) {}
>+  nsAttrHashKey(const nsAttrHashKey& aCopy) : mKey(aCopy.mKey) {}
>+  ~nsAttrHashKey() { }
>+
>+  KeyType GetKey() const { return mKey; }

Is this needed?

>+  KeyTypePointer GetKeyPointer() const { return &mKey; }
>+  PRBool KeyEquals(KeyTypePointer aKey) const
>+    {
>+      return mKey.mLocalName == aKey->mLocalName &&
>+        mKey.mNamespaceID == aKey->mNamespaceID;

Line this up correctly.

> // Helper class that implements the nsIDOMNamedNodeMap interface.
> class nsDOMAttributeMap : public nsIDOMNamedNodeMap
> {
> public:
>   nsDOMAttributeMap(nsIContent* aContent);
>   virtual ~nsDOMAttributeMap();
> 
>   NS_DECL_ISUPPORTS
> 
>   // nsIDOMNamedNodeMap interface
>   NS_DECL_NSIDOMNAMEDNODEMAP
> 
>   void DropReference();
> 
>+  nsIContent* GetContent()
>+    { return mContent; };  


>+  /** 
>+   * Cache of nsDOMAttributes.
>+   */
>+  nsInterfaceHashtable<nsAttrHashKey, nsIDOMNode> mAttributeCache;

You seem to use nsIAttribute more than nsIDOMNode, maybe you should store
nsIAttribute and QI to nsIDOMNode where necessary?

>+  /**
>+   * GetNamedItemNS() implementation taking |aRemove| for GetAttribute(),
>+   * which is used by RemoveNamedItemNS().
>+   */
>+  nsresult GetNamedItemNSInternal(const nsAString& aNamespaceURI,
>+                                  const nsAString& aLocalName,
>+                                  nsIDOMNode** aReturn,
>+                                  PRBool aRemove = PR_FALSE);

Insert a newline here.

>+  /**
>+   * Returns an attribute, either by retrieving it from the cache or by
>+   * creating a new one.
>+   */

>Index: content/base/src/nsDOMAttributeMap.cpp
>===================================================================

> nsDOMAttributeMap::nsDOMAttributeMap(nsIContent* aContent)
>   : mContent(aContent)
> {
>+  mAttributeCache.Init();

This could fail, you should make a nsDOMAttributeMap::Init method

>+
>   // We don't add a reference to our content. If it goes away,
>   // we'll be told to drop our reference
> }
> 
>+/**
>+ * Clear map pointer for attributes.
>+ */
>+PLDHashOperator
>+RemoveMapRef(nsAttrKey aKey, nsCOMPtr<nsIDOMNode>& aData, void* aUserArg) 
>+{
>+  nsCOMPtr<nsIAttribute> attr(do_QueryInterface(aData));
>+  NS_ASSERTION(attr, "non-nsIAttribute somehow made it into the hashmap?!");
>+  attr->SetMap(nsnull);
>+
>+  return PL_DHASH_NEXT;    

I think you could return PL_DHASH_REMOVE here.

> nsDOMAttributeMap::~nsDOMAttributeMap()
> {
>+  mAttributeCache.Enumerate(RemoveMapRef, nsnull);
>+  mAttributeCache.Clear();  

No need to call Clear.

> void
> nsDOMAttributeMap::DropReference()
> {
>+  mAttributeCache.Enumerate(RemoveMapRef, nsnull);

Shouldn't this call Clear or return PL_DHASH_REMOVE from RemoveMapRef?

>   mContent = nsnull;
> }

>+void
>+nsDOMAttributeMap::DropAttribute(PRInt32 aNamespaceID, nsIAtom* aLocalName)
>+{
>+  nsAttrHashKey::KeyType attr(aNamespaceID, aLocalName);
>+  nsCOMPtr<nsIDOMNode> node;
>+  if (mAttributeCache.Get(attr, getter_AddRefs(node))) {

GetWeak?

>+    nsCOMPtr<nsIAttribute> iAttr(do_QueryInterface(node));

>+nsresult
>+nsDOMAttributeMap::GetAttribute(nsINodeInfo* aNodeInfo,
>+                                nsIDOMNode** aReturn,
>+                                PRBool aRemove)
>+{
>+  NS_ASSERTION(aNodeInfo, "GetAttribute() called with aNodeInfo == nsnull!");
>+  NS_ASSERTION(aReturn, "GetAttribute() called with aReturn == nsnull");
>+
>+  *aReturn = nsnull;
>+
>+  nsAttrHashKey::KeyType attr(aNodeInfo->NamespaceID(),
>+                              aNodeInfo->NameAtom());

This can go on one line.

>+
>+  if (!mAttributeCache.Get(attr, aReturn)) {
>+    nsAutoString value;
>+    if (aRemove) {
>+      // As we are removing the attribute we need to set the current value in
>+      // the attribute node.
>+      mContent->GetAttr(aNodeInfo->NamespaceID(),
>+                        aNodeInfo->NameAtom(), value);

Rewrap.

>+    }
>+    nsDOMAttribute *newAttr = new nsDOMAttribute(aRemove ? nsnull : this,
>+                                                 aNodeInfo,
>+                                                 value);

Rewrap.

>+    if (!newAttr) {
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+    if (!aRemove && !mAttributeCache.Put(attr, newAttr)) {
>+      delete newAttr;
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+    NS_ADDREF(*aReturn = newAttr);
>+  } else if (aRemove) {

  }
  else if (...) {

>+nsresult
>+nsDOMAttributeMap::SetNamedItemInternal(nsIDOMNode *aNode,
>+                                        nsIDOMNode **aReturn,
>+                                        PRBool aWithNS)

>-    // get node-info and value of old attribute
>-    nsAutoString name, value;
>+    nsCOMPtr<nsINodeInfo> ni;
>+    nsAutoString name;
>     attribute->GetName(name);
> 
>-    nsCOMPtr<nsINodeInfo> ni = mContent->GetExistingAttrNameFromQName(name);
>-    if (ni) {
>-      rv = mContent->GetAttr(ni->NamespaceID(), ni->NameAtom(), value);
>-      NS_ASSERTION(rv != NS_CONTENT_ATTR_NOT_THERE, "unable to get attribute");
>-      NS_ENSURE_SUCCESS(rv, rv);
>+    // SetNamedItemNS()
>+    if (aWithNS) {
>+      // Return existing attribute, if present
>+      nsAutoString nsURI;
>+      attribute->GetNamespaceURI(nsURI);

Since you already QIed to nsIAttribute, why not use its nsINodeInfo instead of
converting back'n'forth to strings?

>Index: content/base/src/nsDOMAttribute.h
>===================================================================

>@@ -69,36 +69,39 @@ protected:
> 
> // Attribute helper class used to wrap up an attribute with a dom
> // object that implements nsIDOMAttr and nsIDOMNode
> class nsDOMAttribute : public nsIDOMAttr,
>                        public nsIDOM3Node,
>                        public nsIAttribute
> {
> public:
>-  nsDOMAttribute(nsIContent* aContent, nsINodeInfo *aNodeInfo,
>+  nsDOMAttribute(nsDOMAttributeMap* aContent, nsINodeInfo *aNodeInfo,

Rename aContent to aAttrMap.

>Index: content/base/src/nsDOMAttribute.cpp
>===================================================================

>+nsIContent*
>+nsDOMAttribute::GetContent()
>+{
>+  return GetContentInternal();
>+}
>+
>+nsIContent*
>+nsDOMAttribute::GetContentInternal()
>+{
>+  return mAttrMap ? mAttrMap->GetContent() : nsnull;
>+}

Why do we need both GetContent and GetContentInternal? Drop GetContentInternal.

> nsresult
> nsDOMAttribute::GetValue(nsAString& aValue)
> {
>-  if (mContent) {
>+  if (GetContentInternal()) {
>     nsAutoString tmpValue;
>-    nsresult attrResult = mContent->GetAttr(mNodeInfo->NamespaceID(),
>-                                            mNodeInfo->NameAtom(),
>-                                            tmpValue);
>-    if (NS_CONTENT_ATTR_NOT_THERE != attrResult) {
>+    nsresult attrResult = GetContentInternal()->GetAttr(mNodeInfo->NamespaceID(),
>+                                                mNodeInfo->NameAtom(),
>+                                                tmpValue);

Store the result of GetContentInternal in a local var instead of calling it
twice.

> nsresult
> nsDOMAttribute::SetValue(const nsAString& aValue)
> {
>   nsresult result = NS_OK;

Please make this rv.

>-  if (mContent) {
>-    result = mContent->SetAttr(mNodeInfo->NamespaceID(), mNodeInfo->NameAtom(),
>-                               mNodeInfo->GetPrefixAtom(), aValue, PR_TRUE);
>+  if (GetContentInternal()) {
>+    result = GetContentInternal()->SetAttr(mNodeInfo->NamespaceID(), mNodeInfo->NameAtom(),
>+                                   mNodeInfo->GetPrefixAtom(), aValue, PR_TRUE);

Line this up correctly.
Store the result of GetContentInternal in a local var instead of calling it
twice.

> nsresult
> nsDOMAttribute::GetSpecified(PRBool* aSpecified)

>+  *aSpecified = PR_FALSE;
> 
>-  *aSpecified = mContent->HasAttr(mNodeInfo->NamespaceID(),
>-                                  mNodeInfo->NameAtom());
>+  if (GetContentInternal()) {
>+    *aSpecified = GetContentInternal()->HasAttr(mNodeInfo->NamespaceID(),
>+                                        mNodeInfo->NameAtom());

Store the result of GetContentInternal in a local var instead of calling it
twice, and make it

  *aSpecified = content && content->HasAttr(...);

> NS_IMETHODIMP
> nsDOMAttribute::GetOwnerElement(nsIDOMElement** aOwnerElement)
> {
>   NS_ENSURE_ARG_POINTER(aOwnerElement);
> 
>-  if (mContent) {
>-    return CallQueryInterface(mContent, aOwnerElement);
>+  if (GetContentInternal()) {
>+    PRBool hasAttr = GetContentInternal()->HasAttr(mNodeInfo->NamespaceID(),
>+                                           mNodeInfo->NameAtom());

Line this up correctly.

>+    if (hasAttr) {
>+      return CallQueryInterface(GetContentInternal(), aOwnerElement);

Store the result of GetContentInternal in a local var instead of calling it
three times.

> NS_IMETHODIMP
> nsDOMAttribute::GetOwnerDocument(nsIDOMDocument** aOwnerDocument)
> {
>   *aOwnerDocument = nsnull;
> 
>   nsresult rv = NS_OK;
>-  if (mContent) {
>-    nsCOMPtr<nsIDOMNode> node = do_QueryInterface(mContent, &rv);
>+  if (GetContentInternal()) {
>+    nsCOMPtr<nsIDOMNode> node = do_QueryInterface(GetContentInternal(), &rv);

Store the result of GetContentInternal in a local var instead of calling it
twice.

>@@ -382,26 +405,27 @@ nsDOMAttribute::SetPrefix(const nsAStrin

>     if (rv == NS_CONTENT_ATTR_HAS_VALUE) {
>-      mContent->UnsetAttr(nameSpaceID, name, PR_TRUE);
>+      content->UnsetAttr(nameSpaceID, name, PR_TRUE);
> 
>-      mContent->SetAttr(newNodeInfo->NamespaceID(), newNodeInfo->NameAtom(),
>+      content->SetAttr(newNodeInfo->NamespaceID(), newNodeInfo->NameAtom(),
>                         newNodeInfo->GetPrefixAtom(), tmpValue, PR_TRUE);

Line this up correctly.

> NS_IMETHODIMP
> nsDOMAttribute::IsSameNode(nsIDOMNode* aOther,
>                            PRBool* aReturn)
> {

>+  NS_ASSERTION(aReturn, "IsSameNode() called with aReturn == nsnull!");
>+  
>+  *aReturn = (NS_STATIC_CAST(nsIDOMNode*, this) == aOther);

Technically you should QI both to nsISupports first.

>Index: content/xtf/src/nsXTFElementWrapper.cpp
>===================================================================

>@@ -327,17 +328,22 @@ nsXTFElementWrapper::UnsetAttr(PRInt32 a

>+    nsDOMSlots *slots = GetExistingDOMSlots();
>+    if(slots && slots->mAttributeMap) {  
>+      slots->mAttributeMap->DropAttribute(aNameSpaceID, aAttr);
>+    }
>     rv = mAttributeHandler->RemoveAttribute(aAttr);

Please add a XXX comment about the problem when RemoveAttribute fails.

Comment 42

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> Why do we need both GetContent and GetContentInternal? Drop GetContentInternal.

That's to avoid a virtual call in a pile of places. GetContent needs to be
virtual since nsDOMAttributeMap.h isn't exported. I think it's worth the extra
few lines of code. IMHO GetContentInternal could even be inlined.

> Technically you should QI both to nsISupports first.

Good point, there's even the function SameCOMIdentity for that.

Comment 43

[Peter Van der Beken [:peterv]]

(In reply to comment #42)
> > Why do we need both GetContent and GetContentInternal? Drop GetContentInternal.
> 
> That's to avoid a virtual call in a pile of places. GetContent needs to be
> virtual since nsDOMAttributeMap.h isn't exported. I think it's worth the extra
> few lines of code. IMHO GetContentInternal could even be inlined.

Wouldn't inlining GetContent help?

Comment 44

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Nope, a virtual function will always be called virtually since the compiler
won't know if there's a subclass somewhere that overrides it.

There are exceptions, but they don't apply here. In the following code

FooClass foo;
foo.someFunc();

the compiler can figure out which function to call since it know the actual
class for |foo|. However inside the class there is no way to know what class
|this| is of.

Comment 45

[Allan Beaufour]

Attachment: Patvh v6/w most comments addressed

(In reply to comment #41)
> (From update of attachment 180692 [details] [diff] [review] [edit])
> Resolve these issues:
> [jst-tool] 

Neat tool. I'll try to remember to run my patches through it first.

I'm not sure about how to format the two nsIScriptSecurityManager:: lines
though.

> Please be consistent about your coding style.

I thought I was, but I live in a fantasty world I can see :-)

> >+class NS_COM nsAttrHashKey : public PLDHashEntryHdr
...
> >+  KeyType GetKey() const { return mKey; }
> 
> Is this needed?

Yes, for nsBaseHashtable.
 
> >+  /** 
> >+   * Cache of nsDOMAttributes.
> >+   */
> >+  nsInterfaceHashtable<nsAttrHashKey, nsIDOMNode> mAttributeCache;
> 
> You seem to use nsIAttribute more than nsIDOMNode, maybe you should store
> nsIAttribute and QI to nsIDOMNode where necessary?

Shouldn't be a problem. I just favoured the speed of retrieval of attributes.
If an extra QI there is fine, then nsIAttribute might be better. Should I
change it? I have no real preference.
 
> >Index: content/base/src/nsDOMAttributeMap.cpp
> >===================================================================
> 
> > nsDOMAttributeMap::nsDOMAttributeMap(nsIContent* aContent)
> >   : mContent(aContent)
> > {
> >+  mAttributeCache.Init();
> 
> This could fail, you should make a nsDOMAttributeMap::Init method

Ok. But do I need to check whether it is initialized in the public functions
too?

> >+	}
> >+	nsDOMAttribute *newAttr = new nsDOMAttribute(aRemove ? nsnull : this,
> >+						     aNodeInfo,
> >+						     value);
> 
> Rewrap.

|new ...| on a new line?
 
> >+nsresult
> >+nsDOMAttributeMap::SetNamedItemInternal(nsIDOMNode *aNode,
> >+					    nsIDOMNode **aReturn,
> >+					    PRBool aWithNS)
> >+	if (aWithNS) {
> >+	  // Return existing attribute, if present
> >+	  nsAutoString nsURI;
> >+	  attribute->GetNamespaceURI(nsURI);
> 
> Since you already QIed to nsIAttribute, why not use its nsINodeInfo instead
of
> converting back'n'forth to strings?

This is old code collected from the previous two functions. I (hopefully)
haven't changed it as "it worked before"...

The rest of the comments are addressed in this patch

Comment 46

[Peter Van der Beken [:peterv]]

Comment on attachment 181963 [details] [diff] [review]
Patvh v6/w most comments addressed

(In reply to comment #45)
> I'm not sure about how to format the two nsIScriptSecurityManager:: lines
> though.

Yeah, there's no nice solution. Use a local var?

> > You seem to use nsIAttribute more than nsIDOMNode, maybe you should store
> > nsIAttribute and QI to nsIDOMNode where necessary?
> 
> Shouldn't be a problem. I just favoured the speed of retrieval of attributes.
> If an extra QI there is fine, then nsIAttribute might be better. Should I
> change it? I have no real preference.

Up to you really :-).

> > This could fail, you should make a nsDOMAttributeMap::Init method
> 
> Ok. But do I need to check whether it is initialized in the public functions
> too?

IMHO no, at most add an NS_ASSERTION.

> > >+	nsDOMAttribute *newAttr = new nsDOMAttribute(aRemove ? nsnull : this,
> > >+						     aNodeInfo,
> > >+						     value);
> > 
> > Rewrap.
> 
> |new ...| on a new line?

No, move 'value' up to the previous line and line up aNodeInfo with aRemove.

> > >+nsresult
> > >+nsDOMAttributeMap::SetNamedItemInternal(nsIDOMNode *aNode,
> > >+					    nsIDOMNode **aReturn,
> > >+					    PRBool aWithNS)
> > >+	if (aWithNS) {
> > >+	  // Return existing attribute, if present
> > >+	  nsAutoString nsURI;
> > >+	  attribute->GetNamespaceURI(nsURI);
> > 
> > Since you already QIed to nsIAttribute, why not use its nsINodeInfo instead
> of
> > converting back'n'forth to strings?
> 
> This is old code collected from the previous two functions. I (hopefully)
> haven't changed it as "it worked before"...

Hmm, ok. It's a bit wasteful.

>Index: content/base/src/nsDOMAttributeMap.h
>===================================================================

>+  nsAttrHashKey(const nsAttrHashKey& aCopy) : mKey(aCopy.mKey) {}
>+  ~nsAttrHashKey() { }

Ubernit: remove that space between the braces.

>Index: content/base/src/nsDOMAttributeMap.cpp
>===================================================================

>+nsresult
>+nsDOMAttributeMap::GetAttribute(nsINodeInfo* aNodeInfo,
>+                                nsIDOMNode** aReturn,
>+                                PRBool aRemove)

>+    nsDOMAttribute *newAttr =
>+      new nsDOMAttribute(aRemove ? nsnull : this, aNodeInfo, value);
>+    if (!newAttr) {
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+    if (!aRemove && !mAttributeCache.Put(attr, newAttr)) {
>+      delete newAttr;
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+    NS_ADDREF(*aReturn = newAttr);

I'd be slightly more comfortable if you did:

    nsCOMPtr<nsIDOMNode> newAttr =
      new nsDOMAttribute(aRemove ? nsnull : this, aNodeInfo, value);
    if (!newAttr) {
      return NS_ERROR_OUT_OF_MEMORY;
    }
    if (!aRemove && !mAttributeCache.Put(attr, newAttr)) {
      return NS_ERROR_OUT_OF_MEMORY;
    }
    newAttr.swap(newAttr);

>+nsresult
>+nsDOMAttributeMap::SetNamedItemInternal(nsIDOMNode *aNode,
>+                                        nsIDOMNode **aReturn,
>+                                        PRBool aWithNS)
>+{

>+    else {
>       // It's OK to just use SetAttr
>       rv = mContent->SetAttr(ni->NamespaceID(), ni->NameAtom(),
>                              ni->GetPrefixAtom(), value, PR_TRUE);
>     }
>+    
>+    if (NS_SUCCEEDED(rv)) {
>+      nsAttrHashKey::KeyType attrkey(ni->NamespaceID(),
>+                                     ni->NameAtom());

Move 'ni->NameAtom());' up to the previous line.

>Index: content/base/src/nsDOMAttribute.h
>===================================================================

>+  nsIContent *GetContentInternal();

Let's inline this one.

>Index: content/base/src/nsDOMAttribute.cpp
>===================================================================

> nsresult
> nsDOMAttribute::SetValue(const nsAString& aValue)
> {
>-  nsresult result = NS_OK;
>-  if (mContent) {
>-    result = mContent->SetAttr(mNodeInfo->NamespaceID(), mNodeInfo->NameAtom(),
>-                               mNodeInfo->GetPrefixAtom(), aValue, PR_TRUE);
>+  nsresult rv = NS_OK;
>+  if (GetContentInternal()) {
>+    rv = GetContentInternal()->SetAttr(mNodeInfo->NamespaceID(),

Store the result of GetContentInternal in a local var instead of calling it
twice.

Carrying r=sicking.

Comment 47

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #45)
> > Since you already QIed to nsIAttribute, why not use its nsINodeInfo instead
> > of converting back'n'forth to strings?
> 
> This is old code collected from the previous two functions. I (hopefully)
> haven't changed it as "it worked before"...

Back when this code was written we didn't have nsIAttribute so that's probably
why it did what it did, so there's no reason not to change it now. That said, if
you'd rather leave it alone that's ok with me too (you've fixed a pile of other
peoples bugs as it is anyway).

Comment 48

[Allan Beaufour]

Comment on attachment 181963 [details] [diff] [review]
Patvh v6/w most comments addressed

Requesting approval for patch (with peterv's above comments fixed).

Comment 49

[Allan Beaufour]

(In reply to comment #46)
> (From update of attachment 181963 [details] [diff] [review] [edit])
> (In reply to comment #45)
> > > You seem to use nsIAttribute more than nsIDOMNode, maybe you should store
> > > nsIAttribute and QI to nsIDOMNode where necessary?
> > 
> > Shouldn't be a problem. I just favoured the speed of retrieval of attributes.
> > If an extra QI there is fine, then nsIAttribute might be better. Should I
> > change it? I have no real preference.
> 
> Up to you really :-).

I'll leave it be then.

> > > >+nsresult
> > > >+nsDOMAttributeMap::SetNamedItemInternal(nsIDOMNode *aNode,
> > > >+					    nsIDOMNode **aReturn,
> > > >+					    PRBool aWithNS)
> > > >+	if (aWithNS) {
> > > >+	  // Return existing attribute, if present
> > > >+	  nsAutoString nsURI;
> > > >+	  attribute->GetNamespaceURI(nsURI);
> > > 
> > > Since you already QIed to nsIAttribute, why not use its nsINodeInfo instead
> > of
> > > converting back'n'forth to strings?
> > 
> > This is old code collected from the previous two functions. I (hopefully)
> > haven't changed it as "it worked before"...
> 
> Hmm, ok. It's a bit wasteful.

I'll get this bug in, and look at it afterwards.
 
> I'd be slightly more comfortable if you did:
> [...]
>     newAttr.swap(newAttr);

*ahem* you find comfort in odd places :)

> > nsresult
> > nsDOMAttribute::SetValue(const nsAString& aValue)
> > {
> >-  nsresult result = NS_OK;
> >-  if (mContent) {
> >-    result = mContent->SetAttr(mNodeInfo->NamespaceID(), mNodeInfo->NameAtom(),
> >-                               mNodeInfo->GetPrefixAtom(), aValue, PR_TRUE);
> >+  nsresult rv = NS_OK;
> >+  if (GetContentInternal()) {
> >+    rv = GetContentInternal()->SetAttr(mNodeInfo->NamespaceID(),
> 
> Store the result of GetContentInternal in a local var instead of calling it
> twice.

Gaaargh, I missed one.

Comment 50

[Eli Friedman]

approval1.8b3? doesn't seem right, considering that the trunk is frozen for 1.8b2.

Comment 51

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 181963 [details] [diff] [review]
Patvh v6/w most comments addressed

a=shaver, but it would be nice if there were bugs filed-and-referenced for new
XXX comments you add.

Comment 52

[Allan Beaufour]

(In reply to comment #51)
> (From update of attachment 181963 [details] [diff] [review] [edit])
> a=shaver, but it would be nice if there were bugs filed-and-referenced for new
> XXX comments you add.

Created bug 296205 for that.

Comment 53

[Allan Beaufour]

(In reply to comment #49)
> (In reply to comment #46)
> > (From update of attachment 181963 [details] [diff] [review] [edit] [edit])
> > (In reply to comment #45)
> > > > >+nsresult
> > > > >+nsDOMAttributeMap::SetNamedItemInternal(nsIDOMNode *aNode,
> > > > >+					    nsIDOMNode **aReturn,
> > > > >+					    PRBool aWithNS)
> > > > >+	if (aWithNS) {
> > > > >+	  // Return existing attribute, if present
> > > > >+	  nsAutoString nsURI;
> > > > >+	  attribute->GetNamespaceURI(nsURI);
> > > > 
> > > > Since you already QIed to nsIAttribute, why not use its nsINodeInfo instead
> > > of
> > > > converting back'n'forth to strings?
> > > 
> > > This is old code collected from the previous two functions. I (hopefully)
> > > haven't changed it as "it worked before"...
> > 
> > Hmm, ok. It's a bit wasteful.
> 
> I'll get this bug in, and look at it afterwards.

Created bug 296207 for that.

Comment 54

[Allan Beaufour]

Checked in.

Comment 55

[Allan Beaufour]

*** Bug 93614 has been marked as a duplicate of this bug. ***

Comment 56

[tabmix.onemen]

Attachment: regression from #235512 [Core]-Fix up nsDOMAttributeMap [All]

Comment 57

[Allan Beaufour]

(In reply to comment #56)
> Created an attachment (id=185190) [edit]
> regression from #235512 [Core]-Fix up nsDOMAttributeMap [All]
> 

Should be fixed in bug 296490. Can you confirm that?

Comment 58

[Boris Zbarsky [:bzbarsky]]

This caused bug 296450, looks like.

Comment 59

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

See bug 415262 comment 5 for a slight mistake in the hashing code in this patch.