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 severe memory leak when using async resizable cells #103

Merged
merged 1 commit into from
Mar 9, 2017

Conversation

klundberg
Copy link
Contributor

@klundberg klundberg commented Mar 4, 2017

Previously, the sizeChangedHandler callback set on resizable cells in BrickCollectionView was accidentally capturing a collection cell reference from outside the closure and causing a retain cycle, even though the cell was passed into the closure as an argument. This change resolves that issue so that the argument is referenced instead of any captured variables.

This also includes a test to ensure that the collection cell is deallocated properly if the brick collection view that owns it is also deallocated. I needed to make a new test async cell since the existing one uses an NSTimer which interferes with the deinitialization in ways I don't fully understand.

I recommend releasing a new version of BrickKit as soon as this is merged since this can leak a lot of memory, especially if the async brick is a collection brick containing child collection views/cells.

@klundberg klundberg force-pushed the fix-memory-leak branch 2 times, most recently from 591b7d4 to 8ac36cb Compare March 4, 2017 05:43
@codecov-io
Copy link

codecov-io commented Mar 4, 2017

Codecov Report

Merging #103 into master will decrease coverage by -0.07%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
- Coverage   93.45%   93.39%   -0.07%     
==========================================
  Files          38       38              
  Lines        3026     3027       +1     
==========================================
- Hits         2828     2827       -1     
- Misses        198      200       +2
Impacted Files Coverage Δ
Source/ViewControllers/BrickCollectionView.swift 95.01% <71.42%> (-0.61%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d09c789...24bb092. Read the comment docs.

@klundberg klundberg force-pushed the fix-memory-leak branch 2 times, most recently from 1b7fab5 to e1332f4 Compare March 4, 2017 05:44
@klundberg
Copy link
Contributor Author

No idea why yet, but the only reason this seems to be failing on that one job is that it's running on an iPhone 5 simulator. The iPhone 5s sim on iOS 9.0 passes the test just fine. Going to look into it a bit more.

@klundberg
Copy link
Contributor Author

Aha! The trouble is that on an iPhone 5 simulator, the unit test appears to hold onto a reference of the BrickCollectionView in another autorelease pool, while iPhone 5s and above does not. I'm not sure why this is, but my best guess is that there's a subtle memory management difference between 32 and 64bit UIKit that apple is emulating in the simulator. Fix incoming which should pass all tests on all environments in travis.

Previously, the sizeChangedHandler callback set on resizable cells in BrickCollectionView was accidentally capturing a collection cell reference from outside the closure and causing a retain cycle, even though the cell was passed into the closure as an argument. This change resolves that issue so that the argument is referenced instead of any captured variables.

This also includes a test to ensure that the collection cell is deallocated properly if the brick collection view that owns it is also deallocated. I needed to make a new test async cell since the existing one uses an NSTimer which interferes with the deinitialization in ways I don't fully understand.

I recommend releasing a new version of BrickKit as soon as this is merged since this can leak a lot of memory, especially if the async brick is a collection brick containing child collection views/cells.
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.

3 participants