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”)

Advertisements