Lately I’ve been fixing crashes in the Mac versions of OmniFocus and OmniOutliner. Some themes emerged.
Key-Value Observing (KVO) Only Pretends to Like You
KVO is wonderfully convenient, and I’m a big fan — until there’s a crash, and then I call it names and go on Twitter and tell everybody how they shouldn’t ever use it. (KVO really stands for “KVetching about key-value Observing.”)
I’m not sure that humans are qualified to use it. It’s a real time-saver, sure, until you get NSKVODeallocate crashes — which are often hard to reproduce and difficult to debug. The time savings of using KVO is often erased by the time spent fixing crashes.
There is really just one problem with KVO, and it’s a big one: the observer has to know about the lifetime of the thing it’s observing. If the thing-being-observed is deallocated while being observed, then the app crashes. (That’s the NSKVODeallocate crash, which is called that because you’ll see NSKVODeallocate in the crash log.)
In the simple case, where there’s a parent-child relationship — when the observer owns the thing-being-observed — you’ll be fine, since you take care to remove the observer before removing the child.
Another case where you’ll be fine: the thing-being-observed lives for the duration of the app. Since it will never be deallocated, you don’t have to worry.
But every single other case is a problem.
And it can become extremely difficult to think about when you add in Cocoa bindings, NSUndo, and keyPathsForValuesAffectingWhatever.
That last one — keyPathsForValuesAffectingWhatever — is worth special mention. It does KVO too, even though you didn’t explicitly set up an observer. And if you return a nested keyPath — foo.bar, foo.bar.baz, or whatever — then your class has to know about the lifetimes of not just foo but also bar and baz. That’s too much; that’s too far removed.
There are some general principles to remember:
Whenever you’re using KVO, you have to be absolutely sure that your object knows about the lifetimes of the objects it’s observing…
…and don’t forget to take Undo into account…
…and your keyPathsForValuesAffecting* methods…
…and maybe just don’t use Cocoa bindings at all. (Because now you’ve set up KVO in Interface Builder where we’ll never find it.)
Well — that last part about not using Cocoa bindings isn’t actually company policy. But the rest of it is just good coding practices: if you’re going to use KVO, you have to ensure that the thing-being-observed outlives the observation, and you have to be aware of all the different ways observations may get set-up.
(This is why I tend to argue for NSNotification instead as the safer means of observing changes.)
Paranoia Is Your Only Friend
We’re an assertion-loving bunch. See our open-source assertions.h — we use all of those, and liberally.
This is great. (Well, there can be a downside if there are too many failing assertions and they get ignored. Over-use is possible. But I’d rather have that problem than the problem of not asserting enough and missing things that should get caught.)
But there is a type of paranoia that gets missed sometimes: AppKit and Foundation methods that crash with a nil parameter.
The way it generally works: we call some Cocoa API that returns a thing that shouldn’t ever be nil. Then we pass that thing on to another Cocoa API — textField.stringValue or menuItem.title or whatever — but it’s nil, and the app crashes.
Lesson: don’t believe their lies!
Or, put another way: it’s good to be aware of which APIs will crash with a nil parameter, and take care in those cases. It may even be a super-rare case — but when an app has lots of users, super-rare cases will happen to a bunch of people every single day. Even on Sundays. Even while you’re asleep.
This extends to cases where we’re using headers with nullability attributes. A system API that says it won’t return nil might return nil anyway, because bugs are always possible. (Maybe one day we’ll be able to rely on this, but it’s too soon right now.)
Mutation Errors
In complex systems — especially complex systems with KVO, which is a side-effect producing machine — enumerating a mutable array can cause crashes.
There’s one sure-fire fix for this: don’t enumerate mutable arrays. Make an immutable copy first, if needed. Boom.
And! Sometimes a system API says it returns an NSArray, but it’s really an NSMutableArray that could get mutated. Just copy any collection the system gives you if you’re going to enumerate it.
NSArrayController Considered a Bugaboo
There were several cases where we had hard-to-reproduce mystery crashes where NSArrayController appeared in the backtrace.
One of the crashes was reproducible if you did the same thing about 30 times in a row. (It wasn’t a constant: it was always around 30.) The others were not reproducible at all, but we had crash logs from users.
In every single case I fixed the bug by switching from NSArrayController over to the old-fashioned NSTableViewDataSource and NSTableViewDelegate methods. It didn’t take much code, and the crashes went away.
I have no explanation for this, but I will say that you won’t find me writing new code that uses NSArrayController.
Memory Management Is Still a Thing
I’ll never kvetch about Automatic Reference Counting (ARC). ARC stands for “Awesome and Really Cool.”
But there are still times when you have to manage memory. There’s a particularly interesting issue with inout NSError ** and @autoreleasepool.
Consider this case:
- (void)someFunction:(NSError **)error {
@autoreleasepool {
*error = …create an error
}
}
Is *error valid after the @autoreleasepool block? Nope.
One way we’ve dealt with this:
- (void)someFunction:(NSError **)error {
@autoreleasepool {
@try {
*error = …create an error
}
@catch (NSException *exception){
}
@finally {
OBStrongRetain(*error); //inside autoreleasepool here
}
}
OBAutorelease(*error); //outside autoreleasepool here
}
It’s not lovely, but it ensures that *error survives until outside the @autoreleasepool block, and that it’s returned as auto-released. Cool.
(See OBUtilities.h for the definitions of OBStrongRetain and OBAutorelease.)
But what happens if there’s also an inner autorelease pool? You need to do that same dance with *error again. And, if you forget, there will be crashes.
General principle: be very careful when mixing NSError ** and @autoreleasepool. It’s less like chocolate and peanut butter and more like egg plants and butterscotch. (“Egg Plant and Butterscotch” was the name of a mid-’60s folk-music-and-comedy duo from Midvale. Their shtick was that they argued all the time. They weren’t very successful, because they argued all the time.)
XPC, or “Is the System Absurdly Loaded?”
Once you’ve figured out which are your own crashes, whatever remains, no matter how annoying, is Apple’s crashes.
And the most common source right now is crashes related to XPC and remote views. Running a save panel is particularly crashy. We’ve filed bug reports with Apple about these. Sometimes the best thing you can do is just report the bug and move on.
