Several memory management issues

[rmaddy]

I'm using the latest version (5.6.1) and I have found a few memory management issues.

1. LiveConnectClientCore.m. In the refreshSessionWithDelegate:userState: method, the assignment to the authRefreshRequest ivar needs to be changed to self.authRefreshRequest or the autorelease needs to be removed.
2. LiveAuthStorage.m. In the initWithClientId: method, the assignment to _clientId needs a copy (or retain). This problem doesn't appear when passing in a string literal but it causes a crash otherwise.

[aclev]

Hi @rmaddy Thanks for the input! Can you submit a pull request with the changes listed?

[rmaddy]

Sorry but I don’t know git well enough to know how to create a pull request.

Rick

On May 6, 2015, at 11:15 AM, Ace Levenberg notifications@github.com wrote:

Hi @rmaddy https://github.com/rmaddy Thanks for the input! Can you submit a pull request with the changes listed?

— Reply to this email directly or view it on GitHub https://github.com/liveservices/LiveSDK-for-iOS/issues/61#issuecomment-99541693.

[aclev]

Hi @rmaddy Does the branch here solve your issues https://github.com/aclev/LiveSDK-for-iOS/tree/fixMemoryIssues ?

[rmaddy]

That seems OK though the dealloc method should access the ivar directly, not go through the property.

[aclev]

Agreed. In this case however we need to call the cancel method on the LiveAuthRefreshRequest before we dealloc, just incase the request is still running. If this is the case it could call its delegate methods on a dealloc delegate. If autolease is removed we will have a memory leak because of the double retain on the property. It is safe to call self at the beginning of dealloc as we have not yet dealloced any of the other properties.

[rmaddy]

You can still call the cancel method and set it to nil. Just call them directly on the ivar instead of the property.

Rick

On May 6, 2015, at 4:23 PM, Ace Levenberg notifications@github.com wrote:

Agreed. In this case however we need to call the cancel method on the LiveAuthRefreshRequest before we dealloc, just incase the request is still running. if this is the case it could call its delegate methods on a dealloc delegate. If autolease is removed we will have a memory leak because of the double retain on the property. It is safe to call self at the beginning of dealloc as we have not yet dealloc any of the other properties.

— Reply to this email directly or view it on GitHub https://github.com/liveservices/LiveSDK-for-iOS/issues/61#issuecomment-99629012.

[aclev]

You must call release on the ivar directly, otherwise it will result in memory leak. Setting the property to nil will drop the retain count and dealloc the request properly. it is possible that the refreshRequest has not been allocated yet (if we have not ever refreshed the token). This will result in a crash on a dealloc of the LiveConnectClientCore. Again, while I agree that the referencing self In the dealloc is not ideal, a crash is worse