Weak reference to ASCollectionElements become nil too soon when deleting ASCollectionNode items. Sometimes.

[jeffdav]

Using the current Texture CocoaPod (2.3).

I'm removing two nodes in a batch update:

    for index in removeIndexes.sorted(by: { $0 > $1 }) {
      dataSource.remove(index)
    }

    let removeIndexPaths = removeIndexes.map { IndexPath(item: $0, section: 0) }
    collectionNode.performBatch(
        animated: true,
        updates: {
          self.collectionNode.deleteItems(at: removeIndexPaths)
        },
        completion: {
          DDLogInfo("Completion.")
        })

Before the completion block is invoked, I sometimes (~60% of the time) crash here:

Thread 1Queue : com.apple.main-thread (serial)
#0	objc_exception_throw ()
#1	+[NSException raise:format:arguments:] ()
#2	-[NSAssertionHandler handleFailureInMethod:object:file:lineNumber:description:] ()
#3	-[UICollectionViewData layoutAttributesForItemAtIndexPath:] ()
#4	-[UICollectionView layoutAttributesForItemAtIndexPath:] ()
#5	::-[ASCollectionView dataController:presentedSizeForElement:matchesSize:](ASDataController *, ASCollectionElement *, CGSize) 
#6	::-[ASDataController relayoutNodes:nodesSizeChanged:](id, NSMutableArray *) 
#7	::-[ASCollectionView layoutSubviews]() 
#8	-[UIView(CALayerDelegate) layoutSublayersOfLayer:] ()
#9	-[CALayer layoutSublayers] ()
#10	::-[_ASDisplayLayer layoutSublayers]() 
#11	CA::Layer::layout_if_needed(CA::Transaction*) ()
#12	-[UIView(Hierarchy) layoutBelowIfNeeded] ()
#13	-[UICollectionView _performBatchUpdates:completion:invalidationContext:tentativelyForReordering:animator:] ()
#14	-[UICollectionView _performBatchUpdates:completion:invalidationContext:tentativelyForReordering:] ()
#15	-[UICollectionView _performBatchUpdates:completion:invalidationContext:] ()
#16	-[UICollectionView performBatchUpdates:completion:] ()
#17	::-[ASCollectionView _superPerformBatchUpdates:completion:](void (^)(), void (^)(BOOL)) 
#18	::__59-[ASCollectionView rangeController:didUpdateWithChangeSet:]_block_invoke() 
#19	ASPerformBlockWithoutAnimation(bool, void () block_pointer) 
#20	::-[ASCollectionView rangeController:didUpdateWithChangeSet:](ASRangeController *, _ASHierarchyChangeSet *) 
#21	::-[ASRangeController dataController:didUpdateWithChangeSet:](ASDataController *, _ASHierarchyChangeSet *) 
#22	::__40-[ASDataController updateWithChangeSet:]_block_invoke_3() 
#23	::__30-[ASMainSerialQueue runBlocks]_block_invoke() 

The crash is because we pass nil to layoutAttributesForItemAtIndexPath. At the point we throw:

- (BOOL)dataController:(ASDataController *)dataController presentedSizeForElement:(ASCollectionElement *)element matchesSize:(CGSize)size
{
  NSIndexPath *indexPath = [self indexPathForNode:element.node];
  UICollectionViewLayoutAttributes *attributes = [self layoutAttributesForItemAtIndexPath:indexPath];  // <--- THROWS
  ...

...observe:

(lldb) po element
 nil

Popping up the stack, at:

- (void)relayoutNodes:(id<NSFastEnumeration>)nodes nodesSizeChanged:(NSMutableArray *)nodesSizesChanged
{
  NSParameterAssert(nodesSizesChanged);
  
  ASDisplayNodeAssertMainThread();
  if (!_initialReloadDataHasBeenCalled) {
    return;
  }
  
  for (ASCellNode *node in nodes) {
    NSString *kind = node.collectionElement.supplementaryElementKind ?: ASDataControllerRowNodeKind;
    NSIndexPath *indexPath = [_pendingMap indexPathForElement:node.collectionElement];
    ASSizeRange constrainedSize = [self constrainedSizeForNodeOfKind:kind atIndexPath:indexPath];
    [self _layoutNode:node withConstrainedSize:constrainedSize];
    BOOL matchesSize = [_dataSource dataController:self presentedSizeForElement:node.collectionElement matchesSize:node.frame.size];  // <-- THROWS
    ...

...observe:

(lldb) po node
<Steller.ComposerImageNode: 0x7fd717ca0140; ...snip... >

(lldb) po node.collectionElement
 nil

Investigation shows that ASCellNode.collectionElement is declared weak, nullable:

@property (weak, nullable) ASCollectionElement *collectionElement;

And, as far as I can tell, they are only ever stored in ASDataController's element maps:

@property (nonatomic, strong, readonly) ASElementMap *visibleMap;
@property (nonatomic, strong, readonly) ASElementMap *pendingMap;

So the idea is ASDataController has the strong reference through the map(s) and the node has a weak reference to prevent a cycle. OK, great.

Now let us consider ASDataController.updateWithChangeSet, which, we should note at this time, asynchronously executes the block which is frame 22 in the crashing stack above.

The events leading up to this are as follows:

I assume this comment to be true:

  // Since we waited for _editingTransactionGroup at the beginning of this method, at this point we can guarantee that _pendingMap equals to _visibleMap.

So now both _pendingMap and _visibleMap contain a strong reference to the ASCollectionElements in question:

(lldb) po _pendingMap
<ASElementMap: 0x608000248400; items = (
        (
        "<ASCollectionElement: 0x608000362e80>",
        "<ASCollectionElement: 0x608000363600>",
        "<ASCollectionElement: 0x6080003636c0>",
        "<ASCollectionElement: 0x608000363780>",
        "<ASCollectionElement: 0x608000363840>",
        "<ASCollectionElement: 0x608000363900>"
    )
); supplementaryElements = {
}>

Continuing:

  // Mutable copy of current data.
  ASMutableElementMap *mutableMap = [_pendingMap mutableCopy];

Now mutableMap does too. Continuing:

  // Step 1: update the mutable copies to match the data source's state
  [self _updateSectionContextsInMap:mutableMap changeSet:changeSet];
  __weak id<ASTraitEnvironment> environment = [self.environmentDelegate dataControllerEnvironment];
  __weak ASDisplayNode *owningNode = (ASDisplayNode *)environment; // This is gross!
  ASPrimitiveTraitCollection existingTraitCollection = [environment primitiveTraitCollection];
  [self _updateElementsInMap:mutableMap changeSet:changeSet owningNode:owningNode traitCollection:existingTraitCollection];

Now mutable map does not.

  // Step 2: Clone the new data
  ASElementMap *newMap = [mutableMap copy];

Now note the objects have been removed:

(lldb) po newMap
<ASElementMap: 0x61800024a500; items = (
        (
        "<ASCollectionElement: 0x608000362e80>",
        "<ASCollectionElement: 0x608000363600>",
        "<ASCollectionElement: 0x6080003636c0>",
        "<ASCollectionElement: 0x608000363900>"
    )
); supplementaryElements = {
}>

Continuing:

  _pendingMap = newMap;

Now _pendingMap and mutableMap and newMap have all given up their strong references:

(lldb) po _pendingMap
<ASElementMap: 0x61800024a500; items = (
        (
        "<ASCollectionElement: 0x608000362e80>",
        "<ASCollectionElement: 0x608000363600>",
        "<ASCollectionElement: 0x6080003636c0>",
        "<ASCollectionElement: 0x608000363900>"
    )
); supplementaryElements = {
}>

(lldb) po _visibleMap
<ASElementMap: 0x608000248400; items = (
        (
        "<ASCollectionElement: 0x608000362e80>",
        "<ASCollectionElement: 0x608000363600>",
        "<ASCollectionElement: 0x6080003636c0>",
        "<ASCollectionElement: 0x608000363780>",
        "<ASCollectionElement: 0x608000363840>",
        "<ASCollectionElement: 0x608000363900>"
    )
); supplementaryElements = {
}>

Note that _visibleMap is the last guy to maintain a strong reference. Continuing:

  dispatch_group_async(_editingTransactionGroup, _editingTransactionQueue, ^{
    // Step 3: Layout **all** new elements without batching in background.
    NSArray<ASCollectionElement *> *unmeasuredElements = [ASDataController unmeasuredElementsFromMap:newMap];
    [self batchLayoutNodesFromContexts:unmeasuredElements batchSize:unmeasuredElements.count batchCompletion:^(id, id) {
      ASSERT_ON_EDITING_QUEUE;
      [_mainSerialQueue performBlockOnMainThread:^{
        [_delegate dataController:self willUpdateWithChangeSet:changeSet];

        // Step 4: Deploy the new data as "completed" and inform delegate
        _visibleMap = newMap;

Now _visibleMap has released its strong references as well. At this point, sometimes, if one implements dealloc for ASCollectionElement and sets a breakpoint there, you will see them get deallocated.

Continuing:

        [_delegate dataController:self didUpdateWithChangeSet:changeSet];

And this is where we are in stack frame 22.

I'm honestly not sure how this ever works other than we get lucky with the autorelease pool. Though this code is pretty complicated and I don't fully understand all the moving pieces. All I know is this is biting me. Let me know if you have any questions.

[Adlai-Holler]

@jeffdav Thanks for making an incredibly detailed and easy-to-follow investigation into this issue. I think the attached diff will fix the issue in a sane way.