Set delegate to nil when deallocating #134

Closed
wants to merge 5 commits into from

5 participants

@zoul

Otherwise the code crashes when the KKGridView instance gets deallocated. It appears that [UIScrollView dealloc] releases the delegate (which gets deallocated) and then there’s a call to [KKGridView setDelegate:] with a nil argument. The call goes through the setter, which references the already freed _delegate and EXC_BAD_ACCESS ensues.

@zadr

I agree, awesome change! :D

However, why access the delegate through a property now, instead of direct ivar access like elsewhere?

And, I think that it would make sense to keep the gridDelegate property around (where -setGridDelegate: does nothing but call -setDelegate: on self) and marked as deprecated (with __attribute__((deprecated))), instead of getting rid of it completely. This way, people who update KKGridView won't suddenly find their build broken.

Collaborator

I'm with @jonsterling on this one, it's still in an early development phrase.

The phase of development doesn't matter much, if people are using it in the wild.

Collaborator
@zoul zoul Set delegate to nil when deallocating.
Otherwise the code crashes when the KKGridView instance gets
deallocated. It appears that [UIScrollView dealloc] releases the
delegate (which gets deallocated) and then there’s a call to [KKGridView
setDelegate:] with a nil argument. The call goes through the setter,
which references the already freed _delegate and EXC_BAD_ACCESS ensues.
343fde5
@zoul

This appears to be a bit more complex. After some more debugging I realized that the code still crashes even when setting the delegate to nil in dealloc. It looks like the property is not weak, even though my target supports weak properties. I’d say it’s because of the UIScrollView rewrite. The delegate property is now defined like this:

@property (nonatomic, kk_weak) IBOutlet id <KKGridViewDelegate> delegate;

…but since we piggy-back on the delegate property of UIScrollView, there is just a @dynamic delegate in the implementation and therefore the property modifiers from the the KKGridView header are not used. The property is synthesized according to the declaration in UIScrollView.h:

@property(nonatomic, assign) id<UIScrollViewDelegate> delegate;

In other words, the delegate is not a weak property, even though the KKGridView headers make it seem so. The patch in the pull request (343fde5) is useless.

@zoul zoul Quick and dirty fix for the weak delegate problem (#134).
The declaration of the delegate property was changed to make it obvious
that the property isn’t weak, and the delegate is (safely) set to nil
during deallocation to cover at least some use cases where the calling
code expects the delegate to be a weak property.
4d02d22
@jonsterling
Collaborator

So, UIScrollView almost certainly does not release its delegate; as you can see, the property is assign, which means that UIScrollView claims to not exert ownership over its delegate's memory. So, there's got to be something else weird that's going on here.

@zoul

You can discard the first comment, the rest explains the issue better. What UIScrollView probably does is setting the delegate to nil ([self setDelegate:nil]) during dealloc. This call goes through the accessor in KKGridView where the code touches self.delegate. If the delegate is already released by then (and not set to nil), we get the segfault. This is only problem for people who rely on the delegate property being weak, because others set the delegate to nil when the delegate is getting deallocated. I hope this makes sense, it’s a bit complicated.

@jonsterling
Collaborator

Ah, now I follow. Thanks!

@kolinkrewinkel

Should I close this?

@zoul

The main problem still persists. In your master the delegate property is marked weak, but is never weak, even when compiled with toolchain that supports weak properties. This is a nasty bug that can easily crash even in very simple scenarios. I did not think about a proper fix much yet, but I'd suggest to have a separate weak gridDelegate property with its own instance variable (ie. not backed up by UIScrollView's delegate).

@jonsterling
Collaborator

Yeah, I'm thinking you're right. Overriding a superclass's delegate seems rather unsafe at this point. Perhaps for now we should mark delegate as assign (to avoid setting incorrect expectations), until we fix this.

@zoul

This is exactly what my 4d02d22 does, along with one more simple precaution for people who already depend on the delegate being a weak property. Other commits have crept into the pull request again because I'm a Git loser :-), but you can cherry-pick.

@jonsterling
Collaborator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment