ValueTree Listener woes - exit crashes

[andym-2]

I’m having a tough time with ValueTree listeners, particularly when using removeListener, which is causing crashes in wrapped iterator when shutting down. The ValueTree I’m using originates in the Processor and gets passed to many classes including the voices in the juce Synth structure. All of my addListeners are in the constructors assigned as customData(cData) during construction, and all of the removeListeners are in the deconstructions. I’ve found that using a listener in voice class causes a lot of these issues & so have refrained from that, but it also affects classes which were passed in during construction which also originate in the processor class. The behaviour is erratic, it is different classes that cause the errors each time. I’ve tried commenting out all of the addListener/removeListeners and the crashes don’t happen. All of my passed classes are in unique/shared ptrs. I’m at a total loss as to why it happens, seems I’m missing something.

As I understand it, every addListener should have a paired removeListener in the deconstructor? If anyone has any advice on why this could be happening I’d be appreciative, I’ve been trying to solve it in & off for days & starting wonder about scrapping ValueTrees altogether, although I’m not sure how else to achieve the functionality they provide. I do pass the VT to the GUI & have issues there too sporadically when shutting down the Editor, it will poof Studio One for example.

I do use APVTS, but the valueTree I’m talking about is not part of it, I keep it separate & add/remove to the save state manually. The reason I do that is because the replace state breaks all listeners in the .state but seems fine with the parameters as far as I’m aware & so I can replace that state, and iterate through the values of my separate ValueTree manually updating each value.

Here is an example error, they are mostly the same but from different classes each time, always related to ValueTree listeners:

0x2ab6488ac juce::ListenerList<juce::ValueTree::Listener, juce::Array<juce::ValueTree::Listener*, juce::DummyCriticalSection, 0>>::WrappedIterator::invalidate() + 12

[aaronleese1]

+1

There is another thread on this, but not so helpful for this situation.
ListenerList crashes on AudioProcessor destruction - #64 by chkn

[andym-2]

This thread should be locked really, I found this one shortly after making it and continued there:

ListenerList crashes on AudioProcessor destruction

Hi all, My plug-in has a DataModel class that acts as a wrapper around a ValueTree, with its own Listener class and ListenerList for callbacks when changes are made to the data model. It is set-up in a similar manner to the JUCE - SamplerPlugin demo project. I have one main instance of the DataModel in my AudioProcessor which then gets copied around to other classes that then register as listeners to their copy. This way each class owns the DataModel object that it refers to and any Listeners …

The only solution for me was to use the JUCE 6 listener classes, it’s absolutely rock solid when they are swapped into v7, so the issue is with v7. Hopefully it will be fixed, or maybe the v6 code can be moved back to 7. I haven’t kept up to date with the builds so I don’t know if any progress has been made.

[aaronleese1]

I was able to fix it by adding a check in the ValueTree destructor.

The listener list is empty, but the remove statement is crashing, as you note.

Seems like this will work for me for now … but that WrappedIterator class seems like it has some issues

void ValueTree::removeListener (Listener* listener)
{ 
    if (listeners.size() == 0)
        return;
...
}

[aaronleese1]

Seems like it’s only an issue when there are multiple listeners, and multiple copies of the ValueTree (or parts of it).

So … simple plugins are fine, but processor made up of multiple processors that all use the same ValueTree (or parts of it) have major issues on destruction.

If I set all but the top level processor to copy the ValueTree and use that, then the problem resolves.

so something about the ref counting isn’t translating into the wrappedIterator maybe?

Just guessing, but I think it’s something like that

[andym-2]

When you say copy, do you mean clone it to a new value tree?

I think you are right, it’s likely down to reference counting, initially I was using a single valuetree for everything, but now between the processor & editor, I use multiple that are only updated in one direction. It didn’t solve the crashing though, but seems to be a better general practice.

[aaronleese1]

Yeah … ValueTree::createCopy().

None of mine are in the UI, just in different AudioProcessors. The crash doesn’t seem to have anything to do with threading issues. At least, when the crash happens in the destructor no other threads are doing anything that uses the valuetrees.

[andym-2]

Ahhh, that’s interesting, I spent a lot of time assuming it was thread related. Thanks for letting me know!

[aaronleese1]

Still figuring this out, but this is the basic situation that is crashing (for me) is something like this.

class SubProc : ValueTree::Listener
{
        ValueTree subState;

        SubProc (ValueTree node)
        { 
             subState = node;
             subState.addListener(this);
        }

       ~SubProc()
       {
             subState.removeListener(this);
       }

};

class Proc  : ValueTree::Listener
{
    std::unique_ptr<SubProc> sub;
    ValueTree state;

    Proc ()
    {
          state.addListener(this);

          sub.reset(new subProc(state.getChildWithName("whatever")));

    }
   
    ~Proc ()
    {
          state.removeListener(this);  // !!! CRASHES here 
    }

}

[LiamG]

ValueTree::Listeners are held by the lightweight ValueTree object itself, not by the underlying SharedObject that it points to. This means that you don’t ever need to call state.removeListener(this) in the destructor (assuming that state is a member variable). The array of listeners is about to go out of scope and be destructed, so there’s no need to remove an element from it.

That being said, the fact that you’re experiencing a crash here indicates that you’re doing something wrong, and you should probably try to fix it before removing the destructor altogether. It’s possible that you’re trying to dereference a nullptr somewhere, or that Proc is being deleted twice. Can you share more details about your runtime error?

By the way, the code you shared leaks an instance of SubProc, and you should be using std::unique_ptr. You probably know this already, but I’m just pointing it out.

[aaronleese1]

Sure, thanks for jumping in!

I had not realize the listeners lived in the wrapper, that makes a ton of sense.

The exact error is bad access in juce_ListenerList.h 112:

  WrappedIterator::forEach (activeIterators, [&] (auto& iter)
        {
            if (0 <= index && index < iter.get().index)
                --iter.get().index;
        });

I should note that if I don’t call removeListener in the destructor, I hit (basically) the same error when the ValueTree tires to destruct and cleans up the ListenerList (in ~ListenerList() there is a similar loop).

All points taken about the leaking / std_unique. Just not worrying about that for posting purposes here, my code makes use of all that as you suggest

[LiamG]

My guess is that you are deleting SubProc twice. Either that or you’re trying to deference a nullptr to SubProc (though that wouldn’t usually trigger the destructor, so it seems a bit less likely).

[oxxyyd]

@aaronleese1

~SubProc()
       {
             node.removeListener(this);
       }

Where does node come from?

SubProc(ValueTree node)
        { 
             node.addListener(this);
        }

The code above is rather meaningless as it appends a listener to a ValueTree object that is going out of scope at the following }. A dead/out-of-scoped ValueTree won’t hear a thing…

[aaronleese1]

Ok - fixed - added the unique_ptr too.

[oxxyyd]

When Proc is destroyed, state is destroyed first and thereafter sub. Maybe that’s problematic, while sub refers to a child of the newly destroyed state?

[aaronleese1]

Yeah, I thought about that. Here the code goes in the other order (subProc is explicitly deleted in the destructor), but I don’t think it should matter?

[aaronleese1]

Nope, no progress yet

[kunz]

I had this too. I’m using a dummy PluginProcessor with a ValueTree because I don’t want to notify the host of some parameters but still use the attachments and other useful features. It always crashed in deconstruction. Only Windows was affected and it was with one of the later JUCE 7 builds.

Reordering the declaration of the AudioProcesser and ValueTree in my class fixed the problem. I had to declare the TreeState below the AudioProcessor—this way the TreeState is destroyed before the audio processor. The order of the declaration is also the reverse order of deconstruction in C++ as far as I know.

The ValueTree probably accesses the AudioProcessor in deconstruction.

[aaronleese1]

@kunz

Reordering the declaration of the AudioProcesser and ValueTree

Right! Yeah, I saw this in the other thread too, and was already looking for that.

My issue is a bit further down the chain it seems. The above situation doesn’t crash unless I also link the ValueTree nodes to individual AudioProcessorParameters (I have a subclass with a ValueTree living inside it, any time a preset loads the new value tree nodes get handed out to all the parameters and things).

I’m sure someone will tell me that is a terrible idea (I’ll love to hear it, I can’t think of why that is bad design, but probably it is??).

[anthony-nicholls]

If the crash is happening in WrappedIterator::forEach then I would assume another thread is iterating over the listeners, otherwise there would be no active iterators for it to iterate over!

That being said I’ve finally managed to get some tweaks to the ListenerList into the develop branch. I’m not convinced your issue will be fixed with the changes though as I suspect the listeners are being notified, probably due to an audio parameter change (via automation maybe?) while the ListenerList is being destroyed.

I had hoped to adding some additional asserts but it turns out there are too many false positives when we those due to the way some hosts manage processing audio across multiple threads

ListenerList: Fix some edge cases when iterating the listeners