ListenerList: Fix some edge cases when iterating the listeners

- Prevent out of scope access of the listeners lock
- Allow clearing the listener list from a callback

BREAKING_CHANGES.md

@@ -2,6 +2,26 @@
 
 # develop
 
+## Change
+
+The ListenerList::Iterator class has been removed.
+
+**Possible Issues**
+
+Any code directly referencing the ListenerList::Iterator will fail to compile.
+
+**Workaround**
+
+In most cases there should be a public member function that does the required
+job, for example, call, add, remove, or clear. In other cases you can access the
+raw array of listeners to iterate through them by calling getListeners().
+
+**Rationale**
+
+Iterating through the listeners using the ListenerList::Iterator could in a
+number of cases lead to surprising results and undefined behavior.
+
+
 ## Change
 
 The background colour of the Toolbar::CustomisationDialog has been changed from

modules/juce_core/containers/juce_ListenerList.cpp

@@ -300,6 +300,119 @@ public:
                 expect (test.wereAllNonRemovedListenersCalled (numCalls));
             }
         }
+
+        beginTest ("Deleting the listener list from a callback");
+        {
+            struct Listener
+            {
+                std::function<void()> onCallback;
+                void notify() { onCallback(); }
+            };
+
+            auto listeners = std::make_unique<juce::ListenerList<Listener>>();
+
+            const auto callback = [&]
+            {
+                expect (listeners != nullptr);
+                listeners.reset();
+            };
+
+            Listener listener1 { callback };
+            Listener listener2 { callback };
+
+            listeners->add (&listener1);
+            listeners->add (&listener2);
+
+            listeners->call (&Listener::notify);
+
+            expect (listeners == nullptr);
+        }
+
+        beginTest ("Using a BailOutChecker");
+        {
+            struct Listener
+            {
+                std::function<void()> onCallback;
+                void notify() { onCallback(); }
+            };
+
+            ListenerList<Listener> listeners;
+
+            bool listener1Called = false;
+            bool listener2Called = false;
+            bool listener3Called = false;
+
+            Listener listener1 { [&]{ listener1Called = true; } };
+            Listener listener2 { [&]{ listener2Called = true; } };
+            Listener listener3 { [&]{ listener3Called = true; } };
+
+            listeners.add (&listener1);
+            listeners.add (&listener2);
+            listeners.add (&listener3);
+
+            struct BailOutChecker
+            {
+                bool& bailOutBool;
+                bool shouldBailOut() const { return bailOutBool; }
+            };
+
+            BailOutChecker bailOutChecker { listener2Called };
+            listeners.callChecked (bailOutChecker, &Listener::notify);
+
+            expect (  listener1Called);
+            expect (  listener2Called);
+            expect (! listener3Called);
+        }
+
+        beginTest ("Using a critical section");
+        {
+            struct Listener
+            {
+                std::function<void()> onCallback;
+                void notify() { onCallback(); }
+            };
+
+            struct TestCriticalSection
+            {
+                TestCriticalSection()  { isAlive() = true; }
+                ~TestCriticalSection() { isAlive() = false; }
+
+                static void enter() noexcept { numOutOfScopeCalls() += isAlive() ? 0 : 1; }
+                static void exit() noexcept  { numOutOfScopeCalls() += isAlive() ? 0 : 1; }
+
+                static bool tryEnter() noexcept
+                {
+                    numOutOfScopeCalls() += isAlive() ? 0 : 1;
+                    return true;
+                }
+
+                using ScopedLockType = GenericScopedLock<TestCriticalSection>;
+
+                static bool& isAlive()
+                {
+                    static bool inScope = false;
+                    return inScope;
+                }
+
+                static int& numOutOfScopeCalls()
+                {
+                    static int numOutOfScopeCalls = 0;
+                    return numOutOfScopeCalls;
+                }
+            };
+
+            auto listeners = std::make_unique<juce::ListenerList<Listener, Array<Listener*, TestCriticalSection>>>();
+
+            const auto callback = [&]{ listeners.reset(); };
+
+            Listener listener { callback };
+
+            listeners->add (&listener);
+            listeners->call (&Listener::notify);
+
+            expect (listeners == nullptr);
+            expect (TestCriticalSection::numOutOfScopeCalls() == 0);
+        }
     }
 
 private:

modules/juce_core/containers/juce_ListenerList.h

@@ -25,11 +25,12 @@ namespace juce
 
 //==============================================================================
 /**
-    Holds a set of objects and can invoke a member function callback on each object
-    in the set with a single call.
+    Holds a set of objects and can invoke a member function callback on each
+    object in the set with a single call.
 
     Use a ListenerList to manage a set of objects which need a callback, and you
-    can invoke a member function by simply calling call() or callChecked().
+    can invoke a member function by simply calling call(), callChecked(), or
+    callExcluding().
 
     E.g.
     @code
@@ -47,20 +48,30 @@ namespace juce
     listeners.call ([] (MyListenerType& l) { l.myCallbackMethod (1234, true); });
     @endcode
 
-    It is guaranteed that every Listener is called during an iteration if it's inside the
-    ListenerList before the iteration starts and isn't removed until its end. This guarantee
-    holds even if some Listeners are removed or new ones are added during the iteration.
+    It is safe to add listeners, remove listeners, clear the listeners, and
+    delete the ListenerList itself during any listener callback.
 
-    Listeners added during an iteration are guaranteed to be not called in that iteration.
+    If a Listener is added during a callback, it is guaranteed not to be called
+    in the same iteration.
 
-    Sometimes, there's a chance that invoking one of the callbacks might result in the
-    list itself being deleted while it's still iterating - to survive this situation, you can
-    use callChecked() instead of call(), passing it a local object to act as a "BailOutChecker".
-    The BailOutChecker must implement a method of the form "bool shouldBailOut()", and
-    the list will check this after each callback to determine whether it should abort the
-    operation. For an example of a bail-out checker, see the Component::BailOutChecker class,
-    which can be used to check when a Component has been deleted. See also
-    ListenerList::DummyBailOutChecker, which is a dummy checker that always returns false.
+    If a Listener is removed during a callback, it is guaranteed not to be
+    called if it hasn't already been called.
+
+    If the ListenerList is cleared or deleted during a callback, it is
+    guaranteed that no more listeners will be called.
+
+    By default a ListenerList is not thread safe. If thread-safety is required,
+    you can provide a thread-safe Array type as the second type parameter e.g.
+    @code
+    using ThreadSafeList = ListenerList<MyListenerType, Array<MyListenerType*, CriticalSection>>;
+    @endcode
+
+    When calling listeners the iteration can be escaped early by using a
+    "BailOutChecker". A BailOutChecker is a type that has a public member function
+    with the following signature:
+    @code bool shouldBailOut() const @endcode
+    This function will be called before making a call to each listener.
+    For an example see the DummyBailOutChecker.
 
     @tags{Core}
 */
@@ -74,44 +85,42 @@ public:
     ListenerList() = default;
 
     /** Destructor. */
-    ~ListenerList()
-    {
-        WrappedIterator::forEach (activeIterators, [&] (auto& iter)
-        {
-            iter.invalidate();
-        });
-    }
+    ~ListenerList() { clear(); }
 
     //==============================================================================
     /** Adds a listener to the list.
         A listener can only be added once, so if the listener is already in the list,
         this method has no effect.
+
+        If a Listener is added during a callback, it is guaranteed not to be called
+        in the same iteration.
+
         @see remove
     */
     void add (ListenerClass* listenerToAdd)
     {
         if (listenerToAdd != nullptr)
-            listeners.addIfNotAlreadyThere (listenerToAdd);
+            listeners->addIfNotAlreadyThere (listenerToAdd);
         else
-            jassertfalse;  // Listeners can't be null pointers!
+            jassertfalse; // Listeners can't be null pointers!
     }
 
     /** Removes a listener from the list.
         If the listener wasn't in the list, this has no effect.
+
+        If a Listener is removed during a callback, it is guaranteed not to be
+        called if it hasn't already been called.
     */
     void remove (ListenerClass* listenerToRemove)
     {
         jassert (listenerToRemove != nullptr); // Listeners can't be null pointers!
 
-        typename ArrayType::ScopedLockType lock (listeners.getLock());
+        const ScopedLockType lock (listeners->getLock());
 
-        const auto index = listeners.removeFirstMatchingValue (listenerToRemove);
-
-        WrappedIterator::forEach (activeIterators, [&] (auto& iter)
-        {
-            if (0 <= index && index < iter.get().index)
-                --iter.get().index;
-        });
+        if (const auto index = listeners->removeFirstMatchingValue (listenerToRemove); index >= 0)
+            for (auto* indexPtr : *indices)
+                if (index <= *indexPtr)
+                    --*indexPtr;
     }
 
     /** Adds a listener that will be automatically removed again when the Guard is destroyed.
@@ -127,250 +136,188 @@ public:
     }
 
     /** Returns the number of registered listeners. */
-    int size() const noexcept                                { return listeners.size(); }
+    int size() const noexcept                                { return listeners->size(); }
 
     /** Returns true if no listeners are registered, false otherwise. */
-    bool isEmpty() const noexcept                            { return listeners.isEmpty(); }
+    bool isEmpty() const noexcept                            { return listeners->isEmpty(); }
 
-    /** Clears the list. */
-    void clear()                                             { listeners.clear(); }
+    /** Clears the list.
+
+        If the ListenerList is cleared during a callback, it is guaranteed that
+        no more listeners will be called.
+    */
+    void clear()                                             { listeners->clear(); }
 
     /** Returns true if the specified listener has been added to the list. */
-    bool contains (ListenerClass* listener) const noexcept   { return listeners.contains (listener); }
+    bool contains (ListenerClass* listener) const noexcept   { return listeners->contains (listener); }
 
-    /** Returns the raw array of listeners. */
-    const ArrayType& getListeners() const noexcept           { return listeners; }
+    /** Returns the raw array of listeners.
+
+        Any attempt to mutate the array may result in undefined behaviour.
+
+        If the array uses a mutex/CriticalSection, reading from the array without first
+        obtaining the lock may potentially result in undefined behaviour.
+
+        @see add, remove, clear, contains
+    */
+    const ArrayType& getListeners() const noexcept           { return *listeners; }
 
     //==============================================================================
-    /** Calls a member function on each listener in the list, with multiple parameters. */
+    /** Calls an invokable object for each listener in the list. */
     template <typename Callback>
     void call (Callback&& callback)
     {
-        typename ArrayType::ScopedLockType lock (listeners.getLock());
-
-        for (WrappedIterator iter (*this, activeIterators); iter.get().next();)
-            callback (*iter.get().getListener());
+        callCheckedExcluding (nullptr,
+                              DummyBailOutChecker{},
+                              std::forward<Callback> (callback));
     }
 
-    /** Calls a member function with 1 parameter, on all but the specified listener in the list.
-        This can be useful if the caller is also a listener and needs to exclude itself.
+    /** Calls an invokable object for each listener in the list, except for the
+        listener specified by listenerToExclude.
     */
     template <typename Callback>
     void callExcluding (ListenerClass* listenerToExclude, Callback&& callback)
     {
-        typename ArrayType::ScopedLockType lock (listeners.getLock());
+        callCheckedExcluding (listenerToExclude,
+                              DummyBailOutChecker{},
+                              std::forward<Callback> (callback));
 
-        for (WrappedIterator iter (*this, activeIterators); iter.get().next();)
-        {
-            auto* l = iter.get().getListener();
-
-            if (l != listenerToExclude)
-                callback (*l);
-        }
     }
 
-    /** Calls a member function on each listener in the list, with 1 parameter and a bail-out-checker.
+    /** Calls an invokable object for each listener in the list, additionally
+        checking the bail-out checker before each call.
+
         See the class description for info about writing a bail-out checker.
     */
     template <typename Callback, typename BailOutCheckerType>
     void callChecked (const BailOutCheckerType& bailOutChecker, Callback&& callback)
     {
-        typename ArrayType::ScopedLockType lock (listeners.getLock());
-
-        for (WrappedIterator iter (*this, activeIterators); iter.get().next (bailOutChecker);)
-        {
-            callback (*iter.get().getListener());
-        }
+        callCheckedExcluding (nullptr,
+                              bailOutChecker,
+                              std::forward<Callback> (callback));
     }
 
-    /** Calls a member function, with 1 parameter, on all but the specified listener in the list
-        with a bail-out-checker. This can be useful if the caller is also a listener and needs to
-        exclude itself. See the class description for info about writing a bail-out checker.
+    /** Calls an invokable object for each listener in the list, except for the
+        listener specified by listenerToExclude, additionally checking the
+        bail-out checker before each call.
+
+        See the class description for info about writing a bail-out checker.
     */
     template <typename Callback, typename BailOutCheckerType>
     void callCheckedExcluding (ListenerClass* listenerToExclude,
                                const BailOutCheckerType& bailOutChecker,
                                Callback&& callback)
     {
-        typename ArrayType::ScopedLockType lock (listeners.getLock());
+        const auto localListeners = listeners;
+        const ScopedLockType lock { localListeners->getLock() };
 
-        for (WrappedIterator iter (*this, activeIterators); iter.get().next (bailOutChecker);)
+        int index{};
+
+        indices->push_back (&index);
+        const ScopeGuard scope { [i = indices, &index]
         {
-            auto* l = iter.get().getListener();
+            i->erase (std::remove (i->begin(), i->end(), &index), i->end());
+        } };
 
-            if (l != listenerToExclude)
-                callback (*l);
+        for (const auto end = localListeners->size(); index < jmin (end, localListeners->size()); ++index)
+        {
+            if (bailOutChecker.shouldBailOut())
+                return;
+
+            auto* listener = localListeners->getUnchecked (index);
+
+            if (listener == listenerToExclude)
+                continue;
+
+            callback (*listener);
         }
     }
 
     //==============================================================================
-    /** A dummy bail-out checker that always returns false.
-        See the ListenerList notes for more info about bail-out checkers.
-    */
-    struct DummyBailOutChecker
-    {
-        bool shouldBailOut() const noexcept                 { return false; }
-    };
-
-    using ThisType      = ListenerList<ListenerClass, ArrayType>;
-    using ListenerType  = ListenerClass;
-
-    //==============================================================================
-    /** Iterates the listeners in a ListenerList. */
-    struct Iterator
-    {
-        explicit Iterator (const ListenerList& listToIterate) noexcept
-            : list (listToIterate), index (listToIterate.size())
-        {}
-
-        //==============================================================================
-        bool next() noexcept
-        {
-            if (index <= 0)
-                return false;
-
-            auto listSize = list.size();
-
-            if (--index < listSize)
-                return true;
-
-            index = listSize - 1;
-            return index >= 0;
-        }
-
-        template <class BailOutCheckerType>
-        bool next (const BailOutCheckerType& bailOutChecker) noexcept
-        {
-            return (! bailOutChecker.shouldBailOut()) && next();
-        }
-
-        ListenerClass* getListener() const noexcept
-        {
-            return list.getListeners().getUnchecked (index);
-        }
-
-    private:
-        const ListenerList& list;
-        int index;
-
-        friend ListenerList;
-
-        JUCE_DECLARE_NON_COPYABLE (Iterator)
-    };
-
-    //==============================================================================
-   #ifndef DOXYGEN
-    void call (void (ListenerClass::*callbackFunction)())
-    {
-        call ([=] (ListenerClass& l) { (l.*callbackFunction)(); });
-    }
-
-    void callExcluding (ListenerClass* listenerToExclude, void (ListenerClass::*callbackFunction)())
-    {
-        callExcluding (listenerToExclude, [=] (ListenerClass& l) { (l.*callbackFunction)(); });
-    }
-
-    template <class BailOutCheckerType>
-    void callChecked (const BailOutCheckerType& bailOutChecker, void (ListenerClass::*callbackFunction)())
-    {
-        callChecked (bailOutChecker, [=] (ListenerClass& l) { (l.*callbackFunction)(); });
-    }
-
-    template <class BailOutCheckerType>
-    void callCheckedExcluding (ListenerClass* listenerToExclude,
-                               const BailOutCheckerType& bailOutChecker,
-                               void (ListenerClass::*callbackFunction)())
-    {
-        callCheckedExcluding (listenerToExclude, bailOutChecker, [=] (ListenerClass& l) { (l.*callbackFunction)(); });
-    }
-
+    /** Calls a specific listener method for each listener in the list. */
     template <typename... MethodArgs, typename... Args>
     void call (void (ListenerClass::*callbackFunction) (MethodArgs...), Args&&... args)
     {
-        typename ArrayType::ScopedLockType lock (listeners.getLock());
-
-        for (Iterator iter (*this); iter.next();)
-            (iter.getListener()->*callbackFunction) (static_cast<typename TypeHelpers::ParameterType<Args>::type> (args)...);
+        callCheckedExcluding (nullptr,
+                              DummyBailOutChecker{},
+                              callbackFunction,
+                              std::forward<Args> (args)...);
     }
 
+    /** Calls a specific listener method for each listener in the list, except
+        for the listener specified by listenerToExclude.
+    */
     template <typename... MethodArgs, typename... Args>
     void callExcluding (ListenerClass* listenerToExclude,
                         void (ListenerClass::*callbackFunction) (MethodArgs...),
                         Args&&... args)
     {
-        typename ArrayType::ScopedLockType lock (listeners.getLock());
-
-        for (Iterator iter (*this); iter.next();)
-            if (iter.getListener() != listenerToExclude)
-                (iter.getListener()->*callbackFunction) (static_cast<typename TypeHelpers::ParameterType<Args>::type> (args)...);
+        callCheckedExcluding (listenerToExclude,
+                              DummyBailOutChecker{},
+                              callbackFunction,
+                              std::forward<Args> (args)...);
     }
 
+    /** Calls a specific listener method for each listener in the list,
+        additionally checking the bail-out checker before each call.
+
+        See the class description for info about writing a bail-out checker.
+    */
     template <typename BailOutCheckerType, typename... MethodArgs, typename... Args>
     void callChecked (const BailOutCheckerType& bailOutChecker,
                       void (ListenerClass::*callbackFunction) (MethodArgs...),
                       Args&&... args)
     {
-        typename ArrayType::ScopedLockType lock (listeners.getLock());
-
-        for (Iterator iter (*this); iter.next (bailOutChecker);)
-            (iter.getListener()->*callbackFunction) (static_cast<typename TypeHelpers::ParameterType<Args>::type> (args)...);
+        callCheckedExcluding (nullptr,
+                              bailOutChecker,
+                              callbackFunction,
+                              std::forward<Args> (args)...);
     }
 
+    /** Calls a specific listener method for each listener in the list, except
+        for the listener specified by listenerToExclude, additionally checking
+        the bail-out checker before each call.
+
+        See the class description for info about writing a bail-out checker.
+    */
     template <typename BailOutCheckerType, typename... MethodArgs, typename... Args>
     void callCheckedExcluding (ListenerClass* listenerToExclude,
                                const BailOutCheckerType& bailOutChecker,
                                void (ListenerClass::*callbackFunction) (MethodArgs...),
                                Args&&... args)
     {
-        typename ArrayType::ScopedLockType lock (listeners.getLock());
-
-        for (Iterator iter (*this); iter.next (bailOutChecker);)
-            if (iter.getListener() != listenerToExclude)
-                (iter.getListener()->*callbackFunction) (static_cast<typename TypeHelpers::ParameterType<Args>::type> (args)...);
+        callCheckedExcluding (listenerToExclude, bailOutChecker, [&] (ListenerClass& l)
+        {
+            (l.*callbackFunction) (args...);
+        });
     }
-   #endif
 
-private:
-    class WrappedIterator
+    //==============================================================================
+    /** A dummy bail-out checker that always returns false.
+        See the class description for info about writing a bail-out checker.
+    */
+    struct DummyBailOutChecker
     {
-    public:
-        WrappedIterator (const ListenerList& listToIterate, WrappedIterator*& listHeadIn)
-            : it (listToIterate), listHead (listHeadIn), next (listHead)
-        {
-            // GCC 12.2 with O1 and above gets confused here
-            JUCE_BEGIN_IGNORE_WARNINGS_GCC_LIKE ("-Wdangling-pointer")
-            listHead = this;
-            JUCE_END_IGNORE_WARNINGS_GCC_LIKE
-        }
-
-        ~WrappedIterator()
-        {
-            if (valid)
-                listHead = next;
-        }
-
-        auto& get() noexcept { return it; }
-
-        template <typename Callback>
-        static void forEach (WrappedIterator* wrapped, Callback&& cb)
-        {
-            for (auto* p = wrapped; p != nullptr; p = p->next)
-                cb (*p);
-        }
-
-        void invalidate() noexcept { valid = false; }
-
-    private:
-        Iterator it;
-        WrappedIterator*& listHead;
-        WrappedIterator* next = nullptr;
-        bool valid = true;
+        constexpr bool shouldBailOut() const noexcept { return false; }
     };
 
     //==============================================================================
-    ArrayType listeners;
-    WrappedIterator* activeIterators = nullptr;
+    using ThisType      = ListenerList<ListenerClass, ArrayType>;
+    using ListenerType  = ListenerClass;
 
+private:
+    //==============================================================================
+    using ScopedLockType = typename ArrayType::ScopedLockType;
+
+    //==============================================================================
+    using SharedListeners = std::shared_ptr<ArrayType>;
+    const SharedListeners listeners = std::make_shared<ArrayType>();
+
+    using SafeIndices = std::vector<int*>;
+    using SharedIndices = std::shared_ptr<SafeIndices>;
+    const SharedIndices indices = std::make_shared<SafeIndices>();
+
+    //==============================================================================
     JUCE_DECLARE_NON_COPYABLE (ListenerList)
 };