117020 2013-05-30 01:20:06 -0700 REGRESSION (r150653): Reproducible crash with triple-finger-tap "define word" gesture on a Netflix video 2013-06-07 13:16:45 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 thorton webkit-unassigned commit-queue darin deniau rniwa webkit-bug-importer oldest_to_newest 895408 0 thorton 2013-05-30 01:20:06 -0700 100% repro. Open a Netflix video, then three-finger-tap on it. In a debug build, I see: ASSERTION FAILED: count == 0 0x10d457240 WTFCrash 0x10f99c1b5 WebCore::CharacterIterator::advance(int) 0x10f99d1ae WebCore::characterSubrange(WebCore::CharacterIterator&, int, int) 0x10f99d139 WebCore::TextIterator::subrange(WebCore::Range*, int, int) 0x10bbd9218 WebKit::WebPage::performDictionaryLookupAtLocation(WebCore::FloatPoint const&) In a release build: KERN_INVALID_ADDRESS at 0x0000000000000010 com.apple.WebCore 0x000000010a21e1c2 WebCore::characterSubrange(WebCore::CharacterIterator&, int, int) + 98 com.apple.WebCore 0x000000010a21e136 WebCore::TextIterator::subrange(WebCore::Range*, int, int) + 134 com.apple.WebKit2 0x000000010984ee6e WebKit::WebPage::performDictionaryLookupAtLocation(WebCore::FloatPoint const&) + 1920 895410 1 thorton 2013-05-30 01:28:13 -0700 This regressed in http://trac.webkit.org/changeset/150653. 895413 2 webkit-bug-importer 2013-05-30 01:28:35 -0700 <rdar://problem/14021387> 895536 3 deniau 2013-05-30 07:58:43 -0700 extractedRange.length is -1, possibly because startOfLine() and endOfLine() sometimes return an empty VisualPosition. I guess this happens when there is no text at all on the page. I have a patch. 895548 4 203359 deniau 2013-05-30 08:27:28 -0700 Created attachment 203359 Patch 895571 5 203359 darin 2013-05-30 09:37:27 -0700 Comment on attachment 203359 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203359&action=review Code change is fine. Code is not written in correct WebKit style, though, so please submit a new patch with the style. Fixes. Why no regression test? Is there no easy way to write a test for this? Normally we require tests for any bug fix. > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:540 > + if (!lineStart.isNull()) contextStart = lineStart; WebKit style requires breaking this into two lines: if (!lineStart.isNull()) contextStart = lineStart; I am also a bit surprised that startOfLine doesn’t have this behavior. I would have expected it to return the passed-in position if it can’t find a start of line rather than returning null. > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:543 > + if (!lineEnd.isNull()) contextEnd = lineEnd; Ditto. > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:553 > + if ((extractedRange.location == NSNotFound) || (extractedRange.length <= 0)) { > + return; > + } WebKit style does not use the extra parentheses or braces here. Also, range length is an unsigned value, so <= 0 is sort of silly. For WebKit coding style this should be: if (extractedRange.location == NSNotFound || !extractedRange.length) return; In addition, I’m pretty sure we don’t need to check both the location and the length. When NSNotFound is returned, the length is also 0 so the length check alone should suffice. 895572 6 darin 2013-05-30 09:38:13 -0700 (In reply to comment #3) > extractedRange.length is -1 How can it be -1? It’s an unsigned type? Is it really 0xFFFFFFFF or 0xFFFFFFFFFFFFFFFF? 895889 7 deniau 2013-05-31 07:30:06 -0700 Sorry, I confused the location and the length in the debugger. The location is 0xFFFFFFFFFFFFFFFF and shows up as -1 once cast to signed in TextIterator::subrange. Sorry about the style issues; I thought webkit-patch upload checked the style of the patch, too, so I didn't pay too much attention to it... I'll fix them. 896137 8 darin 2013-06-01 16:29:05 -0700 (In reply to comment #7) > I thought webkit-patch upload checked the style of the patch It does, but it uses a script. Our style guidelines contain many concepts that are either not checkable by a script or at least not checked by the script we have. 896823 9 203676 deniau 2013-06-04 03:58:44 -0700 Created attachment 203676 Patch 898271 10 203676 commit-queue 2013-06-07 13:16:42 -0700 Comment on attachment 203676 Patch Clearing flags on attachment: 203676 Committed r151332: <http://trac.webkit.org/changeset/151332> 898272 11 commit-queue 2013-06-07 13:16:45 -0700 All reviewed patches have been landed. Closing bug. 203359 2013-05-30 08:27:28 -0700 2013-06-04 03:58:37 -0700 Patch bug-117020-20130530172604.patch text/plain 2347 deniau U3VidmVyc2lvbiBSZXZpc2lvbjogMTUwOTUzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggNjRlOGEyNDVhOGNlNWM3 NjkyYjI3OGQ4MTcyMjQ3MjNjZmFhZTMzYy4uNGIzMDY2MTlmZThkNjljZWI5Y2E4MjcwMWM1NTk4 Yjg5MDA5ZjkzMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEzLTA1LTMwICBUaG9t YXMgRGVuaWF1ICA8ZGVuaWF1QGFwcGxlLmNvbT4KKworICAgICAgICBSZXByb2R1Y2libGUgY3Jh c2ggd2l0aCB0cmlwbGUtZmluZ2VyLXRhcCAiZGVmaW5lIHdvcmQiIGdlc3R1cmUgb24gYSBOZXRm bGl4IHZpZGVvCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xMTcwMjAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICAqIFdlYlByb2Nlc3MvV2ViUGFnZS9tYWMvV2ViUGFnZU1hYy5tbToKKyAgICAgICAgKFdlYktp dDo6V2ViUGFnZTo6cGVyZm9ybURpY3Rpb25hcnlMb29rdXBBdExvY2F0aW9uKToKKyAgICAgICAg RG9uJ3QgaW52b2tlIHRoZSBEaWN0aW9uYXJ5IGxvb2t1cCB3aGVuIHdlIGZhaWxlZCB0byBleHRy YWN0IHRoZSByYW5nZSBvZiB0aGUgdG9rZW4KKyAgICAgICAgc3Vycm91bmRpbmcgdGhlIHRhcHBl ZCBwb3NpdGlvbi4KKwogMjAxMy0wNS0yOSAgVGltIEhvcnRvbiAgPHRpbW90aHlfaG9ydG9uQGFw cGxlLmNvbT4KIAogICAgICAgICBFeHBvc2UgaW5jcmVtZW50YWxSZW5kZXJpbmdTdXBwcmVzc2lv blRpbWVvdXQgdmlhIFdLMgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9X ZWJQYWdlL21hYy9XZWJQYWdlTWFjLm1tIGIvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJQ YWdlL21hYy9XZWJQYWdlTWFjLm1tCmluZGV4IGU1NGM4ODEwZGU3NmEyMDE4OTNjNWZlOTU5ODI1 NjUyMTc2YWFhMWQuLjg3ZTVmN2RjMTYwNmZjMzBjODZkMTBmMTU5MDRjODY5ZDBjZGJlNTMgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUGFnZS9tYWMvV2ViUGFnZU1h Yy5tbQorKysgYi9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL1dlYlBhZ2UvbWFjL1dlYlBhZ2VN YWMubW0KQEAgLTUzNSw4ICs1MzUsMTIgQEAgdm9pZCBXZWJQYWdlOjpwZXJmb3JtRGljdGlvbmFy eUxvb2t1cEF0TG9jYXRpb24oY29uc3QgRmxvYXRQb2ludCYgZmxvYXRQb2ludCkKICAgICAgICAg ICAgIGJyZWFrOwogICAgICAgICBjb250ZXh0RW5kID0gbjsKICAgICB9Ci0gICAgY29udGV4dFN0 YXJ0ID0gc3RhcnRPZkxpbmUoY29udGV4dFN0YXJ0KTsKLSAgICBjb250ZXh0RW5kID0gZW5kT2ZM aW5lKGNvbnRleHRFbmQpOworCisgICAgVmlzaWJsZVBvc2l0aW9uIGxpbmVTdGFydCA9IHN0YXJ0 T2ZMaW5lKGNvbnRleHRTdGFydCk7CisgICAgaWYgKCFsaW5lU3RhcnQuaXNOdWxsKCkpIGNvbnRl eHRTdGFydCA9IGxpbmVTdGFydDsKKworICAgIFZpc2libGVQb3NpdGlvbiBsaW5lRW5kID0gZW5k T2ZMaW5lKGNvbnRleHRFbmQpOworICAgIGlmICghbGluZUVuZC5pc051bGwoKSkgY29udGV4dEVu ZCA9IGxpbmVFbmQ7CiAgICAgCiAgICAgTlNSYW5nZSByYW5nZVRvUGFzcyA9IE5TTWFrZVJhbmdl KFRleHRJdGVyYXRvcjo6cmFuZ2VMZW5ndGgobWFrZVJhbmdlKGNvbnRleHRTdGFydCwgcG9zaXRp b24pLmdldCgpKSwgMCk7CiAKQEAgLTU0NCw2ICs1NDgsOSBAQCB2b2lkIFdlYlBhZ2U6OnBlcmZv cm1EaWN0aW9uYXJ5TG9va3VwQXRMb2NhdGlvbihjb25zdCBGbG9hdFBvaW50JiBmbG9hdFBvaW50 KQogICAgIFN0cmluZyBmdWxsUGxhaW5UZXh0U3RyaW5nID0gcGxhaW5UZXh0KGZ1bGxDaGFyYWN0 ZXJSYW5nZS5nZXQoKSk7CiAKICAgICBOU1JhbmdlIGV4dHJhY3RlZFJhbmdlID0gV0tFeHRyYWN0 V29yZERlZmluaXRpb25Ub2tlblJhbmdlRnJvbUNvbnRleHR1YWxTdHJpbmcoZnVsbFBsYWluVGV4 dFN0cmluZywgcmFuZ2VUb1Bhc3MsICZvcHRpb25zKTsKKyAgICBpZiAoKGV4dHJhY3RlZFJhbmdl LmxvY2F0aW9uID09IE5TTm90Rm91bmQpIHx8IChleHRyYWN0ZWRSYW5nZS5sZW5ndGggPD0gMCkp IHsKKyAgICAgICAgcmV0dXJuOworICAgIH0KIAogICAgIFJlZlB0cjxSYW5nZT4gZmluYWxSYW5n ZSA9IFRleHRJdGVyYXRvcjo6c3VicmFuZ2UoZnVsbENoYXJhY3RlclJhbmdlLmdldCgpLCBleHRy YWN0ZWRSYW5nZS5sb2NhdGlvbiwgZXh0cmFjdGVkUmFuZ2UubGVuZ3RoKTsKICAgICBpZiAoIWZp bmFsUmFuZ2UpCg== 203676 2013-06-04 03:58:44 -0700 2013-06-07 13:16:42 -0700 Patch bug-117020-20130604125714.patch text/plain 2451 deniau U3VidmVyc2lvbiBSZXZpc2lvbjogMTUwOTUzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggNjRlOGEyNDVhOGNlNWM3 NjkyYjI3OGQ4MTcyMjQ3MjNjZmFhZTMzYy4uNGIzMDY2MTlmZThkNjljZWI5Y2E4MjcwMWM1NTk4 Yjg5MDA5ZjkzMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEzLTA1LTMwICBUaG9t YXMgRGVuaWF1ICA8ZGVuaWF1QGFwcGxlLmNvbT4KKworICAgICAgICBSZXByb2R1Y2libGUgY3Jh c2ggd2l0aCB0cmlwbGUtZmluZ2VyLXRhcCAiZGVmaW5lIHdvcmQiIGdlc3R1cmUgb24gYSBOZXRm bGl4IHZpZGVvCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xMTcwMjAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICAqIFdlYlByb2Nlc3MvV2ViUGFnZS9tYWMvV2ViUGFnZU1hYy5tbToKKyAgICAgICAgKFdlYktp dDo6V2ViUGFnZTo6cGVyZm9ybURpY3Rpb25hcnlMb29rdXBBdExvY2F0aW9uKToKKyAgICAgICAg RG9uJ3QgaW52b2tlIHRoZSBEaWN0aW9uYXJ5IGxvb2t1cCB3aGVuIHdlIGZhaWxlZCB0byBleHRy YWN0IHRoZSByYW5nZSBvZiB0aGUgdG9rZW4KKyAgICAgICAgc3Vycm91bmRpbmcgdGhlIHRhcHBl ZCBwb3NpdGlvbi4KKwogMjAxMy0wNS0yOSAgVGltIEhvcnRvbiAgPHRpbW90aHlfaG9ydG9uQGFw cGxlLmNvbT4KIAogICAgICAgICBFeHBvc2UgaW5jcmVtZW50YWxSZW5kZXJpbmdTdXBwcmVzc2lv blRpbWVvdXQgdmlhIFdLMgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9X ZWJQYWdlL21hYy9XZWJQYWdlTWFjLm1tIGIvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJQ YWdlL21hYy9XZWJQYWdlTWFjLm1tCmluZGV4IGU1NGM4ODEwZGU3NmEyMDE4OTNjNWZlOTU5ODI1 NjUyMTc2YWFhMWQuLmQwOTRiMmY0YzA1NThmNzdhYTU3N2VhNDVmZDI3NWZhYjI4ZTA2Y2QgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUGFnZS9tYWMvV2ViUGFnZU1h Yy5tbQorKysgYi9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL1dlYlBhZ2UvbWFjL1dlYlBhZ2VN YWMubW0KQEAgLTUzNSw5ICs1MzUsMTUgQEAgdm9pZCBXZWJQYWdlOjpwZXJmb3JtRGljdGlvbmFy eUxvb2t1cEF0TG9jYXRpb24oY29uc3QgRmxvYXRQb2ludCYgZmxvYXRQb2ludCkKICAgICAgICAg ICAgIGJyZWFrOwogICAgICAgICBjb250ZXh0RW5kID0gbjsKICAgICB9Ci0gICAgY29udGV4dFN0 YXJ0ID0gc3RhcnRPZkxpbmUoY29udGV4dFN0YXJ0KTsKLSAgICBjb250ZXh0RW5kID0gZW5kT2ZM aW5lKGNvbnRleHRFbmQpOwotICAgIAorCisgICAgVmlzaWJsZVBvc2l0aW9uIGxpbmVTdGFydCA9 IHN0YXJ0T2ZMaW5lKGNvbnRleHRTdGFydCk7CisgICAgaWYgKCFsaW5lU3RhcnQuaXNOdWxsKCkp CisgICAgICAgIGNvbnRleHRTdGFydCA9IGxpbmVTdGFydDsKKworICAgIFZpc2libGVQb3NpdGlv biBsaW5lRW5kID0gZW5kT2ZMaW5lKGNvbnRleHRFbmQpOworICAgIGlmICghbGluZUVuZC5pc051 bGwoKSkKKyAgICAgICAgY29udGV4dEVuZCA9IGxpbmVFbmQ7CisKICAgICBOU1JhbmdlIHJhbmdl VG9QYXNzID0gTlNNYWtlUmFuZ2UoVGV4dEl0ZXJhdG9yOjpyYW5nZUxlbmd0aChtYWtlUmFuZ2Uo Y29udGV4dFN0YXJ0LCBwb3NpdGlvbikuZ2V0KCkpLCAwKTsKIAogICAgIFJlZlB0cjxSYW5nZT4g ZnVsbENoYXJhY3RlclJhbmdlID0gbWFrZVJhbmdlKGNvbnRleHRTdGFydCwgY29udGV4dEVuZCk7 CkBAIC01NDUsNiArNTUxLDEwIEBAIHZvaWQgV2ViUGFnZTo6cGVyZm9ybURpY3Rpb25hcnlMb29r dXBBdExvY2F0aW9uKGNvbnN0IEZsb2F0UG9pbnQmIGZsb2F0UG9pbnQpCiAKICAgICBOU1Jhbmdl IGV4dHJhY3RlZFJhbmdlID0gV0tFeHRyYWN0V29yZERlZmluaXRpb25Ub2tlblJhbmdlRnJvbUNv bnRleHR1YWxTdHJpbmcoZnVsbFBsYWluVGV4dFN0cmluZywgcmFuZ2VUb1Bhc3MsICZvcHRpb25z KTsKIAorICAgIC8vIFRoaXMgZnVuY3Rpb24gc29tZXRpbWVzIHJldHVybnMge05TTm90Rm91bmQs IDB9IGlmIGl0IHdhcyB1bmFibGUgdG8gZGV0ZXJtaW5lIGEgZ29vZCBzdHJpbmcuCisgICAgaWYg KGV4dHJhY3RlZFJhbmdlLmxvY2F0aW9uID09IE5TTm90Rm91bmQpCisgICAgICAgIHJldHVybjsK KwogICAgIFJlZlB0cjxSYW5nZT4gZmluYWxSYW5nZSA9IFRleHRJdGVyYXRvcjo6c3VicmFuZ2Uo ZnVsbENoYXJhY3RlclJhbmdlLmdldCgpLCBleHRyYWN0ZWRSYW5nZS5sb2NhdGlvbiwgZXh0cmFj dGVkUmFuZ2UubGVuZ3RoKTsKICAgICBpZiAoIWZpbmFsUmFuZ2UpCiAgICAgICAgIHJldHVybjsK