Alpha2: memory bugs

#1 Continues...

This happens when I delete all the objects of a set (in order to change a level)
this time there is no need for the object to be mounted to something...

Memory::treeRemove(Memory::FreeHeader * hdr=0x06b8a710)  Línea 466 + 0x9	C++
Memory::free(void * mem=0x06b8a750, bool array=false)  Línea 942 + 0x9	C++
dFree(void * in_pFree=0x06b8a750)  Línea 1121 + 0xb	C++
Dictionary::Entry::~Entry()  Línea 347 + 0xc	C++
Dictionary::Entry::'scalar deleting destructor'()  + 0x2b	C++
Dictionary::reset()  Línea 303 + 0x2b	C++
Dictionary::~Dictionary()  Línea 289	C++
Dictionary::'scalar deleting destructor'()  + 0x2b	C++
ExprEvalState::popFrame()  Línea 452 + 0x2b	C++
CodeBlock::exec(unsigned int ip=1093, const char * functionName=0x014a3438, Namespace * thisNamespace=0x01420838, unsigned int argc=1, const char * * argv=0x00991180, bool noCalls=false, const char * packageName=0x00000000)  Línea 1253	C++
CodeBlock::exec(unsigned int ip=2337, const char * functionName=0x0093a00c, Namespace * thisNamespace=0x01092f34, unsigned int argc=1, const char * * argv=0x0012f178, bool noCalls=false, const char * packageName=0x00000000)  Línea 1088	C++
Namespace::Entry::execute(int argc=2, const char * * argv=0x0012f178, ExprEvalState * state=0x00998790)  Línea 836 + 0x2f	C++
Con::execute(SimObject * object=0x04d47190, int argc=2, const char * * argv=0x0012f178)  Línea 829 + 0x15	C++
Con::executef(SimObject * object=0x04d47190, int argc=2, ...)  Línea 850 + 0x1b	C++
t2dSceneGraph::updateScene(float elapsedTime=0.018999999)  Línea 2644 + 0x10	C++
t2dSceneTicker::advanceTime(float elapsedTime=0.018999999)  Línea 243 + 0x1a	C++
clientProcess(unsigned int timeDelta=19)  Línea 69	C++
DemoGame::processTimeEvent(TimeEvent * event=0x0012fa60)  Línea 607 + 0x9	C++
GameInterface::processEvent(Event * event=0x0012fa60)  Línea 71 + 0x11	C++
GameInterface::postEvent(Event & event={...})  Línea 153 + 0x11	C++
TimeManager::process()  Línea 1231 + 0x17	C++
DemoGame::main(int argc=3, const char * * argv=0x010badb0)  Línea 420	C++
run(int argc=3, const char * * argv=0x010badb0)  Línea 1148 + 0x1a	C++
WinMain(HINSTANCE__ * hInstance=0x00400000, HINSTANCE__ * __formal=0x00000000, char * lpszCmdLine=0x00141f05, HINSTANCE__ * __formal=0x00000000)  Línea 1214 + 0x17	C++
WinMainCRTStartup()  Línea 251 + 0x1d	C

I don't have a clue about these bugs, I'm going to restore mi svn repository
and test if this happens also with alpha1, in alpha1 I was experiencing this kind of crashes
only when deleting a lot of objects (with safedelete) if the objects were mounted to other sprites.

These are serious bugs for me. Because my game is going to crash whenever the user changes a level
or there is a lot of things on screen.

[update]
I tested with my alpha1 exe and it crashes too...

#2 I have been getting some strange crashes, but did not get that much detailed debug info. How are you running T2D to get the detailed info?

#3 Adam,

Is it possible to send me the script-code so I could take a closer look?

- Melv.

#4 @Tobie: It's quite easy, just compile the project in Debug mode and watch the call stack, I'm using VC++ 2003
the difficult thing is trying to "fix" the bug :)

@Melv:

Just the standar code of this:
thread

I really don't know if this thing is correct:

if (isObject ($go_bonusSet))                  
{                                             
	%num = $go_bonusSet.getCount ();          
                                              
	for (%k = %num - 1; %k >= 0; %k--)        
	{                                         
		%object = $go_bonusSet.getObject (%k);
		%object.free ();                      
	}                                         
	$go_bonusSet.delete ();                   
}                                             
...
//	Free resources and delete actor
function clsBonus::free (%this)
{
	// free action
	if (isObject (%this.gpx.action))	{ %this.gpx.action.free (); }

	// gpx isn't deleted automatically
	%this.gpx.safeDelete ();

	// delete this object
	%this.delete ();
}

Is the same kind of bug, pointers pointing to garbage memory... this time I created a lot of
shining "coins" to be squashed by my player and got this crashes... I got a headache right now.

#5 I got some crashes like this the other day. I have created a smartpointer class which automatically deletes the SimObject it holds. Formerly the class used safeDelete if a dynamic_cast to t2dSceneObject was successful which worked fine before christmas but yesterday got exactly the same errors that Adam posted when my game shut down. I changed the smartpointer to always use deleteObject() and the problems were gone. It really seems to be safeDelete that causes this error.

#6 Hi Melv!!
I was able to isolate the bugs in my actual project...
and I'm going to send you this time for sure the code
of my "demo_crash", I'm adding now screenshoots of my game
in order to keep your moral high and squash these bugs soon ;)

Please keep in touch.

#7 Is this code from t2dSceneGraph.cc safe?

for( typeDeleteVector::iterator itr = mDeleteRequests.begin(); itr < mDeleteRequests.end(); itr++ )
        {
            // Fetch Object.
            pKillMe = (*itr);

            // No Reference.
            if ( pKillMe == NULL )
            {
                // Delete Object.
                destructInPlace(&itr);

                // Erase this iterator.
                mDeleteRequests.erase(itr);

                // Skip to next one.
                continue;
            }

...

If Vector::iterator is just a typedef for T*, and Vector::erase() does a dMemmove() and then a Vector::decrement() , then after erase itr points to the next element in the vector (which just got memmove there) but is incremented after the continue, so one object is left out.

But the error may be here:

itr is a SimObjectPtr<>*, so destructInPlace is called on a SimObjectPtr<>**. Doesn't that mean that the destructor of a pointer (SimObjectPtr<>*) is called?? I imagine that this has no effect in best case and an undefined effect in worst case. Shouldn't it be destructInPlace(itr)?

I am quite tired, I may well be wrong with all that.

- Michael

#8 @Melv: "demo_crash" e-mailed.

#9 I'm getting the same crash for similar issues, so there's definitely a T2D bug here (that crash report looks very familiar). I was able to isolate it and reproduce it on demand so Melv can hunt it down. Also, I found that changing .safeDelete() calls to .delete() will cause this issue to disappear (in my code, anyway).

#10 @Adam: I received the email, thanks!

@Michael: I'll have a look at that portion of code later when I wake up further. That portion of code was a modification submission from someone else that apparently solved an issue a while ago. I've been meaning to go through it in fine detail and I'll do so later now that you've highlighted a potential problem.

@Jason: Yes, there is definately a bug somewhere but it's going to be some misuse of memory/alloc/dealloc somewhere as objects are coming back corrupted.

This bug has been around for a long time now (Issue #773) but so far I've been frustrated by being unable to reproduce it locally. Nobody will be more happier than me if we can squash this bug together.

I'll report back here with my findings and thanks for your help/insight everyone,

- Melv.

#11

Quote:If Vector::iterator is just a typedef for T*, and Vector::erase() does a dMemmove() and then a Vector::decrement() , then after erase itr points to the next element in the vector (which just got memmove there) but is incremented after the continue, so one object is left out.

Maybe this is what's causing this bug, as updateLifetime() is using safeDelete().

#12 Michael,

I've dedicated this evening to going through this problem and the recently reported issue with objects not being deleted. I just got home so I thought I'll have a quick look at the posts before I begin and won't you believe it, your here helping tie them together as one problem ... sweet.

Weary from work but what the hell, lets see if we can't finally resolve this problem to see what's causing it.

Later.

- Melv.

#13 The lifetime bug already is solved.

The problem is the setLifeTime() call in updateLifetime():

void t2dSceneObject::updateLifetime( const F32 elapsedTime )
{
    // Update Lifetime.
    if ( mLifetimeActive )
    {
        // Reduce Lifetime.
        setLifetime( getLifetime() - elapsedTime );             // <-- this one

        // Are we now dead?
        if ( mLessThanOrEqual( getLifetime(), 0.0f) )
        {
            // Yes, so reset lifetime.
            setLifetime( 0.0f );

            // Initiate Death!
            safeDelete();
        }
    }
}

In some cases it would set mLifeTimeActive to false if (getLifetime() - elapsedTime) was small enough but greater zero, causing the "are we dead?"-if not to fire, and at the same time turning of the lifetime check for the object.
See the other thread.

#14 Melv,
I have rewritten the processDeleteRequests() in the afternoon to be safer but I didn't have the time to test it much. Here it is:

void t2dSceneGraph::processDeleteRequests( bool forceImmediate )
{
	if( !mDeleteRequests.size() )
		return;

	if( !forceImmediate )
	{
		bool allSafe = true;
		for( typeDeleteVector::iterator itr = mDeleteRequests.begin(); itr != mDeleteRequests.end(); itr++ )
			if( !(*itr)->getSafeDelete() )
			{
				allSafe = false;
				break;
			}

		forceImmediate = allSafe;	
	}


	if( forceImmediate )
	{
		for( typeDeleteVector::iterator itr = mDeleteRequests.begin(); itr != mDeleteRequests.end(); itr++ )
		{
			if( !(itr->isNull()) )
			{
				Con::executef(this, 2, "onSafeDelete", (*itr)->getIdString() );
				(*itr)->deleteObject();				
			}

			destructInPlace(itr);
		}

		mDeleteRequests.clear();	
	}
	else
	{
		static typeDeleteVector backBuffer;
		backBuffer.clear();
		
		for( typeDeleteVector::iterator itr = mDeleteRequests.begin(); itr != mDeleteRequests.end(); itr++ )
		{
			if( !(itr->isNull()) )
			{
				if( (*itr)->getSafeDelete() )
				{
					Con::executef(this, 2, "onSafeDelete", (*itr)->getIdString() );
					(*itr)->deleteObject();								
				}
				else
				{
					backBuffer.increment();
					constructInPlace( &backBuffer.last(), itr );
				}
			}
			destructInPlace( itr );
		}

		mDeleteRequests.setSize( backBuffer.size() );

		for( U32 i=0; i < backBuffer.size(); ++i )
		{
			constructInPlace( &mDeleteRequests[i], &backBuffer[i] );
			destructInPlace( &backBuffer[i] );
		}	
	}
}

It uses a second vector to solve the problem with erase. I hope it is correct, it worked well for the lifetime - test (but did not resolve that problem..), but actually don't think the else branch of the forceImmediadte-if ever got executed because it only used standard t2dSceneObjects in my test which always will return true for getSafeDelete(), I suspect.
Yet, the destructInPlace(itr) problem is resolved.

Maybe this saves you some time.

#15 Michael,

Thanks for the heads-up. Instead of targetting that specific function, I looked at the "setLifetime()" function and the check for lifetimeactive is bad so I changed that function to...

void t2dSceneObject::setLifetime( const F32 lifetime )
{
    // Usage Flag.
    mLifetimeActive = mGreaterThanZero( lifetime );

    // Is life active?
    if ( mLifetimeActive )
    {
        // Yes, so set to incoming lifetime.
        mLifetime = lifetime;
    }
    else
    {
        // No, so reset it to be safe.
        mLifetime = 0.0f;
    }
}

... and all appears well. If you can see any problem with this then please shout as I'm a little tired over here but it does seem correct.

Big thanks for spotting that one!

- Melv.

#16 Wow, just saw the code above. I'll have a good look through this now and give it a whirl.

You want a job Michael? ;)

- Melv.

No seriously, you want a job?

#17 @Adam: I've tried your code but as far as I can tell (unless you've recompiled), this is the v1.0.2 SDK. It's very hard to help find problems on this version as it is so old and I could easily be chasing an existing bug. I'm not saying that I won't look but if there's any chance of you using the alpha then it'd be much easier.

No biggy if not though as Michael may have come up with a solution above! Checking it now.

- Melv.

#18 @Melv: my code is alpha 2, how do you say is 1.0.2.could you explain me please?
the exe is the 1.2 alpha PLUS the encrypted pngs and the objectarray code of the forums...

#19

Quote:
You want a job Michael? ;)

You got one for me? ;)

#20 Well, the alpha releases report the engine version upon startup e.g. "T2D Engine (v1.1.0 [Alpha#2]) initialized..." whereas yours reports in the old style of "T2D Engine initialized..." which is how it reported in v1.0.2.

I just double-checked by running "echo( getT2DVersion() );" and it reported alpha#2. I presume you just changed the "main.cs"; just confused me for a second. :)

- Melv.