How to break your code porting from Q_PRIVATE_SLOT to context & function object based connection

TL;DR Beware of connections to function objects accessing class members which could be triggered during execution of subclass destructor methods.

Oh, those name-based signal/slot connections feel outdated!

So you are a happy embracer of Qt’s new QObject signal/slot connection way of coding based on function-pointer or functor objects, like myself. This compile-time check of signals and slots feels just so much better. And thus you also port any existing code to it. Including some which uses the pimpl approach for some public classes, borrowing Qt’s macros Q_PRIVATE_SLOT, Q_D & Co.:

class MyWidgetPrivate;

class MyWidget : public QWidget
{
    Q_OBJECT
public:
    explicit MyWidget(QWidget *parent);
    // [...]
    // setting a QWidget-subclass to be used as custom form in this widget
    void setForm(Form *form);
private:
    const QScopedPointer d_ptr;
    Q_PRIVATE_SLOT(d_func(), void handleFormDeleted())
};

// implementation side:

class MyWidgetPrivate
{
// [...]
public:
    void handleFormDeleted() { /*...*/ }
};

MyWidget::MyWidget(QWidget *parent)
    : QWidget(parent)
    , d_ptr(new MyWidgetPrivate)
{
    // [...]
}

void MyWidget::setForm(Form *form)
{
    Q_D(MyWidget);
    // [...]
    connect(form, SIGNAL(destroyed()), this, SLOT(handleFormDeleted()));
}

Got some time, let’s modernize the code

The old code calls to be changed into using a connection from the destroyed signal to a lambda expression calling handleFormDeleted() directly on the private object, with MyWidget instance as context object, thus removing the need for that Q_PRIVATE_SLOT:

class MyWidgetPrivate;

class MyWidget : public QWidget
{
    Q_OBJECT
public:
    explicit MyWidget(QWidget *parent);
    // [...]
    // setting a QWidget-subclass to be used as custom form in this widget
    void setForm(Form *form);
private:
    const QScopedPointer d_ptr;
};


// implementation side:

class MyWidgetPrivate
{
// [...]
public:
    void handleFormDeleted() { /*...*/ }
};

MyWidget::MyWidget(QWidget *parent)
    : QWidget(parent)
    , d_ptr(new MyWidgetPrivate)
{
    // [...]
}

void MyWidget::setForm(Form *form)
{
    Q_D(MyWidget);
    // [...]
    connect(form, &QObject::destroyed,
            this, [this] { Q_D(MyWidget); d->handleFormDeleted(); });
}

Looks fine & compiles. Code feels more future-proof with the compiler now warning if some signal or slots got changed/removed.

Ooops, crashing now?

Just… nooos, it sometimes crashes now, in the destructor of MyWidget. How that on this innocent looking change?

Reading once more closely the documentation of QMetaObject::Connection QObject::connect(const QObject *sender, PointerToMemberFunction signal, const QObject *context, Functor functor, Qt::ConnectionType type = Qt::AutoConnection) we notice the remark:

The connection will automatically disconnect if the sender or the context is destroyed. However, you should take care that any objects used within the function object are still alive when the signal is emitted.

Which subtly hints to the problem we now have: if the form instance is set as child widget of the MyWidget instance, it will be deleted when ~QWidget() is run as part of the MyWidget destructor. And then emit the destroyed signal. At that point in time this as seen by the function object no longer is a proper MyWidget instance. And things go *boom*.

The old string-based connection as well as the member-function-pointer-based one handle that case for us, by some QObject magic using virtual methods which catch that the receiver no longer is a MyWidget and somehow then just drop the slot call (got lost in the code details, but it is something like this).
While with the new function-object-based connection that one will only become automatically inactive by being destroyed if the ~QObject destructor of either sender or receiver is reached. So having a longer lifetime, which can come a bit unexpected to some.

Fixing the modern times, unsure how

Lesson learned: do not blindly port code to the context & function object based connection. Instead beware of the additional traps which there are given that the function object is an independent complex object and not just a member function pointer. I will have to revisit quite some code where I might have missed this trap with the subclass destructor methods :/
As I seemed not the only one hit by this, I filed QTBUG-71432: “API dox of context & function object using QObject::connect should hint about destructor issues” so other people like me might be saved from this from the start.

Curious to learn about best practices for private slots and non-string-based connections. Thus happy to hear about proposals/hints in the comments.

(Update: text now using C++ standard lingo term “function object” instead of “functor”)

7 thoughts on “How to break your code porting from Q_PRIVATE_SLOT to context & function object based connection

  1. The important part about destructors can be read here:
    https://isocpp.org/wiki/faq/dtors

    TLDR: d_ptr.~QScopedPointer() is called before this->~QWidget().

    Afterwards, d_ptr.isNull() should be true, which you can use to guard the method call.

    So, is it possible to use the d_ptr as context for the slot? Should take care of disconnecting it.

    • Thanks for the idea. At least in the given case which I tried to abstract in the example, the Form object is not principally owned by MyWidget, so deleting the Form instance manually or by QScopedPointer ourselves in the MyWidget constructor is not an option. Surely there are cases though were using a QScopedPointer would be an approach.
      But deleting any signal sender ourselves is not the golden bullet to prevent functor slots being invoked after the MyWidget destructor has been done and any subclass destructor has been entered. As the senders could be owned by others and should also survive the end of a MyWidget instance. Edit: In other words, I do not want to prevent the emission of the signal, but the invokation of the slot.

      So what would be needed is something which stops any functor slots being executed after the class own destructor has been done or even simply disconnects automatically.
      The latter could be done by having the private class inherit from some util class which features the option to collect connections (needs explicit passing of the rhv of a connect call) and which in its own destructor would iterate over all of the collected ones and disconnect them.
      That though is still not the same as the simple “connect(other, signal, this, privatslot);” as one could do before, instead it would be “d->addConnection(connect(other, signal, this, privatslot));”.

      What I would hope for is something where connections to private functor slots do not look that different in the creation than connections to normal memberfunction slots or the name-based private method slots.

  2. Something ate my <…> …

    What I originally wrote (amended a little bit for clarification):

    ~MyWidget calls d_ptr.~QScopedPointer<MyWidgetPrivate>() first and ~QWidget() afterwards.

    After the MyWidgetPrivate instace *d_ptr has been destroyed, d_ptr itself is still valid. You can just check d_ptr.isNull() in the slot.

    For automatic disconnection, there are several possibilities:
    – if *form is stored inside MyWidget or MyWidgetPrivate, you can disconnect(form, 0, 0, 0) in the destructor
    – store the QMetaObject::Connection returned from QObject::connect and disconect from the destructor

    I think one can formulate a new rule:
    Newer use “this” as a context if you connect to one of its members, as the object will outlive the members.

  3. Nice article!

    One little nit-pick – please use ‘function object’ instead of ‘functor’ (as named by the c++ standard) even if Qt documentation uses the later.

    We should try to fix the nomenclature in public posts – it can confuse people when they actually want to learn what functors are.

    • I surely still need to take my classes in modern C++ lingo instead of just picking up what I hear on the streets, thanks for the term hint, updated the post with it.

Leave a comment

This site uses Akismet to reduce spam. Learn how your comment data is processed.