fix SA xcode 7 pointed out #118
Conversation
| @@ -152,6 +156,8 @@ -(instancetype)initWithReachabilityRef:(SCNetworkReachabilityRef)ref | |||
| self.reachableOnWWAN = YES; | |||
| self.reachabilityRef = ref; | |||
|
|
|||
| CFRetain(self.reachabilityRef); | |||
There was a problem hiding this comment.
matches CFRelease(self.reachabilityRef); in dealloc
There was a problem hiding this comment.
Except there's still a potential leak according to the analyzer, since SCNetworkReachabilityCreateWithName has already increased the retain count.
While I agree that this line is necessary to match the CFRelease call in dealloc, the ref should also be released in the factory methods to match the create calls.
There was a problem hiding this comment.
Just re-ran the SA on my branch. No SA errors visible. Running xCode 7.2
Line 98 & 111 each create a ref locally, therefore I would say and hopefully everyone agrees it should be released locally. (My fix)
Ignoring all other retain / release we have sufficiently balanced the life cycle, we have 1 retain count added and one retain count removed. We have a well balanced life cycle in local scope.
Now looking at the actual instance of Reachability, it also has a stake in the lifecycle of the ref. Because it has a release in dealloc on the ref, it should keep that balance and have a retain on the ref in the init. Which is what I did.
In summary, functionality wise nothing changes. Under current impl, theres not a leak, just the SA being grumpy. So all I did was remove a retain count in the class methods, so the SA would be happy, however since I did that I created a zombie so I had to rebalance and shove a retain down the common method (the init).
Again, this is purely a commit to make the SA go away. No memory leaks introduced, and no memory leaks fixed because there are none :)
There was a problem hiding this comment.
The analyzer is correct. Consider the case where the SCNetworkReachabilityRef is created, and the initWithReachabilityRef: call fails. The ref will leak.
There was a problem hiding this comment.
Right, so my implementation fixes that. Unless I'm missing something.
Line 98, SCNetworkReachabilityRef is created, retain count 1.
Line 101, init does something with it. Sucess or fail, doesn't matter. As far as we know, the retain count is still 1. So ...
Line 103, we release it. If other objects are claiming ownership its one them
There was a problem hiding this comment.
My mistake. I missed the last CFRetain call on line 159 because of the way the inline comments split up the diff.
|
Hi, |
|
Any chance of this getting merged in? Seems like the correct way to fix it without changing all the code to apple's new version of the base code. |
|
Still nothing ? |
|
I ended up forking it and adding the changes from @jcarroll3 - have this deployed to our live app. |
|
Would love to see this merged and part of an official release. Thanks |
quick fix to SA warnings found in xcode 7
#114