Fix order of mutation event firing for attribute removals. Bug 339494, r+sr=sicking

content/base/src/nsAttrAndChildArray.cpp

@@ -427,14 +427,16 @@ nsAttrAndChildArray::SetAndTakeAttr(nsINodeInfo* aName, nsAttrValue& aValue)
 
 
 nsresult
-nsAttrAndChildArray::RemoveAttrAt(PRUint32 aPos)
+nsAttrAndChildArray::RemoveAttrAt(PRUint32 aPos, nsAttrValue& aValue)
 {
   NS_ASSERTION(aPos < AttrCount(), "out-of-bounds");
 
   PRUint32 mapped = MappedAttrCount();
   if (aPos < mapped) {
     if (mapped == 1) {
-      // We're removing the last mapped attribute.
+      // We're removing the last mapped attribute.  Can't swap in this
+      // case; have to copy.
+      aValue.SetTo(*mImpl->mMappedAttrs->AttrAt(0));
       NS_RELEASE(mImpl->mMappedAttrs);
 
       return NS_OK;
@@ -445,12 +447,13 @@ nsAttrAndChildArray::RemoveAttrAt(PRUint32 aPos)
                                       getter_AddRefs(mapped));
     NS_ENSURE_SUCCESS(rv, rv);
 
-    mapped->RemoveAttrAt(aPos);
+    mapped->RemoveAttrAt(aPos, aValue);
 
     return MakeMappedUnique(mapped);
   }
 
   aPos -= mapped;
+  ATTRS(mImpl)[aPos].mValue.SwapValueWith(aValue);
   ATTRS(mImpl)[aPos].~InternalAttr();
 
   PRUint32 slotCount = AttrSlotCount();

content/base/src/nsAttrAndChildArray.h

@@ -102,7 +102,10 @@ public:
   nsresult SetAttr(nsIAtom* aLocalName, const nsAString& aValue);
   nsresult SetAndTakeAttr(nsIAtom* aLocalName, nsAttrValue& aValue);
   nsresult SetAndTakeAttr(nsINodeInfo* aName, nsAttrValue& aValue);
-  nsresult RemoveAttrAt(PRUint32 aPos);
+
+  // Remove the attr at position aPos.  The value of the attr is placed in
+  // aValue; any value that was already in aValue is destroyed.
+  nsresult RemoveAttrAt(PRUint32 aPos, nsAttrValue& aValue);
 
   // Returns attribute name at given position, *not* out-of-bounds safe
   const nsAttrName* AttrNameAt(PRUint32 aPos) const;

content/base/src/nsGenericElement.cpp

@@ -3397,29 +3397,17 @@ nsGenericElement::UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
     }
   }
 
-  if (aNotify && nsContentUtils::HasMutationListeners(this,
-                   NS_EVENT_BITS_MUTATION_ATTRMODIFIED)) {
-    nsCOMPtr<nsIDOMEventTarget> node =
-      do_QueryInterface(NS_STATIC_CAST(nsIContent *, this));
-    nsMutationEvent mutation(PR_TRUE, NS_MUTATION_ATTRMODIFIED);
+  PRBool hasMutationListeners = aNotify &&
+    nsContentUtils::HasMutationListeners(this,
+                                         NS_EVENT_BITS_MUTATION_ATTRMODIFIED);
 
+  // Grab the attr node if needed before we remove it from the attr map
+  nsCOMPtr<nsIDOMAttr> attrNode;
+  if (hasMutationListeners) {
+    // XXXbz namespaces, dude!
     nsAutoString attrName;
     aName->ToString(attrName);
-    nsCOMPtr<nsIDOMAttr> attrNode;
     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);
-    if (!value.IsEmpty())
-      mutation.mPrevAttrValue = do_GetAtom(value);
-    mutation.mAttrChange = nsIDOMMutationEvent::REMOVAL;
-
-    nsEventDispatcher::Dispatch(this, nsnull, &mutation);
   }
 
   // Clear binding to nsIDOMNamedNodeMap
@@ -3428,7 +3416,8 @@ nsGenericElement::UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
     slots->mAttributeMap->DropAttribute(aNameSpaceID, aName);
   }
 
-  nsresult rv = mAttrsAndChildren.RemoveAttrAt(index);
+  nsAttrValue oldValue;
+  nsresult rv = mAttrsAndChildren.RemoveAttrAt(index, oldValue);
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (document) {
@@ -3442,6 +3431,23 @@ nsGenericElement::UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
                                   nsIDOMMutationEvent::REMOVAL);
   }
 
+  if (hasMutationListeners) {
+    nsCOMPtr<nsIDOMEventTarget> node =
+      do_QueryInterface(NS_STATIC_CAST(nsIContent *, this));
+    nsMutationEvent mutation(PR_TRUE, NS_MUTATION_ATTRMODIFIED);
+
+    mutation.mRelatedNode = attrNode;
+    mutation.mAttrName = aName;
+
+    nsAutoString value;
+    oldValue.ToString(value);
+    if (!value.IsEmpty())
+      mutation.mPrevAttrValue = do_GetAtom(value);
+    mutation.mAttrChange = nsIDOMMutationEvent::REMOVAL;
+
+    nsEventDispatcher::Dispatch(this, nsnull, &mutation);
+  }
+
   return NS_OK;
 }
 

content/base/src/nsMappedAttributes.cpp

@@ -234,8 +234,9 @@ nsMappedAttributes::List(FILE* out, PRInt32 aIndent) const
 #endif
 
 void
-nsMappedAttributes::RemoveAttrAt(PRUint32 aPos)
+nsMappedAttributes::RemoveAttrAt(PRUint32 aPos, nsAttrValue& aValue)
 {
+  Attrs()[aPos].mValue.SwapValueWith(aValue);
   Attrs()[aPos].~InternalAttr();
   memmove(&Attrs()[aPos], &Attrs()[aPos + 1],
           (mAttrCount - aPos - 1) * sizeof(InternalAttr));

content/base/src/nsMappedAttributes.h

@@ -95,7 +95,9 @@ public:
     NS_ASSERTION(aPos < mAttrCount, "out-of-bounds");
     return &Attrs()[aPos].mValue;
   }
-  void RemoveAttrAt(PRUint32 aPos);
+  // Remove the attr at position aPos.  The value of the attr is placed in
+  // aValue; any value that was already in aValue is destroyed.
+  void RemoveAttrAt(PRUint32 aPos, nsAttrValue& aValue);
   const nsAttrName* GetExistingAttrNameFromQName(const nsACString& aName) const;
   PRInt32 IndexOfAttr(nsIAtom* aLocalName, PRInt32 aNamespaceID) const;
   

content/xul/content/src/nsXULElement.cpp

@@ -1385,6 +1385,7 @@ nsXULElement::UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aName, PRBool aNotify)
 
     nsCOMPtr<nsIDOMAttr> attrNode;
     if (hasMutationListeners) {
+        // XXXbz namespaces, dude!
         nsAutoString attrName;
         aName->ToString(attrName);
         GetAttributeNode(attrName, getter_AddRefs(attrNode));
@@ -1395,7 +1396,8 @@ nsXULElement::UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aName, PRBool aNotify)
       slots->mAttributeMap->DropAttribute(aNameSpaceID, aName);
     }
 
-    rv = mAttrsAndChildren.RemoveAttrAt(index);
+    nsAttrValue ignored;
+    rv = mAttrsAndChildren.RemoveAttrAt(index, ignored);
     NS_ENSURE_SUCCESS(rv, rv);
 
     // XXX if the RemoveAttrAt() call fails, we might end up having removed