Looking for our open source frameworks?

Notes From Recent OmniFocus Crash Fixes

by Brent Simmons on August 26, 2015

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:

  1. Whenever you’re using KVO, you have to be absolutely sure that your object knows about the lifetimes of the objects it’s observing…

  2. …and don’t forget to take Undo into account…

  3. …and your keyPathsForValuesAffecting* methods…

  4. …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.

Making Tab-Switching and Scrolling Faster in OmniFocus for Mac

by Brent Simmons on August 19, 2015

I’ve been on crash-fix-duty for a while with the Mac version of OmniFocus, and just as I was climbing out and starting work on a new feature, I got sidetracked into working on performance.

Well, I didn’t get sidetracked so much as I sidetracked myself. I said, “Hey, Dave, can I work on this instead?” And Dave said, “Go for it!” (Dave Messent is the OmniFocus product manager, and a swell person, and a fellow Seahawks fan in a company full of soccer fiends.)

The specific performance issues I wanted to work on were tab switching and scrolling. I figured I knew what the problems were: Auto Layout and Key-Value Observing (KVO).

The reason I thought that: the table cell views for OmniFocus are complex. Each cell view has multiple text fields, a stack view (with a stack view inside it), a status circle, a flag button, a disclosure button, and so on. The layout is fluid, and there must be significant costs in laying all these out (especially because string measurement, which is slow, is a big part of layout) — and there must also be costs in setting up and tearing down KVO for all the various properties for each row.

So I launched Instruments, chose the Time Profiler instrument, and started running it while switching tabs and while scrolling — because one thing I’ve learned from experience is that Instruments will tell you the truth, and sometimes the truth is surprising.

The Truth

I was partly right. Layout is slow and takes a bunch of time — but, more specifically, it’s the NSStackViews (including a nested stack view). Changing the visibility priority of a view is especially expensive.

Setting up and tearing down the KVO observations isn’t a big deal, though. I was wrong about that. Updating the contents of a cell view is a big deal, however, and this is done when cells are recycled — but this is mainly a big deal because it triggers layout, which is slow.

So here’s what Instruments told me: NSStackView is slow (at least in our case).

But it also told me something else that completely surprised me: the app was spending a whole bunch of time setting and tearing down NSTrackingAreas.

Since I figured that would be the easier thing to deal with, I started there.

Highlight All the Things

When you move the mouse pointer around — in any app — you’ll notice the mouse cursor changing. The thing underneath the mouse pointer may highlight. Controls may appear and disappear.

One of my favorite examples of this in BBEdit: when the cursor is over the far left side of the window, the cursor becomes a right-pointing cursor, and when you drag-select it works by lines. (Paragraphs, effectively, if you’re writing prose.)

This — selecting-by-line — is a nice feature (that I wish Xcode had), and the change to the right-pointing cursor is what makes it discoverable. (Small world note: Jim Correia used to work on BBEdit, and now his office is a few steps away from mine, and he works on OmniFocus.)

In OmniFocus we highlight some controls, and unhide some other controls, when the mouse is over them. To do this we set up an NSTrackingArea — which tracks the movement of the mouse — for each of the different controls that get highlighted or unhidden.

There could be dozens of these, and the app ends up doing a surprising amount of work setting these up and tearing them down — which it has to do often while scrolling, as views go offscreen and come onscreen.

Turns out!

The Cure

Me: “Doctor, it hurts when I go like this.”

Doctor: “Well, don’t go like that.”

How about, instead of all these NSTrackingAreas, we use just one big one, and the area matches exactly the visible rect of the content outline view?

It seems like this would solve the problem by the simple fact that setting up one, once, has to be faster than setting and tearing dozens as you switch tabs and scroll. And it’s true: it is faster.

But it has a drawback: instead of a tracking area for each control that needs one — and the relatively simple code needed for this — we now have a single tracking area and we have to write code that detects what’s under the mouse and tells that view to do the right thing.

Which isn’t as hard as it sounds. Here are the highlights of the solution:

On the content outline view (an NSOutlineView subclass), we set up the single NSTrackingArea:

self.trackingArea = [[NSTrackingArea alloc] initWithRect:self.visibleRect options:NSTrackingMouseEnteredAndExited | NSTrackingMouseMoved | NSTrackingActiveAlways | NSTrackingInVisibleRect owner:self userInfo:nil];

Note the NSTrackingInVisibleRect part — that makes it so that it follows the visibleRect of the outline. This means we don’t have to continually reset the NSTrackingArea’s rect as the view is scrolled. Very convenient.

I added to the outline view a few methods that get called by mouse-tracking: mouseEntered:, mouseExited:, mouseMoved:, and cursorUpdate:.

These methods detect two things: the hovered-over cell view and the hovered-over control inside the view. Two properties — hoveredCellView and hoveredView — are maintained.

The mouseEntered: and similar messages take an NSEvent, and the event has a locationInWindow property which tells us where the mouse is. By using -[NSView convertPoint:fromView:] and NSPointInRect we can find out which view the mouse is over.

(It loops over the cell view’s subviews recursively to find the leaf view. Which is fast. It doesn’t sound fast, but it is.)

Not every view wants to highlight or unhide. I set up an informal protocol — a category on NSView — called OFIMouseTracking. It has several methods:

- (void)mouseTrackerDidEnter;
- (void)mouseTrackerDidExit;
- (BOOL)wantsMouseTrackerMessages;

The nice thing about informal protocols is that, since it’s really just a category, you can provide default implementations. In this case the first two are no-ops and the third returns NO. In other words, most views don’t care.

But the ones that do care respond YES from wantsMouseTrackerMessage, and they get the mouseTrackerDidEnter and mouseTrackerDidExit messages as the content outline view’s corresponding hoveredCellView and hoveredView properties change. And they do the right things (highlighting and unhighlighting; showing and hiding).

Piece of cake.

But What About the Dog’s Ear?

This all sounds great — but it wasn’t working entirely correctly. On the upper-right, above the status circle, is the flag button, which reminds us of a dog’s ear, so we call it a dog’s ear. (That’s life here in the lab.) That flag button should highlight not when the mouse is over the view — which is a rectangle, as all views are — but when it’s over the view and inside the actual dog’s ear shape.

What to do?

I figured that, since it was already working before, when we had lots of NSTrackingAreas, we must have already solved the problem, because the problem’s the same whether it’s one big tracking area or a whole bunch of small ones. The solution has to be that we’re checking to see if the mouse is within the bounds of a specific bezier path.

I was right: we had solved the problem, with a class called OFIBoundsChecker. It’s a very small class that takes a block that generates the path on demand, and has a method — -[OFIBoundsChecker isPoint:inVirtualBoundsForView:] — that returns YES if the given point (the converted mouse location) is inside the dog’s ear (or whatever) path.

All that stuff had already been set up for the views that were already using it. Cool.

So I added a fourth method to the OFIMouseTracking informal protocol:

- (OFIBoundsChecker *)mouseTrackerBoundsChecker;

Back to the outline view: when it’s checking to see if the mouse is over a view, and it determines that yes, it is, it then asks the view for its mouseTrackerBoundsChecker. If it returns nil, then it just uses the view’s rectangle. But if it returns a bounds checker, then it asks the bounds checker if the mouse is inside, and then does the right thing with maintaining the hoveredView property.

And now we have a functioning dog’s ear. Whew.

Next Steps

In the end, this amounts to a modest-but-respectable performance enhancement when scrolling and switching tabs. It’s expected to ship as part of OmniFocus 2.2.4.

While this was totally worth doing, it’s not the whole story, and the bug is still open. The next step will probably be to swap out the NSStackViews in the content outline for something faster.

(But I can’t promise that that exact change will happen, or when. There is always so much work to do, and, in software, everything is provisional until shipping.)


I also did a bunch of micro-optimizations. Sometimes if you string enough of these together they add up to a noticeable improvement. In this case, I think they help, but the NSTrackingArea change is the big one.

One of my favorite micro-optimizations was utterly simple. I changed code of this form…

[cachedObjects enumerateObjectsUsingBlock:^(ODOObject *oneObject, BOOL *stop) {
    if (![predicate evaluateWithObject:oneObject]) {
        [nonMatchingObjects addObject:oneObject];


for (ODOObject *oneObject in cachedObjects]) {
    if (![predicate evaluateWithObject:oneObject]) {
        [nonMatchingObjects addObject:oneObject];

Why would I do such a thing? Don’t we all love blocks? Aren’t loops old-school?

Here’s why: this simple change took the enclosing method’s time from 1.5% to 0.7% of time spent during tab switching.

Totally worth it.

And, if you ask me, the loop version has fewer entities and is easier to read, and I like that.

(For extra credit, read Mike Ash’s Friday Q&A 2010-04-09: Comparison of Objective-C Enumeration Techniques.)

P.S. To Swift fans: yes, that might be even more readable as let nonMatchingObjects = cachedObjects.filter { !predicate.evaluateWithObject($0) }. But Swift at Omni is a whole other topic. (Spoiler: there is some!)