Hello,

I recently experienced a very strange crash, which after inspecting further turned out to be caused by the destructor of a class being called one time too often in a certain vector creation. I havn't been able to fully determine the excact reason this is happening, so I hope some of you guys has a better idea of what could be going on. I'm using VS2013. This is the code that crashes:

const ecs::ComponentDeclaration& Sprite::GenerateDeclaration(void)
{
	const ecs::ComponentDeclaration::AttributeVector vAttributes = 
	{
		{ L"Texture", core::VariableType::STRING, offsetof(Sprite, stTexture), onSetTexture },
		{ L"Blending", core::VariableType::INT, offsetof(Sprite, blend) },
	}; // this line here

	static const ecs::ComponentDeclaration declaration(L"Sprite", vAttributes);

	return declaration;
}

The crash happens in the commented line, so appearently in the vector construction. Let me show you all related code:

struct ACCLIMATE_API ComponentDeclaration
{
public:
	typedef std::vector<AttributeDeclaration> AttributeVector;
		
//...
}

This is the declaration of the vector.

struct ACCLIMATE_API AttributeDeclaration
{
	typedef void(*SetCallback)(BaseComponent& component);

	AttributeDeclaration(const std::wstring& stName, core::VariableType type, size_t offset, SetCallback pCallback = nullptr);

	std::wstring stName;
	size_t offset;
	SetCallback pCallback;
	ICustomComponentAttributes* pCustom;

private:
	core::Variable m_default;
};

AttributeDeclaration::AttributeDeclaration(const std::wstring& stName, core::VariableType type, size_t offset, SetCallback pCallback) :
    stName(stName), pCallback(pCallback), m_default(type, false), pCustom(nullptr)
{
}

Thats the class that is being stored, with all related variables/methods. It stores a core::Variable class, which acts like a semi-variant data-container, which is defined as follows:

class ACCLIMATE_API Variable
{
public:
	static const unsigned int NO_OBJECT = -1;

	Variable(VariableType type, bool isArray);
	Variable(const Variable& variable);
	Variable(Variable&& variable);
	~Variable(void);

	Variable& operator=(const Variable& variable);
	Variable& operator=(Variable&& variable);

private:

	void* m_pData;
	VariableType m_type;
	bool m_isArray;
	unsigned int m_objectType;
};

Variable::Variable(VariableType type, bool isArray) :
	m_type(type), m_isArray(isArray), m_objectType(NO_OBJECT),
	m_pData(nullptr)
{
	ACL_ASSERT(m_type != VariableType::OBJECT);

	if(type != VariableType::UNKNOWN)
		m_pData = CallByTypeNoObject<newData, void*>();
}

Variable::Variable(const Variable& variable) :
	m_type(variable.m_type), m_isArray(variable.m_isArray), m_objectType(variable.m_objectType)
{
	m_pData = CopyData(variable.m_pData);
}

Variable::Variable(Variable&& variable) :
	m_type(variable.m_type), m_isArray(variable.m_isArray), m_objectType(variable.m_objectType)
{
	m_pData = variable.m_pData;

	variable.m_pData = nullptr;
	variable.m_type = VariableType::UNKNOWN;
	variable.m_objectType = NO_OBJECT;
}

Variable::~Variable(void)
{
	// just for simplicities sake - the actual deletion of the correct type is normally handled
	delete m_pData;
}

Variable& Variable::operator=(const Variable& variable)
{
	if(this != &variable)
	{
		m_type = variable.m_type;
		m_isArray = variable.m_isArray;
		m_objectType = variable.m_objectType;

		DeleteData();

		m_pData = CopyData(variable.m_pData);
	}
	
	return *this;
}

Variable& Variable::operator=(Variable&& variable)
{
	if(this != &variable)
	{
		m_type = variable.m_type;
		m_isArray = variable.m_isArray;
		m_objectType = variable.m_objectType;

		m_pData = variable.m_pData;
		variable.m_pData = nullptr;
		variable.m_type = VariableType::UNKNOWN;
		variable.m_objectType = NO_OBJECT;
	}

	return *this;
}

Now the crash happens inside this classes ctor. Obviously, m_pData is destroyed more then once. The call hierachy looks as follows:

- ctor of first line is being called (line in the first code snipped)

- ctor for second line is being called

- copy-ctor is called from the object of the first line

- copy-ctor is called from the object of the second line

- Destructor is called on the first object

- Destructor is called on the second object

- Destructor is called on the second object again - crash!

Now the reason I know that the destructor is called again, and not m_pData is somehow deleted twice is that by setting m_pData to nullptr in the destructor, the crash resolves. Also note that it is probably not an issue with the Variable-class, as the void* m_pData has already been part of the AttributeDeclaration-class before, as well as part of some other class.

_____________________________________________________

So, thats about all the information I can give. I can't make any sense of the vector-code, so debugging it further is rather out of the question. My question is now: Does someone see something obviously wrong, or knows/has some idea what is going on? Appearently I *could* fix it by simply putting in the nullptr-set in the ctor, but thats way to hackish and obviously just hides the problem. One thing I find interesting is that the move-ctor isn't called, as it is almost always when dealing with this kind of vector-emplacement, maybe thats related to the problem?

will most likely create two attributes first, then use the assignment operator to copy the reference to vector. After this line you have 4 reference of two attributes, two on the "local stack" and two in the vector . Therefor 2 references will share the same m_pData, which will be deleted when leaving the method (two local refernce on stack and the const, but not static vAttributes living on the stack too).

The thing is, the copy-ctor already duplicates m_pData. I missed to include the "CopyData"-method, but thats what it does. So just by the execution, every attribute would have its own "m_pData". Also, like I said, if I do this:

Variable::~Variable(void)
{
     delete m_pData;
     m_pData = nullptr; // that line here fixes it
}

Then the crash goes away. So it appears to be the same instance being deleted twice, or at least the same "shared memory". Also, it crashes way before leaving the method, in the closing bracket line for the vector initializer list. So unless thats what you meant I think you are unfortunately wrong. Still, thanks for the input, I really feel totally screwed up on this one :/

any other ideas?

Best to debug it, try to set breakpoints in the destructor and eventually a breakpoint on the datablock (Variable object) which seems to get deleted a second time. Take a careful look on the call stack to check if really this object got deleted a second time.

I'll give it another shot. I think the problem was that the whole internal call-stack was shown greyed out as "external" in the call-stack, probably due to the initializer list. I'll see if I can get the full call-stack/symbols. In the meantime, if anyone notices anything wrong in the original code, I'd be very happy (won't get back home until in a few hours anyways). Thanks.

Ok, so I looked into it, it definately is the dtor of the same object being called twice. I don't get to see the full source/assemble, but the this-pointer in both dtor calls is identically.

So I don't know whats up with that and why/how it doesn't also crash in the "std::wstring" that the class stores, I quess that does the same thing that I currenclty do with clearing the pointer to nullptr at the end of the dtor...

Your assignment operators look broken to me.

In the copy assignment operator, you copy some metadata values and then call DeleteData. I don't know what that function does or how/if it uses the metadata, but it seems suspect.

In your move assignment operator, you never call DeleteData, so you're leaking the old data whenever you assign an rvalue-ref to it.

DeleteData() does indeed call the correct delete function based on the "m_type" value, so the assignment of the meta-data is vital for it to work. Good catch that I forgot to call it the second time around. Though none of those are actually called, I added them in vein to see if maybe there was something wrong going on here.

This a good example of where the "rule of zero" is helpful. The "rule of five" says you must implement all of the copy constructor, move constructor, both assignment operators, and the destructor, if you implement any of them. The rule of zero says you shouldn't implement any of them because the compiler-generated defaults should just work. If you replace your void* pData with something like std::unique_ptr using a custom deleter, things should Just Work™.

Hm, I might give it a try. I don't know if std::unique_ptr is going to work though, since creating/copying/deleting of the data is coupled to the type-id. I also can't really see how this would solve he issue (unless by "luck" if the unique_ptr also does that nullptr-assignment at the end). I'll give it a try, but I'd really like to know what causes this in the first place... there is nothing wrong in my ctor/copy-ctor, right?

When the debugger fails, there is nothing better than a shedload of std::cout << _LINE_.

Unfortunately, that won't do much here eigther :/ See, I can put breakpoints in whichever code I've written that is called here, like the ctors/dtors and the actual calling code, but the debugger won't let me see what the initialization list is doing - so unless I get to go into that specific code somehow, there is no way to debug it, "classically" or by doing debug output :/

Think you really need a solid grasp on exactly what the program flow is doing here.

True, if I had that, I probably wouldn't need to ask here... but as said above, there appears to be no way to get it, since I get the actual dtor-calls, but not from where exactly the call came from... can't you generally look at source of the initalizer-list of some kind?

Sean quite right of course. A smart pointer can do all this for you.

Well it would probably hide the problem by some smart self-cleanup, but really, this would probably just hide it. There would still be an additional dtor-call to the AttributeDeclaration-class, no matter what... and I don't like that very much. I quess for being workable I can use a smart-pointer, but I still have a bad feeling about the whole thing...