Fix memory leak which keeps strong reference to every target appearance methods are performed on#4
Open
carllindberg wants to merge 1 commit intoCollect3:masterfrom
Conversation
…e target) into an internal NSArray when retainArguments is turned on, and never removes them. This array is only freed up when the invocation itself is dealloced. Because CTAppearance by necessity has its stored invocations retain their arguments, every object the calls were invoked with got added to these arrays, and were never freed. They will not show up in Leaks because there are valid pointers to them. My fix is to only turn on retainArguments if any of the arguments are in fact objects, since we never care about retaining the targets (and I don't *think* we care about retaining the return values, so I didn't check for that). Most setter methods of scalar values will fall into this category, so it seems worth it to check. When invoking, if the NSInvocation does not retain arguments, we can call invokeWithTarget: directly. Otherwise, we have to make a new NSInvocation instance as a copy, and call invokeWithTarget on the copy instead, to avoid the target from being retained.
|
Truly marvellous. Mind if I adopt this this to my https://github.com/sanekgusev/SGVAppearanceProxy? |
Author
|
Sure, go right ahead. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NSInvocation appears to add all retained arguments (which includes the target) into an internal NSArray when retainArguments is turned on, and never removes them. This array is only freed up when the invocation itself is dealloced. Because CTAppearance by necessity has its stored invocations retain their arguments, every target the calls were invoked on got added to these arrays, and were never freed. They will not show up in Leaks because there are valid pointers to them.
My fix is to only turn on retainArguments if any of the arguments are in fact objects, since we never care about retaining the targets (and I don't think we care about retaining the return values, so I didn't check for that). Most setter methods of scalar values will fall into this category, so it seems worth it to check. When invoking, if the NSInvocation does not retain arguments, we can call invokeWithTarget: directly. Otherwise, we have to make a new NSInvocation instance as a copy, and call invokeWithTarget on the copy instead, to avoid the target from being retained, or if it is retained, then the copy will be dealloced right away and the strong reference will be released.