Double Delete using Shared Pointers, even though there is still a shared_ptr to the object, its destructor is being called

Question

I apologize if this is a lot of code to read, if I can simplify with explanations please let me know, also if you'd like to comment on my design/practices feel free.

So my Player is being deleted twice, I'm not sure why. If you look at the call stack, you'll see that GameEventManager is actually calling the Player destructor before the GameState destructor, even though GameState is the one with the pointer to Player. I'm thinking maybe this is because it's also destroying the vector first, so it finds Player in the vector and tries to destroy it. I don't know why it would try to destroy the Player though, since there is still a reference to Player that the GameState object knows about. The shared pointer functionality should prevent the Player from being destroyed.

Maybe I am going about it the wrong way, if so could someone point me in the correct direction for best practices here? Thanks

Call stack:

enter image description here GameState.h

#ifndef _level
#define _level
#include <vector>
#include "Player.h"
#include <memory>

using namespace std;

class GameState // A representation of the Level/Game/Scene
{
public:
    GameState();
    virtual ~GameState() {}
    //Keep track of the game's state

    //Maybe get rid of the Level class, and make this a class, and move the Level functionality here?

    static void EndGame(const bool & b) { mbEndGame = b; }

    static const bool & EndGame() { return mbEndGame; }
private:
    void SetPlayer(shared_ptr<Player> sptrPlayer) { msptrPlayer = sptrPlayer; }
    static bool mbEndGame;
    shared_ptr<Player> msptrPlayer;
    vector<shared_ptr<Player>> msptrMob; // Representation of all the NPCs in the game. Eventually make a Mob class but for now just use Player
    // shared_ptr<LevelMap> mMap // Representation of what the game looks like visually
    // Renderer // Should the level have the renderer to create the graphics? Or should this be handled by another "GUI Layer" and interact with this layer as little as possible ?
};

#endif

GameState.cpp

#include "stdafx.h"
#include "GameState.h"

bool GameState::mbEndGame(false);

GameState::GameState() : msptrPlayer(NULL)
{
    shared_ptr<Player> sptrPlayer(new Player);
    SetPlayer(sptrPlayer);
}
GameObject.h
#ifndef _game_object
#define _game_object
#include <memory>

// Game Object Classes inherit this so they are registered with GameEventManager.
using namespace std;
class GameObject{
public:

    GameObject();
    virtual ~GameObject() {}
    virtual void Update() = 0;
    virtual void Start() = 0;


};
#endif
GameObject.cpp

#include "stdafx.h"
#include "GameObject.h"
#include "GameEventManager.h"

GameObject::GameObject()
{
    shared_ptr<GameObject> ptr(this);
    GameEventManager::GetGameEventManager()->RegisterGameObject(ptr);
}

Player.h

#ifndef _player
#define _player

#include "GameObject.h"
//A representation of the Player. Used by Level.
class Player : public GameObject
{
public:
    Player();
    virtual ~Player() {}
private:
    virtual void Update();
    virtual void Start();
    int miMaxHealth;

};
#endif

Player.cpp

#include "stdafx.h"
#include "Player.h"
#include "GameState.h"

Player::Player() : miMaxHealth(100) {};
void
Player::Start()
{

}

void
Player::Update() // reimplement GameObject::Update(), which is called by the GameEventManager
{
    --miMaxHealth;
    if (miMaxHealth <= 0)
    {
        GameState::EndGame(true);
    }
}

GameEventManager.h

#ifndef _game_event_manager
#define _game_event_manager

#include "stdafx.h"
#include <memory>
#include <vector>
#include "GameObject.h"
#include "GameState.h"

using namespace std;

class GameEventManager{ 
    //Object which inherit from GameObject are automatically registered with GameEventManager when they are constructed.
    // GameEventManager creates the level object to represent the game, and then runs Start() on all registered GameObjects
    // and then continually runs Update() on all registered game objects until the GameState is set to EndGame.
public:
    virtual ~GameEventManager(){} // This gets called at the end of the program (I guess whne static variables are destroyed), and crashes during vector<shared pointer <GameObject>> destruction, probably because
                                  // Player already destroyed it. So... not sure what to do. If I make it non-static  

    void StartGameEvents(); 
    const static shared_ptr<GameEventManager>& GetGameEventManager();
    const shared_ptr<GameState>& GetLevel();
    void RegisterGameObject(shared_ptr<GameObject> sptrGameObject);

    const shared_ptr<vector<shared_ptr<GameObject>>>& GetRegisteredGameVector() const { return mvecRegisteredGameVector; }

private:
    GameEventManager(); //singleton
    void AddGameObject(shared_ptr<GameObject>);
    shared_ptr<GameState> mLevel;
    shared_ptr<vector<shared_ptr<GameObject>>> mvecRegisteredGameVector; //Reference because shared pointer will double delete otherwise. ~Level() still deletes it but this way I guess it doesn't try to delete again? but...
                                                                        //Now I'm trying it as a shared_ptr, but it's not working. ~Level() still deletes it even though there is a shared pointer to a vector pointing to the Player. Why is ~Level() doing this?

    static shared_ptr<GameEventManager> msptrGameEventManager;
};
#endif

GameEventManager.cpp

#include "stdafx.h"
#include "GameEventManager.h"
#include "GameState.h"

shared_ptr<GameEventManager> GameEventManager::msptrGameEventManager(new GameEventManager);

void
GameEventManager::StartGameEvents()
{
    //run once
    int size = GetRegisteredGameVector()->size();
    vector<shared_ptr<GameObject>> & vecsptrRegisteredGameVector = (*GetRegisteredGameVector());
    for (int i = 0; i < GetRegisteredGameVector()->size(); ++i)
    {
        vecsptrRegisteredGameVector[i]->Start(); //nothing for now 
    }
    //keep running
    while (GetLevel()->EndGame() != true)
    {
        for (int i = 0; i < GetRegisteredGameVector()->size(); i++)
        {
            (*GetRegisteredGameVector())[i]->Update(); //Player's life goes from 100 to zero, see Player::Update

        }
    }
    return; 
    // GameState destructor is called and destroys player for some reason, even though it's still being referenced by the GameEventManager's vector.
}

GameEventManager::GameEventManager() : mvecRegisteredGameVector(new vector<shared_ptr<GameObject>>) , mLevel(NULL) //Instantiating the level before the GameEventManager is fully instantiated causes an infinite recursion.
{
    return;
}

const shared_ptr<GameEventManager>& 
GameEventManager::GetGameEventManager()
{
    if (!msptrGameEventManager)
    {
        msptrGameEventManager.reset(new GameEventManager);
    }
    return msptrGameEventManager;
}

const shared_ptr<GameState>&
GameEventManager::GetLevel()
{
    if (!mLevel)
    {
        mLevel.reset(new GameState);
    }
    return mLevel;
}
void 
GameEventManager::RegisterGameObject(shared_ptr<GameObject> sptrGameObject)
{
    GetGameEventManager()->AddGameObject(sptrGameObject);
}
void
GameEventManager::AddGameObject(shared_ptr<GameObject> sptrGameObject)
{
    GetRegisteredGameVector()->push_back(sptrGameObject);
}

`_player` is an extremely bad choice for an include guard in terms of colliding with an innocent variable, not to mention the fact that it's a [reserved identifier](http://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier). — chris, Jul 27 '14 at 03:14
@tillaert, I would, too. It's probably the only non-standard alternative to something that I actually freely use. — chris, Jul 27 '14 at 03:19
I would never put `using namespace std;` in a header. That can cause a lot of problems in files that include it. — tillaert, Jul 27 '14 at 03:21
@chris If I didn't use underscores would that be acceptable? For tillaert's recommendation of #pragma once, you say it's non-standard, are there issues with using it? — byronaltice, Aug 03 '14 at 23:39
@tillaert Would there be a way to isolate the header with a `using` directive so that `std::` isn't thrown around everywhere, but `#include` ing it won't cause that using directive to be included as well? The only thing I can think of is not using `using` and instead doing `#define shared_ptr std::shared_ptr` for each thing I use from std, but if there's a better way then it would be nice to know. Thanks. — byronaltice, Aug 03 '14 at 23:42
@byronaltice, Typically, include guards use either a unique ID, or something like `__H`, or a combination. While removing the underscore would be allowable, it alone would be a poor choice because then every occurrence of `player` in any file that includes that header would be replaced with nothing. Imagine something like this, which is actually similar to a bug `windows.h` gave me: `class Foo {Player m_player; Foo(Player player) : m_player(player) {} };` Oops, it compiles, but value-initializes `m_player`. The only issue with `#pragma once` is supporting older compilers AFAIK. — chris, Aug 03 '14 at 23:47
@byronaltice The `#define` has the same problem as the `using namespace std` in this case. For headers, I would just write the full namespace. It also adds readability. If you want to use a `#define`, you have to use it after the `include` statements in your header and you will have to `#undef` the define at the end of your header. But, you get a maintenance problem (you have to remember an extra thing and you need to make sure everybody else on the team also remembers) and you can get into trouble if somebody else already used that define. For me, it is simpler to just type the full namespace — tillaert, Aug 04 '14 at 05:52

aschepler · Accepted Answer · 2014-07-27T03:40:31.653

9

GameObject::GameObject()
{
    shared_ptr<GameObject> ptr(this);
    GameEventManager::GetGameEventManager()->RegisterGameObject(ptr);
}

The ptr in this function does not share ownership with any other shared_ptr constructed independently, like the one declared at shared_ptr<Player> sptrPlayer(new Player);. Creating two shared_ptr from raw pointers, rather than copying the first one, generally leads to a double delete.

Instead, you could do something like this:

class GameObject :
    public std::enable_shared_from_this<GameObject>
{
protected:
    GameObject(); // creates the original shared_ptr
    virtual ~GameObject();
};

class Player : public GameObject
{
public:
    static std::shared_ptr<Player> create();
private:
    Player() : GameObject() {}
    virtual ~Player() {}
};

std::shared_ptr<Player> Player::create() {
    return dynamic_pointer_cast<Player>((new Player)->shared_from_this());
}

edited Jul 27 '14 at 03:40

answered Jul 27 '14 at 03:33

aschepler

70,891
9
107
161

See the quoted implementation of `GameObject::GameObject`. And `shared_from_this()` returns a `shared_ptr`, not `shared_ptr`. – aschepler Jul 27 '14 at 11:38
Yes it is owned by a shared pointer: a copy of the one created in base class constructor `GameObject::GameObject()`, which would not be changed from the OP's version. – aschepler Jul 27 '14 at 11:41
Sorry, it's too early on a Sunday to follow this twisted logic. I'll retract my comments and assume this somehow works. – Mike Seymour Jul 27 '14 at 11:43
@aschepler This works and does not double delete. I have an additional quick question though, I assume `shared_from_this()` returns a special shared pointer to `this`, but why am I still able to do this in `GameObject::GameObject()` ? `shared_ptr ptr(this); ` Wouldn't I need to do `shared_ptr ptr(shared_from_this);` ? I would think that we would want to instantiate with shared_ptrs everywhere. It does work, I'm just wondering why it works even though I don't instantiate using your method in my base class as well. When do we need to use shared_from_this vs not? – byronaltice Aug 03 '14 at 23:46

tillaert · Answer 2 · 2014-07-27T03:39:42.190

A player is inherited from a GameObject (class Player : public GameObject). During the gamestate construction, it is set as the player.

 GameState::GameState() : msptrPlayer(NULL)
 {
     shared_ptr<Player> sptrPlayer(new Player);
     SetPlayer(sptrPlayer);
 }

You add the object to a std::shared_ptr object. But, the parent class GameObject also creates a std::shared_ptr object, and adds the same pointer:

 GameObject::GameObject()
 {
     shared_ptr<GameObject> ptr(this);
     GameEventManager::GetGameEventManager()->RegisterGameObject(ptr);
 }

The same pointer is added to two separate std::shared_ptr objects. So both shared pointer objects will delete the object, causing your problem.

This answers my question, but aschepler provided a solution as well. I wish I could give you both the answer credit. — byronaltice, Aug 03 '14 at 23:44

Double Delete using Shared Pointers, even though there is still a shared_ptr to the object, its destructor is being called

2 Answers2