CargoBayProductRequestDelegate used by SKProductRequest after being destroyed
xinsight opened this issue · 26 comments
In...
-[CargoBay productsWithIdentifiers:success:^(NSArray *products,
NSArray *invalidIdentifiers) failure:^(NSError *error)]
...I'm seeing the following crash:
[CargoBayProductRequestDelegate respondsToSelector:]: message sent to deallocated instance
From what I understand, -[SKProductsRequest _handleReply:] calls productsRequest:didReceiveResponse: on the delegate. The delegate calls the success block, unregisters itself with +[CargoBayProductRequestDelegate unregisterDelegate:] and is promptly destroyed (zombied).
I only started seeing this in the new [redacted] SDK, but it seems that core problem is the delegate object shouldn't destroy itself after running the success block. (The delegate has no way of knowing that the caller is completely finished.)
Not sure what the best solution/workaround here is. Thoughts?
Hi there, unfortunately I'm experiencing the same crash with the updated code...let me know if I can provide any additional info that'd be of use...
I think the issue isn't that it should absorb the call without crashing, but it's that the instance doens't live long enough to receive the actual response (it crashes before the response is received, not after). So weakness would prevent crash but product info would never be received either...
Sorry Mattt, I still haven't been able to properly test your changes. I suspect that using weak references won't fix the issue. StoreKit is calling 'respondsToSelector:' on the CargoBayProductRequestDelegate after the delegate has been destroyed. So, I suspect the opposite approach will fix the issue - keeping a strong ref to the CargoBayProductRequestDelegate (or delaying it's destruction).
To anyone else hitting this problem and looking for a workaround, I've reverted to simply using plain old StoreKit and implementing SKProductsRequestDelegate. It's about the same amount of code, and there are much more useful bits in CargoBay than just getting product info.
I seem to have been able to get this working in iOS7. I'm using
NSMutableSet instead of NSHashTable, and continue to use
unregisterDelegate. I did remove requestDidFinish as well as all of the
casing around ARC. Seems to be working well.
On Wed, Aug 28, 2013 at 9:40 AM, xinsight notifications@github.com wrote:
Sorry Mattt, I still haven't been able to properly test your changes. I
suspect that using weak references won't fix the issue. StoreKit is calling
'respondsToSelector:' on the CargoBayProductRequestDelegate after the
delegate has been destroyed. So, I suspect the opposite approach will fix
the issue - keeping a strong ref to the CargoBayProductRequestDelegate (or
delaying it's destruction).To anyone else hitting this problem and looking for a workaround, I've
reverted to simply using plain old StoreKit and implementing
SKProductsRequestDelegate. It's about the same amount of code, and there
are much more useful bits in CargoBay than just getting product info.—
Reply to this email directly or view it on GitHubhttps://github.com//issues/33#issuecomment-23414498
.
I'm seeing this on iOS6.0 with the latest CargoBay changes (NSHashTable with weak refs). If I switch the hash to use strong refs, the problem goes away.
Of course, there is now a leak. Your approach of using NSMutableSet and explicitly unregistering sounds like the way to go.
Since I can't reproduce this... @xinsight or @alfiehanssen, would you mind sending a pull request with the changes necessary to get this working?
Nice. I'm not getting any crashes now that you've reverted the NSHashTable commit. I added a unit test that might help some of these issues in the future. (Although with the NSHashTable version, the unit test was going into an endless loop, as the test was crashing with EXC_BAD_ACCESS and never exiting the loop in the block. So the unit tests aren't terribly helpful with this kind of problem where variables are unexpectedly released.)
I seem to have included some extra junk in the pull request branch:
So, here's the relevant commit:
Can it be that the merge into master lost some information? I reviewed both issues (#39, #33) and it seems that some commits of #39 seem to be lost on master? At least I can only find one of the mentioned commits xinsight@7e4b8c2.
Even if all information were merged I still experience the same crash with the latest version.
Yep, I'm seeing the same crash on iOS7 GM build
Same issue...
Another crash here, 7.0.0 :-/
I'm getting this crash
Same here. I'm trying to fix the issue but no luck since. Some of my products are invalidated
BTW, I get this working by commenting the unregister on blocks, and calling unregister in requestDidFinish or fail. Basically, the same thing that turning #if __has_feature(objc_arc_weak) to #if !__has_feature(objc_arc_weak) and inverse. Not sure though if any regressions are coming next. ipodishima@4d3eab9
I got this crash too but when I disabled the NSZombieEnable (in Environment Variables) I never get this crash again
I also got it working when commenting out the #if __has_feature(objc_arc_weak) to #if !__has_feature(objc_arc_weak) lines.
Note that it worked fine before I started to target iPhone 5S (arm64).
same issue here
I inspected this crash as after updating to 1.0.0 (yeah still this version because of AFNetworking still remaining at 1.x) this started happening again even without NSZombieEnabled = 0.
The crash reason is simple. In -productsRequest:didReceiveResponse: the success block is called and which in return will unregister the product request delegate. But after StoreKit calls this method it tries to determine if its delegate also supports -requestDidFinish: which of course crashes if ARC is fast enough to deallocate the object before StoreKit has the chance. That is also why commenting the above lines out will solve the problem since then unregister happens after -requestDidFinish:.
I would propose to remove the fancy #if checks because they're not necessary. Also to do the unregister after everything is done, this means moving it down after the failure block. I will do a pull request once I got this release out.
@mattt Can we reopen this issue? It is still existing on iOS 7.1 and AFNetworking 2.0.3.
As promised the mentioned pull request. Removes some of the magic I normally like, but then it isn't really needed and in this case works against us.
Fixed by c41dd18. Thanks @cipherCOM.
I am still getting this error using version 1.0 pod. Which you have to use in a scenario where a dependency to AFNetworking 1.3.3 already exists.
For now I have added this
pod 'CargoBay', :git => 'https://github.com/segiddins/CargoBay.git', :commit => '333b13622ff465539d124a7537b18d4677a6b397'
Which seems to fix my problem but I would prefer basing my dependancies off a master repository if possible. Can anything be reasonably done?