Don’t pass objects as context pointers

2009-04-03 09:27:24 UTC

Cocoa has several functions and methods that run a panel (usually as a sheet) asynchronously. These functions and methods take a target object, a selector, and a context pointer, and when they finish, they send a message to the target with that selector, passing that context pointer.

A common practice is to pass an object in the context pointer. You retain the object before you call the run-panel function or method, then release it in your callback. For example:

    NSBeginAlertSheet(/*title*/ NSLocalizedStringFromTableInBundle(@"Update Available", nil, bundle, @""),
                      ⋮
                      /*modalDelegate*/ self,
                      /*didEndSelector*/ NULL,
                      /*didDismissSelector*/ @selector(downloadSelector:returnCode:contextInfo:),
                      /*contextInfo*/ (void *)downloadURL,
                      …);

In this example, downloadURL is a local variable, and the object in it was retained above, or maybe not autoreleased in the first place. The downloadSelector:returnCode:contextInfo: method will cast the context pointer back to an object type, probably do something with it, then release it.

This has recently become a visible problem because of the Clang Static Analyzer. The analyzer doesn’t know that NSBeginAlertSheet will retain the object, so it flags this call as a leak. This is the most-often-cited example of the checker’s “false positives”: things that it flags as bugs but that aren’t really bugs.

But is it really not a bug? Is it really appropriate to use an object as a context pointer?

Let’s think about what we’re doing here:

  1. The panel doesn’t know the context pointer is really an object, so it doesn’t retain it. To compensate for this, we’re retaining an object on behalf of another object (the panel). This alone should make you feel dirty.

  2. The panel doesn’t know the context pointer is really an object, so it won’t release the object, either. This is particularly a problem if something releases the panel (quite possible in the case of, say, an NSAlert instance). Then it really is a leak.

  3. If we get released (and, hopefully, take the panel with us or at least cancel it), then we leak the object, because we are only expecting to have to release it (or even to have access to it!) in the callback.

Passing objects through context pointers is an anti-pattern. I hope the Clang Static Analyzer developers never remove this warning, because this is not a false bug—it is a real bug that we should all fix in our programs.

Solutions

That object needs to go in an instance variable somewhere.

The lightweight solution is to create an instance variable on the object that is running the sheet. You store the object in the instance variable when you run the sheet (and leave the context pointer NULL), then retrieve the object from the instance variable in your callback. You’ll still need to retain it when putting it in, and release it both in your callback and in dealloc—those aspects don’t change.

I can hear you saying that this is a waste of an instance variable. What are they, rationed? There’s nothing to waste; instance variables are cheap. And leaking objects is far more of a waste of memory than adding an instance variable.

But this isn’t the only solution.

Another way is to create your own subclass of the panel class (NSOpenPanel, NSSavePanel, or NSAlert), and give the subclass a property you can use instead of the context pointer. You could make it abstract (making the property similar to NSMenuItem’s representedObject, nothing more), or make it specific to your application. The former approach is simpler, whereas the latter gives you a base on which to build a more-customized alert if you later decide you want one. It’s up to you.

The advantage of this solution is that, since you’ve formalized the instance variable as part of a class’s API (and since the instance variable is no longer a bit player but the central part of a class), it’ll be harder for you to forget to manage your ownership of it correctly. Memory problems solved.

One way or another, though, you need to put that object into an instance variable somewhere to avoid leaking memory, feeling dirty, and having the Clang Static Analyzer chew you out.

(And yes, all this applies with garbage collection as well. See the section “GC Gotchas” in episode 36 of Late Night Cocoa, but I disagree with Andre Pang’s recommendation of CFRetain and CFRelease; the right solution is not to dip into Core Foundation, but to avoid the context pointer altogether and use an instance variable instead.)

11 Responses to “Don’t pass objects as context pointers”

  1. Vincent Gable Says:

    By the same reasoning, is passing anything, not just objects, to the context-pointer a bad idea, or are there cases where it’s not a dirty thing to do?

  2. Jeff Johnson Says:

    I’m not sure I understand point 3. Let’s talk about NSAlert, because you really shouldn’t be using NSBeginAlertSheet() anymore. ;-) If you called -[NSAlert beginSheetModalForWindow:modalDelegate:didEndSelector:contextInfo:], and you’re about to be deallocated, you should call -[NSApplication endSheet:returnCode:], in which case the didEndSelector is called.

  3. Peter Hosey Says:

    Vincent Gable: If you’re certain that there’s no memory leak, then you can use the context pointer. But you’re right that it’s dubious even for non-objects—consider the case of something you’ve mallocked, for example.

    Jeff Johnson: Hm. I’m not sure it’s OK to pass NSAlert’s window to endSheet:returnCode:. The documentation doesn’t say it is; all it says definitely is that you shouldn’t use orderOut: on the window from outside the did-end method.

  4. Jeff Johnson Says:

    Peter, you’re right, it appears not to be documented whether you can use endSheet: in that case. I’ll see if I can find an example somewhere.

    I did notice this in the documentation, however: “The NSAlert class is not designed for subclassing.”

  5. Peter Hosey Says:

    Jeff Johnson: Yeah, I saw that, too. Shouldn’t be a problem as long as you only extend and not override.

  6. Chuck Says:

    This strikes me as unnecessarily introducing more code. As this pattern is normally used, I can’t figure out what would lead to a memory leak. What is the situation where we gain any benefit except warm fuzzies from introducing ivars and setters and what-have-you?

  7. Peter Hosey Says:

    Chuck: In this very post, I noted not one but two cases in which you leak the context object.

  8. Petteri Kamppuri Says:

    It seems that Apple recommends the CFRetain/CFRelease solution, at least in a GC environment.

    http://developer.apple.com/documentation/Cocoa/Conceptual/GarbageCollection/Articles/gcUsing.html#//apple_ref/doc/uid/TP40008006-SW4

  9. Peter Hosey Says:

    Petteri Kamppuri: Yes, since -retain and -release are no-ops. This doesn’t solve the problem that you’re putting an object through the context pointer in the first place; all it is is retaining and releasing it in a different way. As I said in the post, everything I said there applies just as much with GC as without it.

    I disagree with Apple’s statement that “The solution is to use a CFRetain/CFRelease pair as the value is put into/taken out of the void * parameter.” The solution is to hold your own strong reference to the object.

  10. BJ Homer Says:

    What if you created an instance variable in self to store the object and then pass self in the context pointer? The sheet can access the object through the context pointer, but doesn’t need to retain it. It’s a weak pointer, and if self goes away, the object isn’t leaked. In this case, the context pointer really does become more of a context pointer.

  11. BJ Homer Says:

    Nevermind that; it ends up being pretty much the same solution you already suggested. You could just as easily pass the ivar in the context pointer; the sheet already has access to self by virtue of being a delegate.

Leave a Reply

Do not delete the second sentence.