Iglistkit: Wrong index in sizeForItem

[rafalszastok]

New issue checklist

• [x] I have reviewed the README and documentation
• [X] I have searched existing issues and this is not a duplicate

General information

• IGListKit version: 3.1.1
• iOS version(s): iOS 10 & iOS 11
• Carthage version: 0.25
• Xcode version: Xcode 8 & Xcode 9
• Devices/Simulators affected: All devices and simulators
• Reproducible in the demo project? No (modified demo attached below).
• Related issues: NONE

Description

We use modified NestedAdapterViewController example. In subclass of ListSectionController we receive crash on sizeForItem(at: where we receive index out of bounds. We also receive crash on cellForItem(at with index out of bounds. It happens performUpdates from adapter, not reloadData.

I'm not sure if we use ListSectionController or adapter incorrectly or confirm it's a bug in IGListKit. I would be grateful for help.

Link to modified example:
https://github.com/rafalszastok/IGListKit/tree/crash-on-embedded-cells
Step to reproduce: After list of examples appears, select "Out custom example" at the bottom. It should crash automatically after few seconds.

Classes added to project:

• RailsViewController.swift
• RailSectionController.swift
• TileSectionController.swift

• ViewModels.swift

  Modified:

• DemosViewController.swift

Edit

I did some changes and now example does the same as EmbeddedSectionController and this time I receive different error. In this case batch update doesn't finish before the next one starts. This cause the number of times isn't updated yet.

2017-10-04 10:11:37.168914+0200 IGListKitExamples[15977:1224786] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Invalid update: invalid number of sections.  The number of sections contained in the collection view after the update (4) must be equal to the number of sections contained in the collection view before the update (8), plus or minus the number of sections inserted or deleted (2 inserted, 1 deleted).'
*** First throw call stack:
(
    0   CoreFoundation                      0x000000010ea181cb __exceptionPreprocess + 171
    1   libobjc.A.dylib                     0x000000010e37af41 objc_exception_throw + 48
    2   CoreFoundation                      0x000000010ea1d362 +[NSException raise:format:arguments:] + 98
    3   Foundation                          0x000000010de1f089 -[NSAssertionHandler handleFailureInMethod:object:file:lineNumber:description:] + 193
    4   UIKit                               0x0000000110ce597f -[UICollectionView _endItemAnimationsWithInvalidationContext:tentativelyForReordering:animator:] + 15755
    5   UIKit                               0x0000000110cee5ad -[UICollectionView _endUpdatesWithInvalidationContext:tentativelyForReordering:animator:] + 71
    6   UIKit                               0x0000000110cee8f2 -[UICollectionView _performBatchUpdates:completion:invalidationContext:tentativelyForReordering:animator:] + 435
    7   UIKit                               0x0000000110cee71c -[UICollectionView _performBatchUpdates:completion:invalidationContext:tentativelyForReordering:] + 91
    8   UIKit                               0x0000000110cee69e -[UICollectionView _performBatchUpdates:completion:invalidationContext:] + 74
    9   UIKit                               0x0000000110cee5f3 -[UICollectionView performBatchUpdates:completion:] + 53
    10  IGListKit                           0x000000010db41267 __62-[IGListAdapterUpdater performBatchUpdatesWithCollectionView:]_block_invoke.118 + 567
    11  IGListKit                           0x000000010db3f9c6 -[IGListAdapterUpdater performBatchUpdatesWithCollectionView:] + 3958
    12  IGListKit                           0x000000010db43aef __54-[IGListAdapterUpdater queueUpdateWithCollectionView:]_block_invoke + 463
    13  libdispatch.dylib                   0x00000001126c93f7 _dispatch_call_block_and_release + 12
    14  libdispatch.dylib                   0x00000001126ca43c _dispatch_client_callout + 8
    15  libdispatch.dylib                   0x00000001126d56f0 _dispatch_main_queue_callback_4CF + 628
    16  CoreFoundation                      0x000000010e9daef9 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 9
    17  CoreFoundation                      0x000000010e99f662 __CFRunLoopRun + 2402
    18  CoreFoundation                      0x000000010e99ea89 CFRunLoopRunSpecific + 409
    19  GraphicsServices                    0x0000000113dee9c6 GSEventRunModal + 62
    20  UIKit                               0x0000000110214d30 UIApplicationMain + 159
    21  IGListKitExamples                   0x000000010d6f5da7 main + 55
    22  libdyld.dylib                       0x0000000112746d81 start + 1
)
libc++abi.dylib: terminating with uncaught exception of type NSException

Branch:
https://github.com/rafalszastok/IGListKit/tree/crash-on-embedded-cells-2

[rafalszastok]

After futher investigation, I found the reason of my crashes. In short: we change collectionView in adapter in SectionController that creates EmbeddedTableViewCell:

override func cellForItem(at index: Int) -> UICollectionViewCell {
    [...]
    adapter.collectionView = cell.collectionView
    [...]
    return cell
}

Debugging process

I prepared function that periodically and every second returns 3, 4 or 8 elements. After few updates I receive crash:

NSInternalInconsistencyException', reason: 'attempt to delete section 7, but there are only 4 sections before the update

Under the hood

Function numberOfSectionsInCollectionView in IGListAdapter+UICollectionView.m (implements UICollectionViewDataSource) returns 3, 4 or 8 element but for different instances of UICollectionView. After my experiments I know IGListKit prepares 2 different cells for one element (ListAdapterDataSource for RailSectionController returns one EmbeddedCollectionViewCell). As a result every call of cellForRow section controller change collectionView in adapter. When adapter gets performUpdates(animated: it perform inserts and deletes but not always on the same collection view. When it happens the collection view is not up-to-date. For example it might "think" it has 4 elements, but we want to delete 5, 6, 7 and 8 cell.

I was able to crash iOS example with minimum changes:

• Changing number of elements periodically and adding identifier for HorizontalSec
• Updating inner collection view after outer collection view in didUpdateTo

https://github.com/rafalszastok/IGListKit/tree/bug/crash-example

[rnystrom]

Gives me more support to create a first-class embedded controller...

@rafalszastok I’ll take a look at the example when I’m back in town.

I’ve seen similar crashes in Instagram when updating a collection view on a reused cell. Looks very similar.

Given your investigation and repro on the example, do you consider this a bug in IGListKit or just confusing to setup?

_{Sent with GitHawk}

[haitham-reda]

@rafalszastok i'am having the same issue now, any progress to make them in sync ?

[rafalszastok]

Because collection view contains logic to insert and remove items with animations it can't be treated like reusable component. I'm not sure if diff mechanism allows that, but using unique identifier for each cell that contains collectionView (EmbeddedCollectionViewCell) might solve problem. I already tried several approaches but still I can't find the solution or workaround that satisfy me.

[rafalszastok]

@rnystrom Any updates?

[rnystrom]

Sorry, this is on my radar but really low pri vs some other projects right now. Not forgotten.

[rnystrom]

Ok, so there is something wrong here. The cached data inside UICollectionView has gotten out of sync w/ what the collectionView.dataSource reports. Take a look at this debugger output:

new tiles: [IGListKitExamples.Tile(title: "aaa"), IGListKitExamples.Tile(title: "bbb"), IGListKitExamples.Tile(title: "ccc"), IGListKitExamples.Tile(title: "ddd"), IGListKitExamples.Tile(title: "eee"), IGListKitExamples.Tile(title: "fff")]
new tiles: [IGListKitExamples.Tile(title: "aaa"), IGListKitExamples.Tile(title: "bbb"), IGListKitExamples.Tile(title: "ccc"), IGListKitExamples.Tile(title: "ddd"), IGListKitExamples.Tile(title: "eee"), IGListKitExamples.Tile(title: "fff"), IGListKitExamples.Tile(title: "ggg"), IGListKitExamples.Tile(title: "hhh")]
new tiles: [IGListKitExamples.Tile(title: "aaa"), IGListKitExamples.Tile(title: "bbb"), IGListKitExamples.Tile(title: "ccc"), IGListKitExamples.Tile(title: "ddd")]
(lldb) p index
(Int) $R0 = 4
(lldb) p tiles
([IGListKitExamples.Tile]) $R1 = 4 values {
  [0] = (title = "aaa")
  [1] = (title = "bbb")
  [2] = (title = "ccc")
  [3] = (title = "ddd")
}
(lldb) p [collectionView numberOfSections]
(NSInteger) $0 = 1
(lldb) p [collectionView numberOfItemsInSection:0]
(NSInteger) $1 = 6
(lldb) po [self numberOfSectionsInCollectionView:collectionView]
1

(lldb) po [self collectionView:collectionView numberOfItemsInSection:0]
4

I can confirm this happens on both iOS 11 and 10.3.

If you update this line to be:

adapter.collectionView = cell.collectionView
adapter.collectionView?.reloadData()

Then the crash goes away b/c we force the collection view to update its data source.

This seems like a pretty big deal tbh. I'm going to put up an experimental fix to see how often this happens to us. We used nested adapters _a lot_ within Instagram, and almost never get these crashes considering the sample size...

You can use the above as a temp fix. I'll work on an experimental fix, test it, and report back.

[rafalszastok]

@rnystrom Until we didn't check the number of items inside sizeForItem(at: the number of crashes was hundred of times lower. We recently applied fix where we check if index isn't out of bounds in size and cellForItem(at:. Now the number of crashes on new version went back to normal.

[rnystrom]

@rafalszastok that still doesn’t fix the data inconsistency issue. That will avoid the crash, but you might still see wrong or incorrectly sized information.

(edit: fixed wrong tag)

_{Sent with GitHawk}

[rizwankce]

@rnystrom guess you wrongly tagged me. instead of @rafalszastok 😆

[drinkius]

Facing the same issue in our app, would welcome "first-class embedded controller"!

[rafalszastok]

It's already taken to version 3.2.0: https://github.com/Instagram/IGListKit/milestone/6

[rafalszastok]

@rnystrom Is there a chance to have first-class embedded controller in the 3.3.0 release?

[rnystrom]

@rafalszastok not likely because we don’t have a large need for it at Instagram atm. I’m more inclined to accept and review a PR on this (basically take the example and generalize it).

_{Sent with GitHawk}

[jessesquires]

Bumped this to new 3.5.0 milestone since 3.4.0 was released already.

[jessesquires]

Nevermind, bumped to 4.0 since that's the next release. 😄

[yusuftor]

+1 for needing a first-class embedded controller, or some kind of permanent fix. I also get the same crash and the line adapter.collectionView?.reloadData() fixed it for me. I was struggling for days with this until I happened to come across this thread just now!

[iperry90]

We don't have a need for this right now at Instagram, so bumping to 4.1.0. If anyone wants to submit a PR for this, we'd be happy to review and accept.