Does this call stack make sense to anybody?

5   ???                                 0x0000000000000000 0x0 + 0
juce::ValueTree::SharedObject::sendParentChangeMessage() (juce_ValueTree.cpp:126)
juce::ValueTree::SharedObject::removeChild(int, juce::UndoManager*) (juce_ValueTree.cpp:298)
juce::ValueTree::SharedObject::AddOrRemoveChildAction::perform() (juce_ValueTree.cpp:0)
juce::UndoManager::perform(juce::UndoableAction*) (juce_UndoManager.cpp:125)
juce::ValueTree::SharedObject::removeChild(int, juce::UndoManager*) (juce_ValueTree.cpp:291)

Any idea how this could happen calling ValueTree::removeAllChildren? Or is this a symptom of heap corruption somewhere else?

Maybe you forgot to deregister a listener? Best trick to avoid this is to make a copy of the ValueTree and add the listener to this instance:

struct SafeListener: public ValueTree::Listener
{
    SafeListener(ValueTree& v):
       internalTree(v)
    {
        // register to the member object makes sure the lifetime of the listener never exceeds
        // the ValueTree's.
        internalTree.addListener(this);
        
        // this line would cause the listener to register to the external ValueTree object,
        // which might have a longer lifetime than this class.
        //v.addListener(this); //
    }

    ~SafeListener()
    {
        internalTree.removeListener(this); // this line is not necessary, but cleaner.
    }

    ValueTree internalTree;
};
2 Likes

On the subject of forgetting to remove Listeners, I have this in my git pre-commit hook

RED='\033[0;31m'
GREEN='\033[0;32m'
NC='\033[0m'

for file in `git diff --cached --name-only --diff-filter=ACM|grep cpp`
do
	adds=`grep -c -e "add.*Listener" "$file"`
	removes=`grep -c -e "remove.*Listener" "$file"`
	if [ $adds != $removes ]; then
		echo "${RED}WARNING!${NC}  add/remove Listener count does not match in ${GREEN}$file${NC}"
	fi
done

it raises some false positives occasionally, and only checks .cpp files, but more useful than it is problematic :slight_smile:

1 Like

That wouldn’t work for me, since when I listen to a member, I usually omit the removeListener. The thing I am listening to is destroyed before anyway.

1 Like
internalTree.removeListener(this); // this line is not necessary, but cleaner.

I religiously do this as well, just to get into the habit of removing listeners after adding them, I know there are a bunch of situations where you don’t need to, but it doesn’t hurt.

Isn’t that like saying you call object.reset() on all your unique_ptr members in your destructors though?

The goal of RAII is to simplify resource usage to the point of not having to write destructors. Rule of 0: https://en.cppreference.com/w/cpp/language/rule_of_three

1 Like

You were correct, found it. Thanks.

As long as JUCE’s Listener paradigm requires you to remove Listeners or they will be dangling (which could be solved by making the ListenerList pointers WeakReferences), I’ll prefer writing one useless destructor too many than the other way around :slight_smile:

Maybe… but what I’m advocating here is a style that doesn’t require you to have to think about it. The ListenerList paradigm is becoming more and more antiquated with the pervasiveness of std::function callbacks.

These suffer from the same problem but really, how often is it that you’re listening to an object that isn’t a member? Most Buttons, Sliders, TextEditors etc. you have local listeners or onClick callbacks so you don’t need to unregister them.

I’d argue that if you’re listening to a lot of objects from outside your Components then there is probably a better model to be used such as a ValueTree that you can take a local copy of.

The only real places we have to remove listeners is when you have global type objects (like MIDI learn) or more legacy classes. It’s a relatively rare problem.

Static analysis can probably help identify these problems and Asan certainly if you can catch it in the act in a debugger.

Also making ListenerList store WeakReferences is a bit tricky because then all of your classes would have to store a master reference which can bloat a lot of simple classes.
In my experience WeakReferences are most useful for async code and threading. In most other situations it’s better to handle the lifetimes manually or you spend half your time purging dead weak refs. (But I could be swayed on this, other languages have weak-refs built in which can lead to nice non-owning/observing semantics).

3 Likes

This sounds interesting. The software I’m working on is using ValueTrees, but the listeners are added to a single instance. So it’s possible that I take a copy of ValueTree, add a listener to the copy and it will respond to changes in the original ValueTree?

Yes. And a copy of a ValueTree is totally cheap, as it uses a shared object underneath.

2 Likes

Yes, but be careful. If you assign a new ValueTree to the source value tree, the source and copy value tree will now be pointing to different trees and the listeners will no longer work. This can cause some subtle bugs where listeners stop working.

1 Like

Yes, although this may seem a little convoluted at first it’s actually extremely useful.
If you have a situation where your ValueTree source can change (for example in a ListBox custom component) you can simply have your custom component have a ValueTree member, listen to that and then when your source for the row changes, just reassign the ValueTree. No need to de-register the current listener and add a new one etc.

I like to think of a ValueTree as a “view” on to the “model” with listeners owned by that “view”. You can redirect the underlying model to something else and your listeners will now just update corresponding to that new model.