Out of bounds collectionview update (crash)

[heumn]

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

General information

• IGListKit version: 2.1.0
• iOS version(s): 10.0
• CocoaPods/Carthage version: -
• Xcode version: 8.2.1
• Devices/Simulators affected: All
• Reproducible in the demo project? (Yes/No): No

I have attached an example project where this crash occurs (based on the demo project with minor adjustment): https://www.dropbox.com/s/bnis60igq8hpa6c/crash%20in%20nested%20example.zip?dl=0

How to reproduce:
1 - Launch app
2 - Select the NestedAdapterViewController
3 - Select refresh
4 - Scroll down
5 - Select refresh (re-do steps 4-5 if it does not crash immediatly)
Crash with the classical "out of bounds"

What have I done? I have adjusted the example project to use a class (EmbeddedSection) to represent the models for the nested HorizontalSectionControllers so they in turn can embody viewmodels for their respective IGListSectionControllers (EmbeddedSectionController).

I have likely misunderstood some fundamentals about this so I am terribly sorry if this is doing something IGListKit was not intended for.

The crash appears to me to be because the IGListAdapter is getting a re-used IGListCollectionView from the EmbeddedCollectionViewCell.

My only theory to go from here is that something is retaining (retain cycle) the nested HorizontalSectionController and the adapter is trying to update a reused CollectionView that should have been deallocated when you scroll down/refresh.

Before I burn a lot more energy on this I would love be told how wrong I am for even trying to do this in the first place 😂

[rnystrom]

@heumn thanks for reporting and including an example project! I'm going to spin through this and see what's going on.

[heumn]

Thanks. Really interested in what you will find. The crash broke my brain 😅

[PhilCai1993]

This will happen.
According to the code below:

    static func from(int: Int) -> EmbeddedSection? {
        let data: [Any] = (0 ... Int(arc4random_uniform(36))).map { _ in Int(arc4random_uniform(100)) }
        return EmbeddedSection(identifier: String(int) as NSString, items: data as! [IGListDiffable])
    }

Since EmbeddedSection uses identifier as diffIdentifier.

let sectionA = EmbeddedSection.from(int: 5) 
let sectionB = EmbeddedSection.from(int: 5) 

sectionA and sectionB are "equal" according to IGListDiffable. But sectionA's items could be 1,2,3 and sectionB's could be 1,5. This might be the problem.

Following might help:

 static func from(int: Int) -> EmbeddedSection? {
        let data: [Any] = (0 ... Int(arc4random_uniform(36))).map { _ in Int(arc4random_uniform(100)) }
        let identifier = data.map{String($0 as! Int)}.joined(separator: ",")
        return EmbeddedSection(identifier: identifier as NSString, items: data as! [IGListDiffable])
    }

It's recommend to make identifier unique.

[heumn]

First of, thanks a lot for taking the time to check the code in question ❤️

Let me elaborate a bit more about what I want to do.

I want to create a number of EmbeddedSection with ID from 0 ... random(36) with mocked "viewModels" named Items as an array of random Ints. By being explicit about a range starting from 0 and up to a random number to 36 I get to emulate sections that are both unique and updated on each reload (because they have unique identifiers but new Items).

So to extend on this

The sections are created from a random number of integers, starting from 0 up to a random number of ints below 64:

func objects(for listAdapter: IGListAdapter) -> [IGListDiffable] {
        
        let numberOfSections = (0 ... Int(arc4random_uniform(64))).map({ $0 })
        var sections = [IGListDiffable]()
        for integer in numberOfSections {
            sections.append(EmbeddedSection.from(int: integer)!)
        }
        return sections
    }

// Creates ints from 0 -> random number (never duplicates)
let numberOfSections = (0 ... Int(arc4random_uniform(64))).map({ $0 })

In this example it produced this array:

and the following EmbeddedSections

Following this implementation of IGListDiffable:

extension EmbeddedSection: IGListDiffable {

    func diffIdentifier() -> NSObjectProtocol {
        return identifier
    }

    func isEqual(toDiffableObject object: IGListDiffable?) -> Bool {
        if self === object { return true }
        guard let object = object as? EmbeddedSection else { return false }
        return identifier.isEqual(object.identifier)
    }
}

They all look unique to me?

If change the code to your suggestion:

static func from(int: Int) -> EmbeddedSection? {
        let data: [Any] = (0 ... Int(arc4random_uniform(36))).map { _ in Int(arc4random_uniform(100)) }
        let identifier = data.map{String($0 as! Int)}.joined(separator: ",")
        return EmbeddedSection(identifier: identifier as NSString, items: data as! [IGListDiffable])
    }

I will get unique new sections on every update, and that defeats the purpose of simulating updates (I will get new unique EmbeddedSection every time with your suggestion )

Again, I might have missed something obvious here so thanks again for looking into it :)

[PhilCai1993]

Every time objectsForListAdapter: is called, serval instances of EmbeddedSection is created.
It's true that in single objectsForListAdapter: call, all EmbeddedSections are unique (using identifier equal).

But when reload, objectsForListAdapter: is called again to provide datas to IGListAdapter.

And in IGListAdapter there's a sectionMap that records the relationship between object and SectionController. Here the object is EmbeddedSection. And after some algorithms, func isEqual(toDiffableObject object: IGListDiffable?) -> Bool will be called.

  func isEqual(toDiffableObject object: IGListDiffable?) -> Bool {
        if self === object { return true }
        guard let object = object as? EmbeddedSection else { return false }
        let equal = identifier.isEqual(object.identifier)
        if equal {
           if  items.count != object.items.count {
            ❌ print items and object.items, it may be different, at least count of items are different. ❌
           }         
        }
        return equal
    }

[heumn]

I think I understand what you mean and if I understood you correctly what I want to do is not possible (sadface)

But then my understand is that this PR #494 would enable what I want?

I apologize as English is not my first language I might be misunderstanding what you say. Again, thank you so much for helping out 🙌

Even this will crash (make sure that the items are also equal, a missing thing in original zip):

extension EmbeddedSection: IGListDiffable {

    func diffIdentifier() -> NSObjectProtocol {
        return identifier
    }

    func isEqual(toDiffableObject object: IGListDiffable?) -> Bool {
        if self === object { return true }
        guard let object = object as? EmbeddedSection else { return false }
        var equal = identifier.isEqual(object.identifier)
        let ints = items as! [Int]
        let compared = object.items as! [Int]
        equal = ints.sorted() == compared.sorted()
        return equal
    }
}

[heumn]

Btw @rnystrom I used your suggestions from here #357 and even stole some classes from your weather app to create this demo of a heavy load / complex hierarchy with custom section models. Looking forward to your explanation / findings from my example 💯

[PhilCai1993]

Because func diffIdentifier() -> NSObjectProtocol returns "identifier" only.

func isEqual(toDiffableObject object: IGListDiffable?) -> Bool uses both "identifier" and items.sorted.

Make the two methods above consistent.

 func diffIdentifier() -> NSObjectProtocol {
      let ide = identifier as! String
      let ints = items as! [Int]
      let ide2 = ints.sorted().map {
        return String($0)
      }.joined(separator: ",")
      return ide.appending(ide2) as! NSString
    }

    func isEqual(toDiffableObject object: IGListDiffable?) -> Bool {
        if self === object { return true }
        guard let object = object as? EmbeddedSection else { return false }
        var equal = identifier.isEqual(object.identifier)
        let ints = items as! [Int]
        let compared = object.items as! [Int]
        equal = ints.sorted() == compared.sorted()
        return equal
    }

IGListSectionMap uses both diffIdentifier and isEqual to do the algorithm.

By the way, I'm not native English speaker😄

[heumn]

This confuses me. I though one was for identifying unique objects (as in ID) but the other one was for identifying changes in a unique object. Having them return equality for the same object that has changes to it would not only mean you would never get "reload" on sections, it also doesn't make sense to have two methods mean the same thing.

[rnystrom]

@heumn ack, so everything is correct, but the batch update is using the wrong IGListCollectionView!

That's because when the update is queued inside HorizontalSectionController.didUpdate(to object:).

Here's some proof of the issue:

I'm still rooting to fully understand the chain of events, but the biggest issue is the adapter.performUpdates(...) call within didUpdate(to object:), and the fact that IGListAdapter uses the current collection view when updates is called.

Will try to come up w/ an elegant solution that doesn't require a bit update, might require updating like #494... Also lights a 🔥 that we need to get a stock embedded section controller built to help avoid these situations!

Hang in there @heumn, we'll get this! cc @PhilCai1993 too in case you have any ideas.

[heumn]

Thanks a lot for the update! If not tomorrow then over the weekend I will spend some time digging a bit deeper to see if I can find a nice way to solve this :)

[rnystrom]

@heumn there's a good chance this explains some low-firing crashes internally too! Really excited about this find.

[heumn]

Ok, looking more into this was more fun than doing other stuff. To start off, let me repeat again how awesome this library is! I immediately dropped my internal NSFetchedResultsController subclass (a 1500+ LOC beast) solution in favor of IGListKit. My previous solution of the typical UICollectionViews inside UICollectionViewCells" solution/hack required a lot of complex logic to locate the correct UICollectionView to update based on the updates received in NSFetchedResultsControllerDelegate (which originally believes it is dealing with one UITableView/UICollectionView.) The elegant architecture you have started here is simply 👏 🎉 💯 !

While using the framework, I ran into the following code inside a IGListSectionController subclass, which struck me as a little odd.

func cellForItem(at index: Int) -> UICollectionViewCell {
    let cell = collectionContext!.dequeueReusableCell(of: EmbeddedCollectionViewCell.self, for: self, at: index) as! EmbeddedCollectionViewCell
    adapter.collectionView = cell.collectionView
    return cell
}

I know this IGListSectionController owns the adapter, and the IGListSectionController owns the cell, but dequeuing from the collective pool of cells made me think twice.

Especially when I saw this :

We are as the user of the API are essentially giving away any control of the IGListCollectionView to the "black box" IGListAdapter (should it even know it is an IGListCollectionView? I understand you need and probably should protect this class from misuse) and since the section controllers are never reused and can "live forever" you have two separate objects that are connected with seperate life cycles (EmbeddedCollectionViewCell and the IGListAdapter). One "lives forever" (until a reload that drops it) and is never reused. The other does not and is reused with no way of telling the previous "reference" of it (the IGListAdapter) that it is not in charge any more.

When I built my custom controller for a similar type of behaviour I did it the other way around, meaning the adapter didn't really know if the UICollectionView would still be "his" / available, but rather know "ok I need to update, where is the UICollectionView I should update?" and if there were none (he never got the pointer to it, only a protocol), no updates were performed.

Random ideas that comes to mind to solve this

• Actually reuse SectionControllers
• Figure out to tell the adapter that is not needed any more (cell has been reused)
• Tell the root adapter that the section controller is "out of bounds" for the update performed (not on screen ATM)
• Separate the IGListAdapter and the IGListCollectionView with protocols and move the knowledge of visability (?) to the controller or the owner of the section controller

Quickfix:

• Recreate the IGListCollectionView when a cell is reused inside the prepareForReuse(). I have not benchmarked this to see if we take a performance hit it, but it works and fixes the crash.

Apologies for the long comment

[rnystrom]

@heumn awesome that you found a quick fix! I'll try to address all of your points.

I know this IGListSectionController owns the adapter, and the IGListSectionController owns the cell, but dequeuing from the collective pool of cells made me think twice.

The code you outlined (all the work in the setter) was added specifically to handle cell reuse w/ embedded collection views. This setup is how we use it in Instagram 😄

We are as the user of the API are essentially giving away any control of the IGListCollectionView to the "black box" IGListAdapter

That is very very much by design, since the point of the adapter is to handle edge cases, updating, and provide APIs, all of which require the collection view to work in a specific way.

Agreed that there is a bit of magic, but being open source, I wouldn't go as far as saying "black box" 😅.

Actually reuse SectionControllers

This was a #greatdebate when building IGListKit. Reusing SCs makes tracking state (e.g. "selected") trickier b/c you have to map sections/indices/cells to state vs just a @property BOOL selected; on the SC.

Figure out to tell the adapter that is not needed any more (cell has been reused)

Once we provide a setup to do embedded lists, this will definitely be addressed (custom SC and cell).

Tell the root adapter that the section controller is "out of bounds" for the update performed (not on screen ATM)

We actually don't update and fallback to reload when there is no window, but I think cells that are in the reuse pool are still subviews but have hidden = YES. We should do digging here, maybe adding || collectionView.hidden == YES to this.

Separate the IGListAdapter and the IGListCollectionView with protocols and move the knowledge of visability (?) to the controller or the owner of the section controller

We're planning on getting rid of IGListCollectionView in #409, just lower priority. @amonshiz even mentioned adding UITableView support! But that's a ways off.

Can you expand on what visibility means here? Not sure I follow.

[heumn]

The code you outlined (all the work in the setter) was added specifically to handle cell reuse w/ embedded collection views. This setup is how we use it in Instagram

Hehe.. I understood that, but arent you still keeping the state/knowedge about the state of the UICollectionView indirectly in the NSMapTable? Thus the code outlined gives you a false sense of "clean slate" for the UICollectionView as far as I understood :) This seems like the root cause of the "bug" / "edge case". Then when the performUpdatesAnimatedis called, it tries to "connect the dots" from the previous state (before the re-use) of the UICollectionView that now serves as UI layer for another adapter.

That is very very much by design, since the point of the adapter is to handle edge cases, updating, and provide APIs, all of which require the collection view to work in a specific way.

Agreed that there is a bit of magic, but being open source, I wouldn't go as far as saying "black box" 😅.

Haha.. Agree.. Apologies for the wording again (Norwegians and their norweglish and weird translated sentences). What I was trying to articulate was rather the handoff of knowledge and control by doing adapter.collectionView instead of answering to some type of protocol

This was a #greatdebate when building IGListKit. Reusing SCs makes tracking state (e.g. "selected") trickier b/c you have to map sections/indices/cells to state vs just a @Property BOOL selected; on the SC.

Got it. I fully understand there is a lot of discussions / pros / cons weighted in when designing something like this that I am missing :) Adds some limitations, but has some great benefits(!)

We actually don't update and fallback to reload when there is no window, but I think cells that are in the reuse pool are still subviews but have hidden = YES. We should do digging here, maybe adding || collectionView.hidden == YES to this.

I think that would only partially solve the problem because that would only be true for the UICollectionView that is not re-used yet and still in the pool. 🤔 The re-used ones will still crash (as they are not hidden). The old adapter will try to update a UICollectionView that is now "owned" by another adapter

Can you expand on what visibility means here? Not sure I follow.

That was a bad wording (again). What I meant was knowledge about reused / not reused.

We're planning on getting rid of IGListCollectionView in #409, just lower priority. @amonshiz even mentioned adding UITableView support! But that's a ways off.

That sounds like a great idea 👌

What I propose for now is that I research how I can to stop the adapters from updating a re-used UICollectionView they shouldnt update any more

[rnystrom]

@heumn aha, I see your point about "resetting" the collection view! It'll still be in a dirty state. Ok, great, I think I'm on the same page. I'm going to play with some ideas and report back.

[PhilCai1993]

I guess I have found the two mistakes here.
First as @rnystrom metioned:

That's because when the update is queued inside HorizontalSectionController.didUpdate(to object:).
In HorizontalSectionController.swift

func didUpdate(to object: Any) {
      model = object as? EmbeddedSection
//     adapter.performUpdates(animated: true), ❌remove this update call
 }

 func cellForItem(at index: Int) -> UICollectionViewCell {
        let cell = collectionContext!.dequeueReusableCell(of: EmbeddedCollectionViewCell.self, for: self, at: index) as! EmbeddedCollectionViewCell
        adapter.collectionView = cell.collectionView
        cell.collectionView.isPagingEnabled = false
        adapter.performUpdates(animated: true) // do update here ✅
        return cell
    }

In this way, it won't crash, but if reload button is clicked(In the sample project), it's possible the viewcontroller won't refresh, because of the incorrect way of implementation of IGListDiffable for EmbeddedSection.

So make sure the EmbeddedSection's diffIdentifier and equalility function are correct.

After this two steps above, it's solved.

[heumn]

Nice suggestion and this is also a working fix, but in my head only a slightly dirty one. You are exploiting the flow of events that is happening (that cellForItem is called only for visible cells / cells that needs refresh) and not fixing the root problem. The previous IGListAdapter that holds a reference to the UICollectionView still believes it is in charge, when it is not. Any API consumer of this framework down the line will still experience the crash unless he only updates cells in the cellForItem and not when its item changes in the didUpdatecall.

Adding documentation stating something like "if you are re-using cells, do not call performUpdates on your adapter if the cells are potentially re-used" also seems backwards and error prone and to me. I am no framework designer, but as a consumer this sounds a bit weird 🤔

[PhilCai1993]

If I want to implement the same behavior.

I would make adapter a property of the EmbeddedCollectionViewCell. And delete adapter-related code from HorizontalSectionController.

(Currently, adapter frequently changing its collectionView.)

I think it's better to bind an adapter with an collectionView, and never change.

And add a property such as var model: EmbeddedSection? to EmbeddedCollectionViewCell,

//EmbeddedCollectionViewCell.swift
let adapter = IGListAdapter() , adapter.dataSource = self

var model: EmbeddedSection? {
    didSet {
      adapter.performUpdate...
    }
}

implement IGListAdapterDataSource here

//HorizontalSectionController.swift
 func cellForItem(at index: Int) -> UICollectionViewCell {
        let cell = collectionContext!.dequeueReusableCell(of: EmbeddedCollectionViewCell.self, for: self, at: index) as! EmbeddedCollectionViewCell
       cell.model = self.model
        return cell
    }

[heumn]

Not a bad idea 🤔

Pros: the two objects (the IGListAdapter and the UICollectionView) now is explicitly bound to the lifecycle of each other that was previously implicit, hidden and somewhat dangerous.

Cons: You are growing the IGListAdapter to now own a UICollectionView and the framework will probably have taken a performance hit.

Since this framwork is already doing so much related to the UICollectionView it should at least be considered.

It would have been really interesting to see the debate about re-using IGListSectionControllers because that would also solve the root problem.

[rnystrom]

Ok I think it just clicked for me:

• Each section controller is a unique instance, each with its own unique adapter instance
• Cells are being reused, attaching adapter.collectionView = cell.collectionView when the cell is reused

However adapters off-screen still have weak references to collection views that aren't theirs anymore!

I think I have a solution to fix this:

We introduce an associated object on IGListCollectionView for the adapter. When setting the collection view, check if there is an adapter associated. If it exists, set its collection view to nil.

I took your example and made it work by:

• Setting displayDelegate = self HorizontalSectionController.init()
• Add the IGListDisplayDelegate protocol
• In the listAdapter(listAdapter:didEndDisplaying sectionController:cell:at index:) set adapter.collectionView = nil
• Comment out this assert

@heumn IMO this is a bug in IGListKit that we need to fix. Unfortunately that means you'll need to patch your project to get it working, which is a bummer. But the 3.0.0 milestone will include this fix.

cc @jessesquires for all this. It's a good one.

[heumn]

Ok I think it just clicked for me:

Each section controller is a unique instance, each with its own unique adapter instance
Cells are being reused, attaching adapter.collectionView = cell.collectionView when the cell is reused
However adapters off-screen still have weak references to collection views that aren't theirs anymore!

DING-DING-DING-DING

I think I have a solution to fix this:
We introduce an associated object on IGListCollectionView for the adapter. When setting the collection view, check if there is an adapter associated. If it exists, set its collection view to nil.

Perfect! That was somewhat one of my ideas to fix it, just wasnt smart enough to figure out how ;) Doesnt add complexity to the API nor a performance hit. In other words, sweeeeet 👌

Random ideas that comes to mind to solve this

• Figure out to tell the adapter that is not needed any more (cell has been reused)

[jessesquires]

@rnystrom - Not sure I understand the project setup, but this is basically #31, right?

Do we need specific library changes to mitigate the issue here or simply implement #31?

(also related: #133?)

[rnystrom]

@jessesquires we'll still need to do this even for #31 (in order to finish that w/out hacks) b/c how we use single section controller instances vs cell reuse (collection view gets shared between instances).

[jessesquires]

@rnystrom

one other thing:

Each section controller is a unique instance, each with its own unique adapter instance

Is this true? I thought 1 adapter powered a single list. Or is this only true for the "inception" (collection-view-inside-a-cell) case?

[heumn]

Is this true? I thought 1 adapter powered a single list. Or is this only true for the "inception" (collection-view-inside-a-cell) case?

This is only for the inception setups that wraps "collection-view-inside-a-cell" :)

[jessesquires]

Fixed in #517