Add a way to opt out of always-clear-data behavior in ASCollectionView and ASTableView

[nguyenhuy]

Through crashes in the past, we established that:

1. We can't clear data if the collection/table view is still being used. (Clearing data frequently while delegate / dataSource is changing within collection view can cause crashes #1155)
2. We may clear data during deallocation, and even so, we want to do it if user is in ASExperimentalClearDataDuringDeallocation experiment.

As such, the logic change introduced in #1200 is not correct. It causes -clearData to always be executed if user is not in the experiment (which is true by default).

I suspect what you wanted to do is setting up an experiment and hook up with Texture. And even then, we need to fix the implementation of clearData. I'm seeing a crash in production (1st screenshot below) which looks like a memory issue when self (i.e the collection view) is gone and accessing the editing queue (i.e self->_editingTransactionGroup) after that is problematic (see 2nd screenshot).

I think clearData needs to avoid calling -waitUntilAllUpdatesAreProcessed altogether. Instead, it needs to:

1. Destroy the editing queue.
2. Destroy the main serial queue.
3. Clear out the visible and pending maps.

[maicki]

I don't think this is the right logic either as it would change the initial behavior. In this case we would have to enable the ASExperimentalClearDataDuringDeallocation experimental feature to get the initial behavior we had in the first place, calling [_dataController clearData]; every time. Furthermore it would not reflect the initial behavior we had as it is checking _isDeallocating before clearing.

I would suggest we add an overall experimental feature and change the logic to something like this:

if (_asyncDataSource == nil && _asyncDelegate == nil) {
    // Overall experiment to test if different clearing behaviors will fix some issues
    if (ASActivateExperimentalFeature(ASExperimentalTestClearDuringDeallocation)) {
        // If this experiment is activated we only clear if _isDeallocating == TRUE
        if (ASActivateExperimentalFeature(ASExperimentalClearDataDuringDeallocation)) {
            if (_isDeallocating) {
                [_dataController clearData];          
            }
        // Here is room for some other cases we would like to test
        } else {
            // In this case we would not clear at all
        }
    } else {
        // Default behavior as before
        [_dataController clearData];  
    }
}

[nguyenhuy]

The thing is we don’t want to keep the original behavior (i.e always call clearData). The implementation of clearData is problematic, both while the collection view is being used and when it’s being deallocated.

In this case we would have to enable the ASExperimentalClearDataDuringDeallocation experimental feature to get the initial behavior we had in the first place

That is intended as I understand it helps to workaround the root cause. It's an opt-in workaround.

However, the implementation in #1200 and the one you proposed here doesn't give us an easy option to opt-out of the initial behavior which has been causing crashes.

[nguyenhuy]

Ok, I've just updated the logic to check the experiment first. It means you'll have room to test other cases in which clearing data is needed. I still think it should be opt-in as we don't want to keep the original problematic behavior.

[ghost]

🚫 CI failed with log

[maicki]

@nguyenhuy Would be great to have this PR not change the standard behavior where it clears by default. Applications still rely on this behavior and we can not make it opt in. We should make the new behavior (clear only on deallocation) opt in it until all bigger clients of Texture can agree what the final behavior we should strive for.

[nguyenhuy]

@maicki I updated the diff to always clear data (the original behavior) unless a client explicitly opts out (and Pinterest is going to). This essentially reverts all recent changes to get us back to #1136. LGTY?