How is this an "incorrect decrement of a reference count"?

  • I have this in my header file:

    *

    **

    @property (nonatomic, retain) UIImagePickerController* imagePicker;
    **

    *
    The analyzer is complaining about lines like this (but not always):

    *

    self.imagePicker = [[UIImagePickerController alloc] init];

    [self.imagePicker release];

    *
    I do this in many places, but in a few of them the analyzer says:

    *Property returns an Objective-C object with a +0 retain count*
    *Incorrect decrement of the reference count of an object that is not owned
    at this point by the caller*

    Is this just a bug in the analyzer?  I can find no difference between the
    times it makes this complaint and the many more times that it doesn't.

    Thanks for any insight.
  • No, the analyzer is right and your code is somewhat confused. I'm trying to figure out how you ended up with code like that, did you find you had an extra retain somewhere and need to get rid of it? Again you are misunderstanding ownership.

    @property( nonatomic, retain ) UIImagePickerController *imagePicker;

    What does that mean? That means that you have a property which is a UIImagePickerController and when it's *set* with either [ object setImagePickerController:ipc ] or self.imagePickerController=ipc, the syntactic sugar for the method call, ipc is retained. The 'retain' is for the setter,  not the getter.

    That's all it tells you, ie somewhere you either have

    @synthesize imagePickerController=_imagePIckerController;

    or

    -(void)setImagePickerController:(UIImagePickerController*)imagePickerController
    {
    [ imagePickerController retain ];
    [ _imagePickerController release ];
    _imagePickerContoller=imagePickerController;
    }

    -(UIImagePickerController*)imagePickerController
    {
    return _imagePickerController;
    }

    and

    -(void)dealloc
    {
    [ _imagePickerController release ];
    }

    Look at the getter, that doesn't retain imagePickerController, so the getter returns an object *the caller does not own*.

    So looking at the second piece of your code, it's basically equivalent to this (I will use the setXXX: method instead of the dot syntax, it's clearer, they are equivalent).

    UIImagePickerController     *temp = [ self imagePickerController ];
    [ temp release ];

    but the analyser knows that [ self imagePickerController ] returns an object which the caller doesn't own, so you should not release it. self.imagePickerController is *NOT* the same as the ivar, it's a method which returns an object you don't own (the method name doesn't have new, copy or init in it).

    So what should you have done. Let's break the first piece of code down too into an equivalent

    UIImagePickerController *temp = [ [ UIImagePickerController alloc ] init ];
    [ self setImagePickerController temp ];

    temp is an object you do own, so you need to release. Ignore the fact that you pass it to a method (setImagePickerController:) which retains it, you don't care, you don't have to care, not your problem, if that method wants to retain it, up to that method. What you then need to balance this out is

    [ temp release ];

    Or if you want to do the assignment in one line, you have this version

    [ self setImagePickerController:[ [ UIImagePickerController alloc ] init ];

    ok the problem there is that [ [ UIImagePickerController alloc ] init ] returns an object you own but you haven't assigned it to anything you can release after the property setter call. This is where autorelease would help you, it will release the object but only sometime after the property setter

    [ self setImagePickerController:[ [ [ UIImagePickerController ] alloc ] init ] autorelease ];

    [ self.property release ] or [ [ self property ] release ] is plain wrong, even if it happens to be working because you happen to be returning your underlying ivar which happens to have an 'extra' retain on it. The analyser is right, don't do that. I'd expect it to complain about all such calls, you really have the same construct in them all, [ self.someproperty release ]?

    So replace this

    self.property =  [ [ AnObject alloc ] init ];
    [ self.property release ];

    with either

    AnObject *temp = [ [ AnObject alloc ] init ];
    self.property = temp;
    [ temp release ];

    or

    self.property = [ [ [ AnObject alloc ] init ] autorelease ];

    On Mar 19, 2012, at 8:18 PM, G S wrote:

    > I have this in my header file:
    >
    > *
    >
    > **
    >
    > @property (nonatomic, retain) UIImagePickerController* imagePicker;
    > **
    >
    > *
    > The analyzer is complaining about lines like this (but not always):
    >
    > *
    >
    > self.imagePicker = [[UIImagePickerController alloc] init];
    >
    > [self.imagePicker release];
    >
    > *
    > I do this in many places, but in a few of them the analyzer says:
    >
    > *Property returns an Objective-C object with a +0 retain count*
    > *Incorrect decrement of the reference count of an object that is not owned
    > at this point by the caller*
    >
    > Is this just a bug in the analyzer?  I can find no difference between the
    > times it makes this complaint and the many more times that it doesn't.
    >
    > Thanks for any insight.
  • First, what Roland said. In addition to that, keep in mind that when you code

    self.imagePicker = aPicker;
    ...
    anotherPicker = self.imagePicker;

    it is best to assume that aPicker and anotherPicker are different objects (even if ... is empty, there may be other threads, or the setPicker code may do something unexpected with its input). Roland's temp variable is safe, using temp, you know you will be releasing the object you allocated, the one you own. With the code above, you lose the one you own and then release its doppelgänger instead.

    Aaron

    On Mar 19, 2012, at 6:18 AM, Roland King <rols...> wrote:

    > No, the analyzer is right and your code is somewhat confused. I'm trying to figure out how you ended up with code like that, did you find you had an extra retain somewhere and need to get rid of it? Again you are misunderstanding ownership.
    >
    > @property( nonatomic, retain ) UIImagePickerController *imagePicker;
    >
    > What does that mean? That means that you have a property which is a UIImagePickerController and when it's *set* with either [ object setImagePickerController:ipc ] or self.imagePickerController=ipc, the syntactic sugar for the method call, ipc is retained. The 'retain' is for the setter,  not the getter.
    >
    > That's all it tells you, ie somewhere you either have
    >
    > @synthesize imagePickerController=_imagePIckerController;
    >
    > or
    >
    > -(void)setImagePickerController:(UIImagePickerController*)imagePickerController
    > {
    > [ imagePickerController retain ];
    > [ _imagePickerController release ];
    > _imagePickerContoller=imagePickerController;
    > }
    >
    > -(UIImagePickerController*)imagePickerController
    > {
    > return _imagePickerController;
    > }
    >
    > and
    >
    > -(void)dealloc
    > {
    > [ _imagePickerController release ];
    > }
    >
    >
    > Look at the getter, that doesn't retain imagePickerController, so the getter returns an object *the caller does not own*.
    >
    >
    > So looking at the second piece of your code, it's basically equivalent to this (I will use the setXXX: method instead of the dot syntax, it's clearer, they are equivalent).
    >
    > UIImagePickerController    *temp = [ self imagePickerController ];
    > [ temp release ];
    >
    > but the analyser knows that [ self imagePickerController ] returns an object which the caller doesn't own, so you should not release it. self.imagePickerController is *NOT* the same as the ivar, it's a method which returns an object you don't own (the method name doesn't have new, copy or init in it).
    >
    > So what should you have done. Let's break the first piece of code down too into an equivalent
    >
    > UIImagePickerController *temp = [ [ UIImagePickerController alloc ] init ];
    > [ self setImagePickerController temp ];
    >
    > temp is an object you do own, so you need to release. Ignore the fact that you pass it to a method (setImagePickerController:) which retains it, you don't care, you don't have to care, not your problem, if that method wants to retain it, up to that method. What you then need to balance this out is
    >
    > [ temp release ];
    >
    > Or if you want to do the assignment in one line, you have this version
    >
    > [ self setImagePickerController:[ [ UIImagePickerController alloc ] init ];
    >
    > ok the problem there is that [ [ UIImagePickerController alloc ] init ] returns an object you own but you haven't assigned it to anything you can release after the property setter call. This is where autorelease would help you, it will release the object but only sometime after the property setter
    >
    > [ self setImagePickerController:[ [ [ UIImagePickerController ] alloc ] init ] autorelease ];
    >
    >
    > [ self.property release ] or [ [ self property ] release ] is plain wrong, even if it happens to be working because you happen to be returning your underlying ivar which happens to have an 'extra' retain on it. The analyser is right, don't do that. I'd expect it to complain about all such calls, you really have the same construct in them all, [ self.someproperty release ]?
    >
    > So replace this
    >
    > self.property =  [ [ AnObject alloc ] init ];
    > [ self.property release ];
    >
    > with either
    >
    > AnObject *temp = [ [ AnObject alloc ] init ];
    > self.property = temp;
    > [ temp release ];
    >
    > or
    >
    > self.property = [ [ [ AnObject alloc ] init ] autorelease ];
    >
    >
    >
    > On Mar 19, 2012, at 8:18 PM, G S wrote:
    >
    >> I have this in my header file:
    >>
    >> *
    >>
    >> **
    >>
    >> @property (nonatomic, retain) UIImagePickerController* imagePicker;
    >> **
    >>
    >> *
    >> The analyzer is complaining about lines like this (but not always):
    >>
    >> *
    >>
    >> self.imagePicker = [[UIImagePickerController alloc] init];
    >>
    >> [self.imagePicker release];
    >>
    >> *
    >> I do this in many places, but in a few of them the analyzer says:
    >>
    >> *Property returns an Objective-C object with a +0 retain count*
    >> *Incorrect decrement of the reference count of an object that is not owned
    >> at this point by the caller*
    >>
    >> Is this just a bug in the analyzer?  I can find no difference between the
    >> times it makes this complaint and the many more times that it doesn't.
    >>
    >> Thanks for any insight.

    >
  • On Mon, Mar 19, 2012 at 6:18 AM, Roland King <rols...> wrote:

    > No, the analyzer is right and your code is somewhat confused. I'm trying
    > to figure out how you ended up with code like that, did you find you had an
    > extra retain somewhere and need to get rid of it?

    Yes.  The allocated object has a retain count of 1, and the property is set
    to retain.  So there's the extra retention.

    That's all it tells you, ie somewhere you either have
    >
    > @synthesize imagePickerController=_imagePIckerController;
    >

    Exactly. Without the release, the analyzer complains about leaks, and does
    so on ALL cases.
  • On Mar 19, 2012, at 5:18 AM, G S wrote:

    > self.imagePicker = [[UIImagePickerController alloc] init];
    >
    > [self.imagePicker release];
    >
    > I do this in many places, but in a few of them the analyzer says:
    >
    > *Property returns an Objective-C object with a +0 retain count*
    > *Incorrect decrement of the reference count of an object that is not owned
    > at this point by the caller*
    >
    > Is this just a bug in the analyzer?  I can find no difference between the
    > times it makes this complaint and the many more times that it doesn't.

    This is always an anti-pattern, even if the analyzer doesn't always point it out (and I'm not certain why it wouldn't in some places…).

    The reason why its an anti-pattern is because an accessor is not like an ivar. [[self imagePicker] release] means something very very different from [_imagePicker release]. In the former case you are releasing the result of something you do not own (the result of an accessor like -imagePicker is an unowned object, even if it is from an accessor in your own class) and as such releasing it is invalid.

    Now you might think you could do something like this instead:
    self.foo = [[NSFoo alloc] init];
    [_foo release];

    But thats also an anti-pattern, but for a more subtle reason, and that is because (especially if your synthesizing it) your code shouldn't rely on the implementation details of -setFoo: beyond what the property declaration proscribes. For example, if the new incoming value is the same as the old value, you can't be guaranteed that the retain count has actually changed, thus the -release would be invalid.

    But even more so, this gets problematic if you change your property to be a 'copy' property – either of the previous versions will fail completely with a -copy property where the copy actually takes place, as the new object in _foo will only have a single owner – your instance – and you will then release it behind the back of the accessor. Your next access of that accessor will then cause your application to reference an invalid object.

    Basically your safe options are to either:
    1) Use a temporary that you release:
    id foo = [[NSFoo alloc] init];
    self.foo = foo;
    [foo release];

    2) Use -autorelease:
    self.foo = [[[NSFoo alloc] init] autorelease];

    3) Use ARC which will manage this for you.

    I would highly recommend #3 overall.
    --
    David Duncan
  • And do you now understand why you are releasing the wrong thing? if not please go read the replies from Eeyore and I again, you cannot release the result of a method call which returns an object you don't own. Your code as written is a hack, it happens to work because the property is a simple retained one and you get bsck the object you didn't keep when you created it, but it's wrong and it's dangerous (change the property to 'copy' and then see how bad it gets). Then either use a temporary or use autorelease as I suggested (and I see David Duncan has too) so that you are releasing the object you *do* own and both the leak and the analyser error (and your bad code) will go away.

    On Mar 20, 2012, at 6:22 AM, G S wrote:

    > On Mon, Mar 19, 2012 at 6:18 AM, Roland King <rols...> wrote:
    > No, the analyzer is right and your code is somewhat confused. I'm trying to figure out how you ended up with code like that, did you find you had an extra retain somewhere and need to get rid of it?
    >
    > Yes.  The allocated object has a retain count of 1, and the property is set to retain.  So there's the extra retention.
    >
    > That's all it tells you, ie somewhere you either have
    >
    > @synthesize imagePickerController=_imagePIckerController;
    >
    > Exactly. Without the release, the analyzer complains about leaks, and does so on ALL cases.
  • On Mon, Mar 19, 2012 at 3:47 PM, Roland King <rols...> wrote:

    > And do you now understand why you are releasing the wrong thing?
    >

    Well, I understand why my code just happens to be releasing the right
    thing, but easily couldn't.  Obviously the getter might be coded to return
    anything, including a wholly different object.

    Thanks for all the feedback.  I've changed all the offending code to use
    temporary pointers for the assignment and release.  The analyzer's happy.

    I will convert to ARC when I have time, but I'm using a couple of
    open-source libraries that I don't want to convert right now.
  • On Mar 19, 2012, at 5:58 PM, G S wrote:

    > I will convert to ARC when I have time, but I'm using a couple of
    > open-source libraries that I don't want to convert right now.

    You can call non-ARC code from ARC, though. Libraries, even other source files within your own project. That’s the beauty of it.

    Charles
  • Yep, thanks.  Someone pointed that out.

    I went through my whole project and audited every file for memory
    management, reinstating properties for everything.  After forgetting not to
    use properties in the init and dealloc methods and having to correct that,
    the app is running great and there's not a leak to be found.

    That reminds me of another question, though: Someone earlier mentioned the
    need to release elements loaded from the nib, in dealloc.  I didn't think
    this was necessary, so I don't do it.  And there are no apparent leaks as a
    result.  What's the story on that?
  • On Mar 19, 2012, at 3:58 PM, G S <stokestack...> wrote:
    > I will convert to ARC when I have time, but I'm using a couple of open-source libraries that I don't want to convert right now.

    ARC supports that. As long as everybody follows Cocoa's naming conventions, you can compile some files with ARC and other files without ARC. So you can migrate your code to ARC (or even just some of your code), and leave the open-source libraries unchanged.

    --
    Greg Parker    <gparker...>    Runtime Wrangler
  • On Mar 19, 2012, at 4:27 PM, G S <stokestack...> wrote:
    > That reminds me of another question, though: Someone earlier mentioned the
    > need to release elements loaded from the nib, in dealloc.  I didn't think
    > this was necessary, so I don't do it.  And there are no apparent leaks as a
    > result.  What's the story on that?

    Complicated.

    https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Load
    ingResources/CocoaNibs/CocoaNibs.html#//apple_ref/doc/uid/10000051i-CH4-SW6


    --
    Greg Parker    <gparker...>    Runtime Wrangler
  • On Mar 19, 2012, at 5:27 PM, G S wrote:

    > That reminds me of another question, though: Someone earlier mentioned the
    > need to release elements loaded from the nib, in dealloc.  I didn't think
    > this was necessary, so I don't do it.  And there are no apparent leaks as a
    > result.  What's the story on that?

    Are you using NSViewController or NSWindowController? If so, then that's not necessary, since those two classes clean up after themselves. It is also not necessary in the principal nib, since I don't think that ever gets deallocated. It is only necessary if you used NSBundle to load the nib manually.

    Nick Zitzmann
    <http://www.chronosnet.com/>
  • >
    > Are you using NSViewController or NSWindowController?

    UIViewController and derivatives.
  • On Mar 19, 2012, at 6:27 PM, G S wrote:

    > That reminds me of another question, though: Someone earlier mentioned the need to release elements loaded from the nib, in dealloc.  I didn't think this was necessary, so I don't do it.  And there are no apparent leaks as a result.  What's the story on that?

    Memory management in nibs on OS X is perplexingly designed. The best thing to do with these is to use NSWindowController and NSViewController for loading nibs, and not to load them directly with NSBundle or NSNib.

    Charles
  • On Mar 19, 2012, at 4:27 PM, G S wrote:

    > That reminds me of another question, though: Someone earlier mentioned the
    > need to release elements loaded from the nib, in dealloc.  I didn't think
    > this was necessary, so I don't do it.  And there are no apparent leaks as a
    > result.  What's the story on that?

    I may have been the one to suggest this. When I made that statement, I was thinking of pre-ARC iOS programming. See Greg's link for the correct answr for your case.

    Aaron
  • On Mar 20, 2012, at 7:27 AM, G S wrote:

    > Yep, thanks.  Someone pointed that out.
    >
    > I went through my whole project and audited every file for memory management, reinstating properties for everything.  After forgetting not to use properties in the init and dealloc methods and having to correct that, the app is running great and there's not a leak to be found.
    >
    > That reminds me of another question, though: Someone earlier mentioned the need to release elements loaded from the nib, in dealloc.  I didn't think this was necessary, so I don't do it.  And there are no apparent leaks as a result.  What's the story on that?

    Elements loaded from NIBs and just displayed in the view hierarchy don't need to be released (only their superview retains them so they are released as the view is destroyed). If you store them also into properties (IBOutlets) then those properties need to be released in dealloc just like other properties, which you probably do anyway. For UIViewControllers you should also implement viewDidUnload and set those IBOutlet properties to nil else those objects you don't need are retained until the next time the view is loaded which rather ruins the point of the low-memory code.

    If you are using ARC then you can just make the properties weak and whenever the view goes away, the IBOutlet's go away too, very useful.

    I think most of this is in the NIB loading guide and the template code for UIViewController subclases (and the code auto-generated when you add outlets in Xcode using the mouse) does this.
  • >
    > I think most of this is in the NIB loading guide and the template code for
    > UIViewController subclases (and the code auto-generated when you add
    > outlets in Xcode using the mouse) does this.
    >
    >
    Thanks.  I'm not using ARC and won't be for this release of my app.

    The posted link was for the Mac OS version of the nib guide, not iOS.  I'm
    using UIViewController derivatives in iOS.  I'm not using NSNib directly;
    I'm just letting the OS do its thing loading the views I've set up in IB.

    I *am* setting the IBOutlets to nil in viewDidUnload.

    I *am not* releasing the IBOutlets in dealloc, and have yet to see a leak
    in Leaks as a result.
  • On Mar 20, 2012, at 10:48 AM, G S wrote:

    > I *am not* releasing the IBOutlets in dealloc, and have yet to see a leak in Leaks as a result.

    For a myriad of arcane reasons it is fairly difficult to see UIView (or CALayers) in Leaks. That doesn't mean they will eventually be deallocated. I would look into using Heap Shots to see what you are losing by not releasing your outlets in -dealloc.
    --
    David Duncan
  • I think the link was to the "Resource Programming Guide" which covers both Mac OS X and iOS. You will want to look at page 17 in the PDF "Legacy Patterns." If you follow David's advice and want to try Heap Shot analysis, there's a good introduction here:

    http://www.friday.com/bbum/2010/10/17/when-is-a-leak-not-a-leak-using-heaps
    hot-analysis-to-find-undesirable-memory-growth/


    Aaron

    On Mar 20, 2012, at 10:55 AM, David Duncan wrote:

    > On Mar 20, 2012, at 10:48 AM, G S wrote:
    >
    >> I *am not* releasing the IBOutlets in dealloc, and have yet to see a leak in Leaks as a result.
    >
    >
    > For a myriad of arcane reasons it is fairly difficult to see UIView (or CALayers) in Leaks. That doesn't mean they will eventually be deallocated. I would look into using Heap Shots to see what you are losing by not releasing your outlets in -dealloc.
    > --
    > David Duncan
    >
  • On Mar 20, 2012, at 12:19 PM, Eeyore wrote:

    > I think the link was to the "Resource Programming Guide" which covers both Mac OS X and iOS. You will want to look at page 17 in the PDF "Legacy Patterns."

    Sorry, I meant:

    You will want to look at the section "Legacy Patterns" on page 17 of the PDF version of the Guide.

    There isn't a "Legacy Pattern" PDF.

    Aaron
  • OK, thanks for the info and reference material.
previous month march 2012 next month
MTWTFSS
      1 2 3 4
5 6 7 8 9 10 11
12 13 14 15 16 17 18
19 20 21 22 23 24 25
26 27 28 29 30 31  
Go to today