0

I have the following action for a button, which toggles whether an object is shown as favorite or non-favorite:

- (IBAction)addToFavorites:(UIButton *)sender {
  if ([object isFavorite]) {
    [_apiManager removeFromFavorite:[object ID] withCompletion:^ {
      [_favoriteButton setImage:[UIImage imageNamed:@"favorite"] forState:UIControlStateNormal];
    }];
  }

  else {
    [_apiManager addToFavorite:[object ID] withCompletion:^ {
      [_favoriteButton setImage:[UIImage imageNamed:@"favorite_yellow"] forState:UIControlStateNormal];
    }];
  }
}

Both completion blocks are identical, with exception to the image name. XCode is giving to the else case the warning: Capturing 'self' strongly in this block is likely to lead to a retain cycle and pointing at _favoriteButton. However, the same does not happen in the case when the if condition is true.

I imagine that either both or none of the cases should present the warning, and I don't understand why only the later shows it. Is this an Xcode bug? Are both causing retain cycles?

rmaddy
  • 314,917
  • 42
  • 532
  • 579
Guilherme
  • 7,839
  • 9
  • 56
  • 99
  • If you comment out the `else` branch, does it not give the warning in the remaining branch? If that is the case, you've probably just seen the compiler reduce identical warnings into one. – Monolo Oct 09 '13 at 13:50
  • i think it is a xcode bug, try to uncomment the second block and run analyze with only the first one. And yes this could cause retain cycles in both cases. – Jonathan Cichon Oct 09 '13 at 13:52
  • 1
    possible duplicate of [Clang - Blocks retain cycle from naming convention?](http://stackoverflow.com/questions/15535899/clang-blocks-retain-cycle-from-naming-convention) - The ARC compiler uses indeed naming conventions to decide wether capturing self is "good" or "bad". – Martin R Oct 09 '13 at 13:52
  • XCode warnings are sometimes buggy. Have you tried to clean the project, and build again ? – Vinzzz Oct 09 '13 at 13:53
  • Monolo: commenting the else branch does not make a warning appear on the if branch; JonathanCichon: I will run Analyze and test it; MartinR: So this issue in particular is cause by my methods' names? Where can I find info on naming conventions? Vinzzz: yes, I cleaned, nothing changed. – Guilherme Oct 09 '13 at 14:05
  • 1
    @Guilherme: The ARC compiler uses naming conventions to decide whether to warn or not. This seems to be an undocumented heuristic, and I found this only by inspecting the compiler source code, see my answer to the "possible duplicate". And this might change with a new release. - If you *actually have* a "bad" retain cycle does not depend on the method names. It depends on whether the retain cycle is "temporary" (i.e. the block is called and released eventually) or "permanent". – Martin R Oct 09 '13 at 14:10

2 Answers2

3

_favoriteButton is an ivar. It is owned by a specific instance of your class, so using it captures the current self in the block (Reference to instance variables inside a block)

Instead, you should create a weak reference to self, and use property accessors, like this :

- (IBAction)addToFavorites:(UIButton *)sender {
  __weak YourViewControllerClass *weakSelf = self;
  if ([object isFavorite]) {
    [_apiManager removeFromFavorite:[object ID] withCompletion:^ {
      [weakSelf.favoriteButton setImage:[UIImage imageNamed:@"favorite"] forState:UIControlStateNormal];
    }];
  }

  else {
    [_apiManager addToFavorite:[object ID] withCompletion:^ {
      [weakSelf.favoriteButton setImage:[UIImage imageNamed:@"favorite_yellow"] forState:UIControlStateNormal];
    }];
  }
}
Community
  • 1
  • 1
Vinzzz
  • 11,746
  • 5
  • 36
  • 42
  • 1
    In this case this is most probably the right solution. - But generally capturing self can be desired if the completion block is guaranteed to be called eventually, and if the captured object should be kept alive until then. The ARC compiler uses naming conventions to decide whether to warn or not, see above mentioned "possible duplicate". Method names like `-set` or `-add` do *not* cause a warning, all others do. – Martin R Oct 09 '13 at 14:01
  • 1
    I had noticed this behavior, but didn't know of the naming conventions used by ARC, thank you for this precious info ! – Vinzzz Oct 09 '13 at 14:04
  • Yes @Vinzzz, I was aware of how to solve the cycle retaining issue. My question was really about why only the second block was presenting the warning. Thank you. – Guilherme Oct 09 '13 at 14:07
  • You don't have to use property accessors. You can access the instance variable directly. Though you would have to check that the object is not `nil` first. – newacct Oct 09 '13 at 22:37
  • @newacct you do *have* to use property accessors. If you check the question that this was made a duplicate of, you'll see uisng a weak reference to self is necessary. If you access an ivar directly the block will retain self to give itself a strong pointer to the ivar. It doesn't retain the ivar, it retains the object the ivar is on. – Abhi Beckert Dec 16 '14 at 20:05
  • @AbhiBeckert: No, you DO NOT have to use property accessors. Using a weak or strong reference and whether you use instance variables or properties are independent and orthogonal issues. – newacct Dec 16 '14 at 22:24
  • @newacct what I meant was "in this code you will get a retain cycle unless you use accessors (or -> I suppose)". Since a retain cycle is undesired, you must use accessors in this situation. Accessing an ivar directly causes ARC to retain `self`. That's exactly what the question was about, how to avoid a retain cycle. He got a retain cycle because he didn't use a weak reference to self. – Abhi Beckert Dec 17 '14 at 06:31
  • @AbhiBeckert: There is no difference with regard to retaining between using a property and an ivar. If you use property accessor on a pointer other than `self`, you equally use ivars on pointers other than `self`. – newacct Dec 17 '14 at 06:36
0

Actually, in this example isn't clear if there is a retain cycle. The retain cycle would happen if the completion block is declared as property of _apiManager. If it's just a block on the scope of a method (just a method parameter) then there is no retain cycle, however XCode is not smart enough to detect these cases and warnings you of a possible retain cycle.

Regarding to your question, it is just about the order, it warnings you on the first retain cycle, the second one doesn't matter since the block is already retaining self from the first block. If you fix the first warning by using __weak self, it will warning on the second block.

Pablo A.
  • 2,042
  • 1
  • 17
  • 27