139383 2014-12-07 20:28:39 -0800 Hang in TextCheckingHelper when checking a document with no characters 2014-12-09 19:04:05 -0800 1 1 1 Unclassified WebKit HTML Editing 528+ (Nightly build) Unspecified Unspecified NEW InRadar P2 Normal --- 1 jalkut webkit-unassigned darin enrica rniwa oldest_to_newest 1053273 0 242783 jalkut 2014-12-07 20:28:39 -0800 Created attachment 242783 Example HTML to reproduce the hang. NOTE: I reported this bug via Radar (#10338685) 3+ years ago, but I'm revisiting it now with a bug report here on the WebKit's own issue tracker because I've put work into it that may lead to a successful patch. The gist of the bug is TextCheckingHelper::findFirstMisspellingOrBadGrammar is liable to hang when spell-checking is conducted on content that has multiple paragraphs but no bona fide text content. The problem seems to lie in the length of the selection being computed in that method before the target range is "expanded". The result being that a loop intent on evaluating all the text in the selection will never complete because the length is later determined to be 0. To reproduce: 1. Open attached SpellingHang.html in MiniBrowser, Safari, or any other WebKit view host. 2. Click in the editable section (where the bogus IMG elements reside) 3. Right-click to select "Spelling and Grammar" -> "Check Document Now" Result: hard hang in TextCheckingHelper::findFirstMisspellingOrBadGrammar, within the loop that is governed by the test: while (totalLengthProcessed < totalRangeLength) 1053274 1 jalkut 2014-12-07 20:39:01 -0800 The nut of the buggy behavior at least as it pertains to the example HTML attached, is: 1. totalRangeLength is computed to be 1 (I'm fuzzy on why but I think it's because TextIterator's guts end up synthesizing a newline character for some reason). 2. while (totalLengthProcessed < totalRangeLength) opens an infinite loop that can only be escaped by a break statement or by the totalRangeLength (1) being exceeded by totalLengthProcessed. 3. totalLengthProcessed stays pinned at 0 because "currentLength = TextIterator::rangeLength(paragraphRange.get())" always returns 0, owing to paragraphRange having been MODIFIED after the initial totalRangeLength was recorded. The modifications are in the block of code with comment starting "Expand the search range", and comprising: RefPtr<Range> paragraphRange = m_range->cloneRange(IGNORE_EXCEPTION); setStart(paragraphRange.get(), startOfParagraph(m_range->startPosition())); int totalRangeLength = TextIterator::rangeLength(paragraphRange.get()); setEnd(paragraphRange.get(), endOfParagraph(m_range->startPosition())); Note that totalRangeLength is recorded before the setEnd call to paragraphRange, but the bulk of this method call will involve attempting to exhaust the content of paragraphRange. Therefore my naive proposal is that the bug can be avoided, and more correct behavior in general be obtained, by moving the "int totalRangeLength =" line to AFTER the setEnd call, so that it is established before the loop starts that the totalRangeLength for practical purposes is 0. This causes the meat of the loop to never be entered, and at least for my test case, no hang any longer occurs. At least two potential downsides to my simple solution come to mind: 1. totalRangeLength currently is obtained BEFORE the setEnd call for some good reason. I can't fathom what it might be, but hopefully some discussion of this bug could draw in a developer with more experience in the editing component. 2. Possibly the the fact that totalRangeLength returns 1 even when it does reflects a flaw in the TextIterator::rangeLength() method. If so, I can imagine that somebody more experienced with how that class should work would rightly argue that instead of fiddling with the order of things in TextCheckingHelper, the TextIterator should consistently return 0 (i.e. not synthesize a newline?) for any range that has no meaningful text content. I'm hoping to get some good feedback from experienced folks, after which I'd be happy to follow up with a proper patch, assuming it's in the realm of something I'm competent to do ;) 1053278 2 benjamin 2014-12-07 22:30:29 -0800 Andreas already CCed the right engineers. If they miss the notification, you can poke them on #webkit: Ryosuke is rniwa, Enrica is enrica. Ryosuke was on vacation last week but I believe he will be back this week. 1053361 3 jalkut 2014-12-08 05:52:04 -0800 Thanks, Benjamin! I will follow up with them directly if needed. 242783 2014-12-07 20:28:39 -0800 2014-12-07 20:28:39 -0800 Example HTML to reproduce the hang. SpellingHang.html text/html 478 jalkut CjxwPgpCZWxvdyBpcyBhIHRlc3QgY2FzZSBkZW1vbnN0cmF0aW5nIGEgaGFuZyB0aGF0IG9jY3Vy cyBpbiBXZWJDb3JlIHdoZW4gc3BlbGwgY2hlY2tpbmcgY29udGVudCB3aGVyZSBhIGRpc3BsYXk6 YmxvY2sgaW1hZ2Ugc2hhcmVzIGEgcGFyYWdyYXBoIG5vZGUgd2l0aCBhbm90aGVyIGltYWdlLgo8 L3A+Cgo8cD4KQ2xpY2sgaW4gdGhlIGVkaXRhYmxlIGFyZWEgYmVsb3csIHJpZ2h0IGNsaWNrIHRv IHByZXNlbnQgdGhlIGNvbnRleHQgbWVudSwgYW5kIHNlbGVjdCAiU3BlbGxpbmciIC0+ICJDaGVj ayBEb2N1bWVudCBOb3ciIHRvIGhhbmcgeW91ciBicm93c2VyLgo8L3A+Cgo8ZGl2IGlkPSJ0ZXN0 RGl2IiBjb250ZW50RWRpdGFibGU9InRydWUiCjxwPjxpbWcgd2lkdGg9MTAwIGhlaWdodD0xMDAg Lz48L3A+CjxwPjxpbWcgd2lkdGg9MTAwIGhlaWdodD0xMDAgc3R5bGU9ImRpc3BsYXk6YmxvY2s7 IiAvPjxpbWcgLz48L3A+CjwvZGl2Pg==