airbnb / MagazineLayout

A collection view layout capable of laying out views in vertically scrolling grids and lists.
Apache License 2.0
3.3k stars 219 forks source link

Bk/crash fix lazy layout attributes #107

Closed bryankeller closed 2 years ago

bryankeller commented 2 years ago

Details

This PR fixes an iOS 15+ layout loop crash.

I was finally able to reproduce the crash using a demo app that slams UICollectionView with random / shuffled item models, causing pretty crazy batch updates. While quickly scrolling around while the batch updates were occurring (about every 20ms), I was able to consistently encounter the layout loop crash.

NSInternalInconsistencyException: UICollectionView (<EpoxyCollectionView.CollectionView 0x110affa00>) is stuck in its update/layout loop. This can happen for many reasons, including self-sizing views whose preferred attributes are not returning a consistent size. To debug this issue, check the Console app for logs in the "UICollectionViewRecursion" category.

I noticed that leading up to the crash, shouldInvalidateLayout(forPreferredLayoutAttributes preferredAttributes:withOriginalAttributes originalAttributes:) and invalidationContext(forPreferredLayoutAttributes: withOriginalAttributes:) were being called with layout attributes for the same index path over and over, but alternating between the real cell height and the estimated height. This happened until the app crashed with the above NSInternalInconsistencyException. Logging put in preferredLayoutAttributesFitting(_:) confirmed that there was no ambiguous layout or sizing issue.

I finally discovered something very strange - the cached layout attributes dictionary has two layout attributes instances with the same index path. How could this happen? It turns out UICollectionView internally changes the index path of layout attributes in a method called -[UICollectionView _rebasePrefetchedCellIndexPathsWithMapping:]. If you have a layout attributes cache as a performance optimization, and the key for your cache is an index path, then your cache can be put in an invalid state. For example:

Valid layout attributes cache:

[
  {0, 0}: Layout attributes for {0, 0},
  {0, 1}: Layout attributes for {0, 1},
  {0, 2}: Layout attributes for {0, 2},
  {0, 3}: Layout attributes for {0, 3},
]

Invalid layout attributes cache after _rebasePrefetchedCellIndexPathsWithMapping changes the index path of one of your layout attributes instances to something else:

[
  {0, 0}: Layout attributes for {0, 1}, // Now there are 2 layout attributes instances with index path {0, 1}
  {0, 1}: Layout attributes for {0, 1},
  {0, 2}: Layout attributes for {0, 2},
  {0, 3}: Layout attributes for {0, 3},
]

UICollectionViewFlowLayout and UICollectionViewCompositionalLayout also cache layout attributes in a similar way, so I'm not entirely sure why this isn't a problem for those layouts.

To work around this, I've made a few changes:

  1. Create and cache layout attributes just-in-time, rather than in prepareLayout.
  2. Ignore the cached layout attributes if the index path cache key != the corresponding layout attributes' index path

Related Issue

N/A

Motivation and Context

Crash fix and performance improvement.

How Has This Been Tested

Demo app that consistently reproduced the aforementioned crash prior to my fix.

Types of changes

Checklist