Popup's MenuWindow crash


#1

when a popup’s MenuWindow is created without parent, it is given the popupMenu’s lookAndFeel in the constructor :

setLookAndFeel (parent != nullptr ? &(parent->getLookAndFeel())
                                          : menu.lookAndFeel.get());

but if the popupMenu’s look and feel is destroyed, then the MenuWindow will be left with a dangling pointer (and might try to use it in MenuWindow::paint(juce::Graphics&).

to reproduce:
. create a basic plugin with the projucer, and use the code below for the editor.
. open the plugin in reaper
. click the combobox to open its menu
. close the plugin window while the menu is open

class TestAudioProcessorEditor  : public AudioProcessorEditor
{
public:
    TestAudioProcessorEditor (TestAudioProcessor& p) : AudioProcessorEditor (&p)
    {
        addAndMakeVisible (comboBox);
        setSize (400, 300);
    }

    void paint (Graphics& g) override
    {
        g.fillAll (Colours::lightcoral);
    }

    void resized() override
    {
        comboBox.setBounds (30, 30, 200, 40);
    }

private:
    //==============================================================================
    struct MyLookAndFeel
    {
        MyLookAndFeel()  { LookAndFeel::setDefaultLookAndFeel (&lf); }
        ~MyLookAndFeel() { LookAndFeel::setDefaultLookAndFeel (nullptr); }
        LookAndFeel_V3 lf;
    };

    SharedResourcePointer<MyLookAndFeel> myLookAndFeel;

    ComboBox comboBox;

    JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (TestAudioProcessorEditor)
};

#2

Not sure what we could do about that…? It’s really just a prerequisite that you shouldn’t pass it a L+F that may be deleted before the menu is finished.


#3

I understand, but it would be cool if adding a simple combobox in a plugin was just something safe and simple that we could do without having to worry about the MenuWindow lifetime.
Even if I call dismissAllActiveMenus() from the pluginEditor destructor it will still crash (*)

PopupMenu uses a WeakReference<LookAndFeel>, so I’m not sure why it should be different for MenuWindow?

(*) at the moment dismissMenu() calls MenuWindow::hide (const PopupMenu::Item, bool makeInvisible) with makeInvisible being false. Setting it to true fixes it.


#4

Oh, yes, I guess it could have a weak ref to avoid it actually crashing. No harm in adding that, will do…


#5

Why not having the WeakReference<LookAndFeel> lookAndFeel; in Component?


#6

Well, the only counter-argument is that it’d be a bit more overhead in the component destructor, but TBH it’s not a bad idea as it could save a lot of people from crashes. (Probably replacing what would have been crashes with their GUI suddenly being drawn looking wrong!)


#7

Or have LookAndFeel refcounted that would allow custom LnF for a particular widget to be stored within this widget and avoid external dependency ?


#8

If I was designing the class today, yes, I’d make the class ref-counted! But unfortunately there will be thousands of people out there with raw pointers to L+Fs where that would be a big breaking change (and one where for them to change to ref-counting could be non-trivial in many cases)


#9

This is indeed a big breaking change.


#10

But given this current limitation. Do you think a lot of people uses a lot of different LnF in their single app ? Just wondering.


#11

I would expect that most people use a single global one, but quite a few people will probably also use temporary custom ones for dialog boxes and other bits of UI.


#12

I don’t think this is a particularly clever idea, because it turns a very apparent problem (a crash) that could easily be spotted during testing into a more subtle one, which may go unnoticed if the default drawing is not so different from the customized one.
Good luck in understanding why the window gives a strange “flash” or “flicker” when you close it, for example!

This also encourages the practice of writing sloppy code which does not pay attention to the logic order of deletion: delete the widgets first, and the resources that they used last.

In my opinion, if you want to keep the WeakReference approach to avoid crashes, at least put a jassert somewhere, that triggers if the default LookAndFeel is being used because the intended custom one has been deleted too early.


#13

Well, there are actually some good use-cases for this, such as the OP’s situation. Mainly the fact that when destructing a group of component and L+F objects that use them, it can often be hard to make sure the order is correct, and this makes it impossible to make a mistake.

Actually, a much better way to catch bugs than crashing would be for this to add an assertion that’s triggered when the L+F is deleted while in use somewhere. Hmm… I wonder if that would be a good type of smart-pointer: a weak-ref that asserts if it’s deleted while in use?


#14

Could it simply be an additional bool argument for the WeakReference template, which tells whether it should assert when the referenced object gets deleted while the WeakReference is still in existence?

Alternatively, a AssertingWeakReference may be inherited from WeakReference adding that behavior


#15

Hmm, still mumbling about this, I wonder whether the fact that the asserting reference is “weak” is important:

if one desires the reference to assert when the object is deleted, then the intended relationship is not “weak” at all in the intended sense, and such “AssertingReference” would be a helpful tool not because it becomes null, but because it asserts in the moment the pointer goes dangling, rather than the first time one attempts to use it.

The fact that one such AssertingReference would become null could probably be of no help in this case anyway: since it would express a relationship with an object that should exist at all times during the lifetime of the reference itself (hence the assertion if it doesn’t), attempts to use the reference after the object has been deleted will cause a crash anyway, either because the pointer is dangling or because it has been nullified.

So, perhaps the “weak” behavior of your proposed reference could be ditched altogether, and only the part that asserts if the referenced object is deleted could be kept, perhaps only in debug mode so that there is no overhead in using them in release mode


#16

I think I’ve got quite a simple solution for this. IMHO the ideal behaviour is that in debug, you’ll get an assertion when the L+F is actually deleted, which is way more useful than finding out when the dangling pointer is being used. And in a release build, it may draw oddly, but won’t actually crash, at the expense of very little extra overhead. Will push something soon…


#17

Yes, that’s what I said:


#18

Sorry, yes, I was mainly thinking aloud rather than trying to claim it was my idea! :slight_smile:


#19

Following your ref counting of LnF, I always have issue for the default LnF (the one set with uce::LookAndFeel::setDefaultLookAndFeel) as there is always
a ref on it.
I delete this global LnF using a DeleteAtShutdown
Any idea how to solve this ?

Thanks !


#20

Well, you’d need to call setDefaultLookAndFeel (nullptr) before your L+F object gets destroyed