Sign in to follow this  
Sol Blue

Object can be deleted in one dtor but not another! Why?

Recommended Posts

Hi all, I have a very puzzling problem that has been hard to find any help with on Google or on the archives here. I have a particular object which can be deleted correctly in one destructor -- but if instead that deletion occurs in a different destructor, I get a debug assertion failure. I'll post some code and try to explain. A HumanGameView is new'd in another part of the program, and then HumanGameView::init() is called. init() creates two objects: a KeyboardInterpreter and a ProcessManager. My class KeyboardInterpreter derives from class Process, and so it gets loaded into the ProcessManager, which will eventually be responsible for updating it every game cycle. The ProcessManager, in its dtor, goes through all the processes it has been handed (the KeyboardInterpreter is the only one so far) and deletes them. This is where the problem lies! When the ProcessManager dtor tries to delete the KeyboardInterpreter, I get a debug assertion failure in dbgdel.cpp, with the following expression: "_BLOCK_TYPE_IS_VALID(pHead->nBlockUse)". If, relenting, I never add the KeyboardInterpreter to the ProcessManager's list, and just delete it myself in HumanGameView, it works fine -- but this breaks my design. I hope someone with more expertise than me can tell me what's going on!
HumanGameView::HumanGameView() : Process(), mProcessManager(0), mKeyboardInterpreter(0)
{
	// This line isn't involved in the problem.
	HR(D3DXCreateSprite(gDevice, &mSprite));
}

HumanGameView::~HumanGameView()
{
	// This line isn't involved in the problem.
	RELEASECOM(mSprite);

	if (mProcessManager)
	{
		delete mProcessManager;
		mProcessManager = 0;
	}

	// If instead of being given to the ProcessManager, whose dtor
	// deletes it, the KeyboardInterpreter is deleted here, there's no
	// problem.
	if (mKeyboardInterpreter)
	{
		delete mKeyboardInterpreter;
		mKeyboardInterpreter = 0;
	}
}

bool HumanGameView::init()
{
	mKeyboardInterpreter = new KeyboardInterpreter();

	if (!mKeyboardInterpreter->init())
		return false;

	mProcessManager = new ProcessManager();

	// Commenting this next line fixes the problem, because the
	// ProcessManager dtor never tries to delete mKeyboardInterpreter.
	mProcessManager->addProcess(mKeyboardInterpreter);

	return true;
}

ProcessManager::~ProcessManager()
{
	// Delete all the processes in our list. The ProcessManager is
	// responsible for handling pointer deletion, not the Process creator.
	std::list<Process*>::iterator it;
	for (it = mProcessList.begin(); it != mProcessList.end(); ++it)
	{
		Process* prcs = *it;
		
		if (prcs)
		{
			// This delete line, when prcs is the KeyboardInterpreter,
			// causes the debug assertion failure.
			delete prcs;
			prcs = 0;
		}
	}
}

void ProcessManager::addProcess(Process* process)
{
	mProcessList.push_back(process);
}

Share this post


Link to post
Share on other sites
If you are sharing pointers between classes, best to make it explicit by using boost::shared_ptr<>. In idiomatic C++, you rarely call delete, instead you use RAII pointer objects to manage memory. You express ownership semantics by choosing between the different types of RAII pointer objects.

Remember, setting a deleted pointer to 0 cannot help when there are additional pointers pointing at the (now deallocated) memory.

Share this post


Link to post
Share on other sites
Quote:
Is ~HumanGameView being called before ~ProcessManager?


Yes. ~HumanGameView calls ~ProcessManager, both of which will delete the KeyboardInterpreter if it is 0. It won't be deleted twice, though. The ProcessManager destructor deletes the KeyboardInterpreter and sets the pointer to 0. Because the pointer is 0, nothing tries to delete it again.

I guess I should learn Boost, but I didn't think I was writing code complex enough to need it yet. It seems like what I'm doing here should work.

Does anyone know what that assert failure even means? Does it imply a bad pointer, or something's out of scope, or...? From my testing, I haven't been able to find anything I know how to check...:(

Share this post


Link to post
Share on other sites
The KeyboardInterpreter is deleted in your ProcessManager, but mKeyboardInterpreter in HumanGameView still points to this deleted memory (it isn't set to 0 by ProcessManager). kaboom.

Share this post


Link to post
Share on other sites
Quote:
Original post by Sol Blue
Because the pointer is 0, nothing tries to delete it again.


Incorrect. You have two distinct pointers. While you delete one and set it to 0, the other still holds a value.

You need to define: who owns the pointer. Only the pointers owner should delete it. In the case of shared pointers, it can be difficult to assign an owner. In this case, boost::shared_ptr<> is the best solution. Download boost, it will do no end of good for your C++ programming.

Share this post


Link to post
Share on other sites
Quote:
Original post by boersch
The KeyboardInterpreter is deleted in your ProcessManager, but mKeyboardInterpreter in HumanGameView still points to this deleted memory (it isn't set to 0 by ProcessManager). kaboom.


I think the assertion fails before I get to that second deletion, but I'll try removing it when I get back to my room again. You and rip-off are right about the pointers, so I'll take it out anyway. Thanks!


Quote:
Original post by
And, incidentally, doing

if (pFoo)

{

delete pFoo;

pFoo = 0;

}



is pointless. It's perfectly safe to delete a NULL pointer.


Ah, I didn't know that, thanks. It doesn't give you an access violation error for trying to access 0x00000000? I saw things deleted like this in a couple of books, so I imitated it. Is there another reason people sometimes put that in there?

Share this post


Link to post
Share on other sites
Quote:
Original post by Sol Blue
Quote:
Original post by boersch
The KeyboardInterpreter is deleted in your ProcessManager, but mKeyboardInterpreter in HumanGameView still points to this deleted memory (it isn't set to 0 by ProcessManager). kaboom.


I think the assertion fails before I get to that second deletion, but I'll try removing it when I get back to my room again.


Unfortunately, it does indeed fail before that second deletion. (Thanks for your reply, though, I appreciate it!) In other words, the valid pointer that the ProcessManager is holding cannot be deleted, which strikes me as extremely strange.

I'll probably just take rip-off's suggestion and go learn boost::shared_ptr, but does anyone have any last ideas?

Share this post


Link to post
Share on other sites
Quote:
Original post by Sol Blue
Ah, I didn't know that, thanks. It doesn't give you an access violation error for trying to access 0x00000000?


Trying to access "0x00000000" (not really accurate, BTW: a NULL pointer does NOT necessarily correspond to a zero memory address, even in the extremely limited sense in which pointers correspond to memory addresses at all) is undefined behaviour, but deleting a pointer does not really access the pointed-at memory. It first explicitly checks, in the implementation, for a NULL pointer, and otherwise accesses the *bookkeeping information* for that pointed-at memory (which happens to be adjacent in many common implementations).

Quote:
I saw things deleted like this in a couple of books, so I imitated it. Is there another reason people sometimes put that in there?


Old misinformation. Paranoid bad-style code persists even longer than technically-broken bad-style code or unmaintainable bad-style code, because the technically-broken code will eventually actually break for someone and the unmaintainable bad-style code will eventually require maintenance. But the presumed reason why anyone ever thought it was necessary in the first place is that it is necessary with the C moral equivalent (free()).

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this