Fix: crashes because delegate not cleaned up properly

[KingOfTheHuns]

Hi,

First, I'd like to thank you for creating this great pod for cocos2d. It is really great. :)

While I was testing it in my prototype app I stumbled upon occasional crashes during scene transition. (BTW: I've used CCNode-SFGestureRecognizers along with other pods like cocos2d and without ARC.) I've investigated the problem and found that removeGestureRecognizer: did not clean up properly aGestureRecognizer.delegate. In addition there was an incorrect call "objc_setAssociatedObject(self, AH_BRIDGE(UIGestureRecognizerSFGestureRecognizersPassingDelegateKey), nil, OBJC_ASSOCIATION_RETAIN);" Here the problem was that the method should use aGestureRecognizer instead of self. Probably it was a typo.

Anyway, not setting aGestureRecognizer.delegate to nil caused that originalDelegate member of SFGestureRecognizersPassingDelegate was not set to nil as well and in some cases after the removeGestureRecognizer called SFGestureRecognizersPassingDelegate object still has received gestureRecognizerShouldBegin callback. The gestureRecognizerShouldBegin callback was referring to originalDelegate and it caused a crash.

The changes I propose fix this problem. (Maybe in ARC case this problem wouldn't surface since is a weak pointer so it will be reset to nil upon it is released.)

[krzysztofzablocki]

association does looks like a typo, thanks for finding that!

About nullifying the delegate, what do you think about instead of clearing it we would call

aGestureRecognizer.delegate = delegate->originalDelegate;

It looks more inline with how UIGestureRecognizers work's natively?

[KingOfTheHuns]

aGestureRecognizer.delegate = delegate->originalDelegate;

should be called after code:

objc_setAssociatedObject(aGestureRecognizer, AH_BRIDGE(UIGestureRecognizerSFGestureRecognizersPassingDelegateKey), nil, OBJC_ASSOCIATION_RETAIN);

to be able replace the delegate of aGestureRecognizer, This however, doesn't solve the problem of dangling pointer. At this point delegate->originalDelegate can contain pointer to an already deallocated object. So, this also might cause crash. (Again I'm talking about non-ARC case here.)

I still think that

aGestureRecognizer.delegate = nil;

is safer way to ensure proper cleanup of delegate.

Anyway, the basic problem is how to determine within the call of

- (void)removeGestureRecognizer:(UIGestureRecognizer *)aGestureRecognizer

that delegate->originalDelegate is still a valid object.

Any thoughts?

[krzysztofzablocki]

Ok, for non-arc case it makes it harder. What if instead of clearing it inside removeObserver, we nil it out inside the dealloc block ? That would be better because if developer want's to remove gesture recognizer manually (instead automatically when object dies) it won't modify the delegate ?

Then inside removeGestureRecognizer you should still repin the originalDelegate as the proper one.

id realDelegate = delegate->originalDelegate;
objc_setAssociatedObject(aGestureRecognizer, AH_BRIDGE(UIGestureRecognizerSFGestureRecognizersPassingDelegateKey), nil, OBJC_ASSOCIATION_RETAIN);
aGestureRecognizer.delegate = realDelegate;

What do you think?

[KingOfTheHuns]

I think that is a very good idea. Go for it. :)

[krzysztofzablocki]

Any chance you could modify your pull request with the solution I mentioned? :)

[KingOfTheHuns]

Sure. I''l do it :)

[krzysztofzablocki]

Almost there, could you just move aGestureRecognizer.delegate = nil; in dealloc block to calling it after removeObserver not before.

If we call it before we will break ability to retrieve deallocBlock from delegate.

[KingOfTheHuns]

I think the code doesn't break the ability to retrieve the deallocBlock. The reason because calling

aGestureRecognizer.delegate = nil;

is must be legit code any time e.g. it could be called by application code as well.

aGestureRecognizer.delegate = nil;

doesn't release any object because it was never retained by the delegate.

`__SFGestureRecognizersPassingDelegate *delegate = objc_getAssociatedObject(aGestureRecognizer, AH_BRIDGE(UIGestureRecognizerSFGestureRecognizersPassingDelegateKey));

works just fine even after nullifying the delegate because aGestureRecognizer is still a valid object.

So, I think the code works. or am I missing something? :)

[krzysztofzablocki]

I think you are right:+1:, it will nil it out because at that point it's going to modify originalDelegate instead of passingDelegate. I've looked at it wrongly.

[KingOfTheHuns]

Thanks a lot. I really appreciate that you've acted so quickly. :+1: Are you planning to create a new tag and update the CCNode-SFGestureRecognizers.podspec file?

[krzysztofzablocki]

Good idea, just did that :)

[KingOfTheHuns]

Thanks a lot. :)