Bug 138165 - [iOS] iPad: Occasional <select> crashes attempting to scroll to non-existing row 0 in viewWillAppear
Summary: [iOS] iPad: Occasional <select> crashes attempting to scroll to non-existing ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-10-28 18:15 PDT by Joseph Pecoraro
Modified: 2014-10-29 11:22 PDT (History)
5 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (2.20 KB, patch)
2014-10-28 18:20 PDT, Joseph Pecoraro
ddkilzer: review+
ddkilzer: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2014-10-28 18:15:17 PDT
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:]
Comment 1 Joseph Pecoraro 2014-10-28 18:15:35 PDT
<rdar://problem/18546075>
Comment 2 Joseph Pecoraro 2014-10-28 18:20:01 PDT
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.
Comment 3 David Kilzer (:ddkilzer) 2014-10-28 20:27:01 PDT
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.
Comment 4 Joseph Pecoraro 2014-10-29 10:57:46 PDT
(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.
Comment 5 Joseph Pecoraro 2014-10-29 11:22:51 PDT
https://trac.webkit.org/r175334