Bug 138165 – [iOS] iPad: Occasional <select> crashes attempting to scroll to non-existing row 0 in viewWillAppear

• Component: WebKit2
• Status: RESOLVED FIXED

[Joseph Pecoraro]

iPad: Occasional <select> crashes attempting to scroll to non-existing row 0 in viewWillAppear

<Error> *** Terminating app due to uncaught exception 'NSRangeException', reason: '-[UITableView _contentOffsetForScrollingToRowAtIndexPath:atScrollPosition:]: row (0) beyond bounds (0) for section (0).'

Under: -[WKSelectTableViewController viewWillAppear:]

[Joseph Pecoraro]

<rdar://problem/18546075>

[Joseph Pecoraro]

Created attachment 240586 [details]
[PATCH] Proposed Fix

Tested with many different <select>s. This still properly scrolls to the selected option in a <select> when displaying the table. I was unable to encounter a case that would have caused the original exception/crash though. Auditing the code I am not sure how it would be possible either.

[David Kilzer (:ddkilzer)]

Comment on attachment 240586 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=240586&action=review

r=me.  Did you try using JavaScript to mutate the <select> element when trying to change it?  We now fire on change events, right?  Do we allow JavaScript to run while the wheel-of-time is up?

> Source/WebKit2/UIProcess/ios/forms/WKFormSelectPopover.mm:147
> +    if (_singleSelectionIndex != NSNotFound && _singleSelectionSection < (NSUInteger)[self.tableView numberOfSections] && _singleSelectionIndex < (NSUInteger)[self.tableView numberOfRowsInSection:_singleSelectionSection]) {

This would read better if new test conditions were pulled out into their own descriptively named BOOL variables.

[Joseph Pecoraro]

(In reply to comment #3)
> Comment on attachment 240586 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240586&action=review
> 
> r=me.  Did you try using JavaScript to mutate the <select> element when
> trying to change it?  We now fire on change events, right?  Do we allow
> JavaScript to run while the wheel-of-time is up?

I did test this as much as I could. The UI gets the list once, never updates it (even if JavaScript modifies the <select> in the background) and this scroll-to within the select is only within the list of items we've already gotten.


> > Source/WebKit2/UIProcess/ios/forms/WKFormSelectPopover.mm:147
> > +    if (_singleSelectionIndex != NSNotFound && _singleSelectionSection < (NSUInteger)[self.tableView numberOfSections] && _singleSelectionIndex < (NSUInteger)[self.tableView numberOfRowsInSection:_singleSelectionSection]) {
> 
> This would read better if new test conditions were pulled out into their own
> descriptively named BOOL variables.

Each condition builds off of the previous condition. I don't think we should run any of the later ones if an earlier one failed. I may restructure with early returns then.

[Joseph Pecoraro]

https://trac.webkit.org/r175334