QGraphicsScene onSelectionChanged crashes application on close. BUG?

JoeCFD

@StudentScripter it is not something bad to write disconnection in the destructor. Actually, I do it often. It is not that you must do it. However, sometimes things can happen like
the signal is sent out, but the receiver is then destroyed like in your case.
Your app will crash.

StudentScripter

@JoeCFD Ok thanks,
i thought this was some kind of odd behaviour as i've read somewhere that qt manages this out of the box (and with the old syntax it works without disconneting)

Chris Kawa

@StudentScripter said:

i've read somewhere that qt manages this out of the box

It does in general, but you've hit a very niche corner case of polymorphic destruction. The order of destruction for your class is the reverse order of construction, so:

~CustomGraphicsScene()
~QGraphicsScene()
~QObject()

The automatic disconnection is handled in the QObject destructor, so last in this case. Before it happens and connection is still alive a QGraphicsScene destructor is called, which apparently signals selectionChanged, probably unselecting stuff before destruction. The problem is it is connected to onSelectionChanged, which is a method of CustomGraphicsScene. The destructor of CustomGraphicsScene was already invoked at that time, so CustomGraphicsScene part of the object is not valid and calling this->update() on it is undefined behavior. In this case it crashes.

The disconnection in CustomGraphicsScene destructor fixes the issue because the entire object is still alive at that point, so you can refer to it and do the disconnection.

The macro based connection also works because it does string based lookup at runtime, and since CustomGraphicsScene part doesn't exist anymore at the signal time, the lookup silently fails and doesn't invoke the slot. Pointer based connection is more like a direct call on a pointer, so it goes through whether the object is valid or not.

JonB

@Chris-Kawa
In view of all of this, perhaps @StudentScripter should do the disconnect() for all slots, in case there are others (maybe in the future)?

JoeCFD

@JonB Maybe he can also try to call CustomGraphicsScene->deleteLater() without disconnection. We do clean-up our connections in the destructors in a heavily multi-threaded app.

Chris Kawa

@JoeCFD said:

Maybe he can also try to call CustomGraphicsScene->deleteLater() without disconnection

I don't see how this would change anything. deleteLater just postpones the deletion. The problem is still in the destructor, whenever it is called.

The easiest is, as the OP did - do the disconnection in the destructor. You could also do like @JonB said, call disconnect() in the destructor to break all connections to this object. You do risk disconnecting something you might not want to though, like somebody connected to QObject::destroyed signal, so it's usually not a good idea to do a wide sweep like that.

If that's the only connection causing trouble in that class you can also be more subtle and just call clearSelection() in the CustomGraphicsScene destructor, so that the signal won't trigger down the destructor stack, avoiding the whole kerfuffle altogether.

All in all it's a very specific corner case. It's similar to something like this:

class Base {
   ~Base() { ((Derived*)this)->something(); }
}

just a bit more covered up. It's not something you do on purpose.

JonB

@Chris-Kawa said in QGraphicsScene onSelectionChanged crashes application on close. BUG?:

like somebody connected to QObject::destroyed signal, so it's usually not a good idea to do a wide sweep like that.

OOhh, good one, I didn't think of that! I meant all the ones other than destroyed() ;-)

StudentScripter

@JonB @Chris-Kawa Very good insights on this one. Definitely learned a new thing here. Thanks you all, i'll closing this post as my concerns have been adressed. Have a nice day. :D

Chris Kawa

@JonB said:

I meant all the ones other than destroyed()

Well, destroyed() is probably not that common, but imagine a QAction somewhere that is enabled/disabled on selection change. That's something you see often. Disconnecting it early means it wouldn't get that last unselect signal and could be left enabled when it shouldn't be and crash when triggered. You could be trading one crash for another one somewhere else.

That's why I usually avoid any sweeping actions. You never know what gets caught in them that shouldn't.

JonB

@Chris-Kawa
Well, since you have brought this up (and the OP is satisfied with the answers in this thread). Plenty of posts here and on SO are quite happy to recommend calling blockSignals() in situations where people don't want signals delivered. Which I have always regraded as devil's spawn for precisely the reasons you are mentioning, you have no idea what you are blocking and what the consequences might be....

Chris Kawa

@JonB said:

Which I have always regraded as devil's spawn

Agreed. I see blockSignals() as an advanced tool to be used with great care and only when you're very aware of all consequences in given case. Similar to sender(). Definitely not something to be applied on a "let's see if it helps" basis.