Without digging too deep in, and addressing other stuff than your direct issue, during code review these would be some of the red (or at the very least orange) flags for me in the code you pasted in email:
- I assume PRImage's bitRep field ivar is represented by the property bitmapRep; it is good practice to have the properties backed by either ivars of the same name, or prefixed by underscore
- You're having an instance of PRImage modify another instance of PRImage, which is fine, although preferably avoided -- and in this case avoidable. Should you really ->bitRep = nil? Doesn't [objCopy setBitmapRep:] do that already?
Looking at PRImage.h, I'd change:
@private NSBitmapImageRep *bitRep;
into
@private
NSBitmapImageRep *bitRep;
to make it clearer that @private addresses not just that one ivar, but all that follow (until @public or so appear).
Next, your implementation of -setBitmapRep: looks quite odd to me in the first place. Do you really need to store a copy of the bitmap representation in your subclass of NSImage? Does it make sense to /just/ add a new representation (which may confuse NSImage)? Why not implement -bitmapRep: as
return [[self representations] objectAtIndex:0];
and -setBitmapRep: as something that, if [self bitmapRep] != rep, clears away all representations and adds just one new one? That way you avoid a lot of unnecessary juggling in your own code -- including need to override copyWithZone:. Right now you have a very surprising behavior -- calling -setBitmapRep: actually doesn't release the old bitmap rep; it pretty much stays inside the representations array in NSImage.
If you do as above, suddenly it becomes feasible to implement this as nothing but a category over NSImage, right? Let's call this dynamic property -primaryRepresentation: and let's add all other methods into that category as well.
So I'd try fixing the issue by not having it at all.
And I may have spotted another bug just before sending the email, and it looks to me to be in -setBitmapRep:; at the very least I'd change these lines:
126 - (void)setBitmapRep:(NSBitmapImageRep *)rep
127 {
128 if (bitRep != rep)
129 {
130 [bitRep release];
131 bitRep = rep;
132 [bitRep retain];
133 [self addRepresentation:bitRep];
134 }
135 }
This should read:
[rep retain];
[bitRep release];
bitRep = rep;
In fact, even better would be to also move call to -addRepresentation: to the first place::
[self addRepresentation:rep];
[rep retain];
[bitRep release];
bitRep = rep;
Note that doing /just/ that would probably mask the refcounting bug (that is, it wouldn't manifest), as NSImage's code (which probably uses NSMutableArray) would call -retain on rep in the right place.
Why is your code incorrect? Let's imagine two PRImage objects stored at addresses 0xDEADBEEF and 0xBADC0DE. 0xDEADBEEF has refcount 1 and is the current value of the bitRep ivar. 0xBADC0DE has just been passed as the rep variable, also with refcount 1.
[0xDEADBEEF release];
// deadbeef refcount is now 0 so it gets deallocated
bitRep = 0xBADC0DE;
[0xBADC0DE retain];
// badcode refcount now 2
Or if rep == bitRep:
[0xDEADBEEF release];
// deadbeef released
bitRep = 0xDEADBEEF;
[0xDEADBEEF retain];
// crash!
'if bitRep != rep' should be enough to ensure this doesn't happen. For sake of code cleanliness, I'd still fix it as above.
Or (as stated) preferably I'd get rid of the PRImage subclass.