Crash when ISceneNode deleted by user

vi-wer · Post by **vi-wer** » Sat Feb 16, 2008 11:12 am

When a SceneNode is deleted by the user without telling the parent node that it does not exist, it causes a crash because the parent tries to access the non-existing node to remove it.
I guess a setParent(0) within the nodes destructor could fix that.

hybrid · Post by **hybrid** » Sat Feb 16, 2008 1:46 pm

You have to remove a scene node. Deleting reference counted objects is a bad thing anyway. Does that fix the problem?

vi-wer · Post by **vi-wer** » Sat Feb 16, 2008 7:15 pm

Yeah, right. I remembered that. But if there's no way to solve this maybe a warning comment in the destructor/doc that tells to use remove() instead of delete could be added.

Halifax · Post by **Halifax** » Sat Feb 16, 2008 7:47 pm

vi-wer wrote:Yeah, right. I remembered that. But if there's no way to solve this maybe a warning comment in the destructor/doc that tells to use remove() instead of delete could be added.

Umm, it's clearly explained in documentation, and has been mentioned over and over and over again on the forms that you don't delete anything. And also you only call drop() on things that were made with a function that has the word 'create' in it.

CuteAlien · Post by **CuteAlien** » Sat Feb 16, 2008 8:46 pm

vi-wer wrote:Yeah, right. I remembered that. But if there's no way to solve this maybe a warning comment in the destructor/doc that tells to use remove() instead of delete could be added.

That's not possible, as there are also valid reasons to use the destructor sometimes. For example the engine has to do so.

vi-wer · Post by **vi-wer** » Sun Feb 17, 2008 9:56 am

How about a warning that is logged from the destructor if the ReferenceCounter is not 0 telling the object might have been removed incorrect? At least for the debug version. Or is there something like this already implemented?

Dorth · Post by **Dorth** » Sun Feb 17, 2008 5:16 pm

This would only encourage faulty usage and buggy behavior. Better have a crash.

vi-wer · Post by **vi-wer** » Sun Feb 17, 2008 10:15 pm

Well, the application still would crash but the user would get a warning that a reference counted object has not been destroyed properly, that's what I thought.

Halifax · Post by **Halifax** » Sun Feb 17, 2008 11:26 pm

vi-wer wrote:Well, the application still would crash but the user would get a warning that a reference counted object has not been destroyed properly, that's what I thought.

It's not going to happen! So why argure for it? The API provides ample documentation how to use it right. As stated above, it promotes bad usage of the library.

If you want to add a warning for this, then do it in your own version of an Irrlicht build.

hybrid · Post by **hybrid** » Sun Feb 17, 2008 11:38 pm

Moreover, it would have to be added to every destructor of each object (due to virtual destructors). No, won't happen.

vitek · Post by **vitek** » Mon Feb 18, 2008 3:52 am

What? Are you confused about how destructors work? The destructor of the most derived class [IUnkown or CRefCounted in this case] is automatically called by the destructor of the derived class, regardless of the virtualness of the destructor in the base class.

Please don't take this as me saying I'd want the requested enhancement to be implemented. I'm just responding to hybrid's comment above.

Travis

hybrid · Post by **hybrid** » Mon Feb 18, 2008 8:44 am

Well, sort of. It was already quite late on a merely frusrating weekend. So I didn't thought about the proper destruction of the inheritance hierarchy.

rogerborg · Post by **rogerborg** » Mon Feb 18, 2008 11:16 am

vi-wer wrote:Well, the application still would crash but the user would get a warning that a reference counted object has not been destroyed properly, that's what I thought.

Ideally, yes, but it's problematical in practice. If you create a reference counted object on the stack or as a member of another object (i.e. it has no valid heap address of its own), it's still created with a ReferenceCounter of 1. So you either let it be cleaned up with its ReferenceCounter still at 1 - and therefore eat the assert() - or you drop() its ReferenceCounter to 0, at which point it calls delete on itself and this happens:

Possible solutions:

Add a method to IReferenceCounted that allows silent destruction of non-0 reference counted objects. It could be an explicit dropNoDelete() or a disableReferenceCounting() that disables the use of ReferenceCounter altogether for stack objects.
Have IReferenceCounted objects create themselves with a refcount of 0 instead of 1, and oblige their creator to explicitly grab a reference to them after creating them on the heap.

Since either solution will involve a fair bit of work (and the latter one will really screw with the heads of current users), it's most likely to happen if you get on and do it yourself.

rogerborg · Post by **rogerborg** » Mon Feb 18, 2008 12:01 pm

Urgh, against my better judgement, here's something to get you started. Tested with Win32 / OpenGL / the Irrlicht Demo app. There's doubtless a long way to go.

Code: Select all

Index: include/IReferenceCounted.h
===================================================================
--- include/IReferenceCounted.h	(revision 1248)
+++ include/IReferenceCounted.h	(working copy)
@@ -13,8 +13,8 @@
 	//! Base class of most objects of the Irrlicht Engine.
 	/** This class provides reference counting through the methods grab() and drop().
 	It also is able to store a debug string for every instance of an object.
-	Most objects of the Irrlicht
-	Engine are derived from IReferenceCounted, and so they are reference counted.
+	Most objects of the Irrlicht Engine are derived from IReferenceCounted, and so 
+	they are reference counted.
 
 	When you create an object in the Irrlicht engine, calling a method
 	which starts with 'create', an object is created, and you get a pointer
@@ -37,10 +37,20 @@
 	You will not have to drop the pointer to the loaded texture, because
 	the name of the method does not start with 'create'. The texture
 	is stored somewhere by the driver.
+
+	Note that you may tell an IReferenceCounted object to not use reference
+	counting.  This should only be done when the object can not and must not
+	call delete on itself, i.e. it is declared on the stack or as a Type foo 
+	rather than a Type * foo member of a class.  DO NOT disable reference
+	counting unless you fully understand what you are doing.
 	*/
 	class IReferenceCounted
 	{
 	public:
+		enum
+		{
+			NotReferenceCounted = -1
+		};
 
 		//! Constructor.
 		IReferenceCounted()
@@ -51,6 +61,10 @@
 		//! Destructor.
 		virtual ~IReferenceCounted()
 		{
+			// A reference counted object should only be destroyed when its ref count is 0.
+			// Destroying it under other circumstances is an error in usage, unless it
+			// has been explictly declared to not use reference counting.
+			_IRR_DEBUG_BREAK_IF(ReferenceCounter > 0)
 		}
 
 		//! Grabs the object. Increments the reference counter by one.
@@ -81,8 +95,17 @@
 		//! You will not have to drop the pointer to the loaded texture, because
 		//! the name of the method does not start with 'create'. The texture
 		//! is stored somewhere by the driver.
-		void grab() const { ++ReferenceCounter; }
+		void grab() const
+		{
+			// It's an error to grab() an object that's not using reference counting
+			// or which has a 0 reference count (because it should already have
+			// been destroyed!)
+			_IRR_DEBUG_BREAK_IF(ReferenceCounter <= 0);
 
+			if(ReferenceCounter > 0)
+				++ReferenceCounter;
+		}
+
 		//! Drops the object. Decrements the reference counter by one.
 		//! Returns true, if the object was deleted.
 		//! The IReferenceCounted class provides a basic reference counting mechanism
@@ -111,8 +134,13 @@
 		//! is stored somewhere by the driver.
 		bool drop() const
 		{
-			_IRR_DEBUG_BREAK_IF(ReferenceCounter <= 0) // someone is doing bad reference counting.
+			// It's an error to drop() an object with a reference count of 0, or which
+			// has had reference counting disabled.
+			_IRR_DEBUG_BREAK_IF(ReferenceCounter <= 0)
 
+			if(ReferenceCounter < 1)
+				return false;
+
 			--ReferenceCounter;
 			if (!ReferenceCounter)
 			{
@@ -137,6 +165,17 @@
 			return DebugName;
 		}
 
+		//! Disables reference counting for this object.  Typically you will only do
+		//! this if the object is created on the stack and you don't want it to assert
+		//! because it's being destroyed with a non-0 ReferenceCounter.  *DO NOT* call
+		//! this simply because you don't understand reference counting semantics.
+		//
+		//! After disabling reference counting, it is an error to call grab() or drop().
+		void disableReferenceCounting(void)
+		{
+			ReferenceCounter = NotReferenceCounted;
+		}
+
 	protected:
 
 		//! Sets the debug name of the object. The Debugname may only be set and
Index: source/Irrlicht/CAnimatedMeshMD2.cpp
===================================================================
--- source/Irrlicht/CAnimatedMeshMD2.cpp	(revision 1248)
+++ source/Irrlicht/CAnimatedMeshMD2.cpp	(working copy)
@@ -305,6 +305,9 @@
 	IAnimatedMesh::setDebugName("CAnimatedMeshMD2 IAnimatedMesh");
 	IMesh::setDebugName("CAnimatedMeshMD2 IMesh");
 	#endif
+
+	// This isn't a pointer, so I don't want to deal with reference counting
+	InterpolationBuffer.disableReferenceCounting();
 }
 
 
Index: source/Irrlicht/CAnimatedMeshSceneNode.cpp
===================================================================
--- source/Irrlicht/CAnimatedMeshSceneNode.cpp	(revision 1248)
+++ source/Irrlicht/CAnimatedMeshSceneNode.cpp	(working copy)
@@ -42,6 +42,8 @@
 	BeginFrameTime = os::Timer::getTime();
 
 	setMesh(mesh);
+
+	MD3Special.AbsoluteTagList.disableReferenceCounting();
 }
 
 
Index: source/Irrlicht/CColladaFileLoader.cpp
===================================================================
--- source/Irrlicht/CColladaFileLoader.cpp	(revision 1248)
+++ source/Irrlicht/CColladaFileLoader.cpp	(working copy)
@@ -271,7 +271,8 @@
 : SceneManager(smgr), FileSystem(fs), DummyMesh(0),
 	FirstLoadedMesh(0), LoadedMeshCount(0), CreateInstances(false)
 {
-
+	// This isn't a pointer, so I don't want to deal with reference counting
+	Parameters.disableReferenceCounting();
 }
 
 
Index: source/Irrlicht/CIrrDeviceStub.cpp
===================================================================
--- source/Irrlicht/CIrrDeviceStub.cpp	(revision 1248)
+++ source/Irrlicht/CIrrDeviceStub.cpp	(working copy)
@@ -42,6 +42,8 @@
 	os::Printer::log(s.c_str(), ELL_INFORMATION);
 
 	checkVersion(version);
+
+	VideoModeList = new video::CVideoModeList();
 }
 
 
@@ -73,6 +75,9 @@
 
 	if (Logger->drop())
 		os::Printer::Logger = 0;
+
+	if(VideoModeList)
+		VideoModeList->drop();
 }
 
 
@@ -146,7 +151,7 @@
 //! by the gfx adapter.
 video::IVideoModeList* CIrrDeviceStub::getVideoModeList()
 {
-	return &VideoModeList;
+	return VideoModeList;
 }
 
 
Index: source/Irrlicht/CIrrDeviceStub.h
===================================================================
--- source/Irrlicht/CIrrDeviceStub.h	(revision 1248)
+++ source/Irrlicht/CIrrDeviceStub.h	(working copy)
@@ -110,7 +110,7 @@
 		scene::ISceneManager* SceneManager;
 		ITimer* Timer;
 		gui::ICursorControl* CursorControl;
-		video::CVideoModeList VideoModeList;
+		video::CVideoModeList* VideoModeList;
 		IEventReceiver* UserReceiver;
 		CLogger* Logger;
 		IOSOperator* Operator;
Index: source/Irrlicht/CIrrDeviceWin32.cpp
===================================================================
--- source/Irrlicht/CIrrDeviceWin32.cpp	(revision 1248)
+++ source/Irrlicht/CIrrDeviceWin32.cpp	(working copy)
@@ -735,7 +735,7 @@
 //! by the gfx adapter.
 video::IVideoModeList* CIrrDeviceWin32::getVideoModeList()
 {
-	if (!VideoModeList.getVideoModeCount())
+	if (VideoModeList->getVideoModeCount())
 	{
 		// enumerate video modes.
 		DWORD i=0;
@@ -745,17 +745,17 @@
 
 		while (EnumDisplaySettings(NULL, i, &mode))
 		{
-			VideoModeList.addMode(core::dimension2d<s32>(mode.dmPelsWidth, mode.dmPelsHeight),
+			VideoModeList->addMode(core::dimension2d<s32>(mode.dmPelsWidth, mode.dmPelsHeight),
 				mode.dmBitsPerPel);
 
 			++i;
 		}
 
 		if (EnumDisplaySettings(NULL, ENUM_CURRENT_SETTINGS, &mode))
-			VideoModeList.setDesktop(mode.dmBitsPerPel, core::dimension2d<s32>(mode.dmPelsWidth, mode.dmPelsHeight));
+			VideoModeList->setDesktop(mode.dmBitsPerPel, core::dimension2d<s32>(mode.dmPelsWidth, mode.dmPelsHeight));
 	}
 
-	return &VideoModeList;
+	return VideoModeList;
 }
 
 
Index: source/Irrlicht/CNullDriver.cpp
===================================================================
--- source/Irrlicht/CNullDriver.cpp	(revision 1248)
+++ source/Irrlicht/CNullDriver.cpp	(working copy)
@@ -413,6 +413,7 @@
 
 	SSurface s;
 	SDummyTexture dummy(filename);
+	dummy.disableReferenceCounting(); // Because it's on the stack
 	s.Surface = &dummy;
 
 	s32 index = Textures.binary_search(s);
Index: source/Irrlicht/CParticleSystemSceneNode.cpp
===================================================================
--- source/Irrlicht/CParticleSystemSceneNode.cpp	(revision 1248)
+++ source/Irrlicht/CParticleSystemSceneNode.cpp	(working copy)
@@ -46,6 +46,8 @@
 	}
 
 	setParticleSize();
+
+	Buffer.disableReferenceCounting();
 }
 
 
Index: source/Irrlicht/CQ3LevelMesh.cpp
===================================================================
--- source/Irrlicht/CQ3LevelMesh.cpp	(revision 1248)
+++ source/Irrlicht/CQ3LevelMesh.cpp	(working copy)
@@ -1261,7 +1261,7 @@
 		meshBuffer->Indices.push_back ( msize + Bezier.Patch->Indices[j] );
 	}
 
-	delete Bezier.Patch;
+	Bezier.Patch->drop();
 }
 
 
Index: source/Irrlicht/CQuake3ShaderSceneNode.cpp
===================================================================
--- source/Irrlicht/CQuake3ShaderSceneNode.cpp	(revision 1248)
+++ source/Irrlicht/CQuake3ShaderSceneNode.cpp	(working copy)
@@ -39,6 +39,9 @@
 	loadTextures ( fileSystem );
 
 	setAutomaticCulling ( scene::EAC_BOX );
+
+	MeshBuffer.disableReferenceCounting();
+	Original.disableReferenceCounting();
 }
 
 
Index: source/Irrlicht/CSceneManager.cpp
===================================================================
--- source/Irrlicht/CSceneManager.cpp	(revision 1248)
+++ source/Irrlicht/CSceneManager.cpp	(working copy)
@@ -165,6 +165,9 @@
 	ISceneNode::setDebugName("CSceneManager ISceneNode");
 	#endif
 
+	// This isn't a pointer, so I don't want to deal with reference counting
+	Parameters.disableReferenceCounting();
+
 	if (Driver)
 		Driver->grab();

rogerborg · Post by **rogerborg** » Wed Feb 20, 2008 11:12 am

vi-wer, just so we're clear, if you want this implemented, it's up to you to do so, and to make a compelling argument for merging the changes into Irrlicht. I'm really not bothered about it.