138165 2014-10-28 18:15:17 -0700 [iOS] iPad: Occasional <select> crashes attempting to scroll to non-existing row 0 in viewWillAppear 2014-10-29 11:22:51 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 joepeck joepeck buildbot ddkilzer enrica joepeck rniwa oldest_to_newest 1044673 0 joepeck 2014-10-28 18:15:17 -0700 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:] 1044674 1 joepeck 2014-10-28 18:15:35 -0700 <rdar://problem/18546075> 1044675 2 240586 joepeck 2014-10-28 18:20:01 -0700 Created attachment 240586 [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. 1044686 3 240586 ddkilzer 2014-10-28 20:27:01 -0700 Comment on attachment 240586 [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. 1044786 4 joepeck 2014-10-29 10:57:46 -0700 (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. 1044788 5 joepeck 2014-10-29 11:22:51 -0700 https://trac.webkit.org/r175334 240586 2014-10-28 18:20:01 -0700 2014-10-28 20:27:01 -0700 [PATCH] Proposed Fix scroll-to-row.patch text/plain 2254 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCBiNmEyOTQ4Li42MDEwN2ZlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMjMg QEAKIDIwMTQtMTAtMjggIEpvc2VwaCBQZWNvcmFybyAgPHBlY29yYXJvQGFwcGxlLmNvbT4KIAor ICAgICAgICBbaU9TXSBpUGFkOiBPY2Nhc2lvbmFsIDxzZWxlY3Q+IGNyYXNoZXMgYXR0ZW1wdGlu ZyB0byBzY3JvbGwgdG8gbm9uLWV4aXN0aW5nIHJvdyAwIGluIHZpZXdXaWxsQXBwZWFyCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMzgxNjUKKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGlzIGlzIGEgc3Bl Y3VsYXRpdmUgZml4IGZvciBhIGNyYXNoIGF0dGVtcHRpbmcgdG8gc2Nyb2xsIHRvIGEgcm93IGlu IGEKKyAgICAgICAgc2VsZWN0IHBpY2tlciBvbiBpUGFkLiBJbiB0aGVzZSBjYXNlcyB3ZSBhcmUg dHJ5aW5nIHRvIHNjcm9sbCB0byB0aGUgZmlyc3QKKyAgICAgICAgcm93IG9mIHRoZSBmaXJzdCBz ZWN0aW9uLCBidXQgbm8gc3VjaCByb3cgYXBwZWFycyB0byBleGlzdC4gSSB3YXMgdW5hYmxlCisg ICAgICAgIHRvIHJlcHJvZHVjZSB0aGUgaXNzdWUsIGJ1dCBpZiBpdCBpcyBoYXBwZW5pbmcgd2Ug c2hvdWxkIGJlIGFibGUgdG8gcHJvdGVjdAorICAgICAgICBzYWZlbHkgcHJvdGVjdCBhZ2FpbnN0 IGNyYXNoaW5nLgorCisgICAgICAgICogVUlQcm9jZXNzL2lvcy9mb3Jtcy9XS0Zvcm1TZWxlY3RQ b3BvdmVyLm1tOgorICAgICAgICAoLVtXS1NlbGVjdFRhYmxlVmlld0NvbnRyb2xsZXIgdmlld1dp bGxBcHBlYXI6XSk6CisgICAgICAgIFByb3RlY3QgYWdhaW5zdCB0cnlpbmcgdG8gc2Nyb2xsIHRv IGEgc2VjdGlvbi9yb3cgdGhhdCBkb2VzIG5vdCBleGlzdAorICAgICAgICBieSBwcmUtY2hlY2tp bmcgdGhhdCB0aGUgc2VjdGlvbi9yb3cgaXMgdmFsaWQuCisKKzIwMTQtMTAtMjggIEpvc2VwaCBQ ZWNvcmFybyAgPHBlY29yYXJvQGFwcGxlLmNvbT4KKwogICAgICAgICBbaU9TXSBpUGhvbmUgc2hv dWxkIG5vdCBhbGxvdyBzZWxlY3RpbmcgPG9wdGdyb3VwPiBpbiA8c2VsZWN0IG11bHRpcGxlPgog ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTM3OTkxCiAK ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9pb3MvZm9ybXMvV0tGb3JtU2Vs ZWN0UG9wb3Zlci5tbSBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9pb3MvZm9ybXMvV0tGb3Jt U2VsZWN0UG9wb3Zlci5tbQppbmRleCA2NzkyMGNiLi5jMDYzOWI5IDEwMDY0NAotLS0gYS9Tb3Vy Y2UvV2ViS2l0Mi9VSVByb2Nlc3MvaW9zL2Zvcm1zL1dLRm9ybVNlbGVjdFBvcG92ZXIubW0KKysr IGIvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL2lvcy9mb3Jtcy9XS0Zvcm1TZWxlY3RQb3BvdmVy Lm1tCkBAIC0xNDMsOCArMTQzLDggQEAgc3RhdGljIE5TU3RyaW5nICpzdHJpbmdXaXRoV3JpdGlu Z0RpcmVjdGlvbihOU1N0cmluZyAqc3RyaW5nLCBVSVRleHRXcml0aW5nRGlyZWMKIC0gKHZvaWQp dmlld1dpbGxBcHBlYXI6KEJPT0wpYW5pbWF0ZWQKIHsKICAgICBbc3VwZXIgdmlld1dpbGxBcHBl YXI6YW5pbWF0ZWRdOwotICAgIAotICAgIGlmIChfc2luZ2xlU2VsZWN0aW9uSW5kZXggIT0gTlNO b3RGb3VuZCkgeworCisgICAgaWYgKF9zaW5nbGVTZWxlY3Rpb25JbmRleCAhPSBOU05vdEZvdW5k ICYmIF9zaW5nbGVTZWxlY3Rpb25TZWN0aW9uIDwgKE5TVUludGVnZXIpW3NlbGYudGFibGVWaWV3 IG51bWJlck9mU2VjdGlvbnNdICYmIF9zaW5nbGVTZWxlY3Rpb25JbmRleCA8IChOU1VJbnRlZ2Vy KVtzZWxmLnRhYmxlVmlldyBudW1iZXJPZlJvd3NJblNlY3Rpb246X3NpbmdsZVNlbGVjdGlvblNl Y3Rpb25dKSB7CiAgICAgICAgIE5TSW5kZXhQYXRoICppbmRleFBhdGggPSBbTlNJbmRleFBhdGgg aW5kZXhQYXRoRm9yUm93Ol9zaW5nbGVTZWxlY3Rpb25JbmRleCBpblNlY3Rpb246X3NpbmdsZVNl bGVjdGlvblNlY3Rpb25dOwogICAgICAgICBbc2VsZi50YWJsZVZpZXcgc2Nyb2xsVG9Sb3dBdElu ZGV4UGF0aDppbmRleFBhdGggYXRTY3JvbGxQb3NpdGlvbjpVSVRhYmxlVmlld1Njcm9sbFBvc2l0 aW9uTWlkZGxlIGFuaW1hdGVkOk5PXTsKICAgICB9Cg==