Prevent duplicate item deletes and drop reload collisions #657

rnystrom · 2017-04-17T16:10:00Z

Changes in this pull request

a415ef5 exposed a bug in UICollectionView where its state gets corrupted when deleting the same index path more than once in a single batch update block. This resulted crashes like

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x4)
#1  0x000000010c63a451 in -[UICollectionViewData layoutAttributesForItemAtIndexPath:] ()
#2  0x000000010c63a648 in -[UICollectionViewData layoutAttributesForGlobalItemIndex:] ()
#3  0x000000010c5f18d8 in -[UICollectionView _viewAnimationsForCurrentUpdate] ()
#4  0x000000010c5f6c7f in __71-[UICollectionView _updateWithItems:tentativelyForReordering:animator:]_block_invoke.2012 ()
#5  0x000000010bd5bd3e in +[UIView(Animation) performWithoutAnimation:] ()
#6  0x000000010c5f58b9 in -[UICollectionView _updateWithItems:tentativelyForReordering:animator:] ()
#7  0x000000010c5efbbf in -[UICollectionView _endItemAnimationsWithInvalidationContext:tentativelyForReordering:animator:] ()
#8  0x000000010c5f8259 in -[UICollectionView _endUpdatesWithInvalidationContext:tentativelyForReordering:animator:] ()
#9  0x000000010c5f85a0 in -[UICollectionView _performBatchUpdates:completion:invalidationContext:tentativelyForReordering:animator:] ()
#10 0x000000010c5f83c8 in -[UICollectionView _performBatchUpdates:completion:invalidationContext:tentativelyForReordering:] ()
#11 0x000000010c5f834a in -[UICollectionView _performBatchUpdates:completion:invalidationContext:] ()
#12 0x000000010c5f829f in -[UICollectionView performBatchUpdates:completion:] ()

And bizarre ones like

+[NSError _isCell]: unrecognized selector sent to class 0x1aa34

This PR prevents that crash and fixes the flakiness of test:

test_whenDeletingItemsTwice_withDataUpdatedTwice_thatAllUpdatesAppliedWithoutException

I originally fixed the crash by "staggering" duped delete index paths (see function in IGListBatchUpdateData), but that revealed another bug when reloads (which are converted to delete+insert) collide w/ actual deletes. In that case we want to drop the reload entirely. Fixing this required introducing another data structure, IGListReloadIndexPath.

Issue fixed: #651

Internal tasks t17306420, t17422443

Checklist

All tests pass. Demo project builds and runs.
I added tests, an experiment, or detailed why my change isn't tested.
I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.

facebook-github-bot · 2017-04-17T16:10:28Z

@rnystrom updated the pull request - view changes

iglistkit-bot · 2017-04-17T16:25:37Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

facebook-github-bot · 2017-04-17T16:45:34Z

@rnystrom updated the pull request - view changes

rnystrom · 2017-04-17T17:40:34Z

Test failed b/c of timeout

jessesquires

a few minor nits, but looks good to me!

🎉 🙌 🥇

jessesquires · 2017-04-17T17:35:48Z

CHANGELOG.md

@@ -58,6 +58,8 @@ This release closes the [3.0.0 milestone](https://github.com/Instagram/IGListKit

 - `IGListBatchUpdateData` replaced its `NSSet` properties with `NSArray` instead. [Ryan Nystrom](https://github.com/rnystrom) [(#616)](https://github.com/Instagram/IGListKit/pull/616)

+- `IGListUpdatingDelegate` now has a required method to handle reloading cells between index paths. [Ryan Nystrom](https://github.com/rnystrom) [(#657)](https://github.com/Instagram/IGListKit/pull/657)


let's note the method

jessesquires · 2017-04-17T17:37:12Z

Source/Common/IGListBatchUpdateData.mm

        const auto it = map.find(path.section);
        if (it != map.end() && it->second != nil) {
-            [indexPaths removeObject:path];
+            [indexPaths removeObjectAtIndex:i];


uh.. 🤔 is this mutating while enumerating?

Ya, but reversed the enum so mutations happen at the tail.

jessesquires · 2017-04-17T17:39:17Z

Source/IGListUpdatingDelegate.h

+ */
+- (void)reloadItemInCollectionView:(UICollectionView *)collectionView
+                     fromIndexPath:(NSIndexPath *)fromIndexPath
+                       toIndexPath:(NSIndexPath *)toIndexPath;


jessesquires · 2017-04-17T17:39:38Z

Source/Internal/IGListReloadIndexPath.h

+ */
+ (instancetype)new NS_UNAVAILABLE;
+
+@end


add nullability macros?

jessesquires · 2017-04-17T17:40:01Z

Source/Internal/IGListReloadIndexPath.h

+#import <IGListKit/IGListMacros.h>
+
+IGLK_SUBCLASSING_RESTRICTED
+@interface IGListReloadIndexPath : NSObject


need docs for class

amonshiz · 2017-04-17T18:15:40Z

I don't completely understand how this won't cause other items to be deleted within a section that has duplicate deletes for the same index path.

amonshiz · 2017-04-17T17:59:21Z

Source/Common/IGListBatchUpdateData.mm

@@ -29,16 +29,45 @@ static void convertMoveToDeleteAndInsert(NSMutableSet<IGListMoveIndex *> *moves,

 @implementation IGListBatchUpdateData

+/**
+ Finds duplicate index paths and increments increments the item index so eacy operation is unique. Duplicate delete


double increments

amonshiz · 2017-04-17T18:07:55Z

Source/Common/IGListBatchUpdateData.mm

+    NSIndexPath *nextPath = [NSIndexPath indexPathForItem:path.item + count inSection:path.section];
+    NSIndexPath *staggeredPath = staggeredIndexPath(dupes, nextPath);
+    [dupes addObject:staggeredPath];
+    return staggeredPath;


this method is a bit confusing to read due to the fact that the return value isn't just the new index path (which happens to be the last one generated, i think) but also the implicit change to dupes. i'd prefer this to return a struct that returns the NSCountedSet and NSIndexPath as elements, even if that set is the same reference as what was passed in.

👍 will add some docs to this.

I think the struct will get really messy w/ the recursion tho

amonshiz · 2017-04-17T18:15:28Z

Tests/IGListBatchUpdateDataTests.m

+                                                                           moveIndexPaths:@[]];
+    XCTAssertEqual(result.deleteIndexPaths.count, 4);
+
+    NSArray *expected = @[newPath(2, 0), newPath(2, 1), newPath(2, 3), newPath(2, 4)];


while i understand this matches the logic, won't this end up causing the data source to delete the path (2,3) even though that wasn't an index path that should be deleted?

Ya, but I think that'll be ok b/c they were telling the datasource to delete {2,0} and {2,0} and updating their data source to reflect 2 deletions. So we have to delete 2 things, otherwise UICV will throw. And UICV will bug if we delete {2,0} 2x, so instead {2,0} {2,1} is the next-best thing.

Tho I think there might be a bug here, I think the expectation should be

NSArray *expected = @[newPath(2, 0), newPath(2, 1), newPath(2, 2), newPath(2, 4)];

Made me double-check how this works. There's a bug that if you delete the last item twice, it will throw b/c its deleting an item that doesn't exist...

What about (2,2)? Is it going to be deleted? Won't that be an internal inconsistency since it really wasn't deleted and there is no corresponding insert to ensure that the correct number of items is in the end?

rnystrom · 2017-04-17T21:49:55Z

Ok backing out my "stagger" idea b/c if you delete the last item twice you will delete something that doesn't exist.

Maybe we could change the API so it validates that deletes are within the range of [collectionView numberOfItemsInSection:section]?

Changing deletes back to NSSet leaves us open to data inconsistency crashes...

rnystrom · 2017-04-18T02:02:54Z

Eating my words, I want to fix this for real. Going to need to get clever. Dang u collection view.

Here are our options as I see it:

Inserting two items at the same index path is legit
- e.g. inserting stuff into the first index path twice (2 head inserts)
Deleting two items at the same index path is a little more complicated
- Deleting from the same tail index twice is user error. If you have 4 items and delete index 3 twice, after the first delete there are only 3 items left in the array.
- However, IMO its totally ok to delete the head (0) twice

So I'm thinking maybe we keep the staggering stuff, and if it errors it errors? Maybe a follow up is adding our own error and exception handling to be more helpful than the UICV exception nonsense?

cc @jessesquires @amonshiz for thoughts

jessesquires · 2017-04-18T14:44:37Z

could we just bounds-check the "double delete" case and ignore the second delete?

rnystrom · 2017-04-18T22:11:34Z

@jessesquires If we drop one of the deletes but a data source is updated assuming 2 deletes are applied, UICV will throw for inconsistency.

rnystrom · 2017-04-19T13:35:11Z

Ok stopgap solution for now:

Restore the delete test with a FIXME prefix so it doesn't run
File an issue that this is a known problem
Land this PR with NSSet to dedupe delete operations

We've survived shipping IGListKit on Instagram for over a year w/out this, so we're in no hurry to get this totally working. However w/out a fix this is crashing now and causing tests to fail. Let's get this cleaned up and working for now.

facebook-github-bot · 2017-04-19T14:44:53Z

@rnystrom updated the pull request - view changes

rnystrom · 2017-04-19T14:55:05Z

Importing to get internal CI running on this, not landing

facebook-github-bot · 2017-04-19T14:55:05Z

@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot added the CLA Signed label Apr 17, 2017

rnystrom requested review from jessesquires and amonshiz April 17, 2017 16:10

jessesquires approved these changes Apr 17, 2017

View reviewed changes

amonshiz reviewed Apr 17, 2017

View reviewed changes

rnystrom mentioned this pull request Apr 18, 2017

Fix broken builds on travis-ci #668

Closed

rnystrom mentioned this pull request Apr 19, 2017

Method to Get Old Index Path for New Index Path in Update #672

Open

rnystrom added this to the 3.0.0 milestone Apr 19, 2017

Ryan Nystrom added 5 commits April 19, 2017 10:41

Prevent duplicate item deletes and drop reload collisions

ff16a89

fixup examples

de60e3c

changelog

9f43d86

nits, docs, remove stagger

6309b02

restore delete test with FIXME, rebase

180ff76

rnystrom force-pushed the multiple-delete branch from 135e624 to 180ff76 Compare April 19, 2017 14:44

facebook-github-bot closed this in 073fc07 Apr 20, 2017

jessesquires deleted the multiple-delete branch April 20, 2017 15:56

rnystrom mentioned this pull request Apr 22, 2017

[AppleBug] Deleting the same index path twice crashes in batch updates #689

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent duplicate item deletes and drop reload collisions #657

Prevent duplicate item deletes and drop reload collisions #657

rnystrom commented Apr 17, 2017

facebook-github-bot commented Apr 17, 2017

iglistkit-bot commented Apr 17, 2017 •

edited

Loading

facebook-github-bot commented Apr 17, 2017

rnystrom commented Apr 17, 2017

jessesquires left a comment

jessesquires Apr 17, 2017

jessesquires Apr 17, 2017

rnystrom Apr 17, 2017

jessesquires Apr 17, 2017

jessesquires Apr 17, 2017

jessesquires Apr 17, 2017

amonshiz commented Apr 17, 2017

amonshiz Apr 17, 2017

amonshiz Apr 17, 2017

rnystrom Apr 17, 2017

amonshiz Apr 17, 2017

rnystrom Apr 17, 2017

rnystrom Apr 17, 2017

amonshiz Apr 17, 2017

rnystrom commented Apr 17, 2017

rnystrom commented Apr 18, 2017 •

edited

Loading

jessesquires commented Apr 18, 2017

rnystrom commented Apr 18, 2017

rnystrom commented Apr 19, 2017

facebook-github-bot commented Apr 19, 2017

rnystrom commented Apr 19, 2017

facebook-github-bot commented Apr 19, 2017

		@@ -58,6 +58,8 @@ This release closes the [3.0.0 milestone](https://github.com/Instagram/IGListKit

		- `IGListBatchUpdateData` replaced its `NSSet` properties with `NSArray` instead. [Ryan Nystrom](https://github.com/rnystrom) [(#616)](https://github.com/Instagram/IGListKit/pull/616)

		- `IGListUpdatingDelegate` now has a required method to handle reloading cells between index paths. [Ryan Nystrom](https://github.com/rnystrom) [(#657)](https://github.com/Instagram/IGListKit/pull/657)

Prevent duplicate item deletes and drop reload collisions #657

Prevent duplicate item deletes and drop reload collisions #657

Conversation

rnystrom commented Apr 17, 2017

Changes in this pull request

Checklist

facebook-github-bot commented Apr 17, 2017

iglistkit-bot commented Apr 17, 2017 • edited Loading

facebook-github-bot commented Apr 17, 2017

rnystrom commented Apr 17, 2017

jessesquires left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amonshiz commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnystrom commented Apr 17, 2017

rnystrom commented Apr 18, 2017 • edited Loading

jessesquires commented Apr 18, 2017

rnystrom commented Apr 18, 2017

rnystrom commented Apr 19, 2017

facebook-github-bot commented Apr 19, 2017

rnystrom commented Apr 19, 2017

facebook-github-bot commented Apr 19, 2017

iglistkit-bot commented Apr 17, 2017 •

edited

Loading

rnystrom commented Apr 18, 2017 •

edited

Loading