From 0f3fd694d31ebb76a28ee814358faa2399095584 Mon Sep 17 00:00:00 2001
From: Maxime Ollivier <maxo@fb.com>
Date: Thu, 19 Mar 2020 10:10:01 -0700
Subject: [PATCH] add assert to avoid UICollectionView data inconsistencies

Summary:
A common cause of `NSInternalInconsistencyException` is when a section controller tries to access a cell or modify the `UICollectionView` in `-didUpdateToObject`. If the collection view doesn't have data yet, this will kick off the regular calls (ex: `-numberOfSectionsInCollectionView`, `-numberOfItemsInSection`), which won't have the right information yet because the `IGListAdpater` is in the middle of updating the object list. Eventually, this can lead to a crash, which are hard to debug because the callstack doesn't include the call that caused the premature update.

Lets add an assert to catch when this happens.

Reviewed By: candance

Differential Revision: D20527372

fbshipit-source-id: 4ad0b2d89e5b126d6ae888a5c3b1d980945b7a4c
---
 Source/IGListKit/IGListAdapter.m                      |  6 ++++++
 .../Internal/IGListAdapter+UICollectionView.m         | 11 +++++++++++
 Source/IGListKit/Internal/IGListAdapterInternal.h     |  5 ++++-
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/Source/IGListKit/IGListAdapter.m b/Source/IGListKit/IGListAdapter.m
index 4c2741df..c3ab4f95 100644
--- a/Source/IGListKit/IGListAdapter.m
+++ b/Source/IGListKit/IGListAdapter.m
@@ -598,6 +598,9 @@
 - (void)_updateObjects:(NSArray *)objects dataSource:(id<IGListAdapterDataSource>)dataSource {
     IGParameterAssert(dataSource != nil);
 
+    // Should be the first thing called in this function.
+    _isInObjectUpdateTransaction = YES;
+
 #if DEBUG
     for (id object in objects) {
         IGAssert([object isEqualToDiffableObject:object], @"Object instance %@ not equal to itself. This will break infra map tables.", object);
@@ -661,6 +664,9 @@
     }
 
     [self _updateBackgroundViewShouldHide:![self _itemCountIsZero]];
+
+    // Should be the last thing called in this function.
+    _isInObjectUpdateTransaction = NO;
 }
 
 - (void)_updateBackgroundViewShouldHide:(BOOL)shouldHide {
diff --git a/Source/IGListKit/Internal/IGListAdapter+UICollectionView.m b/Source/IGListKit/Internal/IGListAdapter+UICollectionView.m
index 41780db8..6aa70d8d 100644
--- a/Source/IGListKit/Internal/IGListAdapter+UICollectionView.m
+++ b/Source/IGListKit/Internal/IGListAdapter+UICollectionView.m
@@ -12,15 +12,20 @@
 #import <IGListKit/IGListSectionController.h>
 #import <IGListKit/IGListSectionControllerInternal.h>
 
+#import "IGListAdapterInternal.h"
+
 @implementation IGListAdapter (UICollectionView)
 
 #pragma mark - UICollectionViewDataSource
 
 - (NSInteger)numberOfSectionsInCollectionView:(UICollectionView *)collectionView {
+    _assertNotInMiddleOfObjectUpdate(self.isInObjectUpdateTransaction);
     return self.sectionMap.objects.count;
 }
 
 - (NSInteger)collectionView:(UICollectionView *)collectionView numberOfItemsInSection:(NSInteger)section {
+    _assertNotInMiddleOfObjectUpdate(self.isInObjectUpdateTransaction);
+
     IGListSectionController * sectionController = [self sectionControllerForSection:section];
     IGAssert(sectionController != nil, @"Nil section controller for section %li for item %@. Check your -diffIdentifier and -isEqual: implementations.",
              (long)section, [self.sectionMap objectForSection:section]);
@@ -304,4 +309,10 @@
     return attributes;
 }
 
+#pragma mark - Assert helpers
+
+static void _assertNotInMiddleOfObjectUpdate(BOOL isInObjectUpdateTransaction) {
+    IGAssert(isInObjectUpdateTransaction == NO, @"The UICollectionView is attempting to update its data while the IGListAdapter is in a middle of updating the object list. This will cause inconsistencies and potentially crashes (which are hard to debug). The most common cause is a section controller tries to access/modify the UICollectionView in -didUpdateToObject.");
+}
+
 @end
diff --git a/Source/IGListKit/Internal/IGListAdapterInternal.h b/Source/IGListKit/Internal/IGListAdapterInternal.h
index 9a8d0f1a..b8d71ba3 100644
--- a/Source/IGListKit/Internal/IGListAdapterInternal.h
+++ b/Source/IGListKit/Internal/IGListAdapterInternal.h
@@ -43,9 +43,12 @@ IGListBatchContext
 
 @property (nonatomic, strong, nullable) UIView *emptyBackgroundView;
 
-// we need to special case interactive section moves that are moved to the last position
+// We need to special case interactive section moves that are moved to the last position
 @property (nonatomic) BOOL isLastInteractiveMoveToLastSectionIndex;
 
+// We're in the middle of updating the objects.
+@property (nonatomic) BOOL isInObjectUpdateTransaction;
+
 /**
  When making object updates inside a batch update block, delete operations must use the section /before/ any moves take
  place. This includes when other objects are deleted or inserted ahead of the section controller making the mutations.