Bug 117020

Summary: REGRESSION (r150653): Reproducible crash with triple-finger-tap "define word" gesture on a Netflix video
Product: WebKit Reporter: Tim Horton <thorton>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, deniau, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Tim Horton 2013-05-30 01:20:06 PDT
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
Comment 1 Tim Horton 2013-05-30 01:28:13 PDT
This regressed in http://trac.webkit.org/changeset/150653.
Comment 2 Radar WebKit Bug Importer 2013-05-30 01:28:35 PDT
<rdar://problem/14021387>
Comment 3 Thomas Deniau 2013-05-30 07:58:43 PDT
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.
Comment 4 Thomas Deniau 2013-05-30 08:27:28 PDT
Created attachment 203359 [details]
Patch
Comment 5 Darin Adler 2013-05-30 09:37:27 PDT
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.
Comment 6 Darin Adler 2013-05-30 09:38:13 PDT
(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?
Comment 7 Thomas Deniau 2013-05-31 07:30:06 PDT
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.
Comment 8 Darin Adler 2013-06-01 16:29:05 PDT
(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.
Comment 9 Thomas Deniau 2013-06-04 03:58:44 PDT
Created attachment 203676 [details]
Patch
Comment 10 WebKit Commit Bot 2013-06-07 13:16:42 PDT
Comment on attachment 203676 [details]
Patch

Clearing flags on attachment: 203676

Committed r151332: <http://trac.webkit.org/changeset/151332>
Comment 11 WebKit Commit Bot 2013-06-07 13:16:45 PDT
All reviewed patches have been landed.  Closing bug.