`-[UITableView deleteSections:withRowAnimation:]` queries for height in `UITableViewDelegate` using old indexPaths

Originator:kamil.borzym
Number:rdar://36072360 Date Originated:December 15 2017
Status:Closed Resolved:
Product:iOS, UIKit Product Version:iOS 11.x
Classification:Bug Reproducible:Rarely
 
Summary:
========

Following code could result in a crash on iOS 11:
```
@property NSTableView *tableView;
@property NSMutableArray<NSMutableArray<CellModel *> *> *dataSourceArray;

- (CGFloat)tableView:(UITableView *)tableView heightForRowAtIndexPath:(NSIndexPath *)indexPath {
    self.dataSourceArray[indexPath.section][indexPath.row].height;
}

- (void)removeFirstTableViewSection {
    [self.dataSourceArray removeObjectAtIndex:0];
    [UIView performWithoutAnimation:^{ // I am not sure if this wrapping matters
        [self.tableView deleteSections:[NSIndexSet indexSetWithIndex:0] withRowAnimation:UITableViewRowAnimationNone];
    }];
}
```

`-[UITableView deleteSections:withRowAnimation:]` checks for internal `_updateCount` value, and in case it is equal to 0, invokes `_setupCellAnimations`. `_setupCellAnimations` uses current state of the tableView – state that could be inconsistent with current dataSource state, because dataSource could already been changed. `_setupCellAnimations` sometimes invokes `tableView:heightForRowAtIndexPath:`, passing pre-dataSource-modification index path. This behaviour could result in a crash.

Example: Let's have two sections in UITableView: one with 5 cells and second with 2 cells. The data source is modified first – first section is deleted. Calling `deleteSections:withRowAnimation:` right after dataSource modification could result in `tableView:heightForRowAtIndexPath:` being called for cell 0-4, unfortunately the section 0 in data source now contains only 2 cells – this could result in a crash.

Workaround: Wrapping any single tableView modification together with dataSource modification in `beginUpdates`/`endUpdates`. `beginUpdates` increments `_updateCount` and invokes `_setupCellAnimations` – thus calling any possible `tableView:heightForRowAtIndexPath:` before dataSource being modified.

Steps to Reproduce:
===================

I have an app in the AppStore. The app contains a code similar to the above snippet. The crash caused by query for unexisting indexPath affects about 0.1% of all app users. I was not able to reproduce it by myself.

Expected Results:
=================

Calling `-[UITableView deleteSections:withRowAnimation:]` after dataSource modification without `beginUpdates`/`endUpdates` wrapping is a common practice and is not denied by the documentation.
Either:
- `deleteSections:withRowAnimation:` should never query dataSource/delegate using old index paths, or
- `deleteSections:withRowAnimation:` documentation should be updated to describe this behaviour.

Actual Results:
===============
Calling `-[UITableView deleteSections:withRowAnimation:]` after dataSource modification without `beginUpdates`/`endUpdates` wrapping sometimes queries for unexisting indexPaths.

Version/Build:
iOS 11.0.3 (15A432), 11.1.2 (15B202), 11.2.0 (15C114). Didn't observe the issue on 11.2.0 yet, although this version's adoption could still be low.

Configuration:
iPhone 5s, 6, 6Plus, 7, 7Plus, X

Comments

Kamil Borzym December 22 2017, 8:37 AM

I think the UITableView documentation should be updated, now it only states that: "Call this method if you want subsequent insertions, deletion, and selection operations (for example, cellForRow(at:) and indexPathsForVisibleRows) to be animated simultaneously.".

Also the "Table View Programming Guide for iOS" should be updated, because it only states that: "To animate a batch insertion, deletion, and reloading of rows and sections, call the corresponding methods within an animation block defined by successive calls to beginUpdates and endUpdates."

There is an error in the code snippet in "An Example of Batched Insertion and Deletion Operations" (https://developer.apple.com/library/content/documentation/UserExperience/Conceptual/TableView_iPhone/ManageInsertDeleteRow/ManageInsertDeleteRow.html#//apple_ref/doc/uid/TP40007451-CH10-SW5), which updates data source outside of -performBatchUpdates:completion:.

By kamil.borzym at Dec. 22, 2017, 7:38 a.m. (reply...)

Apple Developer Relations December 22 2017, 1:41 AM

In order to ensure that the UITableView internal state remains consistent with your data source state, you should only perform your data source updates (e.g. removing from your dataSourceArray) inside the batch updates block passed to -performBatchUpdates:completion: (or between calls to -beginUpdates and -endUpdates). This will guarantee that the table view’s internal state is updated at the same time your data source state is updated. Calling methods like -deleteSections:withRowAnimation: on the table view and mutating your data source, without placing both inside a batch updates block (or begin/end updates calls), is not recommended.

By kamil.borzym at Dec. 22, 2017, 7:20 a.m. (reply...)

Please note: Reports posted here will not necessarily be seen by Apple. All problems should be submitted at bugreport.apple.com before they are posted here. Please only post information for Radars that you have filed yourself, and please do not include Apple confidential information in your posts. Thank you!