nicklockwood / iRate

[DEPRECATED]
http://charcoaldesign.co.uk/source/cocoa#irate
Other
4.1k stars 733 forks source link

Use standard implementation for optional delegate methods #234

Closed chadmoone closed 8 years ago

chadmoone commented 8 years ago

Declaring these methods as optional and then calling them without testing whether they have been implemented can cause crashes in any delegate class where the methods are not implemented.

The current workaround in iRate, which declares a category on NSObject to prevent these crashes, is invalid as per the Apple documentation and can result in undefined behavior:

If the name of a method declared in a category is the same as a method in the original class, or a method in another category on the same class (or even a superclass), the behavior is undefined as to which method implementation is used at runtime.

This PR does not change any functionality, but will prevent any conditions of undefined behavior.

delebedev commented 8 years ago

It' looks like aforementioned non-recomended behaviour breaks when delegate is implemented in Swift.

#0
Crashed: com.apple.main-thread
EXC_BAD_ACCESS KERN_INVALID_ADDRESS 0x000001a1a0435f18
 Raw
0   libobjc.A.dylib 
lookUpImpOrForward + 80
1   libobjc.A.dylib 
_objc_msgSend_uncached_impcache + 56
2   SwiftGift   
iRate.m line 657
-[iRate connectionError:]
3
Foundation  
__NSThreadPerformPerform + 340
9
UIKit   
UIApplicationMain + 204
10  SwiftGift   
main.m line 14
main

@nicklockwood any chance to merge this? It looks like very appropriate change.

chadmoone commented 8 years ago

@nicklockwood any updates? This is an actual intermittent crashing bug for any swift implementation (theoretically objc as well). The PR changes no API or functionality, but simply avoids using the recommended-against and dangerous technique.

This is currently the #2 crash in a popular production app. I think this library is great, but I'm going to have to fork this permanently or roll our own if we can't get this resolved.