Bug 139383 – Hang in TextCheckingHelper when checking a document with no characters

• Component: HTML Editing
• Status: NEW

[Daniel Jalkut]

Created attachment 242783 [details]
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)

[Daniel Jalkut]

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 ;)

[Benjamin Poulain]

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.

[Daniel Jalkut]

Thanks, Benjamin! I will follow up with them directly if needed.