Bug 200589

Summary: Fix Crash in Mail Search
Product: WebKit Reporter: Megan Gardner <megan_gardner>
Component: New BugsAssignee: Megan Gardner <megan_gardner>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, commit-queue, rniwa, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing none

Description Megan Gardner 2019-08-09 14:11:59 PDT
Fix Crash in Mail Search
Comment 1 Megan Gardner 2019-08-09 14:14:13 PDT
Created attachment 375953 [details]
Patch
Comment 2 Tim Horton 2019-08-09 14:17:51 PDT
Comment on attachment 375953 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375953&action=review

> Source/WebKit/ChangeLog:10
> +        It is possible for AppKit reasons to get a -1 for the index
> +        of the found item. Do not try and insert data in this case.
> +
> +        Reviewed by NOBODY (OOPS!).

Something bad happened here with the order.

Also, it would be nice to be more specific about the "AppKit reasons". Isn't it really about reverse search? (And then, why is it OK not to have any rects? does it actually behave correctly?)

> Source/WebKit/UIProcess/mac/WKTextFinderClient.mm:85
> +            if (matchIndex > 0 && matchIndex < matchCount)
> +                allMatches[matchIndex].appendVector(matchRects);

matchIndex 0 is valid, so you want >=
Comment 3 Tim Horton 2019-08-09 14:18:17 PDT
Please test both Mail and Notes, forward and backward, in conversations and not (in Mail).
Comment 4 Tim Horton 2019-08-09 14:18:40 PDT
ALSO ALSO this should totally be API testable
Comment 5 Megan Gardner 2019-08-09 14:38:16 PDT
Created attachment 375960 [details]
Patch
Comment 6 Megan Gardner 2019-08-09 14:45:07 PDT
Created attachment 375962 [details]
Patch
Comment 7 Megan Gardner 2019-08-09 14:56:04 PDT
Created attachment 375964 [details]
Patch
Comment 8 Megan Gardner 2019-08-09 15:33:09 PDT
Created attachment 375968 [details]
Patch
Comment 9 Tim Horton 2019-08-09 15:50:49 PDT
Comment on attachment 375968 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375968&action=review

> Source/WebKit/UIProcess/mac/WKTextFinderClient.mm:84
> -            allMatches[matchIndex].appendVector(matchRects);
> +            if (matchIndex >= 0 && (uint32_t)matchIndex < matchCount)

This is fine for now given current clients, but maybe leave a FIXME that we should have match rects when doing an incremental backwards search (did you test Notes??).
Comment 10 Wenson Hsieh 2019-08-09 19:28:12 PDT
<rdar://problem/53666720>
Comment 11 Ryosuke Niwa 2019-08-09 19:57:13 PDT
Comment on attachment 375968 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375968&action=review

>> Source/WebKit/UIProcess/mac/WKTextFinderClient.mm:84
>> +            if (matchIndex >= 0 && (uint32_t)matchIndex < matchCount)
> 
> This is fine for now given current clients, but maybe leave a FIXME that we should have match rects when doing an incremental backwards search (did you test Notes??).

Also, please use static_cast<uint32_t>.
Comment 12 Megan Gardner 2019-08-12 06:45:34 PDT
Created attachment 376059 [details]
Patch for landing
Comment 13 Megan Gardner 2019-08-12 13:12:15 PDT
Created attachment 376090 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2019-08-12 14:41:03 PDT
Comment on attachment 376090 [details]
Patch for landing

Clearing flags on attachment: 376090

Committed r248548: <https://trac.webkit.org/changeset/248548>
Comment 15 WebKit Commit Bot 2019-08-12 14:41:04 PDT
All reviewed patches have been landed.  Closing bug.