Zombie when calling completion block in background thread

Question

I pass a completion block to my method, this completion block will be called in the background when a network request is finished. Unfortunately, if the calling object is deallocated in the meantime, the app crashes:

ViewController (which may be deallocated because it's popped from the navigation stack) code:

__unsafe_unretained ViewController *weakSelf = self;

[[URLRequester instance] sendUrl:url successBlock:^(id JSON) {
    [weakSelf webserviceCallReturned:JSON];
}];

URLRequester-Code (made simpler, of course):

- (void)sendUrl:(NSString *)urlAfterHost successBlock:(void (^)(id))successBlock {
    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
        sleep(2);
        successBlock(nil);
        return;
    });
}

If, in this 2 seconds, the ViewController gets popped from the navigation stack, the app crashes. What am I missing?

Answer 1

When you use __unsafe_unretained, then the reference remains around even after the object is deallocated. So if the view controller gets popped, then weakSelf is now pointing to a deallocated object.

If you change it to __weak instead, then when the view controller gets deallocated, it will set weakSelf to nil, and you'll be fine. You don't even need to do a check for if weakSelf is set to anything, because calling a method on nil has no effect.

Answer 2

It seems quite a few people think that 'self' inside a block must always be a weak (or unretained) copy. That's usually not the case**.

In this situation the misunderstanding is causing the crash, by leaving a zombie around. The right thing to do is to refer directly to self in the block (not unsafe_unretained, not weak), just like you'd like normal code to look. The effect will be that the block will retain the instance pointed to by 'self' -- the view controller in this case -- and it won't be destroyed until the block is destroyed (presumably by the url requester).

Will it be damaging to the view controller to process the result of the web request even though it has been popped? Almost certainly not, but if you think it will be, check for that condition in the block.

if (![self.navigationController.viewControllers containsObject:self])
    // I must have been popped, ignore the web request result

   // Re the discussion in comments, I think a good coder should have misgivings about
   // this condition.  If you think you need it, ask yourself "why did I design
   // my object so that it does something wrong based on whether some other object
   // (a navigation vc in this case) contains it?"

   // In that sense, the deliberate use of weakSelf is even worse, IMO, because
   // it lets the coder ignore _and_obscure_ an important question.
else {
    // do whatever i do when the web request completes
}

**The need for weak or unretained pointers in blocks stems from the fact that blocks will retain the objects they refer to. If one of those objects directly or indirectly retains the block, then you get a cycle (A retains B which retains A) and a leak. This can happen with any object to which the block refers, not just 'self'.

But in your case (as in many) the view controller referred to by self does not retain the block.

Answer 3

The good practice in using block (especially delayed ones) is to make a local copy of the block in the calling method. In your case it should be done in -(void)sendUrl:successBlock:

successBlockCopy = [successBlock copy];

and then call

successBlockCopy(nil);

It should retain your viewController for a while until completion.

Also it is better to use __weak instead __unsafe_unretained to avoid problems with suddenly released objects.