Random crash in FlexEnvelopeSource::generate

Hi!

On rare occasion, I encounter a crash in FlexEnvelopeSource::generate:

sfizz/src/sfizz/modulations/sources/FlexEnvelope.cpp

Lines 100 to 104 in 6b1a7cd

    
           const Region* region = voice->getRegion(); 
        
           if (egIndex >= region->flexEGs.size()) { 
        
               ASSERTFALSE; 
        
               return; 
        
           }

region is NULL.

Branch : develop
Target : macOS
SFZ file : https://gist.github.com/mathieugarcia/d06fe2d18fe0cb22565fb558e6581e27

I had similar issues. The host is Qtractor (under Linux). Sometimes, I can reproduce it rather quickly when I have a project with lots of sfizz instances playing all simultaneously. Then I start and stop the music multiple times in a row until it crashes.

Unfortunately, I have no experience programming in C++ that is worth mentioning. However, I can reproduce the problem and I can do a little debugging, so I figured out what went wrong on my machine.

This is what happens on my machine:

In the thread with ID 140736795019008

Voice::renderBlock is called
Voice::renderBlock checks if impl.region_ is a null pointer and returns if it is null. In this case, the pointer is not null and Voice::renderBlock continues

In the meanwhile, in the thread with ID 140737096557056

lilv_instance_deactivate() calls
Synth::allSoundOff(), which calls
Voice::reset()

Now I inserted a check in sfz::Voice::reset() that aborts when Voice::reset() is called while Voice::renderBlock() is busy (in another thread).
When reproducing the issue, the application aborts and I can print out the thread ID's from gdb. Without the check that I have inserted, the application would continue as follows:

In the thread with ID 140737096557056

sfz::Voice::reset() would set region_ to a null pointer

In the thread with ID 140736795019008

Voice::renderBlock would at some point use impl.region_ (whis is now a null pointer), crashing sfizz and its host.

Below you can find my patch that I used for diagnosing purposes. (Edit: I've updated the patch since there was a bug in the previous version. The conclusions are the same.) Note: this does not solve the problem, it only makes it easier for me to figure out what is going on.

diff --git a/src/sfizz/Voice.cpp b/src/sfizz/Voice.cpp
index 6380e26d..2a9978bc 100644
--- a/src/sfizz/Voice.cpp
+++ b/src/sfizz/Voice.cpp
@@ -313,6 +313,8 @@ struct Voice::Impl
     PowerFollower powerFollower_;
 
     ExtendedCCValues extendedCCValues_;
+    bool is_rendering {false };
+    std::thread::id rendering_thread_id;
 };
 
 Voice::Voice(int voiceNumber, Resources& resources)
@@ -773,12 +775,16 @@ void Voice::setSamplesPerBlock(int samplesPerBlock) noexcept
 void Voice::renderBlock(AudioSpan<float, 2> buffer) noexcept
 {
     Impl& impl = *impl_;
+    impl.is_rendering = true;
+    impl.rendering_thread_id = std::this_thread::get_id();
     ASSERT(static_cast<int>(buffer.getNumFrames()) <= impl.samplesPerBlock_);
     buffer.fill(0.0f);
 
     const Region* region = impl.region_;
-    if (region == nullptr || region->disabled())
+    if (region == nullptr || region->disabled()) {
+        impl.is_rendering = false;
         return;
+    }
 
     const auto delay = min(static_cast<size_t>(impl.initialDelay_), buffer.getNumFrames());
     auto delayed_buffer = buffer.subspan(delay);
@@ -826,6 +832,7 @@ void Voice::renderBlock(AudioSpan<float, 2> buffer) noexcept
     SFIZZ_CHECK(isReasonableAudio(buffer.getConstSpan(0)));
     SFIZZ_CHECK(isReasonableAudio(buffer.getConstSpan(1)));
 #endif
+    impl.is_rendering = false;
 }
 
 void Voice::Impl::resetCrossfades() noexcept
@@ -1689,6 +1696,11 @@ bool Voice::checkOffGroup(const Region* other, int delay, int noteNumber) noexce
 void Voice::reset() noexcept
 {
     Impl& impl = *impl_;
+    auto volatile is_rendering = impl.is_rendering;
+    if (is_rendering) {
+        volatile std::thread::id reset_thread_id = std::this_thread::get_id();
+        std::abort();
+    }
     impl.switchState(State::idle);
     impl.layer_ = nullptr;
     impl.region_ = nullptr;

In my case, it's probably an issue with the LV2 host (see the issue that mentions this issue).

@mathieugarcia what's the plugin host?

@PieterPenninckx Thanks for checking on all this. It seems a bit weird to me, but maybe I'm the one not following the expected practice with regard to activating/deactivating plugins. As it happens, at some point I had locks against destruction while running within the sfizz engine, but I removed them to delegate this to the plugin host.

@PieterPenninckx Thanks for checking on all this. It seems a bit weird to me, but maybe I'm the one not following the expected practice with regard to activating/deactivating plugins. As it happens, at some point I had locks against destruction while running within the sfizz engine, but I removed them to delegate this to the plugin host.

Indeed, either the host or the plugin has to ensure that the plugin is not deactivated while it is running. As far as I understand, LV2's threading rules put this responsibility on the host. The host that I use (Qtractor) doesn't do this when a particular feature that isn't really intended to be used ("Auto Deactivate"), is used. If I understand the comments on Qtractor's issue correctly, Qtractor is the only host that does this (and only when "Auto Deactivate" is used).

I think it's ok to not use locks any more in this particular situation. (Especially since it's not easy to implement this correctly because you're not supposed to deal with any locks in the real-time audio thread.)

OK well let's see what @mathieugarcia used and how it pans out. Thanks for following this up!

@mathieugarcia any news regarding this bug?

No, you can close it !

	const Region* region = voice->getRegion();
	if (egIndex >= region->flexEGs.size()) {
	ASSERTFALSE;
	return;
	}