Re: NSImage / NSImageRep copy bug/problem

[Fred Kiefer]

Hi Ivan,

you code review is mostly correct. But you really should have read the
previous mail. The problem that Riccardo is trying to resolve is that on
Cocoa sometimes the original representation vanishes from the list of
representations of an NSImage. Telling him to just use the first
representation is of no help in that case.

One small nitpicking on your review. accessing the instance variables of
the copy in the copyWithZone: method isn't bad style. Quite often this
is needed. It really depends on the way the super class implements the
copy operation itself. If this gets done via a shallow copy, some
adjustment of the object instance variables is often needed.

Your diagnosis of the problem in -setBitmapRep: is in general correct,
though you could have pointed to the ASSIGN macro. I still have the
impression that this code is not what is causing the actual problem that
Riccardo is reporting. Looking at his stack trace, what seems to happen
is that a dealloc of a PRImage results in some object being released
twice. This happens at line 622 of NSBitmapImageRep.m and there we have
this line:

  RELEASE(_properties);

I would guess that when copying an NSBitmapImageRep we forget to copy
the _properties ivar. I will try to fix that in gui.

Fred



Am 30.11.2014 um 19:20 schrieb Ivan Vučica:
> 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.
> 
> On Sun, Nov 30, 2014 at 2:27 PM, Riccardo Mottola
> <riccardo.mottola@libero.it <mailto:riccardo.mottola@libero.it>> wrote:
> 
>     Hi,
> 
>     I have updated PRICE's (http://sourceforge.net/__projects/price/
>     <http://sourceforge.net/projects/price/>) PRImage to keep a
>     reference to my local image representation.
> 
>     Here you can see the source code:
>     
> http://price.cvs.sourceforge.__net/viewvc/price/PRICE-osx/__PRImage.m?view=markup
>     
> <http://price.cvs.sourceforge.net/viewvc/price/PRICE-osx/PRImage.m?view=markup>
> 
>     In initWithData I simply do:
>       self = [super initWithData:data];
>       bitRep = [[self representations] objectAtIndex:0];
>       [bitRep retain];
> 
>     My copyWithZone implementation is as following>
>     - (id)copyWithZone:(NSZone *)zone
>     {
>       PRImage *objCopy;
> 
>       objCopy = [super copyWithZone:zone];
>       while( [[objCopy representations] count] > 0 )
>         {
>           [objCopy removeRepresentation:[[objCopy representations]
>     objectAtIndex:0]];
>         }
>       objCopy->bitRep = nil;
>       [objCopy setBitmapRep:[[bitRep copy] autorelease]];
> 
>       return objCopy;
>     }
> 
>     I suppose it is a fine idea to invalidate all representations and
>     then re-adding a copy of the principal one.
> 
>     Copying is used during Undo, when the old image gets saved o during
>     Redo.
> 
>     This work fine on OS-X. I tried both 10.4 (ppc) and 10.6 (intel).
>     However on GNUstep I get a crash (see below).
>     The crash happens when the old image gets released.
> 
>     Undo has 1 level, I just have a copy of the old image. I release it
>     before putting the next one in.
> 
>     Why does it crash?
>     I have two guesses:
>     1) NSBitmapImageRep's copyWithZone doens't work correctly or as on Mac
>     2) NSImage copy is different/buggy? I do a superCopyWithZone
>     3) GS retains/releases its representations differently.
> 
>     It could also be a problem in PRICE's code, perhaps in the copy code
>     and that it works "by chance" on the mac
> 
>     Riccardo
> 
>     #0  0xb76872f3 in objc_msg_lookup ()
>        from /usr/lib/gcc/i686-pc-linux-__gnu/4.8.3/libobjc.so.4
>     #1  0xb7c33f7c in -[NSBitmapImageRep dealloc] (self=0x8231a88,
>         _cmd=0xb7af1dd0 <_OBJC_SELECTOR_TABLE+304>) at
>     NSBitmapImageRep.m:622
>     #2  0xb785382c in -[NSObject release] (self=0x8231a88,
>         _cmd=0xb7f28050 <_OBJC_SELECTOR_TABLE+912>) at NSObject.m:2102
>     #3  0xb7cb7738 in -[GSRepData dealloc] (self=0x822ec28,
>         _cmd=0xb7af1dd0 <_OBJC_SELECTOR_TABLE+304>) at NSImage.m:117
>     #4  0xb785382c in -[NSObject release] (self=0x822ec28,
>         _cmd=0xb7aa5ca0 <_OBJC_SELECTOR_TABLE+96>) at NSObject.m:2102
>     #5  0xb7728aa3 in -[GSArray dealloc] (self=0x81815a8,
>         _cmd=0xb7af1dd0 <_OBJC_SELECTOR_TABLE+304>) at GSArray.m:137
>     #6  0xb785382c in -[NSObject release] (self=0x81815a8,
>         _cmd=0xb7f28050 <_OBJC_SELECTOR_TABLE+912>) at NSObject.m:2102
>     #7  0xb7cb7966 in -[NSImage dealloc] (self=0x8311268,
>         _cmd=0x8099460 <_OBJC_SELECTOR_TABLE+960>) at NSImage.m:417
>     #8  0x0806678e in -[PRImage dealloc] (self=0x8311268,
>         _cmd=0xb7af1dd0 <_OBJC_SELECTOR_TABLE+304>) at PRImage.m:60
>     #9  0xb785382c in -[NSObject release] (self=0x8311268,
>         _cmd=0x8079d08 <_OBJC_SELECTOR_TABLE+1032>) at NSObject.m:2102
>     #10 0x0804ba43 in -[MyDocument saveCurrentImage] (self=0x839e330,
>         _cmd=0x8079db8 <_OBJC_SELECTOR_TABLE+1208>) at MyDocument.m:366
>     #11 0x0804cd9d in -[MyDocument runFilter:with:] (self=0x839e330,
>         _cmd=0x809d2f8 <_OBJC_SELECTOR_TABLE+856>, filter=0x84591a8,
>         parameters=0x0) at MyDocument.m:300