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

Fix circular reference of NYTPhotosViewController #79

Merged
merged 1 commit into from
Oct 16, 2015

Conversation

nmotod
Copy link

@nmotod nmotod commented Oct 16, 2015

NYTPhotosViewController leaks memory by circular reference around interactive transition.

NYTPhotosViewController is not released when you attempt to dismiss interactively.
That occurs even when transition is canceled. Therefore it leaks by swipe-up/down the picture just once.
(-[NYTPhotosViewController dealloc] is never called.)

Cause

A concrete class of UIViewControllerContextTransitioning has got view controllers internally as strong reference.

Circular reference appears to be happening as follows:

  • NYTPhotosViewController has got NYTPhotoTransitionController
  • NYTPhotoTransitionController has got NYTPhotoDismissalInteractionController
  • NYTPhotoDismissalInteractionController has got UIViewControllerContextTransitioning
  • UIViewControllerContextTransitioning has got NYTPhotosViewController internally (Circular!)

Answer

Release NYTPhotoDismissalInteractionController.transitionContext when interactive transition's animation is finished.

@cdzombak
Copy link
Contributor

@meta2 good catch! and thank you for the detailed pull request.

cdzombak added a commit that referenced this pull request Oct 16, 2015
Fix circular reference of NYTPhotosViewController
@cdzombak cdzombak merged commit 6b2cc6f into nytimes:develop Oct 16, 2015
@nmotod
Copy link
Author

nmotod commented Oct 16, 2015

🍺
Thank you for good product!

@tirrorex
Copy link

Thank you for this :)

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

Successfully merging this pull request may close these issues.

None yet

4 participants