Please fix dangerous assertion

In juce_AudioProcessor.cpp, line 900 (as of the last time I puled the code) contains the following seemingly innocuous statement

    jassert (hasEditor() == (ed != nullptr));

However, some implementations of hasEditor(), for example the version in juce_VST3PluginFormat.cpp, starting at line 2779 actually create an editor if one does not exist.

In other words, the call to hasEditor() changes state, i.e, there are side effects.

An assert statement must never invoke anything that can cause side effects since the assert will disappear when the code is compiled in non-debug mode and if something depends on that side effect, then the code will break just by non-debug compilation.

On a cursory examination of the code, it probably makes sense to change that assert to just

   jassert(getActiveEditor() != nullptr);

(I am of course assuming that getActiveEditor() doesn’t in fact try to create an editor

I would rather question the implementation in the juce_VST3PluginFormat.cpp.

bool hasEditor() is a pure abstract that is simply supposed to return true or false:

Your processor subclass must override this and return true if it can create an editor component.

The implementation you quoted is from AudioPluginInstance, which is the hosting counterpart.
It seems the code creates an editor to find out, if one can be constructed. I don’t know the API well enough to answer, if there is a way to check without creating an editor.

1 Like

I agree that "hasEditor()* should not create an editor if one isn’t there. That’s certainly the expected behavior based on the comment for the base abstract method.

One question is whether correcting that code for the VST3 if it can be corrected (see below) would break existing code that depends on that functionality.

We certainly did not expect an editor to be created (it doesn’t happen for the VST2) and that’s how we discovered the problem.

Yep - I don’t know why they’re doing that for VST3 unless there’s something funky about the VST3 SDK such that there is no way to find out if a plugin support an editor without trying to actually create one. But in that case, the (temporarily?) created editor should be immediately destroy.

Indeed - but either the VST3 code or the assertion needs to be fixed.

It looks to me like this code is trying to guard against the case where IEditController::createView might return nullptr. I can’t see any other way on the IEditController interface to check whether or not the edit controller can create an IPlugView.

I’m pretty sure this is what happens:

    bool hasEditor() const override
    {
        // (if possible, avoid creating a second instance of the editor, because that crashes some plugins)
        if (getActiveEditor() != nullptr)
            return true;

        VSTComSmartPtr<IPlugView> view (tryCreatingView(), false);
        return view != nullptr;
    }

The VSTComSmartPtr will call release on the view when it goes out of scope, destroying it.

So, while this assertion has side-effects, I’m not sure that they are truly dangerous side-effects. i.e. the side effects do not change the state of the AudioProcessor, and do not call any functions on the IEditController that it does not support (I think it’s valid to create multiple IPluginView instances simultaneously).

Could easily remove the potential danger by storing the result of hasEditor() in a local temporary before asserting on it? Rather than needing to change the logic of the assertion itself

What’s the ‘potential danger’?

Isn’t the issue that hasEditor() can have side-effects and so the behaviour could be different between a debug build and release build when it’s only called from within the assertion?

If you pull out the call to hasEditor() then it’ll always be called, removing the potential risk of different behaviours?

const auto hasAnEditor = hasEditor();
jassert (hasAnEditor == (ed != nullptr));
1 Like

Like @reuk pointed out before, the wrapper creates an editor that is removed immediately after.
So you can argue if creating a local instance and discard it is really a side effect.
That would qualify a lot of functions as having side-effects.

That is precisely the issue and CS 101 “never ever invoke a function with side effects from an assert” because different build processes will result in different semantics. That’s just a no-no — surprised this is even up for discussion.

If the point of the assertion is to confirm that a plugin editor exists then changing the assertion to

assert(getActiveEditor() != nullptr)

is the solution.

The alternative is changing the actual code in the VST3 class so that it does not produce a side effect.

Stop right there — it’s just damn scary to see this kind of claim.

Do you not understand that if you do this, then when the code is compiled in release mode, there will be no attempt to invoke that (temporary) creation in the first place? Just because you think this is harmless doesn’t mean it is — in fact this broke our code because of something we were intentionally doing .

There are no circumstances where invoking an assertion on a function that has side effects is legitimate, no matter how harmless someone might think are the side effects.

Side effects are often fine in assertions. For example, allocating and freeing memory might be considered a side effect (you’re modifying the state of the system), but it’s quite reasonable to copy a string or some other data structure in the condition of an assertion. The question is whether the side effect (or absence thereof) changes the semantics of the program in a way that introduces breakage.

In this case, I’d argue that the program is valid whether or not the assertion is present, because the VST3 API is being used correctly in both cases. If a particular plugin depends on the assertion being there (or not being there) then that would indicate an issue in the implementation of the plugin.

1 Like

NO - it is not at all reasonable — it is never reasonable.

I quote from a post in Stack Overflow:

We have several moderately sized C code bases that receive commits from developers with a variety of experience levels. Some of the less disciplined programmers commit assert() statements with side effects that cause bugs with assertions disabled. E.g.*

You have absolutely no way to be able to prove that such an operation would be guaranteed safe and to have zero impact based on compilation. The very act of copying can change something - even a ref count in a string — and something else might be depending on that ref count - even just for the purpose of debugging some other issue.

I do not want a plugin editor to be created when running debug version but not to be created when running release version. You have absolutely no idea what some particular plugin editor might do when it is just created that is undesirable under certain circumstances (or the reverse — you might inadvertently be depending on that plugin editor to have been created and then you’ll have a problem when you build in release mode and your code instantly breaks)

Thank goodness we have the source - we’ll fix this ourselves – but with respect, you have a responsibility to ensure that your code base is correct as far as is possible and an assertion with side effects is never correct.

The “often fine” approach is typically not a great rule to use in programming generally. The assertions specifically are there to catch the “odd/unexpected behavior“ so it has to work all the time, not most of the time.

Take this exact example from this post this example - it’s fine until it isn’t and you end up creating VST3 views without knowing it and, what’s infinitely worse - only in the debug build.

So the philosophy of something being fine 99.9% typically doesn’t fare well as a general programming rule.

I think the main real problem is that hasEditor actually has side effects. The name and signature (const) implies, that it doesn’t have side affects (and one might also argue that from the documentation one should expect the method to run in constant fast runtime). This seems to be not the case.
I think from a software design approach you should mark this method as deprecated and introduce a new one that does not imply having no side affects if there is no way around the current implementation for VST3 plugins.
I’m also interested in how this even compiles. If @dhjdhj experienced an actual change of behaviour clearly there is a method const that shouldn’t have been made const.

In either of those two situations it means that the plug-in has behaviour outside what is expected of the plug-in specification. This problem is getting flagged up now, but there’s a decent chance it might also manifest itself elsewhere if the plug-in is being hosted by a specific DAW.

It never occurred to me that I would have to argue about how an assert statement works.

The JUCE developers can certainly decide whether the fix should be to hasEditor() method or to the assert statement. Both of these were written by the JUCE developers (I assume).

All I wanted to do was point out that as it stands, there’s a bug!

A basic “hello world” program that includes those modules would compile. To paraphrase Galileo, it’s still wrong :slight_smile:

Or just remove or correct the bogus assertion!

You might have missed my point. I think the const-correctness in C++ should have prevented this bug to even appear.

When you just remove it, how exactly can you know that the assertion is not added again in a few years by someone else? I would find it extremely frustrating correcting a bug with no check/test whatsoever that protects me from making it again.
Changing the hasEditor method would have the benefit to find a possible related bug.

But could you provide us with the actual steps to reproduce the bug? :slight_smile:

While I understand the criticism from a formal point of view here, I’m still struggling to understand why this certain assertion might actually cause any trouble in a standard conforming real-world plugin.

Given the fact that a plugin should be able to just create and destroy its editor at any time point in order to fulfil the requirements of the common plugin standards and that also common plugin hosts out there will do exactly that when instantiating a plugin for the first time, I would tend to say that any plugin that experiences real problems here is itself not standard conforming and that this only brings up a design issue in the plugin itself.

I think it would be extremely beneficial for this discussion if you provided quite a bit more details of your actual use-case @dhjdhj and how creating and destroying an editor from inside an assertion is different to your plugin from reacting to the same behaviour from a plugin host in the wild?

13 Likes