FR: Add an assertion in Component::getLookAndFeel() to check if this == nullptr

When juce::Component::getLookAndFeel() is called on a nullptr, the function returns a valid (default) look and feel.

juce::TextButton* button = nullptr;
auto* laf = &button->getLookAndFeel();

// laf contains the default LookAndFeel, and so this assertion isn't triggered.
jassert (laf != nullptr);

Would it make sense for getLookAndFeel() to throw an assertion if this == nullptr? It’s almost always going to be a programmer error that it’s being called on a null object.

LookAndFeel& Component::getLookAndFeel() const noexcept
{
    // You called this method on a null object!
    jassert (this != nullptr);

    for (auto* c = this; c != nullptr; c = c->parentComponent)
        if (auto lf = c->lookAndFeel.get())
            return *lf;

    return LookAndFeel::getDefaultLookAndFeel();
}

This wasn’t an easy bug to track down as I would have expected the returned LookAndFeel to at least be nullptr if this is nullptr, but instead it was a perfectly valid object - just not of the derived type I was expecting. It took me a quite a while to figure out that the reason it was the wrong type was because the component I was using wasn’t initialised.

It’s undefined behaviour for this to be nullptr - if there’s a chance that this is nullptr then the entire program is broken. It also wouldn’t be very consistent to add a check in just this location - if there’s a chance that this might be nullptr then we would probably want to add this sort of assertion to every member function, which is not practical.

I’d recommend trying out Address Sanitizer and Undefined Behaviour Sanitizer to debug these sorts of issues. The sanitizers should be able to detect and provide stack traces for uses of uninitialised memory, without requiring any manual changes to the source code of the program.

2 Likes

Thanks, I’ll have to take a look at these - could come in useful!

Something still feels odd about having a member function be callable through a null object, I understand it’s undefined behaviour but the fact it doesn’t necessarily cause any immediate issues makes it hard to debug. Even given the tools you mentioned, I’d have to know it was an issue with undefined behaviour first.

Instead, could the implementation change to result in a hard crash when this == nullptr? Perhaps:

LookAndFeel& Component::getLookAndFeel() const noexcept
{
    auto* c = this;

    do
    {
        if (auto lf = c->lookAndFeel.get())
            return *lf;
        
        c = c->parentComponent;
    }
    while (c != nullptr);

    return LookAndFeel::getDefaultLookAndFeel();
}

It’s not quite as clean an implementation, but it does result in a hard crash without any need for checking for undefined behavior.

Bonus points, after a quick check in GodBolt it seems the implementation with do-while is identical to the previous implementation but saves a jmp operation:

The behaviour you’re seeing would be the same for all member functions on all classes. Any member function of any class could be called on an invalid this pointer. Rather than adding explicit null checks to every member function, it’s better to just use the right tool for the job (i.e. the sanitizers).

Additionally, the null checks only (potentially) solve a narrow range of a broader problem - consider the case where you call a member function on a pointer which has been deleted. In this case, this would not be null, but calling the function would still result in undefined behaviour. Null checks wouldn’t find this problem, but the sanitizers would.

I don’t think that’s true. The sanitizers can flag up a wide range of issues and they don’t add that much overhead, so it’s reasonable to run them as a matter of course whenever you have a weird unexplained crash.

1 Like

I agree that this likely isn’t the only place this behavior occurs, but it certainly won’t happen for every member function. Most member functions will always use this, whereas in getLookAndFeel() it’s optional… You are already checking this for being nullptr through the use of the for-loop: first you initialise c to this, then you (indirectly) check if this is nullptr.

But that’s exactly my point - calling getLookAndFeel() on a nullptr doesn’t cause a crash, the program continues executing just fine!

Sorry, I should have given a more detailed example:

/** A handy utility class for accessing a custom LookAndFeel
    type from a Component without the need to have dynamic_casts
    all over the place.
*/
template<typename CustomLaF>
class LookAndFeelAccessor
{
public:
    LookAndFeelAccessor() = default;

    void setComponent (juce::Component* comp)
    {
        m_component = comp;
    }

    bool hasValidLookAndFeel() const
    {
        return dynamic_cast<CustomLaF*>(&m_component->getLookAndFeel()) != nullptr;
    }

    CustomLaF& getLookAndFeel() const
    {
        return *dynamic_cast<CustomLaF*>(&m_component->getLookAndFeel());
    }

private:
    juce::Component* m_component;
};


class MyWidget : public juce::Component
{
public:
    struct LookAndFeelMethods
    {
        virtual ~LookAndFeelMethods() = default;

        virtual void someCustomLafFunction() = 0;
    };

    void lookAndFeelChanged() override
    {
        // Even though this component does have the expected LookAndFeel type,
        // hasValidLookAndFeel() returns false because I forgot to set the component
        // pointer in LookAndFeelAccessor.
        // This silently fails as it just returns the default LookAndFeel.
        if (! m_lafAccessor.hasValidLookAndFeel())
            return;

        // My custom LookAndFeel method is never called, so the only indication
        // I get that something went wrong is a visual error where this widget
        // isn't displaying properly.
        auto& laf = m_lafAccessor.getLookAndFeel();
        laf.someCustomLafFunction();
    }

private:
    LookAndFeelAccessor<LookAndFeelMethods> m_lafAccessor;
};


class MyLaF : public juce::LookAndFeel_V4
            , public MyWidget::LookAndFeelMethods
{
    void someCustomLafFunction() override {}
};


class ParentComponent : public juce::Component
{
public:
    ParentComponent()
    {
        addAndMakeVisible (m_widget);

        // Causes MyWidget's lookAndFeelChanged callback to be called.
        setLookAndFeel (&m_laf);
    }

private:
    MyWidget m_widget;
    MyLaF m_laf;
};

I’m not asking to handle the undefined behavior, I’m asking for it to result in a hard crash with the undefined behavior to make it much easier to debug. WIth the implementation I proposed, I get a BAD_ACCESS error and the callstack clearly shows me that I didn’t call setComponent() on the accessor.

In my PluginGuiMagic I also use custom LookAndFeel methods for Components that are not shipped with JUCE. Maybe my approach helps you to get rid of the dynamic_cast on each lookup. It won’t be possible entirely without.

class MyComponent : public juce::Component
{
    void paint (juce::Graphics& g) override
    {
        myLookAndFeel->renderMyStuff (g, getLocalBounds(), *this);
    }

    void lookAndFeelChanged() overrride
    {
        if (auto* lnf = dynamic_cast<MyLookAnfDeel*>(&getLookAndFeel()))
            myLookAndFeel = lnf;
        else
            myLookAndFeel = &lookAndFeelFallback;
    }
private:
    MyLookAndFeel lookAndFeelFallback;
    MyLookAndFeel* myLookAndFeel { &lookAndFeelFallback };
};

For the original problem I am with @reuk, since it is undefined behaviour you cannot deduce from your observation to behaviour anywhere else. On a different platform, a different OS version, a different compiler it might behave completely differently rendering your “fix” useless or even counterproductive.

I think I did have a look at some of your public repos when I was designing my LookAndFeelAccessor to see how others might have approached the problem. Personally, I feel the solution you provided is a little cumbersome, especially if it needs to be implemented in several places. The idea of my LookAndFeelAccessor is to wrap up all of the custom-look-and-feel handling stuff in one object with a minimal interface. The idea of a fallback is nice though, I may implement that.

I hadn’t considered other platforms, and to be clear I’m not suggesting the solution I provided is the perfect solution, it was simply an example that seems to give the behavior I would expect (on my platform at least [Mac]).

I can easily fix this on my end by just checking for m_component != nullptr whenever it’s used (which I should have been doing anyway). So for me, this issue isn’t likely to come up again - I just wanted to suggest a change to potentially save some other poor sod from the headache I had of figuring out why my widget didn’t have the right LookAndFeel… even though it did!