Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AnimatableImageView with no content causes crash when deallocated #53

Closed
flyinghyrax opened this issue Mar 31, 2016 · 3 comments
Closed

Comments

@flyinghyrax
Copy link
Contributor

If an AnimatableImageView is initialized but otherwise never used, the application will crash when the AnimatableImageView is deallocated.

I believe the source of the problem is here: https://github.com/kaishin/Gifu/blob/master/Source/AnimatableImageView.swift#L94

If the view was created but no other methods were called, then the lazy displayLink property will not have been initialized, so the CADisplayLink is created when it is first accessed inside the deinit block.

The CSDisplayLink instance will attempt to retain the AnimatableImageView as its target, then immediately release it since the next thing that happens is the call to invalidate().

This can be avoided by creating a Bool property that is set to true when the displayLink property is first accessed (it can be set from inside the closure that constructs the property). Then check the property in deinit and only invalidate the display link if it was already otherwise initialized.

The issue can be reproduced with the sample project here (see the master branch of that repo for a Readme file with more detail).

I will open a pull request for this change, but it will conflict with #52.

flyinghyrax added a commit to flyinghyrax/Gifu that referenced this issue Mar 31, 2016
flyinghyrax added a commit to flyinghyrax/Gifu that referenced this issue Mar 31, 2016
Don't invalidate the CADisplayLink on deinit unless the  lazy display
link instance was already initialized previously.

- Closes kaishin#53
@pbruz
Copy link

pbruz commented Apr 8, 2016

If fact there is more common problem with this deinit. IMO it has no chance to work properly. If you think about object life cycle, you will see that there is a releasing cycle and in fact it leads to crash. Even if you call startAnimatingGIF.
The problem is, as you mentioned, with deinit method.
screen shot 2016-04-08 at 15 16 17

screen shot 2016-04-08 at 15 20 04

Because CADisplayLink is always targeting `self`, after `deinit` is invoked, it causes calling `deinit` one more time. Never ending story...

@flyinghyrax
Copy link
Contributor Author

@pbruz this may help with that: #55

@pbruz
Copy link

pbruz commented Apr 8, 2016

@mr-seiler Yeah, you're right. Sorry, I didn't see this. I had a problem with that before #55 was created. As a solution I used not so pretty key-value coding. But it would be nice to fix this :)

flyinghyrax added a commit to flyinghyrax/Gifu that referenced this issue Apr 9, 2016
Don't invalidate the CADisplayLink on deinit unless the  lazy display
link instance was already initialized previously.

- Closes kaishin#53
flyinghyrax added a commit to flyinghyrax/Gifu that referenced this issue Apr 10, 2016
Don't invalidate the CADisplayLink on deinit unless the
lazy display link instance was already initialized previously.

- Closes kaishin#53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants