Argh! Wasted two hours on stupid bug. (Nat!'s Web Journal)

I am using the DragNDropOutlineView from the developer examples as the basis for my IDE organization screen. Now I didn't do anything spectular, but I already ran into a dreaded retain, release crasher.

This function in ImageAndTextCell.m is wrong:

- copyWithZone:(NSZone *)zone {
    ImageAndTextCell *cell = (ImageAndTextCell *)[super copyWithZone:zone];
    cell->image = [image retain];
    return cell;
}

Do you know what could go wrong ? Sorry, no price money for this.

Hint 1: This is also defined in ImageAndTextCell a subclass of NSTextFieldCell:

- (void)setImage:(NSImage *)anImage {
    if (anImage != image) {
        [image release];
        image = [anImage retain];
    }
}

- (NSImage *)image {
    return image;
}

Hint 2: Copy would not solve the problem.

Hint 3: The superclass calls setImage: during copyWithZone:

Hint 4: NSCopyObjectdoes a memcpy of the source object into the copy.

The solution

ImageAndTextCell ist declared thusly:

@interface ImageAndTextCell : NSTextFieldCell {
@private
    NSImage	*image;
}

Notice the clever use of @private there.

NSTextFieldCell subclasses eventually from NSCell. NSCell implements setImage: but has it's own instance variable _support which it uses for setImage: (amongst others).

Now NSCopyObject, which is apparently used, copies both instance variables over to the new cell. When -[NSCell copyWithZone:] calls setImage: to copy over the information (yet again) it calls -[ImageAndTextCell setImage:] with an argument of nil because _support isn't set. This will now release our image and of course that is no good.

Who's to blame ? The use of an accessor in copyWithZone: is probably wrong. The original coder should have known that _support is already copied and just retain it. The demo code that overrides setImage: but uses a different instance variable (for good reason) is no piece of genius either. Better would have been a renamed accessor.

OK it wasn't easy. ZNeK got it about 99% right.

If you assume that _support is always nil, and that doesn't seem right, a solution would be:

- copyWithZone:(NSZone *)zone {
    [image retain];
    ImageAndTextCell *cell = (ImageAndTextCell *)[super copyWithZone:zone];
    cell->image = [image retain];
    return cell;
}

The real solution is to not override setImage:.

Comments (4)

Stephane:

Usually, the code is more:

if (anImage!=image)
{
[image release];

image=[anImage copy];
}

and:

return [[image copy] autorelease];

what about a copyWithZone on the copyWithZone too.

Posted by Stephane | May 25, 2004 11:07 AM

Posted on May 25, 2004 11:07

Nat! :

Copy on an NSImage is in general too costly IMO.

Posted by Nat! | May 25, 2004 2:20 PM

Posted on May 25, 2004 14:20

Helje:

Hint 4 is the issue, the NSObject documentation states: "NSObject does not itself support the NSCopying protocol. Subclasses must support the protocol and implement the copyWithZone: method. A subclass version of the copyWithZone: method should send the message to super first, to incorporate its implementation, unless the subclass descends directly from NSObject."
Read: if you inherit from NSObject, do not call [super copyWithZone:] (last part of the sentence). So I guess NSCell is broken wrt Foundation API? ;-)

Posted by Helje | June 2, 2004 7:01 PM

Posted on June 2, 2004 19:01

OK, my last post was nonsense. As the documentation states NSObject (at least on Panther) really does not implement -copyWithZone: (I thought it implemented the method, but didn't formally conform to the NSCopying protocol).
This in turn makes it incompatible with libFoundation and with quite some code in OGo which needs to add -copyWithZone: methods ;-)

Posted by Helje | June 8, 2004 5:34 PM

Posted on June 8, 2004 17:34

Nat!'s Web Journal

Argh! Wasted two hours on stupid bug.

The solution

Comments (4)

Search

About