Range Controller Updates During Deallocation Can Cause Assertion Failure

[garrettmoon]

From @Adlai-Holler on March 31, 2017 22:19

• ASCollectionNode in full range mode
• Data source & delegate cleared (UICollectionView calls reloadData -> get empty data)
• (No layout pass happens, old cells are still visible)
• Removed from window
• Interface state goes invisible
• Range controller schedules to observe rendering notifications Update documentation of ASNetworkImageNodeDelegate #trivial #1163 <--- The problem
• Later…
• Rendering finishes
• Range controller updates
• Asks for visible elements. Gets the old index paths from last layout pass before controller dealloc.
• Data controller's element map is empty, due to reloadData
• Can't get the ASCollectionElement for the cell, fail assertion

Small Fix

Have the range controller only listen to rendering notifications when it's NOT in full mode. This seems solid, and I'll do it in a diff soon.

Medium Fix

Add a layoutIfNeeded call into [ASCollectionView visibleElementsForRangeController:] to ensure that the layout is up-to-date. I confirm that this fixes the issue in the limited case.

Large Fix

Stay in closer sync with UICollectionView by only actually committing reloadData when the layout pass happens. In some cases, we will need to manually trigger layout passes in order to get data preloading. @nguyenhuy toyed with this idea but had some issues – could you document them here?

Main Thread:

Dealloc Queue Thread:

@maicki @appleguy as well

Copied from original issue: facebookarchive/AsyncDisplayKit#3235

[garrettmoon]

From @nguyenhuy on April 3, 2017 14:8

@Adlai-Holler Great investigation. Regarding the fixes, I've just approved the "small" one (#3236). It should buy us some time to re-consider the "medium" one (#3237) and possibly skip it entirely in favor of the "large" and permanent fix. The medium fix doesn't address the underlying issue and introduces a dangerous example that might encourage us to call layoutIfNeeded whenever the layout object is out-of-sync.

I agree that the ultimate fix involves integrating more closely with UICollectionView and only applying visibleMap when the layout pass actually happens. That is, it's possible that for a short period of time the pendingMap, which is synced with our data source, doesn't equal to the current visibleMap, which UICollectionView, its collection layout and other objects are using, and which in turn doesn't equal to a pending visibleMap.

I did play around with manually triggering layout passes on ASCollectionNode to support preloading in nested collections. There are a couple of small issues with this approach: when the node enters preload state, I had to force a view allocation and then make sure the view has a proper bounds before calling layoutIfNeeded on it. This is because the view and its layout object are highly coupled, and the node hierarchy setup can be tricky (i.e closeup cell node).

[garrettmoon]

From @Adlai-Holler on April 3, 2017 17:7

Good call @nguyenhuy I think we could deploy that fix pretty easily, by "committing" the visible map change on [super layoutSubviews] in the case of reloadData – animated updates are committed immediately.