REGRESSION (r150653): Reproducible crash with triple-finger-tap "define word" gesture on a Netflix video

[Tim Horton]

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

[Tim Horton]

This regressed in http://trac.webkit.org/changeset/150653.

[Radar WebKit Bug Importer]

<rdar://problem/14021387>

[Thomas Deniau]

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.

[Thomas Deniau]

Created attachment 203359 [details]
Patch

[Darin Adler]

Comment on attachment 203359 [details]
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.

[Darin Adler]

(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?

[Thomas Deniau]

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.

[Darin Adler]

(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.

[Thomas Deniau]

Created attachment 203676 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 203676 [details]
Patch

Clearing flags on attachment: 203676

Committed r151332: <http://trac.webkit.org/changeset/151332>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.