Weakify self's in dispatch_async's

[paulb777]

Fix #1317. Specifically address @baolocdo's comment at #1317 (comment)

[wilhuff]

The reason why we get all those implicit retain warnings is that self is implicitly strongly retained in blocks like this.

What weakifying does it make it possible for the block to outlive the object that submitted the object into the queue. That doesn’t seem like a goal here so this seems like the wrong change.

[paulb777]

Thanks @wilhuff - it seems the backtraces at #1317 (comment) are because nothing is pointing at the self's - not that the self has been deallocated.

[paulb777]

@wilhuff I was thinking more about this and isn't the reason that these accesses are zombies would be because of the extra retain in the dispatch_async block? If that retain is removed, then the objects can be reclaimed when their references are released.

[ryanwilson]

Are there any problems with [weakSelf->_objects removeObjectForKey:key] here instead?

[baolocdo]

Thanks

[wilhuff]

@paulb777 Zombie objects are those whose retain counts have gone to zero but are still getting messages, usually through an unsafe_unretained pointer. The retains of self caused by dispatch_async block would prevent zombies.

I have some detailed comments on the implementation you're proposing, but at a higher level I don't think this addresses the root cause which is that somehow methods are being invoked on objects that don't otherwise already have strong references.

[wilhuff]

This is still racy. The pattern to use is

typeof(self) strongSelf = weakSelf;
if (strongSelf) {
  // Do interesting things with strongSelf.
}

Or

typeof(self) strongSelf = weakSelf;
if (!strongSelf) {
  return;
}

In other words: the if should be testing the strong reference recovered from the weak pointer.

As you've written it it's possible for weakSelf to be allocated on the first line but deallocated on the second (before the assignment).

[wilhuff]

This doesn't do what you want it to do. Between this point and the point where you use it next the object pointed to by weakSelf can still be deallocated. That effectively makes this check worthless.

Since the removal already does its work in another queue, there's little reason to bounce it onto the user callback queue. A better arrangement in this callback block would be:

FIRNetwork *__strong strongSelf = weakSelf;
if (!strongSelf) {
  return;
}
if (sessionID.length) {
  [strongSelf->_requests removeObjectForKey:sessionID];
}

if (handler) {
  dispatch_queue_t queueToDispatch = queue ? queue : dispatch_get_main_queue();
  dispatch_async(queueToDispatch, ^{
    handler(response, data, error);
  });
}

[paulb777]

@wilhuff Thanks for the high and low level feedback. The implementation issues are addressed. I'll dig deeper on the high level questions this afternoon.

[morganchen12]

IIRC a retain call in a block could still cause a zombies crash if the object being retained had been deallocated prior to block capture, but I don't think the weak pointer would help in this case.

[paulb777]

@morganchen12 and I reviewed the zombie report and are convinced that this PR does not solve a real problem - agreeing with @wilhuff. It is not possible for self to be deallocated before these blocks.