groue / GRMustache

Flexible and production-ready Mustache templates for MacOS Cocoa and iOS
http://mustache.github.com/
MIT License
1.44k stars 190 forks source link

Crash using NSObject's -valueForKey: IMP with collections on arm64 #70

Closed nolanw closed 10 years ago

nolanw commented 10 years ago

I must apologize up front for this less than helpful report. As I don't have an arm64 device at hand, I can't personally reproduce this crash, and thus don't trust myself to fix it. I'd appreciate any help others can provide.

There seems to be an issue using NSObject's -valueForKey: implementation for collections on arm64. Here's an example crash report. It points to GRMustacheKeyAccess.m line 80. A collaborator with an arm64 device caught this crash in the debugger and found that the key path was contactInfo.count. The key contactInfo correctly returns an NSArray, so it seems it's the .count that causes something to go haywire.

All reports of this crash come from arm64 devices.

Here's a test case that may trigger the crash. Be sure to run it on an arm64 device; it is not reproducible in the simulator (either x86 or x86_64).

[GRMustacheTemplate renderObject:@{ @"things": @[ @1, @2 ] }
                      fromString:@"{{#things.count}}There's things!{{/}}"
                           error:nil];

In the linked crash report, as well as in the test case above, I was following the runtime guide's suggestion for rendering a section only when a collection is nonempty. It's easy enough to work around the problem, but that section of the guide needs updating until a solution can be found.


As an aside, while I haven't done much research into the matter, I suspect GRMustache's current practice, of retrieving NSObject's -valueForKey: implementation and calling it on instances of NSArray et al, is not documented to work. I'm happy to file a radar asking for clarification and/or a fix, if that would be helpful, but I'm not the ideal candidate given my lack of arm64 hardware.

ultramiraculous commented 10 years ago

To add a little more color, the problem isn't specific to collections. If you attempt to use GRValueForKeyNSObjectIMP on anything at all, it goes equally haywire. i.e. replacing

        return [object valueForKey:key];

with

        return GRValueForKeyNSObjectIMP(object, @selector(valueForKey:), key);

causes crashes to happen for any object when you go to access a key value.

I can also confirm that Nolan's code snippet crashes gloriously on my Retina iPad Mini running 7.1 beta 5 and my iPhone 5s running 7.0.4.

groue commented 10 years ago

Thanks to all for your reports. I'm on vacation right now, so I won't attack this issue before two weeks. GRMustacheKeyAccess.m has a detailed comment on how NSArray, NSSet and NSOrderedSet are handled by GRMustache. Feel free to dig deeper on why IMPs can't be used directly on ARM64. Another option to look at is objcMsgSendSuper, should IMPs be banned for any reason.

groue commented 10 years ago

Hi @nolanw, @ultramiraculous.

I just pushed a build of GRMustache on the issue70 branch.

In this branch, GRMustacheKeyAccess.m has been updated, as well as the static library lib/libGRMustache6-iOS.a, so that objc_msgSendSuper is used instead of NSObject's implementation of valueForKey: (see commit 1c94c248429c8f48bab4f29aa02c5bd0107089b4).

Can you tell me if this solves the issue (I don't have any arm64 device)?

ultramiraculous commented 10 years ago

Same crash, unfortunately, but good idea. I threw up a radar just now, so I'll let you know if anything comes of it. I messed around with it more, and it's really just doing anything other than [object valueForKey:] seems to make it crash. Even objc_msgSend-ing valueForKey: to a vanilla NSObject doesn't work. Kinda bizarre.

groue commented 10 years ago

@nolanw wrote:

I was following the runtime guide's suggestion for rendering a section only when a collection is nonempty. It's easy enough to work around the problem, but that section of the guide needs updating until a solution can be found.

Yep. Let's update the doc.

Could anyone of you push a full stack trace of the exception here, please? (Enter the bt command in LLDB, and paste the output here)

groue commented 10 years ago

BTW, thanks @ultramiraculous for your test. I'm quite disappointed :-)

groue commented 10 years ago

All right, here we go for another try, with a home-made implementation of valueForKey: (see commit 3133a3bda5044473a7649f8483cd6c3c559fad75). @ultramiraculous, I hope your device won't crash this time :-)

groue commented 10 years ago

@nolanw Thanks for the Crashlytics report, I didn't notice it the first time.

ultramiraculous commented 10 years ago

Yeah, that certainly appears to work. Wish it didn't have to come to this, but happy for the fix :)

groue commented 10 years ago

OK, so I'll merge this into master, and issue a new release. Thanks a lot, @ultramiraculous !!

groue commented 10 years ago

Hello @ultramiraculous, @nolanw. Thank you again for your much appreciated reports and help. GRMustache v6.9.2 is out, with a fix for this issue.