From a1dc1b1fce580b8cf5c5877827445f9bf46a9684 Mon Sep 17 00:00:00 2001 From: Tom Poole Date: Tue, 19 Mar 2019 11:44:15 +0000 Subject: [PATCH] Fixed an issue iterating arrays of owned objects from object destructors --- .../juce_core/containers/juce_OwnedArray.cpp | 64 ++++++++++++++++++- .../juce_core/containers/juce_OwnedArray.h | 23 ++++--- .../containers/juce_ReferenceCountedArray.cpp | 58 +++++++++++++++++ .../containers/juce_ReferenceCountedArray.h | 33 ++++++---- 4 files changed, 152 insertions(+), 26 deletions(-) diff --git a/modules/juce_core/containers/juce_OwnedArray.cpp b/modules/juce_core/containers/juce_OwnedArray.cpp index acb4027154..2d96b78f8c 100644 --- a/modules/juce_core/containers/juce_OwnedArray.cpp +++ b/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& 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& 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 arr; + + for (int i = 0; i < 2; ++i) + arr.add (new DestructorObj (*this, arr)); + } + + OwnedArray 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; diff --git a/modules/juce_core/containers/juce_OwnedArray.h b/modules/juce_core/containers/juce_OwnedArray.h index 8393b701c3..3ab95275b7 100644 --- a/modules/juce_core/containers/juce_OwnedArray.h +++ b/modules/juce_core/containers/juce_OwnedArray.h @@ -625,17 +625,16 @@ public: if (numberToRemove > 0) { + Array objectsToDelete; + if (deleteObjects) - { - for (int i = startIndex; i < endIndex; ++i) - { - ContainerDeletePolicy::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::destroy (o); + if ((values.size() << 1) < values.capacity()) minimiseStorageOverheads(); } @@ -792,10 +791,14 @@ private: void deleteAllObjects() { - for (auto& e : values) - ContainerDeletePolicy::destroy (e); + auto i = values.size(); - values.clear(); + while (--i >= 0) + { + auto* e = values[i]; + values.removeElements (i, 1); + ContainerDeletePolicy::destroy (e); + } } template diff --git a/modules/juce_core/containers/juce_ReferenceCountedArray.cpp b/modules/juce_core/containers/juce_ReferenceCountedArray.cpp index 15d380f566..91cb9cbe5a 100644 --- a/modules/juce_core/containers/juce_ReferenceCountedArray.cpp +++ b/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 arr; + + for (int i = 0; i < 2; ++i) + arr.add (new DestructorObj (*this, arr)); + } + + ReferenceCountedArray 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() = default; + + JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (TestBaseObj) }; struct TestDerivedObj : public TestBaseObj @@ -112,6 +142,34 @@ private: using Ptr = ReferenceCountedObjectPtr; TestDerivedObj() = default; + + JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (TestDerivedObj) + }; + + struct DestructorObj : public ReferenceCountedObject + { + DestructorObj (ReferenceCountedArrayTests& p, + ReferenceCountedArray& 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& objectArray; + int data = 374; + + JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (DestructorObj) }; }; diff --git a/modules/juce_core/containers/juce_ReferenceCountedArray.h b/modules/juce_core/containers/juce_ReferenceCountedArray.h index db9eded81a..975451f1da 100644 --- a/modules/juce_core/containers/juce_ReferenceCountedArray.h +++ b/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 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)