Fixed an issue iterating arrays of owned objects from object destructors

modules/juce_core/containers/juce_OwnedArray.cpp

@@ -27,17 +27,49 @@ namespace juce
 
 static struct OwnedArrayTest : public UnitTest
 {
-    OwnedArrayTest() : UnitTest { "OwnedArray" } {}
-
     struct Base
     {
+        Base() = default;
         virtual ~Base() = default;
+
+        JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (Base)
     };
 
     struct Derived : Base
     {
+        Derived() = default;
+
+        JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (Derived)
+    };
+
+    struct DestructorObj
+    {
+        DestructorObj (OwnedArrayTest& p,
+                       OwnedArray<DestructorObj>& arr)
+            : parent (p), objectArray (arr)
+        {}
+
+        ~DestructorObj()
+        {
+            data = 0;
+
+            for (auto* o : objectArray)
+            {
+                parent.expect (o != nullptr);
+                parent.expect (o != this);
+                parent.expectEquals (o->data, 956);
+            }
+        }
+
+        OwnedArrayTest& parent;
+        OwnedArray<DestructorObj>& objectArray;
+        int data = 956;
+
+        JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (DestructorObj)
     };
 
+    OwnedArrayTest() : UnitTest ("OwnedArray", "Containers") {}
+
     void runTest() override
     {
         beginTest ("After converting move construction, ownership is transferred");
@@ -58,6 +90,34 @@ static struct OwnedArrayTest : public UnitTest
 
             expectEquals (base.size(), 3);
         }
+
+        beginTest ("Iterate in destructor");
+        {
+            {
+                OwnedArray<DestructorObj> arr;
+
+                for (int i = 0; i < 2; ++i)
+                    arr.add (new DestructorObj (*this, arr));
+            }
+
+            OwnedArray<DestructorObj> arr;
+
+            for (int i = 0; i < 1025; ++i)
+                arr.add (new DestructorObj (*this, arr));
+
+            while (! arr.isEmpty())
+                arr.remove (0);
+
+            for (int i = 0; i < 1025; ++i)
+                arr.add (new DestructorObj (*this, arr));
+
+            arr.removeRange (1, arr.size() - 3);
+
+            for (int i = 0; i < 1025; ++i)
+                arr.add (new DestructorObj (*this, arr));
+
+            arr.set (500, new DestructorObj (*this, arr));
+        }
     }
 } ownedArrayTest;
 

modules/juce_core/containers/juce_OwnedArray.h

@@ -625,17 +625,16 @@ public:
 
         if (numberToRemove > 0)
         {
+            Array<ObjectClass*> objectsToDelete;
+
             if (deleteObjects)
-            {
-                for (int i = startIndex; i < endIndex; ++i)
-                {
-                    ContainerDeletePolicy<ObjectClass>::destroy (values[i]);
-                    values[i] = nullptr; // (in case one of the destructors accesses this array and hits a dangling pointer)
-                }
-            }
+                objectsToDelete.addArray (values.begin() + startIndex, numberToRemove);
 
             values.removeElements (startIndex, numberToRemove);
 
+            for (auto& o : objectsToDelete)
+                ContainerDeletePolicy<ObjectClass>::destroy (o);
+
             if ((values.size() << 1) < values.capacity())
                 minimiseStorageOverheads();
         }
@@ -792,10 +791,14 @@ private:
 
     void deleteAllObjects()
     {
-        for (auto& e : values)
-            ContainerDeletePolicy<ObjectClass>::destroy (e);
+        auto i = values.size();
 
-        values.clear();
+        while (--i >= 0)
+        {
+            auto* e = values[i];
+            values.removeElements (i, 1);
+            ContainerDeletePolicy<ObjectClass>::destroy (e);
+        }
     }
 
     template <class OtherObjectClass, class OtherCriticalSection>

modules/juce_core/containers/juce_ReferenceCountedArray.cpp

@@ -97,6 +97,34 @@ public:
             for (auto o : derivedArray)
                 expectEquals (o->getReferenceCount(), 3);
         }
+
+        beginTest ("Iterate in destructor");
+        {
+            {
+                ReferenceCountedArray<DestructorObj> arr;
+
+                for (int i = 0; i < 2; ++i)
+                    arr.add (new DestructorObj (*this, arr));
+            }
+
+            ReferenceCountedArray<DestructorObj> arr;
+
+            for (int i = 0; i < 1025; ++i)
+                arr.add (new DestructorObj (*this, arr));
+
+            while (! arr.isEmpty())
+                arr.remove (0);
+
+            for (int i = 0; i < 1025; ++i)
+                arr.add (new DestructorObj (*this, arr));
+
+            arr.removeRange (1, arr.size() - 3);
+
+            for (int i = 0; i < 1025; ++i)
+                arr.add (new DestructorObj (*this, arr));
+
+            arr.set (500, new DestructorObj (*this, arr));
+        }
     }
 
 private:
@@ -105,6 +133,8 @@ private:
         using Ptr = ReferenceCountedObjectPtr<TestBaseObj>;
 
         TestBaseObj() = default;
+
+        JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (TestBaseObj)
     };
 
     struct TestDerivedObj : public TestBaseObj
@@ -112,6 +142,34 @@ private:
         using Ptr = ReferenceCountedObjectPtr<TestDerivedObj>;
 
         TestDerivedObj() = default;
+
+        JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (TestDerivedObj)
+    };
+
+    struct DestructorObj : public ReferenceCountedObject
+    {
+        DestructorObj (ReferenceCountedArrayTests& p,
+                       ReferenceCountedArray<DestructorObj>& arr)
+            : parent (p), objectArray (arr)
+        {}
+
+        ~DestructorObj()
+        {
+            data = 0;
+
+            for (auto* o : objectArray)
+            {
+                parent.expect (o != nullptr);
+                parent.expect (o != this);
+                parent.expectEquals (o->data, 374);
+            }
+        }
+
+        ReferenceCountedArrayTests& parent;
+        ReferenceCountedArray<DestructorObj>& objectArray;
+        int data = 374;
+
+        JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (DestructorObj)
     };
 };
 

modules/juce_core/containers/juce_ReferenceCountedArray.h

@@ -437,8 +437,9 @@ public:
 
             if (indexToChange < values.size())
             {
-                releaseObject (values[indexToChange]);
+                auto* e = values[indexToChange];
                 values[indexToChange] = newObject;
+                releaseObject (e);
             }
             else
             {
@@ -570,9 +571,9 @@ public:
 
         if (isPositiveAndBelow (indexToRemove, values.size()))
         {
-            auto** e = values.begin() + indexToRemove;
-            releaseObject (*e);
+            auto* e = *(values.begin() + indexToRemove);
             values.removeElements (indexToRemove, 1);
+            releaseObject (e);
 
             if ((values.size() << 1) < values.capacity())
                 minimiseStorageOverheads();
@@ -595,10 +596,10 @@ public:
 
         if (isPositiveAndBelow (indexToRemove, values.size()))
         {
-            auto** e = values.begin() + indexToRemove;
-            removedItem = *e;
-            releaseObject (*e);
+            auto* e = *(values.begin() + indexToRemove);
+            removedItem = e;
             values.removeElements (indexToRemove, 1);
+            releaseObject (e);
 
             if ((values.size() << 1) < values.capacity())
                 minimiseStorageOverheads();
@@ -656,14 +657,14 @@ public:
 
         if (numberToRemove > 0)
         {
-            for (int i = startIndex; i < endIndex; ++i)
-            {
-                releaseObject (values[i]);
-                values[i] = nullptr; // (in case one of the destructors accesses this array and hits a dangling pointer)
-            }
+            Array<ObjectClass*> objectsToRemove;
+            objectsToRemove.addArray (values.begin() + startIndex, numberToRemove);
 
             values.removeElements (startIndex, numberToRemove);
 
+            for (auto& o : objectsToRemove)
+                releaseObject (o);
+
             if ((values.size() << 1) < values.capacity())
                 minimiseStorageOverheads();
         }
@@ -848,10 +849,14 @@ private:
 
     void releaseAllObjects()
     {
-        for (auto& v : values)
-            releaseObject (v);
+        auto i = values.size();
 
-        values.clear();
+        while (--i >= 0)
+        {
+            auto* e = values[i];
+            values.removeElements (i, 1);
+            releaseObject (e);
+        }
     }
 
     static void releaseObject (ObjectClass* o)