From 64ba4712012f074acbafa3fee05aefb28fa06fd2 Mon Sep 17 00:00:00 2001 From: Maxime Ollivier Date: Mon, 27 Oct 2025 14:38:36 -0700 Subject: [PATCH] assert when diffIdentifier changes between updates Summary: `diffIdentifier` should not have changed before an update starts, otherwise we can't look up the corresponding section-controller. Lets add an assert when we detect this issue. In a few months, we should evaluate if this is still useful. Differential Revision: D85456085 fbshipit-source-id: 9aee649477618e2fdcd5b8e972d8eae0719bed85 --- CHANGELOG.md | 2 + Source/IGListKit/Internal/IGListSectionMap.h | 3 + Source/IGListKit/Internal/IGListSectionMap.m | 100 ++++++++++++++++--- 3 files changed, 92 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e185eec8..c96e16ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag - Added `autoDeselectEnabled` on `IGListAdapter` to free each section-controller from having to do this and avoid bugs [Maxime Ollivier](https://github.com/maxolls) (tbd) +- Added assert when a section's `diffIdentifier` changed before an update starts, which could manifest in a few different crashes [Maxime Ollivier](https://github.com/maxolls) (tbd) + 5.0.0 ----- diff --git a/Source/IGListKit/Internal/IGListSectionMap.h b/Source/IGListKit/Internal/IGListSectionMap.h index f44d4cd8..9c2c6bd7 100644 --- a/Source/IGListKit/Internal/IGListSectionMap.h +++ b/Source/IGListKit/Internal/IGListSectionMap.h @@ -28,6 +28,9 @@ NS_ASSUME_NONNULL_BEGIN IGLK_SUBCLASSING_RESTRICTED @interface IGListSectionMap : NSObject +/** + @param mapTable Table used to keep a relationship between the object and its section-controller + */ - (instancetype)initWithMapTable:(NSMapTable, IGListSectionController *> *)mapTable NS_DESIGNATED_INITIALIZER; /** diff --git a/Source/IGListKit/Internal/IGListSectionMap.m b/Source/IGListKit/Internal/IGListSectionMap.m index ed9d180b..eb5cd97e 100644 --- a/Source/IGListKit/Internal/IGListSectionMap.m +++ b/Source/IGListKit/Internal/IGListSectionMap.m @@ -23,23 +23,25 @@ @property (nonatomic, strong, nonnull) NSMutableArray> *mObjects; +@property (nonatomic, strong, nullable) NSMutableArray> *diffIdentifiersSnapshot; + @end @implementation IGListSectionMap - (instancetype)initWithMapTable:(NSMapTable, IGListSectionController *> *)mapTable { - IGParameterAssert(mapTable != nil); - - if (self = [super init]) { - _objectToSectionControllerMap = [mapTable copy]; - - // lookup list objects by pointer equality - _sectionControllerToSectionMap = [[NSMapTable alloc] initWithKeyOptions:NSMapTableStrongMemory | NSMapTableObjectPointerPersonality - valueOptions:NSMapTableStrongMemory - capacity:0]; - _mObjects = [NSMutableArray new]; - } - return self; + IGParameterAssert(mapTable != nil); + + if (self = [super init]) { + _objectToSectionControllerMap = [mapTable copy]; + + // lookup list objects by pointer equality + _sectionControllerToSectionMap = [[NSMapTable alloc] initWithKeyOptions:NSMapTableStrongMemory | NSMapTableObjectPointerPersonality + valueOptions:NSMapTableStrongMemory + capacity:0]; + _mObjects = [NSMutableArray new]; + } + return self; } @@ -65,7 +67,9 @@ [self reset]; + [self _validateAllDiffIdentifiers]; self.mObjects = [objects mutableCopy]; + [self _updateAllDiffIdentifiers]; id firstObject = objects.firstObject; id lastObject = objects.lastObject; @@ -132,7 +136,11 @@ id sectionController = [self sectionControllerForObject:object]; [self.sectionControllerToSectionMap setObject:@(section) forKey:sectionController]; [self.objectToSectionControllerMap setObject:sectionController forKey:object]; + + + [self _validateDiffIdentifierAtSection:section]; self.mObjects[section] = object; + [self _updateDiffIdentifierAtSection:section newObject:object]; } - (void)enumerateUsingBlock:(void (^)(id object, IGListSectionController *sectionController, NSInteger section, BOOL *stop))block { @@ -154,12 +162,78 @@ #pragma mark - NSCopying - (id)copyWithZone:(NSZone *)zone { - IGListSectionMap *copy = [[IGListSectionMap allocWithZone:zone] initWithMapTable:self.objectToSectionControllerMap]; + IGListSectionMap *copy = [[IGListSectionMap allocWithZone:zone] initWithMapTable:self.objectToSectionControllerMap]; if (copy != nil) { copy->_sectionControllerToSectionMap = [self.sectionControllerToSectionMap copy]; copy->_mObjects = [self.mObjects mutableCopy]; + copy->_diffIdentifiersSnapshot = [self.diffIdentifiersSnapshot mutableCopy]; } return copy; } + +#pragma mark - Diff Identifiers validation + +#if IG_ASSERTIONS_ENABLED +static void IGListSectionMapValidateDiffIdentifier(NSUInteger section, NSArray> *mObjects, NSArray> *_Nullable diffIdentifiersSnapshot) { + if (mObjects.count != diffIdentifiersSnapshot.count) { + // Don't have an accurate snapshot of the diff identifiers. + return; + } + + if (section < 0 || section >= mObjects.count) { + return; + } + + id const object = mObjects[section]; + id const newDiffIdentifier = [object diffIdentifier]; + id const oldDiffIdentifier = diffIdentifiersSnapshot[section]; + + // Between updates, we don't expect the diffIdentifier to change for the same section. If it does, we lose our ability to find the + // corresponding section-controller in `objectToSectionControllerMap` and usually crash. For example: + // - Section has suddently 0 items, so batch updates are wrong + // - Adapter returns nil cell + // The fix is to make sure -diffIdentifier is not mutable. Generally, -diffIdentifier should be pretty simple (like a UUID) + // and -isEqualToDiffableObject should be where we compare all other relevant properties to trigger an update. + IGAssert([oldDiffIdentifier isEqual:newDiffIdentifier], @"Diff identifier changed for object %@ at section %i, from %@ to %@", + NSStringFromClass([(NSObject *)object class]), + section, + oldDiffIdentifier, + newDiffIdentifier); +} +#endif + +- (void)_validateAllDiffIdentifiers { +#if IG_ASSERTIONS_ENABLED + for (NSUInteger section = 0; section < _mObjects.count; section++) { + IGListSectionMapValidateDiffIdentifier(section, _mObjects, _diffIdentifiersSnapshot); + } +#endif +} + +- (void)_validateDiffIdentifierAtSection:(NSInteger)section { +#if IG_ASSERTIONS_ENABLED + IGListSectionMapValidateDiffIdentifier(section, _mObjects, _diffIdentifiersSnapshot); +#endif +} + +- (void)_updateAllDiffIdentifiers { +#if IG_ASSERTIONS_ENABLED + if (!_diffIdentifiersSnapshot) { + _diffIdentifiersSnapshot = [NSMutableArray new]; + } + + [_diffIdentifiersSnapshot removeAllObjects]; + for (id object in _mObjects) { + [_diffIdentifiersSnapshot addObject:[object diffIdentifier]]; + } +#endif +} + +- (void)_updateDiffIdentifierAtSection:(NSInteger)section newObject:(id)newObject { +#if IG_ASSERTIONS_ENABLED + _diffIdentifiersSnapshot[section] = newObject.diffIdentifier; +#endif +} + @end