Summary: | Fix Crash in Mail Search | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Megan Gardner <megan_gardner> | ||||||||||||||||
Component: | New Bugs | Assignee: | 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
Megan Gardner
2019-08-09 14:11:59 PDT
Created attachment 375953 [details]
Patch
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 >= Please test both Mail and Notes, forward and backward, in conversations and not (in Mail). ALSO ALSO this should totally be API testable Created attachment 375960 [details]
Patch
Created attachment 375962 [details]
Patch
Created attachment 375964 [details]
Patch
Created attachment 375968 [details]
Patch
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 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>. Created attachment 376059 [details]
Patch for landing
Created attachment 376090 [details]
Patch for landing
Comment on attachment 376090 [details] Patch for landing Clearing flags on attachment: 376090 Committed r248548: <https://trac.webkit.org/changeset/248548> All reviewed patches have been landed. Closing bug. |