Is CCTouchDispatcher running in the other thread?

cocos2d for iPhone » Programming » cocos2d support (graphics engine)

Is CCTouchDispatcher running in the other thread?

(16 posts) (3 voices)

Started 1 year ago by cocos
Latest reply from TBBle

Tags:

No tags yet.

cocos
Member
I have a strange crash from time to time:

2011-03-16 21:19:33.228 Test[6333:207] *** -[CButton release]: message sent to deallocated instance 0x850600

#0 0x324129a0 in ___forwarding___
#1 0x32409840 in __forwarding_prep_0___
#2 0x324309ec in CFRelease
#3 0x323fee5c in __CFTypeCollectionRelease
#4 0x3240084c in __CFArrayReleaseValues
#5 0x32450c76 in CFArrayRemoveAllValues
#6 0x308ef7a6 in -[NSCFArray removeAllObjects]
#7 0x000d7d60 in -[CCTouchDispatcher touches:withEvent:withTouchType:] at CCTouchDispatcher.m:283
#8 0x000d8328 in -[CCTouchDispatcher touchesEnded:withEvent:] at CCTouchDispatcher.m:316
#9 0x000da88c in -[EAGLView touchesEnded:withEvent:] at EAGLView.m:329
#10 0x3320c900 in -[UIWindow _sendTouchesForEvent:]
#11 0x3320bf94 in -[UIWindow sendEvent:]
#12 0x33207790 in -[UIApplication sendEvent:]
#13 0x33207094 in _UIApplicationHandleEvent
#14 0x342ea7e4 in PurpleEventCallback
#15 0x3244f52a in CFRunLoopRunSpecific
#16 0x3244ec1e in CFRunLoopRunInMode
#17 0x342e91c8 in GSEventRunModal
#18 0x331b5c30 in -[UIApplication _run]
#19 0x331b4230 in UIApplicationMain
#20 0x00002bbc in main at main.m:13

Printing description of ((('CCTouchDispatcher' *) (self)))->handlersToRemove:

Printing description of ((NSObject *)[((('CCTouchDispatcher' *) (self)))->handlersToRemove objectAtIndex:0]):

*** -[CButton _copyDescription]: message sent to deallocated instance 0x850600

*** -[CButton _cfTypeID]: message sent to deallocated instance 0x850600

It looks like it crashes in CCTouchDispatcher:
```
//
	// Optimization. To prevent a [handlers copy] which is expensive
	// the add/removes/quit is done after the iterations
	//
	locked = NO;
	if( toRemove ) {
		toRemove = NO;
		for( id delegate in handlersToRemove )
			[self forceRemoveDelegate:delegate];
		[handlersToRemove removeAllObjects]; // ←--? crash
	}
```
At the moment of crash ‘handlersToRemove’ has one object to be removed
CButton which should not be nill at that very moment. It is released few lines later in my program.

Is it possible that
-(void) touches:(NSSet*)touches withEvent:(UIEvent*)event withTouchType:(unsigned int)idx;

function is late (simply runs in another thread) and removes CButton which was already released?
Posted 1 year ago #
TBBle
Moderator

This crash suggests you've over-released your CButton somewhere, so it's been deallocated while CCTouchDispatcher still has a reference to it.

Posted 1 year ago #

cocos
Member

@TBBle thank you.

I am totally puzzled. Crash happens only from time to time.

I do a clean remove:

- (void) removeSpriteFromLayer:(id) sprite
{
	if (sprite)
	{
		[sprite removeDelegate];

		// remove from the layer:
		[self removeChild:sprite cleanup:YES]; 

		// release sprite from memory:

		[sprite release]; // it was allocated we have to remove it

		sprite = nil;

	}
	else {
		NSLog(@"BAD:removeSpriteFromLayer\n");
	}

}

The funny thing is as follows. I tried to explicitly simulate the crash:

- (void) removeDelegate // sprite
{
	[[CCTouchDispatcher sharedDispatcher] removeDelegate:self];	

}

- (void) removeSpriteFromLayer:(id) sprite
{
	if (sprite)
	{
		[sprite removeDelegate];

		// remove from the layer:
		[self removeChild:sprite cleanup:YES]; 

		// release sprite from memory:

		[sprite release]; // it was allocated we have to remove it

                [sprite removeDelegate]; // no crash
		sprite = nil;
                [sprite removeDelegate]; // no crash

	}
	else {
		NSLog(@"BAD:removeSpriteFromLayer\n");
	}

}

Application is running fine!

Any ideas greatly appreciated.

Posted 1 year ago #

cocos
Member

I do not think that I over release the object.
But obviously there is a problem somewhere. I do not know what it could be…
So far my traces shows that the last two deallocations happen always outside my function. So the final removal of the sprite object is done outside my scope of control.
(I do not like it at all. I would prefer that it is my release which is the last one!)

I deallocate my sprites as follows:

- (void) removeSpriteFromLayer:(id) sprite
{
    if (sprite)
    {
	[sprite removeDelegate];               //1. RefCount++ = 4
	[self removeChild:sprite cleanup:YES]; //2. RefCount-- = 3
	[sprite release]; 		       //3. RefCount-- = 2
	sprite = nil;
    }
    else
    {
	NSLog(@"BAD:removeSpriteFromLayer\n");
    }

}

// LATER ON done by CCTouch classes:
// 4. Done later (!) by CCTouchHandler RefCount-- = 1
// 5. Done later (!) by CCTouchDispatcher RefCount-- = 0

where:

//1)
-(void) removeDelegate:(id) delegate
{
	if( delegate == nil )
		return;

	if( ! locked ) {
		[self forceRemoveDelegate:delegate];
	} else {
		[handlersToRemove addObject:delegate]; // My object is retained // RefCount = 4
		toRemove = YES;
	}
}

//2)
static inline void ccArrayRemoveObjectAtIndex(ccArray *arr, NSUInteger index)
{
	[arr->arr[index] release]; // My object is released RefCount=3
	arr->num--;

	int remaining = arr->num - index;
	if(remaining>0)
		memmove(&arr->arr[index], &arr->arr[index+1], remaining * sizeof(id));
}

//3) My object is released RefCount=2:
[sprite release];

// OUTSIDE my control, sometime later
//4)
//@implementation CCTouchHandler
- (void)dealloc {
	CCLOGINFO(@"cocos2d: deallocing %@", self);
	[delegate release]; // My object is released RefCount=1
	[super dealloc];
}

//5)
//@implementation CCTouchDispatcher
-(void) touches:(NSSet*)touches withEvent:(UIEvent*)event withTouchType:(unsigned int)idx;
{
//…
	if( toRemove ) {
		toRemove = NO;
		for( id delegate in handlersToRemove )
			[self forceRemoveDelegate:delegate];
		[handlersToRemove removeAllObjects]; // My object is released RefCount=0
	}
…
}

Any suggestions how to debug the problem are greatly appreciated!

Posted 1 year ago #

Lam
Member
One suggestion is to restructure the code so we don't need to do sprite release here.
```
- (void) removeSpriteFromLayer:(id) sprite
{
    if (sprite)
    {
	[sprite removeDelegate];
	[self removeChild:sprite cleanup:YES];
	[sprite release];  //<-- Try not to do this
	sprite = nil;
    }
    else
    {
	NSLog(@"BAD:removeSpriteFromLayer\n");
    }

}
```
That is only if you want to follow memory management guidelines. How I always pictured release and retains in my head is that I only release when I can see that I am responsible for it because I created a retain, alloc, or copy.

So passing sprite through a method where you suddenly release is scary to me. Because that method shouldn't have any responsibility for deciding why it should release sprite.

Now if it was a class ivar then that would have been ok but I'm assuming it isn't since you are passing sprite through as an argument.

I try to follow standard objective c conventions and also trust that everyone else does too. For example if I called CCSprite *ping = [CCSprite spriteWithFilename:@"pong.png"];

I would assume that the creator of that method "spriteWithFilename" properly autoreleased the object so I wouldn't do [ping release].
Now it's the same thing as well for passing in arguments to a method.
```
[MyClass classyClass:ping];
```
If I passed in 'ping' I also assume that classyClass won't just release ping because it's not responsible for that.
Otherwise if I try to access ping afterwards, I'm going to crash.

This probably isn't the actual solution to your problem. It's more of my mindset on how I deal with memory management.
Posted 1 year ago #

cocos
Member

@Lam Thank you very much for your input. I hope that I follow you memory management schema…
Sprite is passed as an argument because it comes back via callback selector.

Sprite is created as follows:

@implementation BoardCtr

-(void) addSprite
{
 CButton *s = [[CButton alloc] initWithFile:@"sprite.png"];
 [s addDelegateForSpriteWithPriority:0]	// touches activated
 s.selectorDragRelease = @selector(onEndOfDrag:); // end of drag callback
 //…
 [self addChild:s];
}

// Then it is callback function causing sprite to be removed:

- (void) onEndOfDrag:(id) sprite
{
    [self removeSpriteFromLayer:sprite]; //!
}

I do not see how I could do it differently in my case.
Thank you again!

Posted 1 year ago #

TBBle
Moderator

Just to be clear, CButton is a subclass of CCSprite? I'm finding your code rather hard to follow at the moment... Are all the uses of "sprite" in the above code actually an instance of CButton?

Here's what I think is happening, memory-wise:
addSprite: Allocates/inits a CButton instance, adds it as a touch delegate, and adds it as a child of the BoardCtr instance. Ref count is 3 here. It should be two, since you don't keep "s". So you should be doing an [s release] at the end of addSprite.

So assuming nothing else retains/releases that object, you call into onEndOfDrag which calls removeSpriteFromLayer with what I assume is the same CButton instance. (CButton calls [self.parent applySelector:self.selectorDragRelease withObject:self]?)

removeSpriteFromLayer: The CButton removes itself as a touch delegate (that's reference -1 to you, even though it's temporarily a +1), the CButton is removed by the BoardCtr instance (another -1) and then you do the release which should have been in addSprite. So at this point we should be fine.

Your attempt to recreate the issue won't work, removeDelegate won't affect something that isn't currently registered, and that includes 'nil'.

The reason your crash happens where it does is that when you attempt to deregister a touch delegate while the touch handlers are running, the touch dispatcher puts aside a new list of touch delegates to remove (+1 reference) then once the handlers are finished executing, the object is removed from both lists (-2 references) and it's here that you're seeing the crash, suggesting that somewhere before or during your touch delegate handler, you're overreleasing your CButton.

So what code are you only running from time-to-time on a CButton?

Posted 1 year ago #

cocos
Member

@TBBle Thank you kindly for your analysis.
1. CButton is a subclass of CCSprite
2. All the uses of "sprite" in the above code are actually an instance of CButton.
3. “So you should be doing an [s release] at the end of addSprite”
--- This made me think hard. Should I?
Somehow, I like to keep the sprite and release it later myself. There are a few subtle reasons for that. I do not like the idea that it is at the mercy of other classes, which may or may not retain it. If I keep the sprite I do not have to trust or know internal implementation of library classes. They may change tomorrow.
4.”I'm finding your code rather hard to follow at the moment.”
I am attaching CButton implementation and new test code.

//  CButton.h
//
#import <Foundation/Foundation.h>
#import "cocos2d.h"

// blueprint
@interface CButton : CCSprite <CCTargetedTouchDelegate> {

id	m_delegate;
SEL	m_selectorTouch; // call on the sprite touch
}

@property (nonatomic, retain)	id delegate;
@property (nonatomic)		SEL selectorTouch;

-(id) init;
-(void) removeDelegate;
@end

//  CButton.m
#import "CButton.h"

@implementation CButton

@synthesize delegate = m_delegate;
@synthesize selectorTouch = m_selectorTouch;

-(id) init
{
	if((self=[super init])){
	}
	return self;
}
- (void) dealloc
{
	[m_delegate release];
	[super dealloc];
}

- (void) removeDelegate
{
	[[CCTouchDispatcher sharedDispatcher] removeDelegate:self];
}

-(BOOL) isTouchOnSprite:(CGPoint)touch{

	if(CGRectContainsPoint(
		CGRectMake(self.position.x - ((self.contentSize.width/2)*self.scale),
		self.position.y - ((self.contentSize.height/2)*self.scale),
		self.contentSize.width*self.scale, self.contentSize.height*self.scale),
		touch))
	return YES;
	else return NO;
}

- (CGPoint) getTouchPoint:(UITouch *)touch
{
	CGPoint touchPoint = [touch locationInView:[touch view]];
	touchPoint = [[CCDirector sharedDirector] convertToGL:touchPoint];
	return touchPoint;
}

- (BOOL)ccTouchBegan:(UITouch *)touch withEvent:(UIEvent *)event
{
   CGPoint touchPoint = [self getTouchPoint:touch];
   if([self isTouchOnSprite:touchPoint])
   {
	NSLog(@"ccTouchBegan");
	if ( m_delegate && m_selectorTouch )
	 [m_delegate performSelector:m_selectorTouch withObject:self];
   }
   return YES;
}
- (void)ccTouchMoved:(UITouch *)touch withEvent:(UIEvent *)event
{
}
- (void)ccTouchEnded:(UITouch *)touch withEvent:(UIEvent *)event
{
}
-(void) ccTouchCancelled:(UITouch *)touch withEvent:(UIEvent *)event
{
  [self ccTouchEnded:touch withEvent:event];
}
@end

5.
“somewhere before or during your touch delegate handler, you're overreleasing your CButton”

//---------------------------
The truth is ugly. Very ugly. I still do not fully believe it. But the test code and instruments show that the problem is with the CCTouchDispatcher.

To make a long story short.
I. CCTouchDispatcher is holding on the sprite. It does not release it.
It creats a memory leak.
II. The sprite reacts to the touches – it should not.

So my non existing sprites where still active and that is why I had a crash.

The following code demonstrates the problem:

- (void) onTouch:(id)sprite
{
// The problem is we should not be called. 	

NSLog(@" UGLY BUG! We should not be called 6. (Expected rc=1) rc= %d \n",[sprite retainCount]);

[[CCTouchDispatcher sharedDispatcher] removeDelegate:sprite]; // AGAIN!

NSLog(@" UGLY BUG! We should not be called 7. (Expected rc=1) rc= %d \n",[sprite retainCount]);
// Only now we get object released from the CCTouchDispatcher!!!
}

TEST CODE:

-(void) addSprite
{
// Create sprite
  CButton *sprite = [[CButton alloc] initWithFile:@"Icon.png"];
  NSLog(@" 1. (Expected rc=1) rc= %d \n",[sprite retainCount]); 

 // attach to dispatcher
 [[CCTouchDispatcher sharedDispatcher] addTargetedDelegate:sprite priority:0 swallowsTouches:NO]; //<<<<
 NSLog(@" 2. (Expected rc=2) rc= %d \n",[sprite retainCount]); 

 sprite.delegate = self;
 sprite.selectorTouch =  @selector(onTouch:);

 sprite.position = ccp(160,240);	

 [self addChild:sprite]; // add sprite to the layer
NSLog(@" 3. (Expected rc=3) rc= %d \n",[sprite retainCount]);

 // remove from dispatcher - we should not receive touches anymore
 [[CCTouchDispatcher sharedDispatcher] removeDelegate:sprite]; //<<<<<<<<<<<<<<<<<<<<<<<
 NSLog(@" 4. (Expected rc=2) rc= %d \n",[sprite retainCount]); 

 [sprite release];    // release the sprite
 NSLog(@" 5. (Expected rc=1) rc= %d \n",[sprite retainCount]);
}

The Icon will accept 1 touch. It should not accept touch at all.

The output:

2011-03-18 21:50:48.304 BoardTest[15139:207] 1. (Expected rc=1) rc= 1
2011-03-18 21:50:48.455 BoardTest[15139:207] 2. (Expected rc=2) rc= 2
2011-03-18 21:50:48.648 BoardTest[15139:207] 3. (Expected rc=3) rc= 3
2011-03-18 21:50:48.841 BoardTest[15139:207] 4. (Expected rc=2) rc= 4
2011-03-18 21:50:49.000 BoardTest[15139:207] 5. (Expected rc=1) rc= 3
2011-03-18 21:50:53.750 BoardTest[15139:207] ccTouchBegan
2011-03-18 21:50:53.932 BoardTest[15139:207] UGLY BUG! We should not be called 6. (Expected rc=1) rc= 2
2011-03-18 21:50:54.122 BoardTest[15139:207] UGLY BUG! We should not be called 7. (Expected rc=1) rc= 3

The real retain count is shown here:

http://ow.ly/i/9jr1

Big thank you to @TBBle and @Lam for assisting me in finding the problem.

Posted 1 year ago #

TBBle
Moderator

I've been over the CCTouchDispatcher code a few times looking for memory leaks people claim, and I've never been able to find one. But you've found a bug, well done. (Not a memory leak, but will easily cause an object to be referenced long after it's expected to be forgotten, which is the next best thing. ^_^)

You're calling addSprite from within a touch handler, aren't you? That's why your "expected rc" value doesn't match at 4. Your expectation there is only correct if we are not inside a touch handler. Inside a touch handler, removeDelegate causes a retain (as I mentioned earlier) which is balanced out after the touch handlers are done along with the other release you expect.

(As an aside here, your 'expected rc' values are your expectations based on how you think the internals of the code should work. I hope you realise that? They don't match because they don't reflect the actual internals of the code you're calling.)

The bug is that your object gets added to the "to be added' list (because we're inside a touch handler) and then added to the "to be removed" list (because we're inside a touch handler) and cocos2d then processes the "to be removed" list before the "to be added" list.

If we weren't inside a touch handler, we'd be adding the touch handler directly to the handlers list, then removing it from the same list, so no bug.

Instead we're (after touch handling is finished) removing it from the handlers list (it already wasn't there, so it's a no-op) and then adding it to the handlers list. That's why it sees the next touch, because it was left in the handlers list when it really shouldn't have been there at all.

So the bug only happens if you call add*Delegate: object and removeDelegate: object inside the same touch event.

Please logdge this one on the issue tracker, I've never seen it happen before, but it's not so far fetched that I believe it's never happened.

However, I'm not sure that this bug affects your original code. Your original code wasn't adding and removing the sprite in the same touch event, that I can see. If you could post the relevant parts of BoardCtr like you did for CButton, that'd probably help. And as far as I can see, the bug you've found _won't_ cause a crash. It'll cause an object to be kept alive too long, your crash is an object killed too soon.

Briefly, is CButton supposed to call its target in touchBegan? The name of the callback suggests it's expecting it to be in touchEnd. Also, your CButton will currently claim any touches on the screen, whether they hit the button or not.

Given the intermittent nature of the original crash, I'm somewhat mystified as to what the problem is. I also find your code to be rather hard to follow. If I was reviewing your code in person, I'd keep pointing at things and asking questions starting with "Why..."

I'm also going to pull you up on the following, because it's a common mistake:

3. “So you should be doing an [s release] at the end of addSprite”
--- This made me think hard. Should I?
Somehow, I like to keep the sprite and release it later myself. There are a few subtle reasons for that. I do not like the idea that it is at the mercy of other classes, which may or may not retain it. If I keep the sprite I do not have to trust or know internal implementation of library classes. They may change tomorrow.

There's a couple of problems here.

You're not keeping the sprite. Your reference to the sprite (s in the code in question) is a local variable, lost at the end of the function. So you're actually claiming ownership, and then forgetting about it, and later on getting it back some other way and releasing that ownership. So you're actually doing the opposite of what you say you want to do.

You're claiming ownership, then throwing that away, and relying on later on getting that object back to release. What if some mutation on that object causes you to get a copy of what was originally referenced in s back? You've just leaked the original object, and over-released the new object.

Also, "containers retain their contents" is not internal class implementation details. It's part of the API of that class. You don't program with protection in case NSArray's getObjectAtIndex: method starts requiring NSString instead of NSUInteger, do you? In this case, the rule is actually also Cocoa idiom, so when programming Cocoa it's the assumed case unless the class specifically claims to _not_ do that.

To do what you actually describe in that first sentence, use a "retain" declared property. That's pretty much what they're for.

On the note of Cocoa idiom and retained properties, it's normal and correct Cocoa idiom to _not_ retain one's delegate. Usually one's delegate is retaining you, or ensuring you are retained, so retaining one's delegate creates a circular reference. (This is the reason you use CCTouchDispatcher's add*Delegate and removeDelegate calls from onEnter and onExit, _not_ init and dealloc. If you do the latter, you get circular references and things responding to touches which you thought were deleted.)

Your whole approach to memory management concerns me as trying to take too much control. Memory management in Cocoa is a shared exercise. You may own an object, but you are almost certainly not the _only_ owner of an object, and insisting that you be so is liable to cause bugs and confusion. ^_^

Posted 1 year ago #
cocos
Member
@TBBle
Yes, you are very right! Sprite was created inside the handler. Your analysis of the problem are brilliant. I am glad that bug was found and can be gone in 1.0. Indeed this is a nasty one. Calling a non existing object is a brutal punch by CCTouchDispatcher.

I create the sprite inside the touch handler.
```
// Button to add more sprites
CCMenuItemFont *add = [CCMenuItemFont itemFromString:@"Add New Sprite" target:self selector:@selector(addSprite:)];
add.position=ccp(100,40);
CCMenu *menu = [CCMenu menuWithItems:add,nil];
[self addChild:menu];
```
Inside ‘addSprite’ I create a new sprite and “sometimes” I destroy it.

This is simplified meta code. Sprite is added in addSprite and after the end of touch it will be removed from the layer by callback.
```
-(void) addSprite:(id)sender{

        // I.
	// Create sprite
	CButton *sprite = [[CButton alloc] initWithFile:@"Icon.png"];

	// attach to dispatcher
	[[CCTouchDispatcher sharedDispatcher] addTargetedDelegate:sprite priority:0 swallowsTouches:NO]; //

	sprite.delegate = self;

	//
	sprite.selectorDragRelease = @selector(onEndOfDrag:);

	sprite.position = randomPosition;	

	[self addChild:sprite]; // add sprite to the layer

  // II.
  // This the extra code which I run from time to time inside the addSprite
  // Sorry I did not post this fragment for the obvious reason.
  // I was looking for a bug inside my code and I did not suspect
  // that the following cleanup will not work:
	if (I_do_not_like_created_sprite)
	{
	// remove from dispatcher - we should not receive touches anymore
	[[CCTouchDispatcher sharedDispatcher] removeDelegate:sprite]; 

	[self removeChild:sprite]; // we do not need it 

	[sprite release];  // release the sprite since we just allocated it.  

	// I am done with the sprite – it does not exist
	// or at least that what I thought.
	return;
	}

// [m_board addSpriteToTheBoard:sprite]; //keep reference to the sprite
}
```
Well, now we know that 'removeDelegate' does not work properly in this case.
So nonexistent sprite (from my point of view) can get a message from CCTouchDispatcher:
```
- (void) onEndOfDrag:(id) sprite
{
[sprite removeDelegate];

// remove from the layer:
[self removeChild:sprite cleanup:YES]; 

[sprite release]; // if it was allocated we have to remove it
// At this point the sprite is gone
}
```
And it WILL crash – since this sprite is long gone and forgotten. This was illegal call from CCTouchDispatcher.
When sprite exists than it is correctly removed above.

“Also, your CButton will currently claim any touches on the screen, whether they hit the button or not.”
Only touches, which hit the button, will cause onTouch to fire. And since I have swallowsTouches:NO the other sprites can get the touch.

The ownership and retention is indeed difficult subject and I thank you for your clarifications.

“You're not keeping the sprite. Your reference to the sprite (s in the code in question) is a local variable, lost at the end of the function”

- True.

Also the code as it was posted would leak memory since at the end of the day all local references to 's' where thrown away and there is no easy direct way to clean memory taken by all remaining sprites still attached to the layer.

I did not show the whole code since I did not want to obscure the main issue. But since it resurfaced I apologies for the confusion and admit that as a matter of fact I retain ‘s’ sprite by adding it to the m_board container, which is NSMutable array of cell objects.
When sprite 's' is dragged it is in fact his (deep) copy of the sprite from the m_board being dragged around the screen. When the drag ends that copy is either removed or added to the different m_board container cell.
Finally when the applications is winded down the dealloc would take care of cleaning of the objects referenced in the m_board container
```
- (void) dealloc
{
[m_board removeAllObjects]; // all the stored BoardCell objects have their retain counts reduced to 0
[m_board release];

[super dealloc];
}
```
“What if some mutation on that object causes you to get a copy of what was originally referenced in s back?”

I am not sure what you are referring to. Could you clarify this? I assume that any drastic operation on the object is done by me (so I should know what I am doing) and not by library classes.

“You may own an object, but you are almost certainly not the _only_ owner of an object, and insisting that you be so is liable to cause bugs and confusion.”

It is very true. I will try to be careful. Thank you again for your deep insight in the matter.
Posted 1 year ago #

TBBle
Moderator

You've tripped over the _same_ mistake again. If you put it in m_board, m_board owns the object, NOT you. So you're still claiming ownership and then throwing that reference away.

And the _crash_ was caused precisely by the memory management problem I am talking about. Specifically, here's why it was crashing when you hit this bug, instead of simply having a reference you didn't know CCTouchDispatcher was holding:

-(void) addSprite:(id)sender{

	CButton *sprite = [[CButton alloc] initWithFile:@"Icon.png"];
	// You own "sprite"

	[[CCTouchDispatcher sharedDispatcher] addTargetedDelegate:sprite priority:0 swallowsTouches:NO]; //
	// CCTouchDispatcher sharedDispatcher might own the object pointed to by "sprite", you don't really know or care.

	sprite.delegate = self;
	sprite.selectorDragRelease = @selector(onEndOfDrag:);
	sprite.position = randomPosition;	

	[self addChild:sprite]; // add sprite to the layer
	// self.children now owns the object pointed to by sprite

	if (I_do_not_like_created_sprite)
	{
	[[CCTouchDispatcher sharedDispatcher] removeDelegate:sprite];
	// CCTouchDispatcher relinquishes any ownership it might have taken above. You don't care. It might relinquish it later. Or due to a bug, never. ^_^

	[self removeChild:sprite]; // we do not need it
	// self.children relinquishes its ownership of the object it claimed above

	[sprite release];  // release the sprite since we just allocated it.
	// You no longer own "sprite"

	return; // You return not owning "sprite"
	}

	// [m_board addSpriteToTheBoard:sprite]; //keep reference to the sprite
	// If you uncomment this, m_board takes ownership of the object pointed to by "sprite". However, that's irrelevant to this matter
	return; // You return owning "sprite".
		// But "sprite" is gone now. So you've leaked a reference.
}

So you can see the problem: In your two code paths, you return in two different ownership states for "sprite". Since "sprite" the local variable is gone when you return, the correct state is to not leak the reference.

- (void) onEndOfDrag:(id) sprite
{
[sprite removeDelegate];
// This is equivalent to "[[CCTouchDispatcher sharedDispatcher] removeDelegate:sprite];"
// It's safe to repeat because "remove" on something that isn't present succeeds.

[self removeChild:sprite cleanup:YES];
// Safe for the same reason

[sprite release];
// Here you're releasing an object you don't actually own. (You don't own function parameters.)
// You're relying on this only being hit once, and only if you leaked a reference when you created the sprite earlier.
}

Note that your dealloc will leak all your CButton objects which are still children of the board when it is called.

As it happens, _all_ of this pain could probably have been avoided if you'd structured your touch delegate code like the wiki page about touch delegates, and made your CCNode subclasses register themselves in onEnter and deregister again in onExit. But that's getting into whole-of-program code structure, we're still working at the "not losing track of our objects" level of code structure.

It's true that an object replacing itself with a copy would be highly unusual (I can't think of a single use-case for this that isn't contrived, or caused by an incredible misuse of NSDictionary keys). But once again, the memory management style you're using is requiring you to make assumptions about what the library does or doesn't do internally.

So here's a contrived example

@property (readwrite,retain) NSMutableDictionary *myCacheDict;

addTextureToCache: (CCTexture2D*) texture withName: (NSString*) name andHDness: (enum hdness) hdness {
 // 00 is the key format ID. Only one format so far, but future-proofing's cheap.
 NSMutableString* cacheKey = [[NSMutableString alloc] initWithFormat: "00%@", [keymaker keyForName: name HDness: hdness]];

 // Removing an existing entry for this name and HDness omitted.

 [self.myCacheDict setObject: texture forKey: cacheKey];
 // Leak cacheKey here. We'll clean it up when we remove the object from the dictionary.
}

fetchTextureByName: (NSString *)name andHDness: (enum hdness) hdness {
 NSString * key = [keymaker keyForName: name HDness: hdness];
 return [self.myCacheDict fetchObjectForKey:key];
}

removeTextureFromCache: (CCTexture2D*) texture {
 NSString *cacheKey = [myCacheDict oneTrueKeyForObject: texture];
 if (cacheKey == nil ) return;
 [self.myCacheDict removeObjectForKey:cacheKey];
 NSLog(@"Removed texture for %@ from cache", cacheKey);
 // We're done with the cacheKey we created earier.
 [cacheKey release];
 // 1 alloc/init, 1 release. But here we crash and burn.
}

removeTextureByName: (NSString*) name andHDness: (enum hdness) hdness {
 [self removeTextureFromCache: [self fetchTextureByName: name andHDNess: hdness]];
}

OK, so fairly contrived, but the problem should be clear. (Clue if you want it: NSDictionary copies its keys)

Posted 1 year ago #

cocos
Member

@TBBle
"You've tripped over the _same_ mistake again"
“You're claiming ownership, then throwing that away, and relying on later on getting that object back to release.”

Finally, I admit that my approach was convoluted at best and plainly wrong from memory management guidelines point of view. Mea Culpa!

I have changed my approach, now:
1) I do not leak reference anymore creating local reference and forgetting about it.
I release object right away.
2) I do not release sprite inside callback function since I do not own function parameters.
3) I trust and depend on containers to retain their contents.

Thank you for the example. It was easy

[self.myCacheDict setObject: texture forKey: cacheKey];  // Leak cacheKey here. We'll clean it up when we remove the object from the dictionary. 

//co! - Reference is lost forever to the original NSMutableString* cache Key
//co! - There is no way to come back to the original cacheKey object after function exits
//co! - We not only leaked reference we also leaked memory.

removeTextureFromCache: (CCTexture2D*) texture {
 NSString *cacheKey = [myCacheDict oneTrueKeyForObject: texture];
//co! This is reference to local copy which myCacheDict made.
//co! This is not reference to our original object

 if (cacheKey == nil ) return;
[self.myCacheDict removeObjectForKey:cacheKey];

//co! Both object and cacheKey are gone. They are released by their owner: myCacheDict container

NSLog(@"Removed texture for %@ from cache", cacheKey);  // We're done with the cacheKey we created earier. 

[cacheKey release];
// 1 alloc/init, 1 release. But here we crash and burn.

//co! We are releasing object that does not belong to us. It was already released.
//co! cacheKey is not the object which we created. Reference to the our original object has been lost.
}

I thank you again for your time and patience. Due to your posts I have changed and mended my ways for better!

Posted 1 year ago #

TBBle
Moderator

@cocos: Gold star for demonstrating your understanding of the problem. ^_^ To be honest I did intend to look up the actual method name, rather than oneTrueKeyForObject...

Posted 1 year ago #
TBBle
Moderator

For the record, the bug identified in the above post regarding both adding and removing a single touchhandler during a single touch event is bug 1139

http://code.google.com/p/cocos2d-iphone/issues/detail?id=1139

Posted 1 year ago #
cocos
Member
@TBBle Thank you kindly for your Gold star. I fill honored! BTW, this look interesting:
http://dysart.cs.byu.edu/CHDataStructures/interface_c_h_bidirectional_dictionary.html
//---
“As it happens, _all_ of this pain could probably have been avoided if you'd structured your touch delegate code like the wiki page about touch delegates, and made your CCNode subclasses register themselves in onEnter and deregister again in onExit.”

I did! I restructured code for CButton. It allowed me to improve a few aspects of the code and find some interesting behavior in the process. I am glad that I went for it.
```
- (void)onEnter
{
	[[CCTouchDispatcher sharedDispatcher] addTargetedDelegate:self priority:0 swallowsTouches:YES];
	[super onEnter];
}

- (void)onExit
{
	[[CCTouchDispatcher sharedDispatcher] removeDelegate:self];
	[super onExit];
}
```
However bug is a bug! And the code behaves inappropriately.

addSprite is called from the touch handler:
```
-(void) addSprite:(id)sender{
// Create sprite
	CButton *sprite = [[CButton alloc] initWithFile:@"Icon.png"];
	NSLog(@" 1. (Expected rc=1) rc= %d \n",[sprite retainCount]); 

	sprite.delegate = self;
	sprite.selectorTouch =  @selector(onTouch:);

	sprite.position = ccp(220,160); // IN THE MIDDLE OF THE SCREEN	

	[self addChild:sprite]; // add sprite to the layer
	NSLog(@" 2. (Expected rc=2) rc= %d \n",[sprite retainCount]);

	[self removeChild:sprite cleanup:YES]; // remove the sprite
	NSLog(@" 3. (Expected rc=1) rc= %d \n",[sprite retainCount]); 

	[sprite release];  // release the sprite - Sprite should be gone from memory and memory released.
	NSLog(@" 4. (Expected rc=0) rc= %d \n",[sprite retainCount]);
}

- (void) onTouch:(id)sprite
{
// The problem is
// 1. This function should not be called!
// 2. Sprite should not be retained by CCTouchDispatcher!

NSLog(@" UGLY BUG! We should not be called 5. (Expected rc=0) rc= %d Sprite is retained for ever!\n",[sprite retainCount]);
}

// expected rc is an approximation of the desired behavior but
// definitely should go to 0 after all cleanups are done
```
Sprite is added and removed. But taping screen in the middle will result in calls to the
```
- (void) onTouch:(id)sprite
```
This is the trace:

BoardTest[10320:207] 1. (Expected rc=1) rc= 1
BoardTest[10320:207] 2. (Expected rc=2) rc= 3
BoardTest[10320:207] 3. (Expected rc=1) rc= 3
BoardTest[10320:207] 4. (Expected rc=0) rc= 2

// taping on non existing sprite:

BoardTest[10320:207] ccTouchBegan
BoardTest[10320:207] UGLY BUG! We should not be called 5. (Expected rc=0) rc= 1 Sprite still retained!
BoardTest[10320:207] ccTouchBegan
BoardTest[10320:207] UGLY BUG! We should not be called 5. (Expected rc=0) rc= 1 Sprite still retained!
BoardTest[10320:207] ccTouchBegan
BoardTest[10320:207] UGLY BUG! We should not be called 5. (Expected rc=0) rc= 1 Sprite still retained!

Memory management for the CButton:

CCTouchDispatcher is holding CButton forever. Also it will collect all dead CButtons delivered by addSprite with every call to addSprite.
Posted 1 year ago #
TBBle
Moderator

Heh, yeah. By "all of this pain" I actually meant the crash. The bug is definitely real, but doesn't _have_ to cause a crash, just a misplaced object, as you're seeing now.

Posted 1 year ago #

RSS feed for this topic

Reply

You must log in to post.