Fix a bug with ListView with sticky headers + RefreshControl

[janicduplessis]

The bug is caused by a weird race condition. What happens is that when calling UIRefreshControl#endRefreshing the UIScrollView delegate scrollViewDidScroll function is called synchronously and then dockClosestSectionHeader crashes because the sticky header indexes are updated but not the contentView children.

I fixed it by adding an updating property on RCTRefreshControl and setting it before calling endRefreshing so we can know not to call dockClosestSectionHeader at that moment.

Tested with both RefreshControl and onRefreshStart prop.

I reproduced the bug by replacing ListViewExample.js in UIExplorer with https://gist.github.com/janicduplessis/05fc58e852f3e80e51b9

Fixes #5440

cc @nicklockwood

[facebook-github-bot]

By analyzing the blame information on this pull request, we identified @nicklockwood, @sahrens and @tadeuzagallo to be potential reviewers.

[facebook-github-bot]

@janicduplessis updated the pull request.

[facebook-github-bot]

@janicduplessis updated the pull request.

[janicduplessis]

Ping @nicklockwood. Could you review this when you get some time, it is the last of my RefreshControl bug fix streak :)

Updated with the little fix I didn't get to commit in #5745.

[nicklockwood]

We've actually got an intermittent error due to sticky header indices being updated out of step with the subviews anyway, so it might be better to fix the problem at its source rather than add this workaround. I'll look into that today.

[janicduplessis]

Ok great!

[janicduplessis]

I thought about that a bit and since stickyHeaderIndices and children are tightly coupled the order for which props setter functions are called become important. Maybe we could remove the stickyHeaderIndices prop from the native ScrollView and add a prop or something to the child sticky header views to be able to differentiate them in native. Now there would be only 1 source so there won't be sync issues anymore.

[nicklockwood]

Maybe we could remove the stickyHeaderIndices prop from the native ScrollView and add a prop or something to the child sticky header views to be able to differentiate them in native.

Sticky headers can be any kind of view, so the only way to do that would be to add the "isSticky" property to <View>, which seems ugly. The alternative would be to create a new <ScrollViewStickyHeaderView>, and require that headers be wrapped in it, but I'm not hugely keen on that approach either.

[janicduplessis]

I meant changing only the native ScrollView API not the js wrapper. Did you have any other idea on how to fix this?

[nicklockwood]

@janicduplessis We don't pass the props of subviews to the parent on the native side, and any props that aren't explicitly exported by the viewmanager are never sent to the native side, so I'm not sure how you'd implement it on the native side without exporting an isStickyHeader prop from RCTViewManager.

[nicklockwood]

My plan to fix it is simply to suppress the warning on scroll, since it's sometimes a false positive. Warning on batchDidComplete should be sufficient, since props and subviews are supposed to be in sync at that point.

[janicduplessis]

Yea you are right that wouldn't work, removing the warning on scroll and just not updating the sticky headers if the indices don't match sounds like a good plan.

[facebook-github-bot]

@janicduplessis updated the pull request.

[janicduplessis]

@nicklockwood Does that looks good to you? I pretty much just removed the error and it still fixes the issue. I left the little contentOffset tweak and changed the class in ScollView to use RCTRefreshControl instead of UIRefreshControl.

[nicklockwood]

Can we keep the warning for now, but just downgrade it to RCTLogWarn? I'd rather keep it until we're sure of what causes it (there are other cases besides pull to refresh).

Also can you add *stop = YES; before the return as well? Just so we don't generate repeat warnings.

[janicduplessis]

@nicklockwood Maybe I could validate the indices in bridgeDidFinishTransaction so we can keep the warning if the indices passed are actually bad but not warn when they are out of sync when called from scrollViewDidScroll. I wanted to move the warning to js initially but the children array in js was pretty different that the reactSubviews in native (reactSubview seems to be flattened somehow).

[nicklockwood]

I'd be fine with that, but still change it to RCTLogWarn, as we're occasionally seeing this error happening with bridgeDidFinishTransaction in the stack trace, so clearly there is at least one scenario where it arises that doesn't relate to scroll event race conditions.

[facebook-github-bot]

@janicduplessis updated the pull request.

[janicduplessis]

The warning does spam a lot if there actually are out of range indices but I guess it's not too bad. Also should I do the check only in debug builds so it doesn't affect perf on release since the check is done on every frame?

[nicklockwood]

@facebook-github-bot shipit

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to Phabricator to review.