Bug 620122. Make sure, when unlinking, to remove the kids from our child list before unbinding them. r=smaug, a=legneato
authorBoris Zbarsky <bzbarsky@mit.edu>
Wed, 02 Nov 2011 23:36:16 -0400
changeset 79161 c845d62ee43f88dd9574de1c9fce367e14bdef11
parent 79160 d518534ad916810a5520b7eb9cd0e2952284f558
child 79162 a78bbe2376beffc838457ab5c3dd731d76597669
push id78
push userclegnitto@mozilla.com
push dateFri, 16 Dec 2011 17:32:24 +0000
treeherdermozilla-release@79d24e644fdd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug, legneato
bugs620122
milestone9.0a2
Bug 620122. Make sure, when unlinking, to remove the kids from our child list before unbinding them. r=smaug, a=legneato
content/base/src/nsAttrAndChildArray.cpp
content/base/src/nsAttrAndChildArray.h
content/base/src/nsGenericElement.cpp
--- a/content/base/src/nsAttrAndChildArray.cpp
+++ b/content/base/src/nsAttrAndChildArray.cpp
@@ -214,32 +214,41 @@ nsAttrAndChildArray::InsertChildAt(nsICo
   SetChildCount(childCount + 1);
   
   return NS_OK;
 }
 
 void
 nsAttrAndChildArray::RemoveChildAt(PRUint32 aPos)
 {
+  // Just store the return value of TakeChildAt in an nsCOMPtr to
+  // trigger a release.
+  nsCOMPtr<nsIContent> child = TakeChildAt(aPos);
+}
+
+already_AddRefed<nsIContent>
+nsAttrAndChildArray::TakeChildAt(PRUint32 aPos)
+{
   NS_ASSERTION(aPos < ChildCount(), "out-of-bounds");
 
   PRUint32 childCount = ChildCount();
   void** pos = mImpl->mBuffer + AttrSlotsSize() + aPos;
   nsIContent* child = static_cast<nsIContent*>(*pos);
   if (child->mPreviousSibling) {
     child->mPreviousSibling->mNextSibling = child->mNextSibling;
   }
   if (child->mNextSibling) {
     child->mNextSibling->mPreviousSibling = child->mPreviousSibling;
   }
   child->mPreviousSibling = child->mNextSibling = nsnull;
 
-  NS_RELEASE(child);
   memmove(pos, pos + 1, (childCount - aPos - 1) * sizeof(nsIContent*));
   SetChildCount(childCount - 1);
+
+  return child;
 }
 
 PRInt32
 nsAttrAndChildArray::IndexOfChild(nsINode* aPossibleChild) const
 {
   if (!mImpl) {
     return -1;
   }
--- a/content/base/src/nsAttrAndChildArray.h
+++ b/content/base/src/nsAttrAndChildArray.h
@@ -90,16 +90,19 @@ public:
   nsIContent* GetSafeChildAt(PRUint32 aPos) const;
   nsIContent * const * GetChildArray(PRUint32* aChildCount) const;
   nsresult AppendChild(nsIContent* aChild)
   {
     return InsertChildAt(aChild, ChildCount());
   }
   nsresult InsertChildAt(nsIContent* aChild, PRUint32 aPos);
   void RemoveChildAt(PRUint32 aPos);
+  // Like RemoveChildAt but hands the reference to the child being
+  // removed back to the caller instead of just releasing it.
+  already_AddRefed<nsIContent> TakeChildAt(PRUint32 aPos);
   PRInt32 IndexOfChild(nsINode* aPossibleChild) const;
 
   PRUint32 AttrCount() const;
   const nsAttrValue* GetAttr(nsIAtom* aLocalName, PRInt32 aNamespaceID = kNameSpaceID_None) const;
   const nsAttrValue* AttrAt(PRUint32 aPos) const;
   nsresult SetAttr(nsIAtom* aLocalName, const nsAString& aValue);
   nsresult SetAndTakeAttr(nsIAtom* aLocalName, nsAttrValue& aValue);
   nsresult SetAndTakeAttr(nsINodeInfo* aName, nsAttrValue& aValue);
--- a/content/base/src/nsGenericElement.cpp
+++ b/content/base/src/nsGenericElement.cpp
@@ -4238,20 +4238,28 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(ns
     PRUint32 childCount = tmp->mAttrsAndChildren.ChildCount();
     if (childCount) {
       // Don't allow script to run while we're unbinding everything.
       nsAutoScriptBlocker scriptBlocker;
       while (childCount-- > 0) {
         // Once we have XPCOMGC we shouldn't need to call UnbindFromTree.
         // We could probably do a non-deep unbind here when IsInDoc is false
         // for better performance.
-        tmp->mAttrsAndChildren.ChildAt(childCount)->UnbindFromTree();
-        tmp->mAttrsAndChildren.RemoveChildAt(childCount);
+
+        // Hold a strong ref to the node when we remove it, because we may be
+        // the last reference to it.  We need to call TakeChildAt() and
+        // update mFirstChild before calling UnbindFromTree, since this last
+        // can notify various observers and they should really see consistent
+        // tree state.
+        nsCOMPtr<nsIContent> child = tmp->mAttrsAndChildren.TakeChildAt(childCount);
+        if (childCount == 0) {
+          tmp->mFirstChild = nsnull;
+        }
+        child->UnbindFromTree();
       }
-      tmp->mFirstChild = nsnull;
     }
   }  
 
   // Unlink any DOM slots of interest.
   {
     nsDOMSlots *slots = tmp->GetExistingDOMSlots();
     if (slots) {
       slots->Unlink(tmp->IsXUL());