Problem with removing QLabel wrapped in a std::shared_ptr from QGraphicsScene

[monnzz]

Hello everybody. I have a very strange behaviour with QLabel and QGraphicsScene.
I develop a simple game, where 2 players how a set of soldiers and basically try to eliminate each other. This is a turn-based strategy and obviously I use QLabel to display QTimer value on a game map.
The aim of developing this game is learning of Qt, c++11 and some programming techniques.

I'm gonna highlight troubled parts with comments containing "ololo". So here is a code:


std::shared_ptr<GameManager> GameManagerCreator::createGameManager()
{
// ololo if remove static everything is fine.
static std::shared_ptr<GameManager> gameManager(new GameManager());
gameManager->init();
qDebug() << "GameManagerCreator::createGameManager use count " << gameManager.use_count();
return gameManager;
}

This is a creation of my game logic class. As u can see, I would like to use here a singleton.


void GameManager::initialiseGameInterface()
{
mp_menuButton.reset(new QPushButton);
setButtonSettings(mp_menuButton, ":/settingImages/menuButton.png");
mp_scene->addWidget(mp_menuButton.get())->setZValue(1.0);
const std::shared_ptr<Tile> rightTopCorner = mp_scene->tile(std::make_pair(0, mp_scene->mapSizeInTiles().second - 1));
mp_menuButton->move(rightTopCorner->rect().x(), rightTopCorner->rect().y());

mp_changeTurnButton.reset(new QPushButton);
setButtonSettings(mp_changeTurnButton, ":/settingImages/changeTurnButton.png");
mp_scene->addWidget(mp_changeTurnButton.get())->setZValue(1.0);
connect(mp_changeTurnButton.get(), SIGNAL(clicked(bool)), this, SLOT(changeTurn()));

QString settingsFolder(":/settingImages/");
mp_moveBlantButton.reset(new QPushButton);
setButtonSettings(mp_moveBlantButton, settingsFolder + "moveButton.png");
mp_scene->addWidget(mp_moveBlantButton.get())->setZValue(1.0);
connect(mp_moveBlantButton.get(), SIGNAL(clicked(bool)), this, SLOT(moveButtonClicked()));

mp_attackBlantButton.reset(new QPushButton);
setButtonSettings(mp_attackBlantButton);
mp_scene->addWidget(mp_attackBlantButton.get())->setZValue(1.0);
connect(mp_attackBlantButton.get(), SIGNAL(clicked(bool)), this, SLOT(attackButtonClicked()));

mp_meleeAttackBlantButton.reset(new QPushButton);
setButtonSettings(mp_meleeAttackBlantButton, settingsFolder + "meleeAttackButton.png");
mp_scene->addWidget(mp_meleeAttackBlantButton.get())->setZValue(1.0);
connect(mp_meleeAttackBlantButton.get(), SIGNAL(clicked(bool)), this, SLOT(meleeAttackButtonClicked()));

mp_timerDisplayer.reset(new QLabel);
setTimerSettings();
// ololo this line is leading to a crash in the destructor!!!
mp_scene->addWidget(mp_timerDisplayer.get())->setZValue(1.0);
placeTimerDisplayer();
}

Here I initialize interface settings (mostly buttons). And look at the line
mp_scene->addWidget(mp_timerDisplayer.get())->setZValue(1.0); This one leads to an exception in the destructor. I can't understand why. There is no difference between initializing buttons and this QLabel. However if I comment this line everything works perfectly except there is no QLabel on the map but no surprise here:)


void GameManager::cleanScene()
{
QList<QGraphicsItem*> list =mp_scene->items();
for(int i=0;i<list.length();i++)
mp_scene->removeItem(list[i]);
}

void GameManager::clean()
{
cleanScene();

// cleaning other things


if(mp_playerInCharge)
mp_playerInCharge.reset();
if(mp_menuButton)
mp_menuButton.reset();
if(mp_moveBlantButton)
mp_moveBlantButton.reset();
if(mp_attackBlantButton)
mp_attackBlantButton.reset();
if(mp_meleeAttackBlantButton)
mp_meleeAttackBlantButton.reset();
if(mp_changeTurnButton)
mp_changeTurnButton.reset();

if(mp_turnTimer)
mp_turnTimer.reset();
if(mp_timerDisplayer) {
//ololo crashes here
mp_timerDisplayer.reset();
}

if(mp_view) {
mp_view.reset();
}
if(mp_scene) {
mp_scene.reset();
}
}

GameManager::~GameManager()
{
clean();
qDebug() << "gamemanager destructor";
}



And here is my destructor. In case you wondering why I don't use mp_scene->clear(), that's because if I have mp_timerDisplayer it crashes. So before deleting all my interface buttons and so on, I remove them from the scene.
The most strange part that everything works perfectly if i don't use singleton. (simply remove static from
static std::shared_ptr<GameManager> gameManager(new GameManager());)

Another strange thing is that if I leave static be and remove this QLabel which displays timer count down everything also works fine.

It's worth mentioning that all other code which is used with mp_timerDisplayer is strictly about styles and setting values, I don't create it anywhere else and I don't delete it anywhere else.
If it's any help I can publish it as well.
The error is access violation and bla bla bla. Looks like I delete the same object twice. First it deleted then I remove it from the scene and the second one is when std::shared_ptr counter goes to zero in mp_timerDisplayer.reset();

But you see there is no difference between button creations and freeing and this QLabel.

Any help would be much appreciated.

[monnzz]

Other facts popped up during the investigation.
First of all, my buttons become visible when you click any soldier. So at the start of the game no buttons visible and QLabel is visible. So if I make it invisible there are no problems during destruction. At the same time if I close the app when any buttons are visible then my app is going down on the first button in destructor.

[d_stranz]

Why are you wrapping your QLabel instance in an std::shared_ptr? QObject instances (of which your QLabel is one) are always owned by their parent QObject. This parent controls the lifetimes of its QObject children. By wrapping the QLabel instance in an std::shared_ptr you are setting up a conflict over ownership, because std::shared_ptr also controls the lifetime of the pointer it wraps. You can't have two owners controlling the lifetime of the same instance. It sounds like an error in your program design if you think you need to pass around a QLabel pointer via std::shared_ptr.

In particular, if your QLabel's QWidget owner causes the QLabel instance to go out of scope and be deleted, the std::share_ptr will be left holding an invalid dangling pointer to freed memory since there is way for it to know of the deletion. If the shared_ptr deletes the QLabel instance first, then the QWidget parent probably will get notified, since the QLabel will emit a signal as it is being destroyed.

[monnzz]

First of all, I would like to thank you for your reply:)

I use std::shared_ptr just to practice new features of C++11. I thought about this
QObject instances (of which your QLabel is one) are always owned by their parent QObject, but i thought I needed explicitly provide relationship parent-child by passing a parent to a child constructor or using setParent function, and I'm not doing any of those things so as to prevent the ownership of QLabel or QPushButton by any other object except std::shared_ptr.

mp_timerDisplayer.reset(new QLabel /* no parent here */);

In my header I declare these members as follows


class GameManger : public QObject, public enable_shared_from_this
{
private:
// some other declarations
std::shared_ptr<QLabel*> mp_timerDisplayer;
std::shared_ptr<QPushButton*> mp_menuButton;
// some other declarations
}

Now, regarding QGraphicsScene and its ownership over passed objects. Before deleting objects I remove them from the scene by calling this function:


void GameManager::cleanScene()
{
QList<QGraphicsItem*> list =mp_scene->items();
for(int i=0;i<list.length();i++)
mp_scene->removeItem(list[i]);
}

It said that removeItem does the next things
Removes the item item and all its children from the scene. The ownership of item is passed on to the caller (i.e., QGraphicsScene will no longer delete item when destroyed).

So in my view, I have only one object which controls the lifetime of QLabel here and it is std::shared_ptr.

If I'm wrong could you please point this out?

The most strange thing that if I remove static from std::shared_ptr in GameManagerCreator everything works like a charm.

[anda_skoa]

Since the static will be destroyed after main() has ended, the widgets could have been deleted by some hook in QApplication's destructor.

Or the label's destructor accesses some part of QApplication which is already gone.

Have you tried making the QApplication object shared-pointered static as well?

Cheers,
_

[monnzz]

anda_skoa, thank you very much, that is the answer! It was enough to make QApplication static, that's it:)