4 ms·

How a bug in ported garbage-collected code trashed our iOS app

66 points by asarazan 13y ago

asarazan 13y ago
I should note that, while we saw something like 0.4% increased crash rate, we actually don't have a number to compare it against for memory crashes. This is because if memory usage gets too high, the OS will send a kill signal to the process, which can be neither detected nor caught. This means that in our original decision to use this fix, all we had was anecdotal evidence of untraceable crashes. Luckily we had dedicated QA that was keeping pretty solid track of them all, and they piled up. In our case I think it was worth it.
- idunno246 13y ago
  We touched a file on disk and deleted it on entering background. If it exists at startup, last run was a crash. Not a great solution, but gave us some idea of # of crashes not caught by our crash reporting.
  - asarazan 13y ago
    We do this for various other reasons (clearing possibly corrupted state, etc), but we've never uploaded the statistics to the servers. We may begin doing this in the future.
- applecore 13y ago
  If you're not monitoring crashes, check out Crashlytics. http://crashlytics.com/ http://crashlytics.com/ You'd be surprised what can be detected and caught.
  - asarazan 13y ago
    We've been using Crittercism in the past and recently switched to Crashlytics, unfortunately memory crashes are one of the few exceptions (hah) to the rule, as they are SIGKILLs from the OS. Another commenter recommended touching a file at launch and at sleep to track untraceable crashes, which we do for various other reasons, but don't upload the stats. We may begin doing this.
Scorponok 13y ago
Aren't you living dangerously by still using garbage collection on iOS at this point? Apple keep making dire statements about how everyone needs to stop using it, so I'd be worried that iOS8 will just remove it entirely, meaning apps that use GC just wouldn't work at all on that OS...
- asarazan 13y ago
  I should clarify. iOS does not have garbage collection and has never had garbage collection. The root of this bug is in old legacy cruft from the garbage collected days of OSX. The code is dormant in a lot of old CoreFoundation classes, and still causes unexpected behavior from time to time.
- duskwuff 13y ago
  ObjC garbage collection never existed on iOS. The issue was related to framework code that was written by Apple for an ObjC-GC environment (probably Mac OS X) and ported to a non-GC environment (iOS).
mikeash 13y ago
The link to garbage collection is a bit odd. The garbage collector does not, of course, exist on iOS, and this bug has nothing to do with garbage collection. It's just a memory leak due to bad manual memory management. It just so happens that this leak is in code that's written to also work when garbage collection is on, but the leak isn't due to, or even related to, garbage collection. This may be nitpicking, but it made it hard for me to pay attention to the real meat of the thing.
- asarazan 13y ago
  I would disagree pretty strongly with that. The function CFMakeCollectable is explicitly a function for dealing with the garbage collector, and its unpredictability in non-garbage-collected code is the direct cause of the bug. Doesn't it make sense to say that without the existence of Apple GC, the bug never would have existed? Doesn't that at least somewhat justify the title? edit: Furthermore the original intent of including garbage collection in the title was as an ironic twist based on the fact that ios has never had garbage collection. Maybe that didn't convey as well as I would have liked.
  - mikeash 13y ago
    There's no unpredictability. CFMakeCollectable does nothing when not using garbage collection. That's explicitly documented. Without GC, this code would have been written as simply: CFRetain(data); And the same bug would manifest. The bug is just a missing release call. Edit: I'd assume they were going for the standard pattern for code that needs to be both GC and non-GC for bridging CF objects out into the Cocoa world: [NSMakeCollectable(cfobj) autorelease]; In this case, "obj" was retained inline. They just forgot the autorelease. They could have forgotten it just as easily without garbage collection, and the NSMakeCollectable (which inlines to CFMakeCollectable) call is unrelated, aside from possibly occupying the wrong spot of the original programmer's brain at the wrong moment.
    - asarazan 13y ago
      I won't argue too strongly on this, as you're far more knowledgable about it than I am. However, imo the intention and semantics behind a call like CFMakeCollectable implies a transfer of ownership to an external system. A newbie Apple coder could be forgiven for thinking it would still transfer ownership in RC environments, just to the autorelease pool instead of a collector. In all likelihood this is what happened. An intern got at the code and didn't know the details about GC. Obviously the point stands that this interpretation is well-documented to be false, but its naming is definitely misleading. Double edit: I see from your edit that some of my basic assumptions about CFMakeCollectable were wrong, having never actually worked with it. My bad.
bogardon 13y ago
What I really want to know is why NSURLCache does not go to disk when it should. And also if it's fixed in iOS 7...
- asarazan 13y ago
  I agree. Would be nice to stop having to worry about this crap at every turn when developing on iOS.
gte910h 13y ago
The title confused me: It was redefinition of a GC related function that lead to the bug. It has nothing to do with the existence of a GC on iOS.
- asarazan 13y ago
  I was going for more of a sly joke with the title, but it appears to have not gone over well. I now regret my decision.
- mikeash 13y ago
  Actually not a redefinition, as that function has had the same behavior since it was introduced. It's just code that's incorrect when run with reference counting.
jkubicek 13y ago
Our app uses this method to cache images and we were seeing huge memory leaks due to this bug. Like the example showed, the bug manifested itself in a retain count being incremented on property access. Our solution was something like this: if ([self.data retainCount] != [self.data retainCount]) { [[[self.data release] release] release]; } Written from memory, so excuse any mistakes. First (and only) time I've ever seen a legitimate use for `retainCount`. @bbum would be proud (or perhaps horrified).
- bennyg 13y ago
  So in the first line, it has 1 and then checking to see if it's equal bumps it to 2 allocs, and finally the third call, to begin releasing, brings it to 3 (then removal of all 3)? That's super weird - I have yet to encounter something along these lines in my Cocoa work so far. Solid idea to beat the system there. I just hope it's got a nice comment above it haha.
- asarazan 13y ago
  Yeah this is the general idea we went with. We did our best to synchronize around it, but unfortunately we can never have total control, which necessarily introduces race conditions. I would imagine this is what leads to the 0.4% crash rate I mentioned at the top of the article :-(
gdonelli 13y ago
There is no GC in iOS
- asarazan 13y ago
  Thanks for reading the article so thoroughly.
notthetup 13y ago
So tl;dr NSURLCache has a memory leak, be careful when using it.. Or?
shaggyfrog 13y ago
What I love about these stories from the trenches isn't just seeing how the mystery gets solved (although that part is great), but seeing the evolution of the investigation -- which might be the more valuable of the two things when it comes to improving your own bug tracking skills.

66 points by asarazan 13y ago

I should note that, while we saw something like 0.4% increased crash rate, we actually don't have a number to compare it against for memory crashes. This is because if memory usage gets too high, the OS will send a kill signal to the process, which can be neither detected nor caught. This means that in our original decision to use this fix, all we had was anecdotal evidence of untraceable crashes. Luckily we had dedicated QA that was keeping pretty solid track of them all, and they piled up. In our case I think it was worth it.

idunno246 13y ago
We touched a file on disk and deleted it on entering background. If it exists at startup, last run was a crash. Not a great solution, but gave us some idea of # of crashes not caught by our crash reporting.
- asarazan 13y ago
  We do this for various other reasons (clearing possibly corrupted state, etc), but we've never uploaded the statistics to the servers. We may begin doing this in the future.
applecore 13y ago
If you're not monitoring crashes, check out Crashlytics. http://crashlytics.com/ http://crashlytics.com/ You'd be surprised what can be detected and caught.
- asarazan 13y ago
  We've been using Crittercism in the past and recently switched to Crashlytics, unfortunately memory crashes are one of the few exceptions (hah) to the rule, as they are SIGKILLs from the OS. Another commenter recommended touching a file at launch and at sleep to track untraceable crashes, which we do for various other reasons, but don't upload the stats. We may begin doing this.

Scorponok 13y ago

Aren't you living dangerously by still using garbage collection on iOS at this point? Apple keep making dire statements about how everyone needs to stop using it, so I'd be worried that iOS8 will just remove it entirely, meaning apps that use GC just wouldn't work at all on that OS...

asarazan 13y ago
I should clarify. iOS does not have garbage collection and has never had garbage collection. The root of this bug is in old legacy cruft from the garbage collected days of OSX. The code is dormant in a lot of old CoreFoundation classes, and still causes unexpected behavior from time to time.
duskwuff 13y ago
ObjC garbage collection never existed on iOS. The issue was related to framework code that was written by Apple for an ObjC-GC environment (probably Mac OS X) and ported to a non-GC environment (iOS).

mikeash 13y ago

The link to garbage collection is a bit odd. The garbage collector does not, of course, exist on iOS, and this bug has nothing to do with garbage collection. It's just a memory leak due to bad manual memory management. It just so happens that this leak is in code that's written to also work when garbage collection is on, but the leak isn't due to, or even related to, garbage collection. This may be nitpicking, but it made it hard for me to pay attention to the real meat of the thing.

asarazan 13y ago
I would disagree pretty strongly with that. The function CFMakeCollectable is explicitly a function for dealing with the garbage collector, and its unpredictability in non-garbage-collected code is the direct cause of the bug. Doesn't it make sense to say that without the existence of Apple GC, the bug never would have existed? Doesn't that at least somewhat justify the title? edit: Furthermore the original intent of including garbage collection in the title was as an ironic twist based on the fact that ios has never had garbage collection. Maybe that didn't convey as well as I would have liked.
- mikeash 13y ago
  There's no unpredictability. CFMakeCollectable does nothing when not using garbage collection. That's explicitly documented. Without GC, this code would have been written as simply: CFRetain(data); And the same bug would manifest. The bug is just a missing release call. Edit: I'd assume they were going for the standard pattern for code that needs to be both GC and non-GC for bridging CF objects out into the Cocoa world: [NSMakeCollectable(cfobj) autorelease]; In this case, "obj" was retained inline. They just forgot the autorelease. They could have forgotten it just as easily without garbage collection, and the NSMakeCollectable (which inlines to CFMakeCollectable) call is unrelated, aside from possibly occupying the wrong spot of the original programmer's brain at the wrong moment.
  - asarazan 13y ago
    I won't argue too strongly on this, as you're far more knowledgable about it than I am. However, imo the intention and semantics behind a call like CFMakeCollectable implies a transfer of ownership to an external system. A newbie Apple coder could be forgiven for thinking it would still transfer ownership in RC environments, just to the autorelease pool instead of a collector. In all likelihood this is what happened. An intern got at the code and didn't know the details about GC. Obviously the point stands that this interpretation is well-documented to be false, but its naming is definitely misleading. Double edit: I see from your edit that some of my basic assumptions about CFMakeCollectable were wrong, having never actually worked with it. My bad.

bogardon 13y ago

What I really want to know is why NSURLCache does not go to disk when it should. And also if it's fixed in iOS 7...

asarazan 13y ago
I agree. Would be nice to stop having to worry about this crap at every turn when developing on iOS.

gte910h 13y ago

The title confused me: It was redefinition of a GC related function that lead to the bug. It has nothing to do with the existence of a GC on iOS.

asarazan 13y ago
I was going for more of a sly joke with the title, but it appears to have not gone over well. I now regret my decision.
mikeash 13y ago
Actually not a redefinition, as that function has had the same behavior since it was introduced. It's just code that's incorrect when run with reference counting.

jkubicek 13y ago

Our app uses this method to cache images and we were seeing huge memory leaks due to this bug. Like the example showed, the bug manifested itself in a retain count being incremented on property access. Our solution was something like this: if ([self.data retainCount] != [self.data retainCount]) { [[[self.data release] release] release]; } Written from memory, so excuse any mistakes. First (and only) time I've ever seen a legitimate use for `retainCount`. @bbum would be proud (or perhaps horrified).

bennyg 13y ago
So in the first line, it has 1 and then checking to see if it's equal bumps it to 2 allocs, and finally the third call, to begin releasing, brings it to 3 (then removal of all 3)? That's super weird - I have yet to encounter something along these lines in my Cocoa work so far. Solid idea to beat the system there. I just hope it's got a nice comment above it haha.
asarazan 13y ago
Yeah this is the general idea we went with. We did our best to synchronize around it, but unfortunately we can never have total control, which necessarily introduces race conditions. I would imagine this is what leads to the 0.4% crash rate I mentioned at the top of the article :-(

gdonelli 13y ago

There is no GC in iOS

asarazan 13y ago
Thanks for reading the article so thoroughly.

notthetup 13y ago

So tl;dr NSURLCache has a memory leak, be careful when using it.. Or?

shaggyfrog 13y ago

What I love about these stories from the trenches isn't just seeing how the mystery gets solved (although that part is great), but seeing the evolution of the investigation -- which might be the more valuable of the two things when it comes to improving your own bug tracking skills.