Bug 570657 - Make the order of releasing objects and removing them in nsCOMArray's consistent; r=shaver

--HG--
extra : rebase_source : 16f05bbfbd31bc01e0545af111dc4e4adbac8e33

xpcom/glue/nsCOMArray.cpp

@@ -55,11 +55,7 @@ nsCOMArray_base::nsCOMArray_base(const nsCOMArray_base& aOther)
 
 nsCOMArray_base::~nsCOMArray_base()
 {
-    PRInt32 count = Count(), i;
-    for (i = 0; i < count; ++i) {
-        nsISupports* obj = ObjectAt(i);
-        NS_IF_RELEASE(obj);
-    }                        
+  Clear();
 }
 
 PRInt32
@@ -135,9 +131,10 @@ nsCOMArray_base::RemoveObjectAt(PRInt32 aIndex)
 {
     if (PRUint32(aIndex) < PRUint32(Count())) {
         nsISupports* element = ObjectAt(aIndex);
-        NS_IF_RELEASE(element);
 
-        return mArray.RemoveElementAt(aIndex);
+        PRBool result = mArray.RemoveElementAt(aIndex);
+        NS_IF_RELEASE(element);
+        return result;
     }
 
     return PR_FALSE;
@@ -155,8 +152,10 @@ ReleaseObjects(void* aElement, void*)
 void
 nsCOMArray_base::Clear()
 {
-    mArray.EnumerateForwards(ReleaseObjects, nsnull);
+    nsAutoVoidArray objects;
+    objects = mArray;
     mArray.Clear();
+    objects.EnumerateForwards(ReleaseObjects, nsnull);
 }
 
 PRBool
@@ -167,10 +166,15 @@ nsCOMArray_base::SetCount(PRInt32 aNewCount)
       return PR_FALSE;
 
     PRInt32 count = Count(), i;
-    for (i = aNewCount; i < count; ++i) {
-        nsISupports* obj = ObjectAt(i);
-        NS_IF_RELEASE(obj);
+    nsAutoVoidArray objects;
+    if (count > aNewCount) {
+        objects.SetCount(count - aNewCount);
+        for (i = aNewCount; i < count; ++i) {
+            objects.ReplaceElementAt(ObjectAt(i), i - aNewCount);
+        }
     }
-    return mArray.SetCount(aNewCount);
+    PRBool result = mArray.SetCount(aNewCount);
+    objects.EnumerateForwards(ReleaseObjects, nsnull);
+    return result;
 }
 

xpcom/glue/nsVoidArray.h

@@ -195,6 +195,12 @@ public:
     SetArray(reinterpret_cast<Impl*>(mAutoBuf), kAutoBufSize, 0, PR_FALSE,
              PR_TRUE);
   }
+
+  nsAutoVoidArray& operator=(const nsVoidArray& other)
+  {
+    nsVoidArray::operator=(other);
+    return *this;
+  }
   
 protected:
   // The internal storage

xpcom/tests/TestCOMArray.cpp

@@ -91,6 +91,72 @@ NS_IMPL_ISUPPORTS1(Foo, IFoo)
 typedef nsCOMArray<IFoo> Array;
 
 
+// {0e70a320-be02-11d1-8031-006008159b5a}
+#define NS_IBAR_IID \
+  {0x0e70a320, 0xbe02, 0x11d1,    \
+    {0x80, 0x31, 0x00, 0x60, 0x08, 0x15, 0x9b, 0x5a}}
+
+class IBar : public nsISupports {
+public:
+
+  NS_DECLARE_STATIC_IID_ACCESSOR(NS_IBAR_IID)
+};
+
+NS_DEFINE_STATIC_IID_ACCESSOR(IBar, NS_IBAR_IID)
+
+class Bar : public IBar {
+public:
+
+  Bar(nsCOMArray<IBar>& aArray, PRInt32 aIndex);
+  ~Bar();
+
+  // nsISupports implementation
+  NS_DECL_ISUPPORTS
+
+  static PRInt32 sReleaseCalled;
+
+private:
+  nsCOMArray<IBar>& mArray;
+  PRInt32 mIndex;
+};
+
+PRInt32 Bar::sReleaseCalled = 0;
+
+typedef nsCOMArray<IBar> Array2;
+
+Bar::Bar(Array2& aArray, PRInt32 aIndex)
+  : mArray(aArray)
+  , mIndex(aIndex)
+{
+}
+
+Bar::~Bar()
+{
+  if (mArray.RemoveObjectAt(mIndex)) {
+    fail("We should never manage to remove the object here");
+  }
+}
+
+NS_IMPL_ADDREF(Bar)
+NS_IMPL_QUERY_INTERFACE1(Bar, IBar)
+
+NS_IMETHODIMP_(nsrefcnt)
+Bar::Release(void)
+{
+  ++Bar::sReleaseCalled;
+  NS_PRECONDITION(0 != mRefCnt, "dup release");
+  NS_ASSERT_OWNINGTHREAD(_class);
+  --mRefCnt;
+  NS_LOG_RELEASE(this, mRefCnt, "Bar");
+  if (mRefCnt == 0) {
+    mRefCnt = 1; /* stabilize */
+    NS_DELETEXPCOM(this);
+    return 0;
+  }
+  return mRefCnt;
+}
+
+
 int main(int argc, char **argv)
 {
   ScopedXPCOM xpcom("nsCOMArrayTests");
@@ -142,5 +208,59 @@ int main(int argc, char **argv)
     }
   }
 
+  PRInt32 base;
+  {
+    Array2 arr2;
+
+    IBar *ninthObject;
+    for (PRInt32 i = 0; i < 20; ++i) {
+      nsCOMPtr<IBar> bar = new Bar(arr2, i);
+      if (i == 8) {
+        ninthObject = bar;
+      }
+      arr2.AppendObject(bar);
+    }
+
+    base = Bar::sReleaseCalled;
+
+    arr2.SetCount(10);
+    if (Bar::sReleaseCalled != base + 10) {
+      fail("Release called multiple times for SetCount");
+    }
+
+    arr2.RemoveObjectAt(9);
+    if (Bar::sReleaseCalled != base + 11) {
+      fail("Release called multiple times for RemoveObjectAt");
+    }
+
+    arr2.RemoveObject(ninthObject);
+    if (Bar::sReleaseCalled != base + 12) {
+      fail("Release called multiple times for RemoveObject");
+    }
+
+    arr2.Clear();
+    if (Bar::sReleaseCalled != base + 20) {
+      fail("Release called multiple times for Clear");
+    }
+  }
+
+  Bar::sReleaseCalled = 0;
+
+  {
+    Array2 arr2;
+
+    for (PRInt32 i = 0; i < 20; ++i) {
+      nsCOMPtr<IBar> bar  = new Bar(arr2, i);
+      arr2.AppendObject(bar);
+    }
+
+    base = Bar::sReleaseCalled;
+
+    // Let arr2 be destroyed
+  }
+  if (Bar::sReleaseCalled != base + 20) {
+    fail("Release called multiple times for nsCOMArray::~nsCOMArray");
+  }
+
   return rv;
 }