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 <
[email protected]> wrote:
> Hi,
>
> I have updated PRICE's (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
>
> 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
>
>
> _______________________________________________
> Discuss-gnustep mailing list
> [email protected]
> https://lists.gnu.org/mailman/listinfo/discuss-gnustep
>
--
Ivan Vučica
[email protected]
_______________________________________________
Discuss-gnustep mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/discuss-gnustep
